Merge pull request #85449 from hamishknight/whats-the-catch

[Sema] Skip type-checking catch bodies when computing the bound error type

Author: hamishknight

lib/Sema/TypeCheckConstraints.cpp

@@ -928,13 +928,7 @@ bool TypeChecker::typeCheckForEachPreamble(DeclContext *dc, ForEachStmt *stmt,
 }
 
 bool TypeChecker::typeCheckCondition(Expr *&expr, DeclContext *dc) {
-  // If this expression is already typechecked and has type Bool, then just
-  // re-typecheck it.
-  if (expr->getType() && expr->getType()->isBool()) {
-    auto resultTy =
-        TypeChecker::typeCheckExpression(expr, dc);
-    return !resultTy;
-  }
+  ASSERT(!expr->getType() && "the bool condition is already type checked");
 
   auto *boolDecl = dc->getASTContext().getBoolDecl();
   if (!boolDecl)

lib/Sema/TypeCheckDecl.cpp

@@ -2727,6 +2727,18 @@ NamingPatternRequest::evaluate(Evaluator &evaluator, VarDecl *VD) const {
 
   if (!namingPattern) {
     if (auto parentStmt = VD->getRecursiveParentPatternStmt()) {
+      // We have a parent stmt. This should only ever be the case for completion
+      // or lazy type-checking, regular type-checking should go through the
+      // StmtChecker and assign types before querying this, otherwise we could
+      // end up double-type-checking.
+      //
+      // FIXME: We ought to be able to enable the following assert once we've
+      // fixed cases where we currently allow forward references to variables to
+      // kick interface type requests
+      // (https://github.com/swiftlang/swift/pull/85141).
+      // ASSERT(Context.SourceMgr.hasIDEInspectionTargetBuffer() ||
+      //        Context.TypeCheckerOpts.EnableLazyTypecheck);
+
       // Try type checking parent control statement.
       if (auto condStmt = dyn_cast<LabeledConditionalStmt>(parentStmt)) {
         // The VarDecl is defined inside a condition of a `if` or `while` stmt.
@@ -2752,18 +2764,20 @@ NamingPatternRequest::evaluate(Evaluator &evaluator, VarDecl *VD) const {
         assert(foundVarDecl && "VarDecl not declared in its parent?");
         (void) foundVarDecl;
       } else {
-        // We have some other parent stmt. Type check it completely.
+        // We have some other statement, e.g a switch or some kind of loop. We
+        // need to type-check it to get the type of the bound variable. We
+        // generally want to skip type-checking any BraceStmts, the only
+        // exception being do-catch bodies since we need to compute the thrown
+        // error type for catch clauses.
+        // FIXME: Rather than going through `typeCheckASTNode` and trying to
+        // exclude type-checker work, we ought to do more granular requests.
+        auto braceCheck = BraceStmtChecking::OnlyDoCatchBody;
+
         if (auto CS = dyn_cast<CaseStmt>(parentStmt))
           parentStmt = CS->getParentStmt();
 
-        bool LeaveBodyUnchecked = true;
-        // type-checking 'catch' patterns depends on the type checked body.
-        if (isa<DoCatchStmt>(parentStmt))
-          LeaveBodyUnchecked = false;
-
         ASTNode node(parentStmt);
-        TypeChecker::typeCheckASTNode(node, VD->getDeclContext(),
-                                      LeaveBodyUnchecked);
+        TypeChecker::typeCheckASTNode(node, VD->getDeclContext(), braceCheck);
       }
       namingPattern = VD->getCanonicalVarDecl()->NamingPattern;
     }

lib/Sema/TypeCheckStmt.cpp

@@ -865,7 +865,6 @@ static bool typeCheckHasSymbolStmtConditionElement(StmtConditionElement &elt,
 static bool typeCheckBooleanStmtConditionElement(StmtConditionElement &elt,
                                                  DeclContext *dc) {
   Expr *E = elt.getBoolean();
-  assert(!E->getType() && "the bool condition is already type checked");
   bool hadError = TypeChecker::typeCheckCondition(E, dc);
   elt.setBoolean(E);
   return hadError;
@@ -1042,9 +1041,9 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
   /// DC - This is the current DeclContext.
   DeclContext *DC;
 
-  /// Skip type checking any elements inside 'BraceStmt', also this is
-  /// propagated to ConstraintSystem.
-  bool LeaveBraceStmtBodyUnchecked = false;
+  /// The BraceStmts that should be type-checked. This should always be `All`,
+  /// unless we're lazy type-checking for e.g completion.
+  BraceStmtChecking BraceChecking = BraceStmtChecking::All;
 
   ASTContext &getASTContext() const { return Ctx; };
 
@@ -1503,7 +1502,7 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     // FIXME: This is a hack to avoid cycles through NamingPatternRequest when
     // doing lazy type-checking, we ought to fix the request to be granular in
     // the type-checking work it kicks.
-    bool skipWhere = LeaveBraceStmtBodyUnchecked &&
+    bool skipWhere = (BraceChecking != BraceStmtChecking::All) &&
       Ctx.SourceMgr.hasIDEInspectionTargetBuffer();
 
     TypeChecker::typeCheckForEachPreamble(DC, S, skipWhere);
@@ -1738,11 +1737,19 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     auto sourceFile = DC->getParentSourceFile();
     checkLabeledStmtShadowing(getASTContext(), sourceFile, S);
 
-    // Type-check the 'do' body.  Type failures in here will generally
-    // not cause type failures in the 'catch' clauses.
-    Stmt *newBody = S->getBody();
-    typeCheckStmt(newBody);
-    S->setBody(newBody);
+    {
+      // If we have do-catch body checking enabled, make sure we visit the
+      // BraceStmt.
+      std::optional<llvm::SaveAndRestore<BraceStmtChecking>> braceChecking;
+      if (BraceChecking == BraceStmtChecking::OnlyDoCatchBody)
+        braceChecking.emplace(BraceChecking, BraceStmtChecking::All);
+
+      // Type-check the 'do' body.  Type failures in here will generally
+      // not cause type failures in the 'catch' clauses.
+      Stmt *newBody = S->getBody();
+      typeCheckStmt(newBody);
+      S->setBody(newBody);
+    }
 
     // Do-catch statements always limit exhaustivity checks.
     bool limitExhaustivityChecks = true;
@@ -2192,7 +2199,7 @@ void StmtChecker::typeCheckASTNode(ASTNode &node) {
 }
 
 Stmt *StmtChecker::visitBraceStmt(BraceStmt *BS) {
-  if (LeaveBraceStmtBodyUnchecked)
+  if (BraceChecking != BraceStmtChecking::All)
     return BS;
 
   // Diagnose defer statement being last one in block (only if
@@ -2218,12 +2225,12 @@ Stmt *StmtChecker::visitBraceStmt(BraceStmt *BS) {
 }
 
 void TypeChecker::typeCheckASTNode(ASTNode &node, DeclContext *DC,
-                                   bool LeaveBodyUnchecked) {
+                                   BraceStmtChecking braceChecking) {
   StmtChecker stmtChecker(DC);
   // FIXME: 'ActiveLabeledStmts', etc. in StmtChecker are not
   // populated. Since they don't affect "type checking", it's doesn't cause
   // any issue for now. But it should be populated nonetheless.
-  stmtChecker.LeaveBraceStmtBodyUnchecked = LeaveBodyUnchecked;
+  stmtChecker.BraceChecking = braceChecking;
   stmtChecker.typeCheckASTNode(node);
 }
 
@@ -2769,7 +2776,7 @@ bool TypeCheckASTNodeAtLocRequest::evaluate(
     }
   }
 
-  TypeChecker::typeCheckASTNode(finder.getRef(), DC, /*LeaveBodyUnchecked=*/false);
+  TypeChecker::typeCheckASTNode(finder.getRef(), DC);
   return false;
 }
 

lib/Sema/TypeChecker.h

@@ -362,6 +362,18 @@ enum class CheckedCastContextKind {
   Coercion,
 };
 
+/// Determines which BraceStmts should be type-checked.
+enum class BraceStmtChecking {
+  /// Type-check all BraceStmts. This should always be the case for regular
+  /// type-checking.
+  All,
+
+  /// Only type-check the body of a do-catch statement, not including catch
+  /// clauses. This is necessary for completion where we still need the caught
+  /// error type to be computed.
+  OnlyDoCatchBody,
+};
+
 namespace TypeChecker {
 // DANGER: callers must verify that elementType satisfies the requirements of
 // the Wrapped generic parameter, as this function will not do so!
@@ -525,7 +537,7 @@ bool typeCheckStmtConditionElement(StmtConditionElement &elt, bool &isFalsable,
 ConcreteDeclRef getReferencedDeclForHasSymbolCondition(Expr *E);
 
 void typeCheckASTNode(ASTNode &node, DeclContext *DC,
-                      bool LeaveBodyUnchecked = false);
+                      BraceStmtChecking braceChecking = BraceStmtChecking::All);
 
 /// Try to apply the result builder transform of the given builder type
 /// to the body of the function.

test/IDE/complete_exception.swift

@@ -189,3 +189,19 @@ func test016() {
     i.#^INSIDE_CATCH_TYPEDERR_DOT?check=INT_DOT^#
   }
 }
+
+// https://github.com/swiftlang/swift/issues/85434
+func issue85434() throws {
+  func foo() throws(Error4) {}
+  do {
+    try foo()
+  } catch let error {
+    switch error {
+    case .#^SWITCH_INSIDE_CATCH^#
+      // SWITCH_INSIDE_CATCH-DAG: Decl[EnumElement]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Convertible]: E1[#Error4#]; name=E1
+      // SWITCH_INSIDE_CATCH-DAG: Decl[EnumElement]/CurrNominal/Flair[ExprSpecific]/TypeRelation[Convertible]: E2({#Int32#})[#Error4#]; name=E2()
+    default:
+      throw error
+    }
+  }
+}

validation-test/IDE/crashers_fixed/ConstraintWalker-walkToExprPost-303c08.swift

@@ -0,0 +1,12 @@
+// RUN: %target-swift-ide-test -code-completion -batch-code-completion -skip-filecheck -code-completion-diagnostics -source-filename %s
+// https://github.com/swiftlang/swift/issues/85434
+protocol MyError: Error {}
+func oops() {
+  do {
+  } catch let error as any MyError {
+    switch error.httpStatus {
+    case .#^COMPLETE^#
+    default: throw error
+    }
+  }
+}