[Diagnostic] Improve diagnostic for trailing closures in statement conditions

This improves the diagnostic for trailing closures in statement conditions to catch cases where one or more trailing closures are used in a chain composed of multiple calls

Author: owenv

lib/Parse/ParseExpr.cpp

@@ -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;
+  }
 }
 
 

lib/Sema/MiscDiagnostics.cpp

@@ -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;
 
@@ -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;
+      }
+      return {true, E};
     }
   };
 
@@ -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() {}`.
     }

test/expr/closure/trailing.swift

@@ -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? {}
@@ -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) {}