Skip to content

Commit 2ca262c

Browse files
nate-chandlernate-chandler
committed
[SIL] Require explicit releasenone.
Functions which which consume their arguments will release them on any branches where the arguments are unused. Users of `@_effects(readnone)` and `@_effects(readonly)` may not consider this when annotating their functions. Require that functions annotated thus are also annotated @_effects(releasenone). In order to support this, adjust SILFunctionBuilder to allow distinct non-custom effects and to interpret such "conflicting" guarantees to provide the strongest guarantee. For example, annotating a function both `@_effects(readnone)` and `@_effects(releasenone)` is equivalent to only annotating it `@_effects(readnone)`.
1 parent 63bdb4a commit 2ca262c

File tree

3 files changed

+193
-9
lines changed

3 files changed

+193
-9
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

‎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)