Merge pull request #71930 from hamishknight/to-err

[CS] Invalidate VarDecls in targets that fail to type-check

Author: hamishknight

include/swift/Sema/SyntacticElementTarget.h

@@ -887,6 +887,10 @@ class SyntacticElementTarget {
     llvm_unreachable("invalid target type");
   }
 
+  /// Marks the target as invalid, filling in ErrorTypes for the AST it
+  /// represents.
+  void markInvalid() const;
+
   /// Walk the contents of the application target.
   std::optional<SyntacticElementTarget> walk(ASTWalker &walker) const;
 };

lib/Sema/SyntacticElementTarget.cpp

@@ -301,6 +301,30 @@ bool SyntacticElementTarget::contextualTypeIsOnlyAHint() const {
   llvm_unreachable("invalid contextual type");
 }
 
+void SyntacticElementTarget::markInvalid() const {
+  class InvalidationWalker : public ASTWalker {
+    PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
+      // TODO: We ought to fill in ErrorTypes for expressions here; ultimately
+      // type-checking should always produce typed AST.
+      return Action::Continue(E);
+    }
+
+    PreWalkAction walkToDeclPre(Decl *D) override {
+      // Mark any VarDecls and PatternBindingDecls as invalid.
+      if (auto *VD = dyn_cast<VarDecl>(D)) {
+        // Only set invalid if we don't already have an interface type computed.
+        if (!VD->hasInterfaceType())
+          D->setInvalid();
+      } else if (isa<PatternBindingDecl>(D)) {
+        D->setInvalid();
+      }
+      return Action::VisitNodeIf(isa<PatternBindingDecl>(D));
+    }
+  };
+  InvalidationWalker walker;
+  walk(walker);
+}
+
 std::optional<SyntacticElementTarget>
 SyntacticElementTarget::walk(ASTWalker &walker) const {
   SyntacticElementTarget result = *this;
@@ -372,7 +396,64 @@ SyntacticElementTarget::walk(ASTWalker &walker) const {
     break;
   }
   case Kind::forEachStmt: {
-    if (auto *newStmt = getAsForEachStmt()->walk(walker)) {
+    // We need to skip the where clause if requested, and we currently do not
+    // type-check a for loop's BraceStmt as part of the SyntacticElementTarget,
+    // so we need to skip it here.
+    // TODO: We ought to be able to fold BraceStmt checking into the constraint
+    // system eventually.
+    class ForEachWalker : public ASTWalker {
+      ASTWalker &Walker;
+      SyntacticElementTarget Target;
+      ForEachStmt *ForStmt;
+
+    public:
+      ForEachWalker(ASTWalker &walker, SyntacticElementTarget target)
+        : Walker(walker), Target(target), ForStmt(target.getAsForEachStmt()) {}
+
+      PreWalkAction walkToDeclPre(Decl *D) override {
+        if (D->walk(Walker))
+          return Action::Stop();
+        return Action::SkipNode();
+      }
+
+      PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
+        // Ignore where clause if needed.
+        if (Target.ignoreForEachWhereClause() && E == ForStmt->getWhere())
+          return Action::SkipNode(E);
+
+        E = E->walk(Walker);
+
+        if (!E)
+          return Action::Stop();
+        return Action::SkipNode(E);
+      }
+
+      PreWalkResult<Stmt *> walkToStmtPre(Stmt *S) override {
+        // We only want to visit the children of the ForEachStmt.
+        if (S == ForStmt)
+          return Action::Continue(S);
+
+        // But not its body.
+        if (S != ForStmt->getBody())
+          S = S->walk(Walker);
+
+        if (!S)
+          return Action::Stop();
+
+        return Action::SkipNode(S);
+      }
+
+      PreWalkResult<Pattern *> walkToPatternPre(Pattern *P) override {
+        P = P->walk(Walker);
+        if (!P)
+          return Action::Stop();
+        return Action::SkipNode(P);
+      }
+    };
+
+    ForEachWalker forEachWalker(walker, *this);
+
+    if (auto *newStmt = getAsForEachStmt()->walk(forEachWalker)) {
       result.forEachStmt.stmt = cast<ForEachStmt>(newStmt);
     } else {
       return std::nullopt;

lib/Sema/TypeCheckConstraints.cpp

@@ -466,6 +466,13 @@ TypeChecker::typeCheckExpression(SyntacticElementTarget &target,
 std::optional<SyntacticElementTarget>
 TypeChecker::typeCheckTarget(SyntacticElementTarget &target,
                              TypeCheckExprOptions options) {
+  auto errorResult = [&]() -> std::optional<SyntacticElementTarget> {
+    // Fill in ErrorTypes for the target if we can.
+    if (!options.contains(TypeCheckExprFlags::AvoidInvalidatingAST))
+      target.markInvalid();
+
+    return std::nullopt;
+  };
   DeclContext *dc = target.getDeclContext();
   auto &Context = dc->getASTContext();
   PrettyStackTraceLocation stackTrace(Context, "type-checking-target",
@@ -475,11 +482,12 @@ TypeChecker::typeCheckTarget(SyntacticElementTarget &target,
   // expression and folding sequence expressions.
   if (ConstraintSystem::preCheckTarget(
           target, /*replaceInvalidRefsWithErrors=*/true)) {
-    return std::nullopt;
+    return errorResult();
   }
 
   // Check whether given target has a code completion token which requires
-  // special handling.
+  // special handling. Returns true if handled, in which case we've already
+  // type-checked it for completion, and don't need the solution applied.
   if (Context.CompletionCallback &&
       typeCheckForCodeCompletion(target, /*needsPrecheck*/ false,
                                  [&](const constraints::Solution &S) {
@@ -517,7 +525,7 @@ TypeChecker::typeCheckTarget(SyntacticElementTarget &target,
   // Attempt to solve the constraint system.
   auto viable = cs.solve(target, allowFreeTypeVariables);
   if (!viable)
-    return std::nullopt;
+    return errorResult();
 
   // Apply this solution to the constraint system.
   // FIXME: This shouldn't be necessary.
@@ -528,7 +536,7 @@ TypeChecker::typeCheckTarget(SyntacticElementTarget &target,
   auto resultTarget = cs.applySolution(solution, target);
   if (!resultTarget) {
     // Failure already diagnosed, above, as part of applying the solution.
-    return std::nullopt;
+    return errorResult();
   }
 
   // Unless the client has disabled them, perform syntactic checks on the
@@ -571,6 +579,13 @@ Type TypeChecker::typeCheckParameterDefault(Expr *&defaultValue,
     DiagnosticTransaction diagnostics(ctx.Diags);
 
     TypeCheckExprOptions options;
+
+    // Avoid invalidating the AST since we'll fall through and try to type-check
+    // with an archetype contextual type opened. Note we also don't need to call
+    // `markInvalid` for the target below since we'll replace the expression
+    // with an ErrorExpr on failure anyway.
+    options |= TypeCheckExprFlags::AvoidInvalidatingAST;
+
     // Expand macro expansion expression at caller side only
     if (!atCallerSide && isa<MacroExpansionExpr>(defaultValue)) {
       options |= TypeCheckExprFlags::DisableMacroExpansions;

lib/Sema/TypeChecker.h

@@ -136,6 +136,10 @@ enum class TypeCheckExprFlags {
 
   /// Don't expand macros.
   DisableMacroExpansions = 0x08,
+
+  /// If set, typeCheckExpression will avoid invalidating the AST if
+  /// type-checking fails. Do not add new uses of this.
+  AvoidInvalidatingAST = 0x10,
 };
 
 using TypeCheckExprOptions = OptionSet<TypeCheckExprFlags>;

test/Constraints/type_inference_from_default_closure.swift

@@ -0,0 +1,6 @@
+// NOTE: This intentionally doesn't use %target-typecheck-verify, this should
+// compile without any errors since we're testing the ASTVerifier is happy.
+// RUN: %target-swift-frontend -typecheck %s
+
+// Make sure we don't prematurely mark 'x' as invalid.
+func testInferenceFromClosureVar<T>(x: T = { var x: Int = 0; return x }()) {}

test/Constraints/type_inference_from_default_exprs.swift

@@ -266,3 +266,6 @@ do {
   // expected-warning@-1 {{dictionary literal of type '[String : Int]' has duplicate entries for string literal key 'a'}}
   // expected-note@-2 2 {{duplicate key declared here}}
 }
+
+func testInferenceFromClosureVarInvalid<T>(x: T = { let x = "" as Int; return x }()) {}
+// expected-error@-1 {{cannot convert value of type 'String' to type 'Int' in coercion}}

test/Index/rdar120012473.swift

@@ -0,0 +1,50 @@
+// RUN: %empty-directory(%t)
+// RUN: not %target-swift-frontend -typecheck %s -index-store-path %t
+
+let k = ""
+
+// Make sure we don't crash here (rdar://120012473).
+func foo() {
+  _ = {
+    let x = switch 1 {
+    case k:
+      return false
+    }
+    return true
+  }
+
+  for x in [0] where ({
+    let x = switch 1 {
+    case k:
+      return false
+    }
+    return true
+  }()) {}
+}
+
+@resultBuilder
+struct Builder {
+  static func buildBlock<T>(_ components: T...) -> T {
+    fatalError()
+  }
+}
+
+@Builder
+func bar() -> Bool {
+  let fn = {
+    let x = switch 1 {
+    case k:
+      return false
+    }
+    return true
+  }
+  fn()
+}
+
+func baz(x: () -> Bool = {
+  let x = switch 1 {
+  case k:
+    return false
+  }
+  return true
+}) {}