Skip to content

Commit db904a9

Browse files
hamishknighthamishknight
authored
Merge pull request #37881 from hamishknight/time-for-branch-5.5
[5.5] [Async Refactoring] Better handle known condition paths
2 parents be6181b + 82195a7 commit db904a9

File tree

4 files changed

+295
-24
lines changed

4 files changed

+295
-24
lines changed

‎lib/IDE/Refactoring.cpp

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4949,6 +4949,12 @@ struct CallbackClassifier {
49494949
const ParamDecl *ErrParam;
49504950
bool IsResultParam;
49514951

4952+
/// This is set to \c true if we're currently classifying on a known condition
4953+
/// path, where \c CurrentBlock is set to the appropriate block. This lets us
4954+
/// be more lenient with unhandled conditions as we already know the block
4955+
/// we're supposed to be in.
4956+
bool IsKnownConditionPath = false;
4957+
49524958
CallbackClassifier(ClassifiedBlocks &Blocks, const FuncDecl *Callee,
49534959
ArrayRef<const ParamDecl *> SuccessParams,
49544960
llvm::DenseSet<SwitchStmt *> &HandledSwitches,
@@ -5264,7 +5270,12 @@ struct CallbackClassifier {
52645270
// placeholders added (ideally we'd remove them)
52655271
// TODO: Remove known conditions and split the `if` statement
52665272

5267-
if (CallbackConditions.empty()) {
5273+
if (IsKnownConditionPath) {
5274+
// If we're on a known condition path, we can be lenient as we already
5275+
// know what block we're in and can therefore just add the conditional
5276+
// straight to it.
5277+
CurrentBlock->addNode(Statement);
5278+
} else if (CallbackConditions.empty()) {
52685279
// Technically this has a similar problem, ie. the else could have
52695280
// conditions that should be in either success/error
52705281
CurrentBlock->addNode(Statement);
@@ -5327,19 +5338,45 @@ struct CallbackClassifier {
53275338

53285339
if (ElseStmt) {
53295340
if (auto *BS = dyn_cast<BraceStmt>(ElseStmt)) {
5330-
setNodes(ElseBlock, ThenBlock, NodesToPrint::inBraceStmt(BS));
5341+
// If this is a guard statement, we know that we'll always exit,
5342+
// allowing us to classify any additional nodes into the opposite block.
5343+
auto AlwaysExits = isa<GuardStmt>(Statement);
5344+
setNodes(ElseBlock, ThenBlock, NodesToPrint::inBraceStmt(BS),
5345+
AlwaysExits);
53315346
} else {
5347+
// If we reached here, we should have an else if statement. Given we
5348+
// know we're in the else of a known condition, temporarily flip the
5349+
// current block, and set that we know what path we're on.
5350+
llvm::SaveAndRestore<bool> CondScope(IsKnownConditionPath, true);
5351+
llvm::SaveAndRestore<ClassifiedBlock *> BlockScope(CurrentBlock,
5352+
ElseBlock);
53325353
classifyNodes(ArrayRef<ASTNode>(ElseStmt),
53335354
/*endCommentLoc*/ SourceLoc());
53345355
}
53355356
}
53365357
}
53375358

5359+
/// Adds \p Nodes to \p Block, potentially flipping the current block if we
5360+
/// can determine that the nodes being added will cause control flow to leave
5361+
/// the scope.
5362+
///
5363+
/// \param Block The block to add the nodes to.
5364+
/// \param OtherBlock The block for the opposing condition path.
5365+
/// \param Nodes The nodes to add.
5366+
/// \param AlwaysExitsScope Whether the nodes being added always exit the
5367+
/// scope, and therefore whether the current block should be flipped.
53385368
void setNodes(ClassifiedBlock *Block, ClassifiedBlock *OtherBlock,
5339-
NodesToPrint Nodes) {
5340-
if (Nodes.hasTrailingReturnOrBreak()) {
5341-
CurrentBlock = OtherBlock;
5369+
NodesToPrint Nodes, bool AlwaysExitsScope = false) {
5370+
// Drop an explicit trailing 'return' or 'break' if we can.
5371+
bool HasTrailingReturnOrBreak = Nodes.hasTrailingReturnOrBreak();
5372+
if (HasTrailingReturnOrBreak)
53425373
Nodes.dropTrailingReturnOrBreakIfPossible();
5374+
5375+
// If we know we're exiting the scope, we can set IsKnownConditionPath, as
5376+
// we know any future nodes should be classified into the other block.
5377+
if (HasTrailingReturnOrBreak || AlwaysExitsScope) {
5378+
CurrentBlock = OtherBlock;
5379+
IsKnownConditionPath = true;
53435380
Block->addAllNodes(std::move(Nodes));
53445381
} else {
53455382
Block->addAllNodes(std::move(Nodes));

‎test/refactoring/ConvertAsync/convert_params_multi.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,13 @@ manyWithError { res1, res2, err in
2525
// MANYBOUND-NEXT: print("got error \(bad)")
2626
// MANYBOUND-NEXT: }
2727

28+
// FIXME: This case is a little tricky: Being in the else block of 'if let str = res1'
29+
// should allow us to place 'if let i = res2' in the failure block. However, this
30+
// is a success condition, so we still place it in the success block. Really what
31+
// we need to do here is check to see if manyWithError has an existing async
32+
// alternative that still returns optional success values, and allow success
33+
// classification in that case. Otherwise, we'd probably be better off leaving
34+
// the condition unhandled, as it's not clear what the user is doing.
2835
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=MANYUNBOUND-ERR %s
2936
manyWithError { res1, res2, err in
3037
print("before")

‎test/refactoring/ConvertAsync/convert_pattern.swift

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -121,9 +121,7 @@ func testPatterns() async throws {
121121
// FALLBACK-NEXT: guard let (str1, str2) = strs, str1 == "hi" else { fatalError() }
122122
// FALLBACK-NEXT: print(str1, str2, err)
123123

124-
// FIXME: Arguably we should be able to classify everything after the guard as
125-
// a success path and avoid the fallback in this case.
126-
// RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=FALLBACK2 %s
124+
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=GUARD-AND-UNHANDLED %s
127125
stringTupleParam { strs, err in
128126
guard let (str1, str2) = strs else { fatalError() }
129127
print(str1, str2)
@@ -134,22 +132,17 @@ func testPatterns() async throws {
134132
}
135133
}
136134

137-
// FALLBACK2: var strs: (String, String)? = nil
138-
// FALLBACK2-NEXT: var err: Error? = nil
139-
// FALLBACK2-NEXT: do {
140-
// FALLBACK2-NEXT: strs = try await stringTupleParam()
141-
// FALLBACK2-NEXT: } catch {
142-
// FALLBACK2-NEXT: err = error
143-
// FALLBACK2-NEXT: }
144-
// FALLBACK2-EMPTY:
145-
// FALLBACK2-NEXT: guard let (str1, str2) = strs else { fatalError() }
146-
// FALLBACK2-NEXT: print(str1, str2)
147-
// FALLBACK2-NEXT: if .random(), err == nil {
148-
// FALLBACK2-NEXT: print("yay")
149-
// FALLBACK2-NEXT: } else {
150-
// FALLBACK2-NEXT: print("nay")
151-
// FALLBACK2-NEXT: }
152-
135+
// GUARD-AND-UNHANDLED: do {
136+
// GUARD-AND-UNHANDLED-NEXT: let (str1, str2) = try await stringTupleParam()
137+
// GUARD-AND-UNHANDLED-NEXT: print(str1, str2)
138+
// GUARD-AND-UNHANDLED-NEXT: if .random(), <#err#> == nil {
139+
// GUARD-AND-UNHANDLED-NEXT: print("yay")
140+
// GUARD-AND-UNHANDLED-NEXT: } else {
141+
// GUARD-AND-UNHANDLED-NEXT: print("nay")
142+
// GUARD-AND-UNHANDLED-NEXT: }
143+
// GUARD-AND-UNHANDLED-NEXT: } catch let err {
144+
// GUARD-AND-UNHANDLED-NEXT: fatalError()
145+
// GUARD-AND-UNHANDLED-NEXT: }
153146

154147
// RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=MIXED-BINDINGS %s
155148
stringTupleParam { strs, err in

‎test/refactoring/ConvertAsync/path_classification.swift

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
// RUN: %empty-directory(%t)
2+
3+
func simpleWithError(completion: (String?, Error?) -> Void) {}
4+
func simpleWithError() async throws -> String {}
5+
6+
func testPathClassification() async throws {
7+
8+
// Both of these test cases should produce the same refactoring.
9+
10+
// RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=ELSE-IF-CLASSIFICATION %s
11+
simpleWithError { str, err in
12+
if err == nil {
13+
print("a")
14+
} else if .random() {
15+
print("b")
16+
} else {
17+
print("c")
18+
}
19+
if err != nil {
20+
print("d")
21+
} else if .random() {
22+
print("e")
23+
} else {
24+
print("f")
25+
}
26+
}
27+
28+
// RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=ELSE-IF-CLASSIFICATION %s
29+
simpleWithError { str, err in
30+
if let str = str {
31+
print("a")
32+
} else if .random() {
33+
print("b")
34+
} else {
35+
print("c")
36+
}
37+
if str == nil {
38+
print("d")
39+
} else if .random() {
40+
print("e")
41+
} else {
42+
print("f")
43+
}
44+
}
45+
46+
// ELSE-IF-CLASSIFICATION: do {
47+
// ELSE-IF-CLASSIFICATION-NEXT: let str = try await simpleWithError()
48+
// ELSE-IF-CLASSIFICATION-NEXT: print("a")
49+
// ELSE-IF-CLASSIFICATION-NEXT: if .random() {
50+
// ELSE-IF-CLASSIFICATION-NEXT: print("e")
51+
// ELSE-IF-CLASSIFICATION-NEXT: } else {
52+
// ELSE-IF-CLASSIFICATION-NEXT: print("f")
53+
// ELSE-IF-CLASSIFICATION-NEXT: }
54+
// ELSE-IF-CLASSIFICATION-NEXT: } catch let err {
55+
// ELSE-IF-CLASSIFICATION-NEXT: if .random() {
56+
// ELSE-IF-CLASSIFICATION-NEXT: print("b")
57+
// ELSE-IF-CLASSIFICATION-NEXT: } else {
58+
// ELSE-IF-CLASSIFICATION-NEXT: print("c")
59+
// ELSE-IF-CLASSIFICATION-NEXT: }
60+
// ELSE-IF-CLASSIFICATION-NEXT: print("d")
61+
// ELSE-IF-CLASSIFICATION-NEXT: }
62+
63+
// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=ELSE-IF-CLASSIFICATION2 %s
64+
simpleWithError { str, err in
65+
if err == nil {
66+
print("a")
67+
} else if .random() {
68+
print("b")
69+
}
70+
if .random() {
71+
print("c")
72+
}
73+
if err != nil, .random() {
74+
print("d")
75+
}
76+
}
77+
78+
// Make sure the classification of 'b' into the error block doesn't affect the
79+
// handling of 'c'.
80+
// ELSE-IF-CLASSIFICATION2: do {
81+
// ELSE-IF-CLASSIFICATION2-NEXT: let str = try await simpleWithError()
82+
// ELSE-IF-CLASSIFICATION2-NEXT: print("a")
83+
// ELSE-IF-CLASSIFICATION2-NEXT: if .random() {
84+
// ELSE-IF-CLASSIFICATION2-NEXT: print("c")
85+
// ELSE-IF-CLASSIFICATION2-NEXT: }
86+
// ELSE-IF-CLASSIFICATION2-NEXT: } catch let err {
87+
// ELSE-IF-CLASSIFICATION2-NEXT: if .random() {
88+
// ELSE-IF-CLASSIFICATION2-NEXT: print("b")
89+
// ELSE-IF-CLASSIFICATION2-NEXT: }
90+
// ELSE-IF-CLASSIFICATION2-NEXT: if <#err#> != nil, .random() {
91+
// ELSE-IF-CLASSIFICATION2-NEXT: print("d")
92+
// ELSE-IF-CLASSIFICATION2-NEXT: }
93+
// ELSE-IF-CLASSIFICATION2-NEXT: }
94+
95+
// FIXME: This case is a little tricky: Being in the else block of 'err == nil'
96+
// should allow us to place 'if let str = str' in the failure block. However, this
97+
// is a success condition, so we still place it in the success block. Really what
98+
// we need to do here is check to see if simpleWithError has an existing async
99+
// alternative that still returns optional success values, and allow success
100+
// classification in that case. Otherwise, we'd probably be better off leaving
101+
// the condition unhandled, as it's not clear what the user is doing.
102+
// RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=ELSE-IF-CLASSIFICATION3 %s
103+
simpleWithError { str, err in
104+
if err == nil {
105+
print("a")
106+
} else if let str = str {
107+
print("b")
108+
} else {
109+
print("c")
110+
}
111+
}
112+
113+
// ELSE-IF-CLASSIFICATION3: do {
114+
// ELSE-IF-CLASSIFICATION3-NEXT: let str = try await simpleWithError()
115+
// ELSE-IF-CLASSIFICATION3-NEXT: print("a")
116+
// ELSE-IF-CLASSIFICATION3-NEXT: print("b")
117+
// ELSE-IF-CLASSIFICATION3-NEXT: } catch let err {
118+
// ELSE-IF-CLASSIFICATION3-NEXT: print("c")
119+
// ELSE-IF-CLASSIFICATION3-NEXT: }
120+
121+
// FIXME: Similar to the case above.
122+
// RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=ELSE-IF-CLASSIFICATION4 %s
123+
simpleWithError { str, err in
124+
if err == nil {
125+
print("a")
126+
} else if let str = str {
127+
print("b")
128+
} else if .random() {
129+
print("c")
130+
} else {
131+
print("d")
132+
}
133+
}
134+
135+
// ELSE-IF-CLASSIFICATION4: do {
136+
// ELSE-IF-CLASSIFICATION4-NEXT: let str = try await simpleWithError()
137+
// ELSE-IF-CLASSIFICATION4-NEXT: print("a")
138+
// ELSE-IF-CLASSIFICATION4-NEXT: print("b")
139+
// ELSE-IF-CLASSIFICATION4-NEXT: } catch let err {
140+
// ELSE-IF-CLASSIFICATION4-NEXT: if .random() {
141+
// ELSE-IF-CLASSIFICATION4-NEXT: print("c")
142+
// ELSE-IF-CLASSIFICATION4-NEXT: } else {
143+
// ELSE-IF-CLASSIFICATION4-NEXT: print("d")
144+
// ELSE-IF-CLASSIFICATION4-NEXT: }
145+
// ELSE-IF-CLASSIFICATION4-NEXT: }
146+
147+
// RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=ELSE-IF-CLASSIFICATION5 %s
148+
simpleWithError { str, err in
149+
if let err = err {
150+
print("a")
151+
} else if let str = str {
152+
print("b")
153+
return
154+
} else if .random() {
155+
print("c")
156+
} else {
157+
print("d")
158+
}
159+
}
160+
161+
// ELSE-IF-CLASSIFICATION5: do {
162+
// ELSE-IF-CLASSIFICATION5-NEXT: let str = try await simpleWithError()
163+
// ELSE-IF-CLASSIFICATION5-NEXT: print("b")
164+
// ELSE-IF-CLASSIFICATION5-NEXT: } catch let err {
165+
// ELSE-IF-CLASSIFICATION5-NEXT: print("a")
166+
// ELSE-IF-CLASSIFICATION5-NEXT: if .random() {
167+
// ELSE-IF-CLASSIFICATION5-NEXT: print("c")
168+
// ELSE-IF-CLASSIFICATION5-NEXT: } else {
169+
// ELSE-IF-CLASSIFICATION5-NEXT: print("d")
170+
// ELSE-IF-CLASSIFICATION5-NEXT: }
171+
// ELSE-IF-CLASSIFICATION5-NEXT: }
172+
173+
// RN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=IF-LET-RETURN-CLASSIFICATION %s
174+
simpleWithError { str, err in
175+
if let str = str {
176+
print("a")
177+
return
178+
}
179+
if .random(), let err = err {
180+
print("b")
181+
} else {
182+
print("c")
183+
}
184+
}
185+
186+
// IF-LET-RETURN-CLASSIFICATION-NEXT: do {
187+
// IF-LET-RETURN-CLASSIFICATION-NEXT: let str = try await simpleWithError()
188+
// IF-LET-RETURN-CLASSIFICATION-NEXT: print("a")
189+
// IF-LET-RETURN-CLASSIFICATION-NEXT: } catch let err {
190+
// IF-LET-RETURN-CLASSIFICATION-NEXT: if .random(), let err = <#err#> {
191+
// IF-LET-RETURN-CLASSIFICATION-NEXT: print("b")
192+
// IF-LET-RETURN-CLASSIFICATION-NEXT: } else {
193+
// IF-LET-RETURN-CLASSIFICATION-NEXT: print("c")
194+
// IF-LET-RETURN-CLASSIFICATION-NEXT: }
195+
// IF-LET-RETURN-CLASSIFICATION-NEXT: }
196+
197+
// RN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=GUARD-CLASSIFICATION %s
198+
simpleWithError { str, err in
199+
guard let str = str else {
200+
print("a")
201+
return
202+
}
203+
guard err == nil, .random() else {
204+
print("b")
205+
return
206+
}
207+
print("c")
208+
}
209+
210+
// GUARD-CLASSIFICATION: do {
211+
// GUARD-CLASSIFICATION-NEXT: let str = try await simpleWithError()
212+
// GUARD-CLASSIFICATION-NEXT: guard <#err#> == nil, .random() else {
213+
// GUARD-CLASSIFICATION-NEXT: print("b")
214+
// GUARD-CLASSIFICATION-NEXT: <#return#>
215+
// GUARD-CLASSIFICATION-NEXT: }
216+
// GUARD-CLASSIFICATION-NEXT: print("c")
217+
// GUARD-CLASSIFICATION-NEXT: } catch let err {
218+
// GUARD-CLASSIFICATION-NEXT: print("a")
219+
// GUARD-CLASSIFICATION-NEXT: }
220+
221+
// RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=SILLY-CLASSIFICATION %s
222+
simpleWithError { str, err in
223+
guard let str = str else { return }
224+
guard let err = err else { return }
225+
print("urr")
226+
}
227+
228+
// In this case we just take whichever guard is last.
229+
// SILLY-CLASSIFICATION: do {
230+
// SILLY-CLASSIFICATION-NEXT: let str = try await simpleWithError()
231+
// SILLY-CLASSIFICATION-NEXT: } catch let err {
232+
// SILLY-CLASSIFICATION-NEXT: print("urr")
233+
// SILLY-CLASSIFICATION-NEXT: }
234+
}

0 commit comments

Comments
 (0)