Skip to content

Commit a3946cd

Browse files
owenvowenv
committed
Revert "Revert "[Diagnostic] Improve diagnostic for trailing closures in statement conditions (swiftlang#25165)""
This reverts commit e3a6b67.
1 parent cd3ada5 commit a3946cd

File tree

3 files changed

+127
-21
lines changed

3 files changed

+127
-21
lines changed

‎lib/Parse/ParseExpr.cpp

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -965,23 +965,40 @@ static bool isValidTrailingClosure(bool isExprBasic, Parser &P){
965965
// the token after the { is on the same line as the {.
966966
if (P.peekToken().isAtStartOfLine())
967967
return false;
968-
969-
968+
970969
// Determine if the {} goes with the expression by eating it, and looking
971-
// to see if it is immediately followed by '{', 'where', or comma. If so,
972-
// we consider it to be part of the proceeding expression.
970+
// to see if it is immediately followed by a token which indicates we should
971+
// consider it part of the preceding expression
973972
Parser::BacktrackingScope backtrack(P);
974973
P.consumeToken(tok::l_brace);
975974
P.skipUntil(tok::r_brace);
976975
SourceLoc endLoc;
977-
if (!P.consumeIf(tok::r_brace, endLoc) ||
978-
P.Tok.isNot(tok::l_brace, tok::kw_where, tok::comma)) {
976+
if (!P.consumeIf(tok::r_brace, endLoc))
979977
return false;
980-
}
981978

982-
// Recoverable case. Just return true here and Sema will emit a diagnostic
983-
// later. see: Sema/MiscDiagnostics.cpp#checkStmtConditionTrailingClosure
984-
return true;
979+
switch (P.Tok.getKind()) {
980+
case tok::l_brace:
981+
case tok::kw_where:
982+
case tok::comma:
983+
return true;
984+
case tok::l_square:
985+
case tok::l_paren:
986+
case tok::period:
987+
case tok::period_prefix:
988+
case tok::kw_is:
989+
case tok::kw_as:
990+
case tok::question_postfix:
991+
case tok::question_infix:
992+
case tok::exclaim_postfix:
993+
case tok::colon:
994+
case tok::equal:
995+
case tok::oper_postfix:
996+
case tok::oper_binary_spaced:
997+
case tok::oper_binary_unspaced:
998+
return !P.Tok.isAtStartOfLine();
999+
default:
1000+
return false;
1001+
}
9851002
}
9861003

9871004

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3027,7 +3027,7 @@ void swift::fixItEncloseTrailingClosure(ASTContext &ctx,
30273027
static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
30283028
if (E == nullptr || isa<ErrorExpr>(E)) return;
30293029

3030-
// Shallow walker. just dig into implicit expression.
3030+
// Walk into expressions which might have invalid trailing closures
30313031
class DiagnoseWalker : public ASTWalker {
30323032
ASTContext &Ctx;
30333033

@@ -3062,13 +3062,22 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
30623062
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
30633063

30643064
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
3065-
// Dig into implicit expression.
3066-
if (E->isImplicit()) return { true, E };
3067-
// Diagnose call expression.
3068-
if (auto CE = dyn_cast<CallExpr>(E))
3069-
diagnoseIt(CE);
3070-
// Don't dig any further.
3071-
return { false, E };
3065+
switch (E->getKind()) {
3066+
case ExprKind::Paren:
3067+
case ExprKind::Tuple:
3068+
case ExprKind::Array:
3069+
case ExprKind::Dictionary:
3070+
case ExprKind::InterpolatedStringLiteral:
3071+
// If a trailing closure appears as a child of one of these types of
3072+
// expression, don't diagnose it as there is no ambiguity.
3073+
return {E->isImplicit(), E};
3074+
case ExprKind::Call:
3075+
diagnoseIt(cast<CallExpr>(E));
3076+
break;
3077+
default:
3078+
break;
3079+
}
3080+
return {true, E};
30723081
}
30733082
};
30743083

@@ -3088,9 +3097,13 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
30883097
static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Stmt *S) {
30893098
if (auto LCS = dyn_cast<LabeledConditionalStmt>(S)) {
30903099
for (auto elt : LCS->getCond()) {
3091-
if (elt.getKind() == StmtConditionElement::CK_PatternBinding)
3100+
3101+
if (elt.getKind() == StmtConditionElement::CK_PatternBinding) {
30923102
checkStmtConditionTrailingClosure(ctx, elt.getInitializer());
3093-
else if (elt.getKind() == StmtConditionElement::CK_Boolean)
3103+
if (auto *exprPattern = dyn_cast<ExprPattern>(elt.getPattern())) {
3104+
checkStmtConditionTrailingClosure(ctx, exprPattern->getMatchExpr());
3105+
}
3106+
} else if (elt.getKind() == StmtConditionElement::CK_Boolean)
30943107
checkStmtConditionTrailingClosure(ctx, elt.getBoolean());
30953108
// No trailing closure for CK_Availability: e.g. `if #available() {}`.
30963109
}

‎test/expr/closure/trailing.swift

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ limitXY(someInt, toGamut: intArray) {} // expected-error{{extra trailing closur
137137
// <rdar://problem/23036383> QoI: Invalid trailing closures in stmt-conditions produce lowsy diagnostics
138138
func retBool(x: () -> Int) -> Bool {}
139139
func maybeInt(_: () -> Int) -> Int? {}
140+
func twoClosureArgs(_:()->Void, _:()->Void) -> Bool {}
140141
class Foo23036383 {
141142
init() {}
142143
func map(_: (Int) -> Int) -> Int? {}
@@ -201,7 +202,82 @@ func r23036383(foo: Foo23036383?, obj: Foo23036383) {
201202

202203
if let _ = maybeInt { 1 }, retBool { 1 } { }
203204
// expected-error@-1 {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{28-28=)}}
204-
// expected-error@-2 {{trailing closure requires parentheses for disambiguation in this context}} {{37-38=(x: }} {{43-43=)}}
205+
// expected-error@-2 {{trailing closure requires parentheses for disambiguation in this context}} {{37-38=(x: }} {{43-43=)}}
206+
207+
if let _ = foo?.map {$0+1}.map {$0+1} {} // expected-error {{trailing closure requires parentheses for disambiguation in this context}} {{33-34=(}} {{40-40=)}}
208+
// expected-error@-1 {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{29-29=)}}
209+
210+
if let _ = foo?.map {$0+1}.map({$0+1}) {} // expected-error {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{29-29=)}}
211+
212+
if let _ = foo?.map {$0+1}.map({$0+1}).map{$0+1} {} // expected-error {{trailing closure requires parentheses for disambiguation in this context}} {{45-45=(}} {{51-51=)}}
213+
// expected-error@-1 {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{29-29=)}}
214+
215+
if twoClosureArgs({}) {} {} // expected-error {{trailing closure requires parentheses for disambiguation in this context}} {{23-25=, }} {{27-27=)}}
216+
217+
if let _ = (foo?.map {$0+1}.map({$0+1}).map{$0+1}) {} // OK
218+
219+
if let _ = (foo?.map {$0+1}.map({$0+1})).map({$0+1}) {} // OK
220+
}
221+
222+
func id<T>(fn: () -> T) -> T { return fn() }
223+
func any<T>(fn: () -> T) -> Any { return fn() }
224+
225+
func testSR8736() {
226+
if !id { true } { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
227+
228+
if id { true } == true { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
229+
230+
if true == id { true } { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
231+
232+
if id { true } ? true : false { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
233+
234+
if true ? id { true } : false { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
235+
236+
if true ? true : id { false } { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
237+
238+
if id { [false,true] }[0] { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
239+
240+
if id { { true } } () { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
241+
242+
if any { true } as! Bool { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
243+
244+
if let _ = any { "test" } as? Int { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
245+
246+
if any { "test" } is Int { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
247+
248+
if let _ = id { [] as [Int]? }?.first { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
249+
250+
if id { true as Bool? }! { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
251+
252+
if case id { 1 } = 1 { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
253+
254+
if case 1 = id { 1 } { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
255+
256+
if case 1 = id { 1 } /*comment*/ { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}
257+
258+
if case (id { 1 }) = 1 { return } // OK
259+
260+
if case 1 = (id { 1 }) { return } // OK
261+
262+
if [id { true }].count == 0 { return } // OK
263+
264+
if [id { true } : "test"].keys.count == 0 { return } // OK
265+
266+
if "\(id { true })" == "foo" { return } // OK
267+
268+
if (id { true }) { return } // OK
269+
270+
if (id { true }) { }
271+
[1, 2, 3].count // expected-warning {{expression of type 'Int' is unused}}
272+
273+
if true { }
274+
() // OK
275+
276+
if true
277+
{
278+
279+
}
280+
() // OK
205281
}
206282

207283
func overloadOnLabel(a: () -> Void) {}

0 commit comments

Comments
 (0)