Skip to content

Commit e778356

Browse files
nate-chandlernate-chandler
authored
Merge pull request #68285 from nate-chandler/effects-diagnostic
[Diagnostics] Require explicit releasenone.
2 parents 2df6e80 + 2637c41 commit e778356

File tree

7 files changed

+201
-17
lines changed

7 files changed

+201
-17
lines changed

‎include/swift/AST/DiagnosticsSIL.def

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,14 @@ ERROR(exclusivity_access_required_unknown_decl_moveonly,none,
102102
NOTE(exclusivity_conflicting_access,none,
103103
"conflicting access is here", ())
104104

105+
WARNING(error_attr_effects_consume_requires_explicit_releasenone_self,none,
106+
"annotation implies no releases, but consumes self", ())
107+
WARNING(error_attr_effects_consume_requires_explicit_releasenone,none,
108+
"annotation implies no releases, but consumes parameter %0", (DeclName))
109+
NOTE(note_attr_effects_consume_requires_explicit_releasenone,none,
110+
"parameter %0 defined here", (DeclName))
111+
NOTE(fixit_attr_effects_consume_requires_explicit_releasenone,none,"add explicit @_effects(releasenone) if this is intended", ())
112+
105113
ERROR(unsupported_c_function_pointer_conversion,none,
106114
"C function pointer signature %0 is not compatible with expected type %1",
107115
(Type, Type))

‎lib/SIL/IR/SILFunctionBuilder.cpp

Lines changed: 65 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,10 @@
1313
#include "swift/SIL/SILFunctionBuilder.h"
1414
#include "swift/AST/AttrKind.h"
1515
#include "swift/AST/Availability.h"
16+
#include "swift/AST/Decl.h"
1617
#include "swift/AST/DiagnosticsParse.h"
18+
#include "swift/AST/DiagnosticsSIL.h"
1719
#include "swift/AST/DistributedDecl.h"
18-
#include "swift/AST/Decl.h"
1920
#include "swift/AST/ParameterList.h"
2021
#include "swift/AST/SemanticAttrs.h"
2122

@@ -60,6 +61,8 @@ void SILFunctionBuilder::addFunctionAttributes(
6061
if (auto *A = Attrs.getAttribute(DAK_EmitAssemblyVisionRemarks))
6162
F->addSemanticsAttr(semantics::FORCE_EMIT_OPT_REMARK_PREFIX);
6263

64+
auto *attributedFuncDecl = constant.getAbstractFunctionDecl();
65+
6366
// Propagate @_specialize.
6467
for (auto *A : Attrs.getAttributes<SpecializeAttr>()) {
6568
auto *SA = cast<SpecializeAttr>(A);
@@ -103,21 +106,74 @@ void SILFunctionBuilder::addFunctionAttributes(
103106
}
104107
}
105108

109+
EffectsAttr const *writeNoneEffect = nullptr;
110+
EffectsAttr const *releaseNoneEffect = nullptr;
106111
llvm::SmallVector<const EffectsAttr *, 8> customEffects;
107112
if (constant) {
108113
for (auto *attr : Attrs.getAttributes<EffectsAttr>()) {
109114
auto *effectsAttr = cast<EffectsAttr>(attr);
110-
if (effectsAttr->getKind() == EffectsKind::Custom) {
115+
switch (effectsAttr->getKind()) {
116+
case EffectsKind::Custom:
111117
customEffects.push_back(effectsAttr);
118+
// Proceed to the next attribute, don't set the effects kind based on
119+
// this attribute.
120+
continue;
121+
case EffectsKind::ReadNone:
122+
case EffectsKind::ReadOnly:
123+
writeNoneEffect = effectsAttr;
124+
break;
125+
case EffectsKind::ReleaseNone:
126+
releaseNoneEffect = effectsAttr;
127+
break;
128+
default:
129+
break;
130+
}
131+
if (effectsAttr->getKind() == EffectsKind::Custom)
132+
continue;
133+
if (F->getEffectsKind() != EffectsKind::Unspecified) {
134+
// If multiple known effects are specified, the most restrictive one
135+
// is used.
136+
F->setEffectsKind(
137+
std::min(effectsAttr->getKind(), F->getEffectsKind()));
138+
} else {
139+
F->setEffectsKind(effectsAttr->getKind());
140+
}
141+
}
142+
}
143+
144+
if (writeNoneEffect && !releaseNoneEffect) {
145+
auto constantType = mod.Types.getConstantFunctionType(
146+
TypeExpansionContext::minimal(), constant);
147+
SILFunctionConventions fnConv(constantType, mod);
148+
149+
auto selfIndex = fnConv.getSILArgIndexOfSelf();
150+
for (auto index : indices(fnConv.getParameters())) {
151+
auto param = fnConv.getParameters()[index];
152+
if (!param.isConsumed())
153+
continue;
154+
if (index == selfIndex) {
155+
mod.getASTContext().Diags.diagnose(
156+
writeNoneEffect->getLocation(),
157+
diag::
158+
error_attr_effects_consume_requires_explicit_releasenone_self);
112159
} else {
113-
if (F->getEffectsKind() != EffectsKind::Unspecified &&
114-
F->getEffectsKind() != effectsAttr->getKind()) {
115-
mod.getASTContext().Diags.diagnose(effectsAttr->getLocation(),
116-
diag::warning_in_effects_attribute, "mismatching function effects");
117-
} else {
118-
F->setEffectsKind(effectsAttr->getKind());
119-
}
160+
auto *pd = attributedFuncDecl->getParameters()->get(index);
161+
mod.getASTContext().Diags.diagnose(
162+
writeNoneEffect->getLocation(),
163+
diag::error_attr_effects_consume_requires_explicit_releasenone,
164+
pd->getName());
165+
mod.getASTContext().Diags.diagnose(
166+
pd->getNameLoc(),
167+
diag::note_attr_effects_consume_requires_explicit_releasenone,
168+
pd->getName());
120169
}
170+
mod.getASTContext()
171+
.Diags
172+
.diagnose(
173+
writeNoneEffect->getLocation(),
174+
diag::fixit_attr_effects_consume_requires_explicit_releasenone)
175+
.fixItInsertAfter(writeNoneEffect->getRange().End,
176+
" @_effects(releasenone)");
121177
}
122178
}
123179

‎lib/SILGen/SILGen.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -715,11 +715,11 @@ SILFunction *SILGenModule::getFunction(SILDeclRef constant,
715715

716716
// Note: Do not provide any SILLocation. You can set it afterwards.
717717
SILGenFunctionBuilder builder(*this);
718-
auto &IGM = *this;
718+
auto &SGM = *this;
719719
auto *F = builder.getOrCreateFunction(
720720
getBestLocation(constant), constant, forDefinition,
721-
[&IGM](SILLocation loc, SILDeclRef constant) -> SILFunction * {
722-
return IGM.getFunction(constant, NotForDefinition);
721+
[&SGM](SILLocation loc, SILDeclRef constant) -> SILFunction * {
722+
return SGM.getFunction(constant, NotForDefinition);
723723
});
724724

725725
assert(F && "SILFunction should have been defined");

‎stdlib/public/core/ArrayShared.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func _deallocateUninitializedArray<Element>(
6868
#if !INTERNAL_CHECKS_ENABLED
6969
@_alwaysEmitIntoClient
7070
@_semantics("array.finalize_intrinsic")
71-
@_effects(readnone)
71+
@_effects(readnone) @_effects(releasenone)
7272
@_effects(escaping array.value** => return.value**)
7373
@_effects(escaping array.value**.class*.value** => return.value**.class*.value**)
7474
public // COMPILER_INTRINSIC

‎stdlib/public/core/SmallString.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ extension _SmallString {
345345
extension _SmallString {
346346
// Resiliently create from a tagged cocoa string
347347
//
348-
@_effects(readonly) // @opaque
348+
@_effects(readonly) @_effects(releasenone) // @opaque
349349
@usableFromInline // testable
350350
internal init?(taggedCocoa cocoa: AnyObject) {
351351
self.init()
@@ -367,7 +367,7 @@ extension _SmallString {
367367
self._invariantCheck()
368368
}
369369

370-
@_effects(readonly) // @opaque
370+
@_effects(readonly) @_effects(releasenone) // @opaque
371371
internal init?(taggedASCIICocoa cocoa: AnyObject) {
372372
self.init()
373373
var success = true

‎stdlib/public/core/StringInterpolation.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ extension String {
230230
/// print(message)
231231
/// // Prints "If one cookie costs 2 dollars, 3 cookies cost 6 dollars."
232232
@inlinable
233-
@_effects(readonly)
233+
@_effects(readonly) @_effects(releasenone)
234234
public init(stringInterpolation: DefaultStringInterpolation) {
235235
self = stringInterpolation.make()
236236
}
@@ -254,7 +254,7 @@ extension Substring {
254254
/// print(message)
255255
/// // Prints "If one cookie costs 2 dollars, 3 cookies cost 6 dollars."
256256
@inlinable
257-
@_effects(readonly)
257+
@_effects(readonly) @_effects(releasenone)
258258
public init(stringInterpolation: DefaultStringInterpolation) {
259259
self.init(stringInterpolation.make())
260260
}

‎test/SIL/diagnose_effects.swift

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
// RUN: %target-swift-emit-silgen %s -o /dev/null -verify
2+
3+
class C {}
4+
struct S {
5+
6+
var c: C
7+
8+
////////////////////////////////////////////////////////////////////////////////
9+
// Implicitly consuming argument. //
10+
////////////////////////////////////////////////////////////////////////////////
11+
12+
@_effects(readnone)
13+
// expected-warning@-1{{annotation implies no releases, but consumes parameter 'c'}}
14+
// expected-note@-2{{add explicit @_effects(releasenone) if this is intended}}
15+
// expected-note@+1{{parameter 'c' defined here}}
16+
init(readnone c: C) { self.c = c }
17+
18+
@_effects(readnone)
19+
// expected-warning@-1{{annotation implies no releases, but consumes parameter 'readnone_noargumentlabel'}}
20+
// expected-note@-2{{add explicit @_effects(releasenone) if this is intended}}
21+
// expected-note@+1{{parameter 'readnone_noargumentlabel' defined here}}
22+
init(readnone_noargumentlabel: C) { self.c = readnone_noargumentlabel }
23+
24+
@_effects(readnone) @_effects(releasenone) // ok
25+
init(readnone_releasenone c: C) { self.c = c }
26+
27+
@_effects(releasenone) @_effects(readnone) // ok
28+
init(releasenone_readnone c: C) { self.c = c }
29+
30+
@_effects(readonly)
31+
// expected-warning@-1{{annotation implies no releases, but consumes parameter 'c'}}
32+
// expected-note@-2{{add explicit @_effects(releasenone) if this is intended}}
33+
// expected-note@+1{{parameter 'c' defined here}}
34+
init(readonly c: C) { self.c = c }
35+
36+
@_effects(readonly) @_effects(releasenone) // ok
37+
init(readonly_releasenone c: C) { self.c = c }
38+
39+
@_effects(releasenone) @_effects(readonly) // ok
40+
init(releasenone_readonly c: C) { self.c = c }
41+
42+
@_effects(releasenone) // ok
43+
init(releasenone c: C) { self.c = c }
44+
45+
////////////////////////////////////////////////////////////////////////////////
46+
// Explicitly consuming argument. //
47+
////////////////////////////////////////////////////////////////////////////////
48+
49+
@_effects(readnone)
50+
// expected-warning@-1{{annotation implies no releases, but consumes parameter 'c'}}
51+
// expected-note@-2{{add explicit @_effects(releasenone) if this is intended}}
52+
// expected-note@+1{{parameter 'c' defined here}}
53+
mutating func readnoneConsumeParam(_ c: consuming C) {
54+
self.c = c
55+
}
56+
57+
@_effects(readnone) @_effects(releasenone) // ok
58+
mutating func readnone_releasenoneConsumeParam(_ c: consuming C) {
59+
self.c = c
60+
}
61+
62+
@_effects(releasenone) @_effects(readnone) // ok
63+
mutating func releasenone_readnoneConsumeParam(_ c: consuming C) {
64+
self.c = c
65+
}
66+
67+
@_effects(readonly)
68+
// expected-warning@-1{{annotation implies no releases, but consumes parameter 'c'}}
69+
// expected-note@-2{{add explicit @_effects(releasenone) if this is intended}}
70+
// expected-note@+1{{parameter 'c' defined here}}
71+
mutating func readonlyConsumeParam(_ c: consuming C) {
72+
self.c = c
73+
}
74+
75+
@_effects(readonly) @_effects(releasenone) // ok
76+
mutating func reasonly_releasenoneConsumeParam(_ c: consuming C) {
77+
self.c = c
78+
}
79+
80+
@_effects(releasenone) @_effects(readonly) // ok
81+
mutating func releasenone_reasonlyConsumeParam(_ c: consuming C) {
82+
self.c = c
83+
}
84+
85+
@_effects(releasenone) // ok
86+
mutating func releasenoneConsumeParam(_ c: consuming C) {
87+
self.c = c
88+
}
89+
90+
////////////////////////////////////////////////////////////////////////////////
91+
// Explicitly consuming self. //
92+
////////////////////////////////////////////////////////////////////////////////
93+
94+
@_effects(readnone)
95+
// expected-warning@-1{{annotation implies no releases, but consumes self}}
96+
// expected-note@-2{{add explicit @_effects(releasenone) if this is intended}}
97+
__consuming func readnoneConsumeSelf() {}
98+
99+
@_effects(readnone) @_effects(releasenone) // ok
100+
__consuming func readnone_releasenoneConsumeSelf() {}
101+
102+
@_effects(releasenone) @_effects(readnone) // ok
103+
__consuming func readnone_readnoneConsumeSelf() {}
104+
105+
@_effects(readonly)
106+
// expected-warning@-1{{annotation implies no releases, but consumes self}}
107+
// expected-note@-2{{add explicit @_effects(releasenone) if this is intended}}
108+
__consuming func readonlyConsumeSelf() {}
109+
110+
@_effects(readonly) @_effects(releasenone) // ok
111+
__consuming func readonly_releasenoneConsumeSelf() {}
112+
113+
@_effects(releasenone) @_effects(readonly) // ok
114+
__consuming func releasenone_readonlyConsumeSelf() {}
115+
116+
@_effects(releasenone) // ok
117+
__consuming func releasenoneConsumeSelf() {}
118+
119+
}
120+

0 commit comments

Comments
 (0)