[QoI] Improvements to function call & closure diagnostics (#7224)

Author: jtbandes

include/swift/AST/DiagnosticsCommon.def

@@ -52,6 +52,8 @@ NOTE(previous_decldef,none,
      "previous %select{declaration|definition}0 of %1 is here",
      (bool, Identifier))
 
+NOTE(brace_stmt_suggest_do,none,
+     "did you mean to use a 'do' statement?", ())
 
 // Generic disambiguation
 NOTE(while_parsing_as_left_angle_bracket,none,

include/swift/AST/DiagnosticsParse.def

@@ -806,17 +806,12 @@ ERROR(illegal_top_level_expr,none,
 ERROR(illegal_semi_stmt,none,
       "';' statements are not allowed", ())
 ERROR(statement_begins_with_closure,none,
-      "statement cannot begin with a closure expression", ())
+      "top-level statement cannot begin with a closure expression", ())
 ERROR(statement_same_line_without_semi,none,
       "consecutive statements on a line must be separated by ';'", ())
-ERROR(brace_stmt_invalid,none,
-      "braced block of statements is an unused closure", ())
 ERROR(invalid_label_on_stmt,none,
       "labels are only valid on loops, if, and switch statements", ())
 
-NOTE(discard_result_of_closure,none,
-     "explicitly discard the result of the closure by assigning to '_'", ())
-
 ERROR(snake_case_deprecated,none,
       "%0 has been replaced with %1 in Swift 3",
       (StringRef, StringRef))
@@ -1065,10 +1060,10 @@ ERROR(unexpected_tokens_before_closure_in,none,
 ERROR(expected_closure_rbrace,none,
       "expected '}' at end of closure", ())
 
-WARNING(trailing_closure_excess_newlines,none,
-        "trailing closure is separated from call site by multiple newlines", ())
-NOTE(trailing_closure_call_here,none,
-     "parsing trailing closure for this call", ())
+WARNING(trailing_closure_after_newlines,none,
+        "braces here form a trailing closure separated from its callee by multiple newlines", ())
+NOTE(trailing_closure_callee_here,none,
+     "callee is here", ())
 
 ERROR(string_literal_no_atsign,none,
       "string literals in Swift are not preceded by an '@' sign", ())

include/swift/AST/DiagnosticsSema.def

@@ -231,9 +231,6 @@ ERROR(cannot_call_with_params, none,
 NOTE(pointer_init_to_type,none,
       "Pointer conversion restricted: use '.assumingMemoryBound(to:)' or '.bindMemory(to:capacity:)' to view memory as a type.", ())
 
-ERROR(expected_do_in_statement,none,
-      "expected 'do' keyword to designate a block of statements", ())
-
 ERROR(cannot_call_non_function_value,none,
       "cannot call value of non-function type %0", (Type))
 
@@ -2590,6 +2587,8 @@ WARNING(guard_always_succeeds,none,
         "'guard' condition is always true, body is unreachable", ())
 
 
+ERROR(expression_unused_closure,none,
+      "closure expression is unused", ())
 ERROR(expression_unused_function,none,
       "expression resolves to an unused function", ())
 ERROR(expression_unused_lvalue,none,

lib/Parse/ParseExpr.cpp

@@ -2645,13 +2645,21 @@ ParserResult<Expr> Parser::parseTrailingClosure(SourceRange calleeRange) {
   if (closure.isNull())
     return makeParserError();
 
-  // Track the original end location of the expression we're trailing so
-  // we can warn about excess newlines.
-  auto origLineCol = SourceMgr.getLineAndColumn(calleeRange.End);
-  auto braceLineCol = SourceMgr.getLineAndColumn(braceLoc);
-  if (((int)braceLineCol.first - (int)origLineCol.first) > 1) {
-    diagnose(braceLoc, diag::trailing_closure_excess_newlines);
-    diagnose(calleeRange.Start, diag::trailing_closure_call_here);
+  // Warn if the trailing closure is separated from its callee by more than
+  // one line. A single-line separation is acceptable for a trailing closure
+  // call, and will be diagnosed later only if the call fails to typecheck.
+  auto origLine = SourceMgr.getLineNumber(calleeRange.End);
+  auto braceLine = SourceMgr.getLineNumber(braceLoc);
+  if (braceLine > origLine + 1) {
+    diagnose(braceLoc, diag::trailing_closure_after_newlines);
+    diagnose(calleeRange.Start, diag::trailing_closure_callee_here);
+    
+    auto *CE = dyn_cast<ClosureExpr>(closure.get());
+    if (CE && CE->hasAnonymousClosureVars() &&
+        CE->getParameters()->size() == 0) {
+      diagnose(braceLoc, diag::brace_stmt_suggest_do)
+        .fixItInsert(braceLoc, "do ");
+    }
   }
 
   return closure;

lib/Parse/ParseStmt.cpp

@@ -189,27 +189,6 @@ void Parser::consumeTopLevelDecl(ParserPosition BeginParserPosition,
   skipUntil(tok::eof);
 }
 
-static void diagnoseDiscardedClosure(Parser &P, ASTNode &Result) {
-  // If we parsed a bare closure as an expression, it will be a discarded value
-  // expression and the type checker will complain.
-
-  if (isa<AbstractClosureExpr>(P.CurDeclContext))
-    // Inside a closure expression, an expression which syntactically looks
-    // like a discarded value expression, can become the return value of the
-    // closure.  Don't attempt recovery.
-    return;
-
-  if (auto *E = Result.dyn_cast<Expr *>()) {
-    if (auto *CE = dyn_cast<ClosureExpr>(E)) {
-      if (!CE->hasAnonymousClosureVars())
-        // Parameters are explicitly specified, and could be used in the body,
-        // don't attempt recovery.
-        return;
-      P.diagnose(CE->getBody()->getLBraceLoc(), diag::brace_stmt_invalid);
-    }
-  }
-}
-
 ///   brace-item:
 ///     decl
 ///     expr
@@ -369,8 +348,6 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
       // This prevents potential ambiguities with trailing closure syntax.
       if (Tok.is(tok::l_brace)) {
         diagnose(Tok, diag::statement_begins_with_closure);
-        diagnose(Tok, diag::discard_result_of_closure)
-          .fixItInsert(Tok.getLoc(), "_ = ");
       }
 
       ParserStatus Status = parseExprOrStmt(Result);
@@ -388,7 +365,7 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
                  Result.is<Stmt*>() ? diag::illegal_top_level_stmt
                                     : diag::illegal_top_level_expr);
       }
-      diagnoseDiscardedClosure(*this, Result);
+
       if (!Result.isNull()) {
         // NOTE: this is a 'virtual' brace statement which does not have
         //       explicit '{' or '}', so the start and end locations should be
@@ -411,7 +388,6 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
       BraceItemsStatus |= ExprOrStmtStatus;
       if (ExprOrStmtStatus.isError())
         NeedParseErrorRecovery = true;
-      diagnoseDiscardedClosure(*this, Result);
       if (!Result.isNull())
         Entries.push_back(Result);
     }
@@ -484,8 +460,6 @@ void Parser::parseTopLevelCodeDeclDelayed() {
   // prevents potential ambiguities with trailing closure syntax.
   if (Tok.is(tok::l_brace)) {
     diagnose(Tok, diag::statement_begins_with_closure);
-    diagnose(Tok, diag::discard_result_of_closure)
-      .fixItInsert(Tok.getLoc(), "_ = ");
   }
 
   parseExprOrStmt(Result);

lib/Sema/CSDiag.cpp

@@ -5861,34 +5861,41 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
   if (!isUnresolvedOrTypeVarType(fnType) &&
       !fnType->is<AnyFunctionType>() && !fnType->is<MetatypeType>()) {
 
-    // If the argument is a trailing ClosureExpr (i.e. {....}) and it is on a
-    // different line than the callee, then the "real" issue is that the user
-    // forgot to write "do" before their brace stmt.
-    if (auto *PE = dyn_cast<ParenExpr>(callExpr->getArg()))
+    auto arg = callExpr->getArg();
+
+    {
+      auto diag = diagnose(arg->getStartLoc(),
+                           diag::cannot_call_non_function_value,
+                           fnExpr->getType());
+      diag.highlight(fnExpr->getSourceRange());
+
+      // If the argument is an empty tuple, then offer a
+      // fix-it to remove the empty tuple and use the value
+      // directly.
+      if (auto tuple = dyn_cast<TupleExpr>(arg)) {
+        if (tuple->getNumElements() == 0) {
+          diag.fixItRemove(arg->getSourceRange());
+        }
+      }
+    }
+
+    // If the argument is a trailing ClosureExpr (i.e. {....}) and it is on
+    // the line after the callee, then it's likely the user forgot to
+    // write "do" before their brace stmt.
+    // Note that line differences of more than 1 are diagnosed during parsing.
+    if (auto *PE = dyn_cast<ParenExpr>(arg))
       if (PE->hasTrailingClosure() && isa<ClosureExpr>(PE->getSubExpr())) {
+        auto *closure = cast<ClosureExpr>(PE->getSubExpr());
         auto &SM = CS->getASTContext().SourceMgr;
-        if (SM.getLineNumber(callExpr->getFn()->getEndLoc()) !=
-            SM.getLineNumber(PE->getStartLoc())) {
-          diagnose(PE->getStartLoc(), diag::expected_do_in_statement)
-            .fixItInsert(PE->getStartLoc(), "do ");
-          return true;
+        if (closure->hasAnonymousClosureVars() &&
+            closure->getParameters()->size() == 0 &&
+            1 + SM.getLineNumber(callExpr->getFn()->getEndLoc()) ==
+            SM.getLineNumber(closure->getStartLoc())) {
+          diagnose(closure->getStartLoc(), diag::brace_stmt_suggest_do)
+            .fixItInsert(closure->getStartLoc(), "do ");
         }
       }
-    
-    auto arg = callExpr->getArg();
-    auto diag = diagnose(arg->getStartLoc(),
-                         diag::cannot_call_non_function_value,
-                         fnExpr->getType());
-    diag.highlight(fnExpr->getSourceRange());
-    
-    // If the argument is an empty tuple, then offer a
-    // fix-it to remove the empty tuple and use the value
-    // directly.
-    if (auto tuple = dyn_cast<TupleExpr>(arg)) {
-      if (tuple->getNumElements() == 0) {
-        diag.fixItRemove(arg->getSourceRange());
-      }
-    }
+
     return true;
   }
 

lib/Sema/TypeCheckStmt.cpp

@@ -1248,8 +1248,21 @@ Stmt *StmtChecker::visitBraceStmt(BraceStmt *BS) {
 
       bool hadTypeError = TC.typeCheckExpression(SubExpr, DC, TypeLoc(),
                                                  CTP_Unused, options);
-      if (isDiscarded && !hadTypeError)
+
+      // If a closure expression is unused, the user might have intended
+      // to write "do { ... }".
+      auto *CE = dyn_cast<ClosureExpr>(SubExpr);
+      if (CE || isa<CaptureListExpr>(SubExpr)) {
+        TC.diagnose(SubExpr->getLoc(), diag::expression_unused_closure);
+        
+        if (CE && CE->hasAnonymousClosureVars() &&
+            CE->getParameters()->size() == 0) {
+          TC.diagnose(CE->getStartLoc(), diag::brace_stmt_suggest_do)
+            .fixItInsert(CE->getStartLoc(), "do ");
+        }
+      } else if (isDiscarded && !hadTypeError)
         TC.checkIgnoredExpr(SubExpr);
+
       elem = SubExpr;
       continue;
     }

test/Constraints/closures.swift

@@ -66,24 +66,88 @@ func test13811882() {
 // <rdar://problem/21544303> QoI: "Unexpected trailing closure" should have a fixit to insert a 'do' statement
 // <https://bugs.swift.org/browse/SR-3671>
 func r21544303() {
-  let val = true
-  var inSubcall = val
-  {   // expected-error {{expected 'do' keyword to designate a block of statements}} {{3-3=do }}
-  }
+  var inSubcall = true
+  {
+  }  // expected-error {{computed property must have accessors specified}}
   inSubcall = false
 
   // This is a problem, but isn't clear what was intended.
-  var somethingElse = val { // expected-error {{cannot call value of non-function type 'Bool'}}
-  }
+  var somethingElse = true {
+  }  // expected-error {{computed property must have accessors specified}}
   inSubcall = false
 
   var v2 : Bool = false
-  v2 = val
-  {  // expected-error {{expected 'do' keyword to designate a block of statements}}
+  v2 = inSubcall
+  {  // expected-error {{cannot call value of non-function type 'Bool'}} expected-note {{did you mean to use a 'do' statement?}} {{3-3=do }}
   }
 }
 
 
+// <https://bugs.swift.org/browse/SR-3671>
+func SR3671() {
+  let n = 42
+  func consume(_ x: Int) {}
+
+  { consume($0) }(42)
+  ;
+
+  ({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
+
+  { print(42) }  // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-note {{did you mean to use a 'do' statement?}} {{3-3=do }} expected-error {{cannot call value of non-function type '()'}}
+  ;
+
+  ({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
+
+  { print($0) }  // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}}
+  ;
+
+  ({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
+
+  { [n] in print(42) }  // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}}
+  ;
+
+  ({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
+
+  { consume($0) }(42)  // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}}
+  ;
+
+  ({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
+
+  { (x: Int) in consume(x) }(42)  // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}}
+  ;
+
+  // This is technically a valid call, so nothing goes wrong until (42)
+
+  { $0(3) }
+  { consume($0) }(42)  // expected-error {{cannot call value of non-function type '()'}}
+  ;
+  ({ $0(42) })
+  { consume($0) }(42)  // expected-error {{cannot call value of non-function type '()'}}
+  ;
+  { $0(3) }
+  { [n] in consume($0) }(42)  // expected-error {{cannot call value of non-function type '()'}}
+  ;
+  ({ $0(42) })
+  { [n] in consume($0) }(42)  // expected-error {{cannot call value of non-function type '()'}}
+  ;
+
+  // Equivalent but more obviously unintended.
+
+  { $0(3) }  // expected-note {{callee is here}}
+
+  { consume($0) }(42)  // expected-error {{cannot call value of non-function type '()'}}
+  // expected-warning@-1 {{braces here form a trailing closure separated from its callee by multiple newlines}}
+
+  ({ $0(3) })  // expected-note {{callee is here}}
+
+  { consume($0) }(42)  // expected-error {{cannot call value of non-function type '()'}}
+  // expected-warning@-1 {{braces here form a trailing closure separated from its callee by multiple newlines}}
+  ;
+
+  // Also a valid call (!!)
+  { $0 { $0 } } { $0 { 1 } }  // expected-error {{expression resolves to an unused function}}
+  consume(111)
+}
 
 
 // <rdar://problem/22162441> Crash from failing to diagnose nonexistent method access inside closure

test/Parse/availability_query.swift

@@ -21,7 +21,7 @@ if #available(OSX 10.51, *) && #available(OSX 10.52, *) { // expected-error {{ex
 }
 
 
-if #available { // expected-error {{expected availability condition}} expected-error {{braced block of statements is an unused closure}} expected-error {{statement cannot begin with a closure expression}} expected-note {{explicitly discard the result of the closure by assigning to '_'}} {{15-15=_ = }} expected-error {{expression resolves to an unused function}}
+if #available { // expected-error {{expected availability condition}} expected-error {{closure expression is unused}} expected-error {{top-level statement cannot begin with a closure expression}} expected-note {{did you mean to use a 'do' statement?}} {{15-15=do }}
 }
 
 if #available( { // expected-error {{expected platform name}} expected-error {{expected ')'}} expected-note {{to match this opening '('}}

test/Parse/errors.swift

@@ -102,7 +102,7 @@ func illformed() throws {
       _ = try genError()
 
     // TODO: this recovery is terrible
-    } catch MSV.CarriesInt(let i) where i == genError()) { // expected-error {{call can throw, but errors cannot be thrown out of a catch guard expression}} expected-error {{expected '{'}} expected-error {{braced block of statements is an unused closure}} expected-error {{expression resolves to an unused function}}
+    } catch MSV.CarriesInt(let i) where i == genError()) { // expected-error {{call can throw, but errors cannot be thrown out of a catch guard expression}} expected-error {{expected '{'}} expected-error {{closure expression is unused}} expected-note {{did you mean to use a 'do' statement?}} {{58-58=do }}
     }
 }