Skip to content

[Diagnostic] Improve diagnostic for trailing closures in statement conditions #25165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 28, 2019
Merged

[Diagnostic] Improve diagnostic for trailing closures in statement conditions #25165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions lib/Parse/ParseExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,23 +1045,40 @@ static bool isValidTrailingClosure(bool isExprBasic, Parser &P){
// the token after the { is on the same line as the {.
if (P.peekToken().isAtStartOfLine())
return false;



// Determine if the {} goes with the expression by eating it, and looking
// to see if it is immediately followed by '{', 'where', or comma. If so,
// we consider it to be part of the proceeding expression.
// to see if it is immediately followed by a token which indicates we should
// consider it part of the preceding expression
Parser::BacktrackingScope backtrack(P);
P.consumeToken(tok::l_brace);
P.skipUntil(tok::r_brace);
SourceLoc endLoc;
if (!P.consumeIf(tok::r_brace, endLoc) ||
P.Tok.isNot(tok::l_brace, tok::kw_where, tok::comma)) {
if (!P.consumeIf(tok::r_brace, endLoc))
return false;
}

// Recoverable case. Just return true here and Sema will emit a diagnostic
// later. see: Sema/MiscDiagnostics.cpp#checkStmtConditionTrailingClosure
return true;
switch (P.Tok.getKind()) {
case tok::l_brace:
case tok::kw_where:
case tok::comma:
return true;
case tok::l_square:
case tok::l_paren:
case tok::period:
case tok::period_prefix:
case tok::kw_is:
case tok::kw_as:
case tok::question_postfix:
case tok::question_infix:
case tok::exclaim_postfix:
case tok::colon:
case tok::equal:
case tok::oper_postfix:
case tok::oper_binary_spaced:
case tok::oper_binary_unspaced:
return !P.Tok.isAtStartOfLine();
default:
return false;
}
}


Expand Down
32 changes: 22 additions & 10 deletions lib/Sema/MiscDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2957,7 +2957,7 @@ void swift::fixItEncloseTrailingClosure(TypeChecker &TC,
static void checkStmtConditionTrailingClosure(TypeChecker &TC, const Expr *E) {
if (E == nullptr || isa<ErrorExpr>(E)) return;

// Shallow walker. just dig into implicit expression.
// Walk into expressions which might have invalid trailing closures
class DiagnoseWalker : public ASTWalker {
TypeChecker &TC;

Expand Down Expand Up @@ -2992,13 +2992,22 @@ static void checkStmtConditionTrailingClosure(TypeChecker &TC, const Expr *E) {
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }

std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
// Dig into implicit expression.
if (E->isImplicit()) return { true, E };
// Diagnose call expression.
if (auto CE = dyn_cast<CallExpr>(E))
diagnoseIt(CE);
// Don't dig any further.
return { false, E };
switch (E->getKind()) {
case ExprKind::Paren:
case ExprKind::Tuple:
case ExprKind::Array:
case ExprKind::Dictionary:
case ExprKind::InterpolatedStringLiteral:
// If a trailing closure appears as a child of one of these types of
// expression, don't diagnose it as there is no ambiguity.
return {E->isImplicit(), E};
case ExprKind::Call:
diagnoseIt(cast<CallExpr>(E));
break;
default:
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To handle binary expressions et al., I think we should maintain "not walk into" list, instead of walk into specific expression kinds.
I mean, currently we walk into only CallExpr (and DotSyntaxCallExpr). But I think we should walk into except ParenExpr, TupleExpr, ArrayExpr, DictionaryExpr, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up adopting a solution like this, my one concern is that a future new expression kind might cause a false positive to be diagnosed if it isn't included in the 'skipped' list. This should be mitigated by the fact that isValidTrailingClosure() still whitelists the individual tokens which may follow a closing brace for something to be considered a possibly valid trailing closure. It doesn't completely eliminate the possibility of future breakage though.

Copy link
Member

@rintaro rintaro Jun 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK. False positive is better than false negative in this case. The user can easily workaround it by not using trailing closure (or enclose whole expression into parentheses).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…as long as we don't break source compat.

return {true, E};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is a change in behavior; is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The intent is to always walk into an expression, unless it's a non-implicit parent/tuple/etc. expression.

}
};

Expand All @@ -3018,9 +3027,12 @@ static void checkStmtConditionTrailingClosure(TypeChecker &TC, const Expr *E) {
static void checkStmtConditionTrailingClosure(TypeChecker &TC, const Stmt *S) {
if (auto LCS = dyn_cast<LabeledConditionalStmt>(S)) {
for (auto elt : LCS->getCond()) {
if (elt.getKind() == StmtConditionElement::CK_PatternBinding)
if (elt.getKind() == StmtConditionElement::CK_PatternBinding) {
checkStmtConditionTrailingClosure(TC, elt.getInitializer());
else if (elt.getKind() == StmtConditionElement::CK_Boolean)
if (auto *exprPattern = dyn_cast<ExprPattern>(elt.getPattern())) {
checkStmtConditionTrailingClosure(TC, exprPattern->getMatchExpr());
}
} else if (elt.getKind() == StmtConditionElement::CK_Boolean)
checkStmtConditionTrailingClosure(TC, elt.getBoolean());
// No trailing closure for CK_Availability: e.g. `if #available() {}`.
}
Expand Down
78 changes: 77 additions & 1 deletion test/expr/closure/trailing.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ limitXY(someInt, toGamut: intArray) {} // expected-error{{cannot invoke 'limitX
// <rdar://problem/23036383> QoI: Invalid trailing closures in stmt-conditions produce lowsy diagnostics
func retBool(x: () -> Int) -> Bool {}
func maybeInt(_: () -> Int) -> Int? {}
func twoClosureArgs(_:()->Void, _:()->Void) -> Bool {}
class Foo23036383 {
init() {}
func map(_: (Int) -> Int) -> Int? {}
Expand Down Expand Up @@ -202,7 +203,82 @@ func r23036383(foo: Foo23036383?, obj: Foo23036383) {

if let _ = maybeInt { 1 }, retBool { 1 } { }
// expected-error@-1 {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{28-28=)}}
// expected-error@-2 {{trailing closure requires parentheses for disambiguation in this context}} {{37-38=(x: }} {{43-43=)}}
// expected-error@-2 {{trailing closure requires parentheses for disambiguation in this context}} {{37-38=(x: }} {{43-43=)}}

if let _ = foo?.map {$0+1}.map {$0+1} {} // expected-error {{trailing closure requires parentheses for disambiguation in this context}} {{33-34=(}} {{40-40=)}}
// expected-error@-1 {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{29-29=)}}

if let _ = foo?.map {$0+1}.map({$0+1}) {} // expected-error {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{29-29=)}}

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=)}}
// expected-error@-1 {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{29-29=)}}

if twoClosureArgs({}) {} {} // expected-error {{trailing closure requires parentheses for disambiguation in this context}} {{23-25=, }} {{27-27=)}}

if let _ = (foo?.map {$0+1}.map({$0+1}).map{$0+1}) {} // OK

if let _ = (foo?.map {$0+1}.map({$0+1})).map({$0+1}) {} // OK
}

func id<T>(fn: () -> T) -> T { return fn() }
func any<T>(fn: () -> T) -> Any { return fn() }

func testSR8736() {
if !id { true } { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if id { true } == true { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if true == id { true } { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if id { true } ? true : false { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if true ? id { true } : false { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if true ? true : id { false } { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if id { [false,true] }[0] { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if id { { true } } () { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if any { true } as! Bool { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if let _ = any { "test" } as? Int { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if any { "test" } is Int { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if let _ = id { [] as [Int]? }?.first { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if id { true as Bool? }! { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if case id { 1 } = 1 { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if case 1 = id { 1 } { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if case 1 = id { 1 } /*comment*/ { return } // expected-error {{trailing closure requires parentheses for disambiguation in this context}}

if case (id { 1 }) = 1 { return } // OK

if case 1 = (id { 1 }) { return } // OK

if [id { true }].count == 0 { return } // OK

if [id { true } : "test"].keys.count == 0 { return } // OK

if "\(id { true })" == "foo" { return } // OK

if (id { true }) { return } // OK

if (id { true }) { }
[1, 2, 3].count // expected-warning {{expression of type 'Int' is unused}}

if true { }
() // OK

if true
{

}
() // OK
}

func overloadOnLabel(a: () -> Void) {}
Expand Down