Skip to content

Commit 78c3c3b

Browse files
kavonkavon
committed
working on adding a test; more fixes
1 parent 792f087 commit 78c3c3b

File tree

2 files changed

+310
-27
lines changed

2 files changed

+310
-27
lines changed

‎lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

Lines changed: 104 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "MoveOnlyDiagnostics.h"
1616

1717
#include "swift/AST/DiagnosticsSIL.h"
18+
#include "swift/AST/Stmt.h"
1819
#include "swift/Basic/Defer.h"
1920
#include "swift/SIL/BasicBlockBits.h"
2021
#include "swift/SIL/BasicBlockDatastructures.h"
@@ -195,38 +196,114 @@ void DiagnosticEmitter::emitMissingConsumeInDiscardingContext(
195196
SILInstruction *discard) {
196197
assert(isa<DropDeinitInst>(discard));
197198

198-
// An instruction corresponding to the logical place where the value is
199-
// destroyed. Ideally an exit point of the function reachable from here.
200-
// If for some reason we can't find an exit, then just use the original.
201-
SILInstruction *logicalDestroyLocation = leftoverDestroy;
202-
203-
// Search for a function exit reachable from this destroy. We do this because
204-
// the move checker may have injected or hoisted an existing destroy from leaf
205-
// blocks to some earlier point. For example, if 'd' represents a destroy of
206-
// self, then we may have this CFG:
207-
//
208-
// before: after:
209-
// . d
210-
// / \ / \
211-
// d d . .
212-
//
213-
BasicBlockWorklist worklist(logicalDestroyLocation->getFunction());
214-
worklist.push(logicalDestroyLocation->getParent());
215-
while (auto *bb = worklist.pop()) {
216-
TermInst *term = bb->getTerminator();
217-
218-
// Looking for a block that exits the function or terminates the program
219-
if (term->getNumSuccessors() == 0) {
220-
logicalDestroyLocation = term;
221-
break;
199+
// A good location is one that has some connection with the original source
200+
// and corresponds to an exit of the function.
201+
auto hasGoodLocation = [](SILInstruction *si) -> bool {
202+
if (!si)
203+
return false;
204+
205+
SILLocation loc = si->getLoc();
206+
if (loc.isNull())
207+
return false;
208+
209+
switch (loc.getKind()) {
210+
case SILLocation::ReturnKind:
211+
case SILLocation::ImplicitReturnKind:
212+
return true;
213+
214+
case SILLocation::RegularKind: {
215+
Stmt *stmt = loc.getAsASTNode<Stmt>();
216+
if (!stmt)
217+
return true; // For non-statements, assume it is exiting the func.
218+
219+
// Prefer statements that can possibly lead to an exit of the function.
220+
// This is determined by whether the statement causes an exit of a
221+
// lexical scope; so a 'break' counts but not a 'continue'.
222+
switch (stmt->getKind()) {
223+
case StmtKind::Throw:
224+
case StmtKind::Return:
225+
case StmtKind::Yield:
226+
case StmtKind::Break:
227+
case StmtKind::Fail:
228+
case StmtKind::PoundAssert:
229+
return true;
230+
231+
case StmtKind::Continue:
232+
case StmtKind::Brace:
233+
case StmtKind::Defer:
234+
case StmtKind::If:
235+
case StmtKind::Guard:
236+
case StmtKind::While:
237+
case StmtKind::Do:
238+
case StmtKind::DoCatch:
239+
case StmtKind::RepeatWhile:
240+
case StmtKind::ForEach:
241+
case StmtKind::Switch:
242+
case StmtKind::Case:
243+
case StmtKind::Fallthrough:
244+
case StmtKind::Discard:
245+
return false;
246+
};
222247
}
223248

224-
for (auto *nextBB : term->getSuccessorBlocks())
225-
worklist.pushIfNotVisited(nextBB);
249+
case SILLocation::InlinedKind:
250+
case SILLocation::MandatoryInlinedKind:
251+
case SILLocation::CleanupKind:
252+
case SILLocation::ArtificialUnreachableKind:
253+
return false;
254+
};
255+
};
256+
257+
// An instruction corresponding to the logical place where the value is
258+
// destroyed. Ideally an exit point of the function reachable from here or
259+
// some relevant statement.
260+
SILInstruction *destroyPoint = leftoverDestroy;
261+
if (!hasGoodLocation(destroyPoint)) {
262+
// Search for a nearby function exit reachable from this destroy. We do this
263+
// because the move checker may have injected or hoisted an existing
264+
// destroy from leaf blocks to some earlier point. For example, if 'd'
265+
// represents a destroy of self, then we may have this CFG:
266+
//
267+
// before: after:
268+
// . d
269+
// / \ / \
270+
// d d . .
271+
//
272+
BasicBlockSet visited(destroyPoint->getFunction());
273+
std::deque<SILBasicBlock *> bfsWorklist = {destroyPoint->getParent()};
274+
while (auto *bb = bfsWorklist.front()) {
275+
visited.insert(bb);
276+
bfsWorklist.pop_front();
277+
278+
TermInst *term = bb->getTerminator();
279+
280+
// Looking for a block that exits the function or terminates the program.
281+
if (term->isFunctionExiting() || term->isProgramTerminating()) {
282+
SILInstruction *candidate = term;
283+
284+
// Walk backwards until we find an instruction with any source location.
285+
// Sometimes a terminator like 'unreachable' may not have one, but one
286+
// of the preceding instructions will.
287+
while (candidate && candidate->getLoc().isNull())
288+
candidate = candidate->getPreviousInstruction();
289+
290+
if (candidate && candidate->getLoc()) {
291+
destroyPoint = candidate;
292+
break;
293+
}
294+
}
295+
296+
for (auto *nextBB : term->getSuccessorBlocks())
297+
if (!visited.contains(nextBB))
298+
bfsWorklist.push_back(nextBB);
299+
}
226300
}
227301

302+
assert(destroyPoint->getLoc() && "missing loc!");
303+
assert(discard->getLoc() && "missing loc!");
304+
228305
diagnose(leftoverDestroy->getFunction()->getASTContext(),
229-
logicalDestroyLocation,
306+
destroyPoint,
230307
diag::sil_movechecking_discard_missing_consume_self);
231308

232309
diagnose(discard->getFunction()->getASTContext(), discard,

‎test/SILOptimizer/discard_checking.swift

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
// RUN: %target-swift-emit-sil -sil-verify-all -verify %s
2+
3+
enum Color {
4+
case red, green, blue, none
5+
}
6+
enum E: Error {
7+
case someError
8+
}
9+
10+
struct Basics: ~Copyable {
11+
consuming func test1(_ b: Bool) {
12+
guard b else {
13+
fatalError("bah!") // expected-error {{must 'consume self' before exiting method that discards self}}
14+
}
15+
discard self // expected-note {{discarded self here}}
16+
}
17+
18+
consuming func test2(_ c: Color) throws {
19+
repeat {
20+
switch c {
21+
case .red: fatalError("bah!") // expected-error {{must 'consume self' before exiting method that discards self}}
22+
case .blue: throw E.someError // expected-error {{must 'consume self' before exiting method that discards self}}
23+
case .green: self = Basics()
24+
default: print("hi")
25+
}
26+
} while true
27+
discard self // expected-note 2{{discarded self here}}
28+
}
29+
30+
consuming func test3(_ c: Color) {
31+
if case .red = c {
32+
discard self // expected-note {{discarded self here}}
33+
return
34+
} else if case .blue = c {
35+
try! test2(c)
36+
return
37+
} else if case .green = c {
38+
return // expected-error {{must 'consume self' before exiting method that discards self}}
39+
} else {
40+
_ = consume self
41+
}
42+
}
43+
44+
consuming func test4(_ c: Color) {
45+
if case .red = c {
46+
discard self // expected-note {{discarded self here}}
47+
}
48+
} // expected-error {{must 'consume self' before exiting method that discards self}}
49+
50+
consuming func test5(_ c: Color) {
51+
if case .red = c {
52+
discard self // expected-note {{discarded self here}}
53+
} else {
54+
return // expected-error {{must 'consume self' before exiting method that discards self}}
55+
}
56+
}
57+
58+
consuming func test6(_ c: Color) throws {
59+
if case .red = c {
60+
discard self // expected-note {{discarded self here}}
61+
} else {
62+
throw E.someError // expected-error {{must 'consume self' before exiting method that discards self}}
63+
}
64+
}
65+
66+
consuming func test7(_ c: Color) throws {
67+
if case .red = c {
68+
discard self // expected-note {{discarded self here}}
69+
}
70+
fatalError("oh no") // expected-error {{must 'consume self' before exiting method that discards self}}
71+
}
72+
73+
consuming func test8(_ c: Color) throws {
74+
if case .red = c {
75+
discard self // expected-note {{discarded self here}}
76+
}
77+
if case .blue = c {
78+
fatalError("hi") // expected-error {{must 'consume self' before exiting method that discards self}}
79+
}
80+
}
81+
82+
consuming func test9(_ c: Color) throws {
83+
if case .red = c {
84+
discard self // expected-note {{discarded self here}}
85+
return
86+
}
87+
88+
do {
89+
throw E.someError
90+
} catch E.someError {
91+
try test8(c) // <- better spot
92+
return
93+
} catch {
94+
print("hi")
95+
return
96+
}
97+
} // expected-error {{must 'consume self' before exiting method that discards self}}
98+
99+
consuming func test10(_ c: Color) throws {
100+
if case .red = c {
101+
discard self // expected-note {{discarded self here}}
102+
return
103+
}
104+
105+
do {
106+
throw E.someError // expected-error {{must 'consume self' before exiting method that discards self}}
107+
} catch E.someError {
108+
return // <- better spot
109+
} catch {
110+
return
111+
}
112+
}
113+
114+
consuming func test11(_ c: Color) {
115+
guard case .red = c else {
116+
discard self // expected-note {{discarded self here}}
117+
return
118+
}
119+
defer { print("hi") }
120+
mutator()
121+
_ = consume self
122+
self = Basics()
123+
borrower()
124+
let x = self
125+
self = x
126+
mutator() // expected-error {{must 'consume self' before exiting method that discards self}}
127+
}
128+
129+
consuming func positiveTest(_ i: Int) {
130+
switch i {
131+
case 0: _ = self
132+
case 1: let _ = self
133+
case 2:
134+
let other = self
135+
_ = other
136+
case 3:
137+
_ = consume self
138+
case 4:
139+
self.test11(.red)
140+
default:
141+
discard self
142+
}
143+
}
144+
145+
// FIXME move checker is treating the defer like a closure capture (rdar://100468597)
146+
// not expecting any errors here
147+
consuming func brokenPositiveTest(_ i: Int) { // expected-error {{missing reinitialization of inout parameter 'self' after consume}}
148+
defer { discard self } // expected-note {{consumed here}}
149+
switch i {
150+
case 0, 1, 2, 3: return
151+
default:
152+
break
153+
}
154+
}
155+
156+
consuming func negativeTest(_ i: Int) throws {
157+
switch i {
158+
case 0:
159+
fallthrough
160+
case 1:
161+
throw E.someError // expected-error 2{{must 'consume self' before exiting method that discards self}}
162+
case 2:
163+
return // expected-error {{must 'consume self' before exiting method that discards self}}
164+
case 3:
165+
fatalError("no") // expected-error {{must 'consume self' before exiting method that discards self}}
166+
default:
167+
discard self // expected-note 4{{discarded self here}}
168+
}
169+
}
170+
171+
consuming func loopyExit(_ i: Int) {
172+
// TODO: ONE LAST ONE
173+
}
174+
175+
mutating func mutator() { self = Basics() }
176+
borrowing func borrower() {}
177+
deinit { print("hi") }
178+
}
179+
180+
struct Money: ~Copyable {
181+
enum Error: Swift.Error {
182+
case insufficientFunds
183+
}
184+
185+
let balance: Int
186+
187+
consuming func spend(_ charge: Int) throws -> Money {
188+
guard charge > 0 else {
189+
fatalError("can't charge a negative amount!") // expected-error {{must 'consume self' before exiting method that discards self}}
190+
}
191+
192+
if balance < charge {
193+
throw Error.insufficientFunds // expected-error {{must 'consume self' before exiting method that discards self}}
194+
} else if balance > charge {
195+
self = Money(balance: balance - charge)
196+
return self
197+
}
198+
199+
discard self // expected-note 2{{discarded self here}}
200+
return Money(balance: 0)
201+
}
202+
203+
deinit {
204+
assert(balance > 0)
205+
}
206+
}

0 commit comments

Comments
 (0)