Merge pull request #85538 from hamishknight/binding-fixes

[Sema] A couple of binding-related crasher fixes

Author: hamishknight

lib/AST/ASTScopeCreation.cpp

@@ -959,11 +959,8 @@ AnnotatedInsertionPoint
 ConditionalClausePatternUseScope::expandAScopeThatCreatesANewInsertionPoint(
     ScopeCreator &scopeCreator) {
   auto *initializer = sec.getInitializer();
-  if (!isa<ErrorExpr>(initializer)) {
-    scopeCreator
-      .constructExpandAndInsert<ConditionalClauseInitializerScope>(
-        this, initializer);
-    }
+  scopeCreator.constructExpandAndInsert<ConditionalClauseInitializerScope>(
+      this, initializer);
 
   return {this,
           "Succeeding code must be in scope of conditional clause pattern bindings"};

lib/Sema/CSGen.cpp

@@ -1322,7 +1322,18 @@ namespace {
     }
 
     virtual Type visitErrorExpr(ErrorExpr *E) {
-      return recordInvalidNode(E);
+      auto ty = recordInvalidNode(E);
+      // If we have an original expression, introduce a conversion to the hole
+      // type of the ErrorExpr. This avoids unnecessary diagnostics for the
+      // original expression in cases where the ErrorExpr could have provided
+      // contextual info, while also still allowing the original expression to
+      // be solved without holes being introduced prematurely, allowing e.g
+      // cursor info to work correctly.
+      if (auto *orig = E->getOriginalExpr()) {
+        CS.addConstraint(ConstraintKind::Conversion, CS.getType(orig), ty,
+                         CS.getConstraintLocator(E));
+      }
+      return ty;
     }
 
     virtual Type visitCodeCompletionExpr(CodeCompletionExpr *E) {
@@ -3156,6 +3167,16 @@ namespace {
       }
 
       case PatternKind::Expr: {
+        // Make sure we invalidate any nested VarDecls early since generating
+        // constraints for a `where` clause may happen before we've generated
+        // constraints for the ExprPattern. We'll record a fix when visiting
+        // the UnresolvedPatternExpr.
+        // FIXME: We ought to use a conjunction for switch cases, then we
+        // wouldn't need this logic.
+        auto *EP = cast<ExprPattern>(pattern);
+        EP->getSubExpr()->forEachUnresolvedVariable([&](VarDecl *VD) {
+          CS.setType(VD, ErrorType::get(CS.getASTContext()));
+        });
         // We generate constraints for ExprPatterns in a separate pass. For
         // now, just create a type variable.
         return setType(CS.createTypeVariable(CS.getConstraintLocator(locator),

lib/Sema/CSSimplify.cpp

@@ -5582,6 +5582,10 @@ bool ConstraintSystem::repairFailures(
     if (!anchor)
       return false;
 
+    // If we have an ErrorExpr anchor, we don't need to do any more fixes.
+    if (isExpr<ErrorExpr>(anchor))
+      return true;
+
     // This could be:
     // - `InOutExpr` used with r-value e.g. `foo(&x)` where `x` is a `let`.
     // - `ForceValueExpr` e.g. `foo.bar! = 42` where `bar` or `foo` are

lib/Sema/TypeCheckStmt.cpp

@@ -1091,12 +1091,14 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     auto &eval = getASTContext().evaluator;
     auto *S =
         evaluateOrDefault(eval, PreCheckReturnStmtRequest{RS, DC}, nullptr);
+    if (!S)
+      return nullptr;
 
-    // We do a cast here as it may have been turned into a FailStmt. We should
-    // return that without doing anything else.
-    RS = dyn_cast_or_null<ReturnStmt>(S);
+    // We do a cast here as we may now have a different stmt (e.g a FailStmt, or
+    // a BraceStmt for error cases). If so, recurse into the visitor.
+    RS = dyn_cast<ReturnStmt>(S);
     if (!RS)
-      return S;
+      return visit(S);
 
     auto TheFunc = AnyFunctionRef::fromDeclContext(DC);
     assert(TheFunc && "Should have bailed from pre-check if this is None");
@@ -1785,16 +1787,31 @@ Stmt *PreCheckReturnStmtRequest::evaluate(Evaluator &evaluator, ReturnStmt *RS,
   auto &ctx = DC->getASTContext();
   auto fn = AnyFunctionRef::fromDeclContext(DC);
 
+  auto errorResult = [&]() -> Stmt * {
+    // We don't need any recovery if there's no result, just bail.
+    if (!RS->hasResult())
+      return nullptr;
+
+    // If we have a result, make sure it's preserved, insert an implicit brace
+    // with a wrapping error expression. This ensures we can still do semantic
+    // functionality, and avoids downstream crashes where we expect the
+    // expression to have been type-checked.
+    auto *result = RS->getResult();
+    RS->setResult(nullptr);
+    auto *err = new (ctx) ErrorExpr(result->getSourceRange(), Type(), result);
+    return BraceStmt::createImplicit(ctx, {err});
+  };
+
   // Not valid outside of a function.
   if (!fn) {
     ctx.Diags.diagnose(RS->getReturnLoc(), diag::return_invalid_outside_func);
-    return nullptr;
+    return errorResult();
   }
 
   // If the return is in a defer, then it isn't valid either.
   if (isDefer(DC)) {
     ctx.Diags.diagnose(RS->getReturnLoc(), diag::jump_out_of_defer, "return");
-    return nullptr;
+    return errorResult();
   }
 
   // The rest of the checks only concern return statements with results.
@@ -1815,8 +1832,7 @@ Stmt *PreCheckReturnStmtRequest::evaluate(Evaluator &evaluator, ReturnStmt *RS,
     if (!nilExpr) {
       ctx.Diags.diagnose(RS->getReturnLoc(), diag::return_init_non_nil)
           .highlight(E->getSourceRange());
-      RS->setResult(nullptr);
-      return RS;
+      return errorResult();
     }
 
     // "return nil" is only permitted in a failable initializer.
@@ -1826,8 +1842,7 @@ Stmt *PreCheckReturnStmtRequest::evaluate(Evaluator &evaluator, ReturnStmt *RS,
       ctx.Diags
           .diagnose(ctor->getLoc(), diag::make_init_failable, ctor)
           .fixItInsertAfter(ctor->getLoc(), "?");
-      RS->setResult(nullptr);
-      return RS;
+      return errorResult();
     }
 
     // Replace the "return nil" with a new 'fail' statement.

test/IDE/complete_return.swift

@@ -1,21 +1,4 @@
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_VOID_1 | %FileCheck %s -check-prefix=RETURN_VOID_1
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_INT_1 | %FileCheck %s -check-prefix=RETURN_INT_1
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_INT_2 | %FileCheck %s -check-prefix=RETURN_INT_2
-
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TRY_RETURN_INT | %FileCheck %s -check-prefix=RETURN_INT_1
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TRY_RETURN_VOID | %FileCheck %s -check-prefix=RETURN_VOID_1
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR1 | %FileCheck %s -check-prefix=RETURN_TR1
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR2 | %FileCheck %s -check-prefix=RETURN_TR2
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR3 | %FileCheck %s -check-prefix=RETURN_TR3
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR1_METHOD | %FileCheck %s -check-prefix=RETURN_TR1
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR2_METHOD | %FileCheck %s -check-prefix=RETURN_TR2
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR3_METHOD | %FileCheck %s -check-prefix=RETURN_TR3
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR1_STATICMETHOD | %FileCheck %s -check-prefix=RETURN_TR1
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR2_STATICMETHOD | %FileCheck %s -check-prefix=RETURN_TR2
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR3_STATICMETHOD | %FileCheck %s -check-prefix=RETURN_TR3
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR1_CLOSURE | %FileCheck %s -check-prefix=RETURN_TR1
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR2_CLOSURE | %FileCheck %s -check-prefix=RETURN_TR2
-// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR3_CLOSURE | %FileCheck %s -check-prefix=RETURN_TR3
+// RUN: %batch-code-completion
 
 struct FooStruct {
   var instanceVar : Int
@@ -64,11 +47,11 @@ func testReturnInt2(_ fooObject: FooStruct) {
 }
 
 func testMisplacedTry() throws -> Int {
-  try return #^TRY_RETURN_INT^#
+  try return #^TRY_RETURN_INT?check=RETURN_INT_1^#
 }
 
 func testMisplacedTryVoid() throws {
-  try return #^TRY_RETURN_VOID^#
+  try return #^TRY_RETURN_VOID?check=RETURN_VOID_1^#
 }
 
 func testTR1() -> Int? {
@@ -111,26 +94,26 @@ struct TestStruct {
     var i : Int
     var oi : Int?
     var fs : FooStruct
-    return #^RETURN_TR1_METHOD^#
+    return #^RETURN_TR1_METHOD?check=RETURN_TR1^#
   }
   func testTR2_method(_ g : Gen) -> Int? {
-    return g.#^RETURN_TR2_METHOD^#
+    return g.#^RETURN_TR2_METHOD?check=RETURN_TR2^#
   }
   func testTR3_method(_ g : Gen) -> Int? {
-    return g.IG.#^RETURN_TR3_METHOD^#
+    return g.IG.#^RETURN_TR3_METHOD?check=RETURN_TR3^#
   }
 
   static func testTR1_static() -> Int? {
     var i : Int
     var oi : Int?
     var fs : FooStruct
-    return #^RETURN_TR1_STATICMETHOD^#
+    return #^RETURN_TR1_STATICMETHOD?check=RETURN_TR1^#
   }
   static func testTR2_static(_ g : Gen) -> Int? {
-    return g.#^RETURN_TR2_STATICMETHOD^#
+    return g.#^RETURN_TR2_STATICMETHOD?check=RETURN_TR2^#
   }
   static func testTR3_static(_ g : Gen) -> Int? {
-    return g.IG.#^RETURN_TR3_STATICMETHOD^#
+    return g.IG.#^RETURN_TR3_STATICMETHOD?check=RETURN_TR3^#
   }
 }
 
@@ -140,12 +123,26 @@ func testClosures(_ g: Gen) {
   var fs : FooStruct
 
   _ = { () -> Int? in
-    return #^RETURN_TR1_CLOSURE^#
+    return #^RETURN_TR1_CLOSURE?check=RETURN_TR1^#
   }
   _ = { () -> Int? in
-    return g.#^RETURN_TR2_CLOSURE^#
+    return g.#^RETURN_TR2_CLOSURE?check=RETURN_TR2^#
   }
   _ = { () -> Int? in
-    return g.IG.#^RETURN_TR3_CLOSURE^#
+    return g.IG.#^RETURN_TR3_CLOSURE?check=RETURN_TR3^#
+  }
+}
+
+// Make sure we can do a completion in an out-of-place return
+do {
+  return TestStruct.#^COMPLETE_IN_INVALID_RETURN^#
+  // COMPLETE_IN_INVALID_RETURN: Decl[StaticMethod]/CurrNominal: testTR1_static()[#Int?#]; name=testTR1_static()
+  // COMPLETE_IN_INVALID_RETURN: Decl[StaticMethod]/CurrNominal: testTR2_static({#(g): Gen#})[#Int?#]; name=testTR2_static(:)
+  // COMPLETE_IN_INVALID_RETURN: Decl[StaticMethod]/CurrNominal: testTR3_static({#(g): Gen#})[#Int?#]; name=testTR3_static(:)
+}
+
+struct TestReturnInInit {
+  init() {
+    return TestStruct.#^COMPLETE_IN_INVALID_INIT_RETURN?check=COMPLETE_IN_INVALID_RETURN^#
   }
 }

test/stmt/statements.swift

@@ -179,6 +179,8 @@ return 42 // expected-error {{return invalid outside of a func}}
 
 return // expected-error {{return invalid outside of a func}}
 
+return VoidReturn1() // expected-error {{return invalid outside of a func}}
+
 func NonVoidReturn1() -> Int {
   _ = 0
   return // expected-error {{non-void function should return a value}}

validation-test/compiler_crashers/ConstraintSystem-getClosureType-aac79a.swift renamed to validation-test/compiler_crashers_fixed/ConstraintSystem-getClosureType-aac79a.swift

@@ -1,5 +1,5 @@
 // {"kind":"typecheck","signature":"swift::constraints::ConstraintSystem::getClosureType(swift::ClosureExpr const*) const","signatureAssert":"Assertion failed: (result), function getClosureType"}
-// RUN: not --crash %target-swift-frontend -typecheck %s
+// RUN: not %target-swift-frontend -typecheck %s
 return {
   lazy var a = if .random() { return }
 }

validation-test/compiler_crashers_fixed/ConstraintSystem-getTypeOfReferencePre-60045c.swift

@@ -0,0 +1,9 @@
+// {"kind":"typecheck","signature":"swift::constraints::ConstraintSystem::getTypeOfReferencePre(swift::constraints::OverloadChoice, swift::DeclContext*, swift::constraints::ConstraintLocatorBuilder, swift::constraints::PreparedOverloadBuilder*)","signatureAssert":"Assertion failed: (func->isOperator() && \"Lookup should only find operators\"), function getTypeOfReferencePre"}
+// RUN: not %target-swift-frontend -typecheck %s
+{
+  switch if .random()  else {
+  }
+  {
+  case let !a where a:
+  }
+}

validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-5c070f.swift

@@ -0,0 +1,7 @@
+// {"kind":"typecheck","signature":"swift::constraints::TypeVarRefCollector::walkToStmtPre(swift::Stmt*)","signatureAssert":"Assertion failed: (result), function getClosureType"}
+// RUN: not %target-swift-frontend -typecheck %s
+{
+  guard let a = (a  if{
+    return
+  }
+) "

validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-e56ccf.swift

@@ -0,0 +1,10 @@
+// {"kind":"typecheck","signature":"swift::constraints::TypeVarRefCollector::walkToStmtPre(swift::Stmt*)","signatureAssert":"Assertion failed: (result), function getClosureType"}
+// RUN: not %target-swift-frontend -typecheck %s
+{
+  switch if <#expression#> {
+    return
+  }
+  {
+  case let !a where a:
+  }
+}