Sema: Skip non-single-expression closure bodies in MiscDiagnostics

This is a defensive move to avoid duplicated work and guard against crashes
when a multi-expression closure body or TapExpr has not been type checked yet.

Fixes <rdar://problem/48852402>.

Author: slavapestov

include/swift/AST/ASTWalker.h

@@ -211,6 +211,14 @@ class ASTWalker {
   /// However, ASTWalker does not walk into LazyInitializerExprs on its own.
   virtual bool shouldWalkIntoLazyInitializers() { return true; }
 
+  /// This method configures whether the walker should visit the body of a
+  /// non-single expression closure.
+  ///
+  /// For work that is performed for every top-level expression, this should
+  /// be overridden to return false, to avoid duplicating work or visiting
+  /// bodies of closures that have not yet been type checked.
+  virtual bool shouldWalkIntoNonSingleExpressionClosure() { return true; }
+
   /// walkToParameterListPre - This method is called when first visiting a
   /// ParameterList, before walking into its parameters.  If it returns false,
   /// the subtree is skipped.

lib/AST/ASTWalker.cpp

@@ -761,6 +761,9 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
       return nullptr;
     }
 
+    if (!Walker.shouldWalkIntoNonSingleExpressionClosure())
+      return expr;
+
     // Handle other closures.
     if (BraceStmt *body = cast_or_null<BraceStmt>(doIt(expr->getBody()))) {
       expr->setBody(body, false);
@@ -1071,12 +1074,14 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
     if (auto oldSubExpr = E->getSubExpr()) {
       if (auto subExpr = doIt(oldSubExpr)) {
         E->setSubExpr(subExpr);
-      }
-      else {
+      } else {
         return nullptr;
       }
     }
 
+    if (!Walker.shouldWalkIntoNonSingleExpressionClosure())
+      return E;
+
     if (auto oldBody = E->getBody()) {
       if (auto body = doIt(oldBody)) {
         E->setBody(dyn_cast<BraceStmt>(body));

lib/Sema/MiscDiagnostics.cpp

@@ -110,6 +110,8 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
     bool walkToDeclPre(Decl *D) override { return false; }
     bool walkToTypeReprPre(TypeRepr *T) override { return true; }
 
+    bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
+
     std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
       // See through implicit conversions of the expression.  We want to be able
       // to associate the parent of this expression with the ultimate callee.
@@ -1408,6 +1410,8 @@ static void diagRecursivePropertyAccess(TypeChecker &TC, const Expr *E,
              cast<VarDecl>(DRE->getDecl())->isSelfParameter();
     }
 
+    bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
+
     std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
       Expr *subExpr;
       bool isStore = false;
@@ -1556,11 +1560,10 @@ static void diagnoseImplicitSelfUseInClosure(TypeChecker &TC, const Expr *E,
       return false;
     }
 
+    bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
+
     std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
       if (auto *CE = dyn_cast<AbstractClosureExpr>(E)) {
-        if (!CE->hasSingleExpressionBody())
-          return { false, E };
-
         // If this is a potentially-escaping closure expression, start looking
         // for references to self if we aren't already.
         if (isClosureRequiringSelfQualification(CE))
@@ -2387,7 +2390,7 @@ class VarDeclUsageChecker : public ASTWalker {
     // other things going on in the initializer expressions.
     return true;
   }
-  
+
   /// The heavy lifting happens when visiting expressions.
   std::pair<bool, Expr *> walkToExprPre(Expr *E) override;
 
@@ -2772,8 +2775,6 @@ void VarDeclUsageChecker::markStoredOrInOutExpr(Expr *E, unsigned Flags) {
   E->walk(*this);
 }
 
-   
-
 /// The heavy lifting happens when visiting expressions.
 std::pair<bool, Expr *> VarDeclUsageChecker::walkToExprPre(Expr *E) {
   // Sema leaves some subexpressions null, which seems really unfortunate.  It
@@ -3020,6 +3021,8 @@ static void checkStmtConditionTrailingClosure(TypeChecker &TC, const Expr *E) {
   public:
     DiagnoseWalker(TypeChecker &tc) : TC(tc) { }
 
+    bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
+
     std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
       // Dig into implicit expression.
       if (E->isImplicit()) return { true, E };
@@ -3151,6 +3154,8 @@ class ObjCSelectorWalker : public ASTWalker {
   ObjCSelectorWalker(TypeChecker &tc, const DeclContext *dc, Type selectorTy)
     : TC(tc), DC(dc), SelectorTy(selectorTy) { }
 
+  bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
+
   std::pair<bool, Expr *> walkToExprPre(Expr *expr) override {
     auto *stringLiteral = dyn_cast<StringLiteralExpr>(expr);
     bool fromStringLiteral = false;
@@ -3827,14 +3832,12 @@ static void diagnoseUnintendedOptionalBehavior(TypeChecker &TC, const Expr *E,
       }
     }
 
+    bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
+
     std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
       if (!E || isa<ErrorExpr>(E) || !E->getType())
         return { false, E };
 
-      if (auto *CE = dyn_cast<AbstractClosureExpr>(E))
-        if (!CE->hasSingleExpressionBody())
-          return { false, E };
-
       if (IgnoredExprs.count(E))
         return { true, E };
 
@@ -3901,6 +3904,8 @@ static void diagnoseDeprecatedWritableKeyPath(TypeChecker &TC, const Expr *E,
       }
     }
 
+    bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
+
     std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
       if (!E || isa<ErrorExpr>(E) || !E->getType())
         return {false, E};

test/attr/attr_noescape.swift

@@ -305,19 +305,16 @@ var escape : CompletionHandlerNE
 var escapeOther : CompletionHandler
 func doThing1(_ completion: (_ success: Bool) -> ()) {
   // expected-note@-1{{parameter 'completion' is implicitly non-escaping}}
-  // expected-error @+2 {{non-escaping value 'escape' may only be called}}
   // expected-error @+1 {{non-escaping parameter 'completion' may only be called}}
   escape = completion // expected-error {{declaration closing over non-escaping parameter 'escape' may allow it to escape}}
 }
 func doThing2(_ completion: CompletionHandlerNE) {
   // expected-note@-1{{parameter 'completion' is implicitly non-escaping}}
-  // expected-error @+2 {{non-escaping value 'escape' may only be called}}
   // expected-error @+1 {{non-escaping parameter 'completion' may only be called}}
   escape = completion // expected-error {{declaration closing over non-escaping parameter 'escape' may allow it to escape}}
 }
 func doThing3(_ completion: CompletionHandler) {
   // expected-note@-1{{parameter 'completion' is implicitly non-escaping}}
-  // expected-error @+2 {{non-escaping value 'escape' may only be called}}
   // expected-error @+1 {{non-escaping parameter 'completion' may only be called}}
   escape = completion // expected-error {{declaration closing over non-escaping parameter 'escape' may allow it to escape}}
 }

validation-test/compiler_crashers_2_fixed/0190-rdar48852402.swift

@@ -0,0 +1,12 @@
+// RUN: not %target-typecheck-verify-swift
+
+struct Foo {
+  public func subscribe(_: @escaping () -> Void) {}
+  public static func foo() {}
+
+  func bind() {
+    subscribe {
+      _ = "\(foo)"
+    }
+  }
+}