[Sema] Ensure performStmtDiagnostics is called for CaseStmts

Previously we would check if we have a SwitchStmt,
and apply diagnostics such as `checkExistentialTypes`
to the CaseStmts individually. This however would
have been missed for `catch` statements. The change
to consistently call `performStmtDiagnostics` in
closures fixed this for `do-catch`'s in closures,
this commit fixes it for those outside of closures.
Because this is source breaking, the existential
diagnostic is downgraded to a warning until Swift
7 for catch statements specifically.

While here, also apply the ambiguous where clause
diagnostic to `catch` statements.

Author: hamishknight

include/swift/AST/DiagnosticsSema.def

@@ -5105,7 +5105,8 @@ ERROR(unknown_case_must_be_last,none,
       "'@unknown' can only be applied to the last case in a switch", ())
 
 WARNING(where_on_one_item, none,
-        "'where' only applies to the second pattern match in this case", ())
+        "'where' only applies to the second pattern match in this "
+        "'%select{case|catch}0'", (bool))
 
 NOTE(add_where_newline, none,
      "disambiguate by adding a line break between them if this is desired", ())

lib/Sema/MiscDiagnostics.cpp

@@ -4419,61 +4419,57 @@ void swift::performAbstractFuncDeclDiagnostics(AbstractFunctionDecl *AFD) {
   }
 }
 
-// Perform MiscDiagnostics on Switch Statements.
-static void checkSwitch(ASTContext &ctx, const SwitchStmt *stmt,
-                        DeclContext *DC) {
-  // We want to warn about "case .Foo, .Bar where 1 != 100:" since the where
-  // clause only applies to the second case, and this is surprising.
-  for (auto cs : stmt->getCases()) {
-    TypeChecker::checkExistentialTypes(ctx, cs, DC);
-
-    // The case statement can have multiple case items, each can have a where.
-    // If we find a "where", and there is a preceding item without a where, and
-    // if they are on the same source line, then warn.
-    auto items = cs->getCaseLabelItems();
-    
-    // Don't do any work for the vastly most common case.
-    if (items.size() == 1) continue;
-    
-    // Ignore the first item, since it can't have preceding ones.
-    for (unsigned i = 1, e = items.size(); i != e; ++i) {
-      // Must have a where clause.
-      auto where = items[i].getGuardExpr();
-      if (!where)
-        continue;
-      
-      // Preceding item must not.
-      if (items[i-1].getGuardExpr())
-        continue;
-      
-      // Must be on the same source line.
-      auto prevLoc = items[i-1].getStartLoc();
-      auto thisLoc = items[i].getStartLoc();
-      if (prevLoc.isInvalid() || thisLoc.isInvalid())
-        continue;
-      
-      auto &SM = ctx.SourceMgr;
-      auto prevLineCol = SM.getLineAndColumnInBuffer(prevLoc);
-      if (SM.getLineAndColumnInBuffer(thisLoc).first != prevLineCol.first)
-        continue;
+static void diagnoseCaseStmtAmbiguousWhereClause(const CaseStmt *CS,
+                                                 ASTContext &ctx) {
+  // The case statement can have multiple case items, each can have a where.
+  // If we find a "where", and there is a preceding item without a where, and
+  // if they are on the same source line, e.g
+  // "case .Foo, .Bar where 1 != 100:" then warn since it may be unexpected.
+  auto items = CS->getCaseLabelItems();
+
+  // Don't do any work for the vastly most common case.
+  if (items.size() == 1)
+    return;
 
-      ctx.Diags.diagnose(items[i].getWhereLoc(), diag::where_on_one_item)
+  // Ignore the first item, since it can't have preceding ones.
+  for (unsigned i = 1, e = items.size(); i != e; ++i) {
+    // Must have a where clause.
+    auto where = items[i].getGuardExpr();
+    if (!where)
+      continue;
+
+    // Preceding item must not.
+    if (items[i - 1].getGuardExpr())
+      continue;
+
+    // Must be on the same source line.
+    auto prevLoc = items[i - 1].getStartLoc();
+    auto thisLoc = items[i].getStartLoc();
+    if (prevLoc.isInvalid() || thisLoc.isInvalid())
+      continue;
+
+    auto &SM = ctx.SourceMgr;
+    auto prevLineCol = SM.getLineAndColumnInBuffer(prevLoc);
+    if (SM.getLineAndColumnInBuffer(thisLoc).first != prevLineCol.first)
+      continue;
+
+    ctx.Diags
+        .diagnose(items[i].getWhereLoc(), diag::where_on_one_item,
+                  CS->getParentKind() == CaseParentKind::DoCatch)
         .highlight(items[i].getPattern()->getSourceRange())
         .highlight(where->getSourceRange());
-      
-      // Whitespace it out to the same column as the previous item.
-      std::string whitespace(prevLineCol.second-1, ' ');
-      ctx.Diags.diagnose(thisLoc, diag::add_where_newline)
-        .fixItInsert(thisLoc, "\n"+whitespace);
-
-      auto whereRange = SourceRange(items[i].getWhereLoc(),
-                                    where->getEndLoc());
-      auto charRange = Lexer::getCharSourceRangeFromSourceRange(SM, whereRange);
-      auto whereText = SM.extractText(charRange);
-      ctx.Diags.diagnose(prevLoc, diag::duplicate_where)
-        .fixItInsertAfter(items[i-1].getEndLoc(), " " + whereText.str())
-        .highlight(items[i-1].getSourceRange());
-    }
+
+    // Whitespace it out to the same column as the previous item.
+    std::string whitespace(prevLineCol.second - 1, ' ');
+    ctx.Diags.diagnose(thisLoc, diag::add_where_newline)
+        .fixItInsert(thisLoc, "\n" + whitespace);
+
+    auto whereRange = SourceRange(items[i].getWhereLoc(), where->getEndLoc());
+    auto charRange = Lexer::getCharSourceRangeFromSourceRange(SM, whereRange);
+    auto whereText = SM.extractText(charRange);
+    ctx.Diags.diagnose(prevLoc, diag::duplicate_where)
+        .fixItInsertAfter(items[i - 1].getEndLoc(), " " + whereText.str())
+        .highlight(items[i - 1].getSourceRange());
   }
 }
 
@@ -6194,8 +6190,8 @@ void swift::performStmtDiagnostics(const Stmt *S, DeclContext *DC) {
 
   TypeChecker::checkExistentialTypes(ctx, const_cast<Stmt *>(S), DC);
 
-  if (auto switchStmt = dyn_cast<SwitchStmt>(S))
-    checkSwitch(ctx, switchStmt, DC);
+  if (auto *CS = dyn_cast<CaseStmt>(S))
+    diagnoseCaseStmtAmbiguousWhereClause(CS, ctx);
 
   checkStmtConditionTrailingClosure(ctx, S);
 

lib/Sema/TypeCheckStmt.cpp

@@ -1607,6 +1607,10 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
       BraceStmt *body = caseBlock->getBody();
       limitExhaustivityChecks |= typeCheckStmt(body);
       caseBlock->setBody(body);
+
+      // CaseStmts don't go through typeCheckStmt, so manually call into
+      // performStmtDiagnostics.
+      performStmtDiagnostics(caseBlock, DC);
     }
   }
 

lib/Sema/TypeCheckType.cpp

@@ -6143,13 +6143,16 @@ class ExistentialTypeSyntaxChecker : public ASTWalker {
   ASTContext &Ctx;
   bool checkStatements;
   bool hitTopStmt;
+  bool warnUntilSwift7;
 
   unsigned exprCount = 0;
   llvm::SmallVector<TypeRepr *, 4> reprStack;
 
 public:
-  ExistentialTypeSyntaxChecker(ASTContext &ctx, bool checkStatements)
-      : Ctx(ctx), checkStatements(checkStatements), hitTopStmt(false) {}
+  ExistentialTypeSyntaxChecker(ASTContext &ctx, bool checkStatements,
+                               bool warnUntilSwift7 = false)
+      : Ctx(ctx), checkStatements(checkStatements), hitTopStmt(false),
+        warnUntilSwift7(warnUntilSwift7) {}
 
   MacroWalking getMacroWalkingBehavior() const override {
     return MacroWalking::ArgumentsAndExpansion;
@@ -6363,6 +6366,7 @@ class ExistentialTypeSyntaxChecker : public ASTWalker {
           inverse && isAnyOrSomeMissing()) {
         auto diag = Ctx.Diags.diagnose(inverse->getTildeLoc(),
                                        diag::inverse_requires_any);
+        diag.warnUntilSwiftVersionIf(warnUntilSwift7, 7);
         emitInsertAnyFixit(diag, T);
         return;
       }
@@ -6380,6 +6384,7 @@ class ExistentialTypeSyntaxChecker : public ASTWalker {
                                proto->getDeclaredInterfaceType(),
                                proto->getDeclaredExistentialType(),
                                /*isAlias=*/false);
+        diag.warnUntilSwiftVersionIf(warnUntilSwift7, 7);
         emitInsertAnyFixit(diag, T);
       }
     } else if (auto *alias = dyn_cast<TypeAliasDecl>(decl)) {
@@ -6410,6 +6415,7 @@ class ExistentialTypeSyntaxChecker : public ASTWalker {
               alias->getDeclaredInterfaceType(),
               ExistentialType::get(alias->getDeclaredInterfaceType()),
               /*isAlias=*/true);
+          diag.warnUntilSwiftVersionIf(warnUntilSwift7, 7);
           emitInsertAnyFixit(diag, T);
         }
       }
@@ -6489,7 +6495,14 @@ void TypeChecker::checkExistentialTypes(ASTContext &ctx, Stmt *stmt,
   if (sourceFile && sourceFile->Kind == SourceFileKind::Interface)
     return;
 
-  ExistentialTypeSyntaxChecker checker(ctx, /*checkStatements=*/true);
+  // Previously we missed this diagnostic on 'catch' statements, downgrade
+  // to a warning until Swift 7.
+  auto downgradeUntilSwift7 = false;
+  if (auto *CS = dyn_cast<CaseStmt>(stmt))
+    downgradeUntilSwift7 = CS->getParentKind() == CaseParentKind::DoCatch;
+
+  ExistentialTypeSyntaxChecker checker(ctx, /*checkStatements=*/true,
+                                       downgradeUntilSwift7);
   stmt->walk(checker);
 }
 

test/stmt/statements.swift

@@ -476,7 +476,7 @@ func r25178926(_ a : Type) {
   switch a { // expected-error {{switch must be exhaustive}}
   // expected-note@-1 {{missing case: '.Bar'}}
   case .Foo, .Bar where 1 != 100:
-    // expected-warning @-1 {{'where' only applies to the second pattern match in this case}}
+    // expected-warning @-1 {{'where' only applies to the second pattern match in this 'case'}}
     // expected-note @-2 {{disambiguate by adding a line break between them if this is desired}} {{14-14=\n       }}
     // expected-note @-3 {{duplicate the 'where' on both patterns to check both patterns}} {{12-12= where 1 != 100}}
     break
@@ -504,6 +504,34 @@ func r25178926(_ a : Type) {
   }
 }
 
+func testAmbiguousWhereInCatch() {
+  protocol P1 {}
+  protocol P2 {}
+  func throwingFn() throws {}
+  do {
+    try throwingFn()
+  } catch is P1, is P2 where .random() {
+    // expected-warning @-1 {{'where' only applies to the second pattern match in this 'catch'}}
+    // expected-note @-2 {{disambiguate by adding a line break between them if this is desired}} {{18-18=\n          }}
+    // expected-note @-3 {{duplicate the 'where' on both patterns to check both patterns}} {{16-16= where .random()}}
+  } catch {
+
+  }
+  do {
+    try throwingFn()
+  } catch is P1,
+          is P2 where .random() {
+  } catch {
+
+  }
+  do {
+    try throwingFn()
+  } catch is P1 where .random(), is P2 where .random() {
+  } catch {
+
+  }
+}
+
 do {
   guard 1 == 2 else {
     break // expected-error {{unlabeled 'break' is only allowed inside a loop or switch, a labeled break is required to exit an if or do}}

test/type/protocol_types.swift

@@ -149,3 +149,99 @@ struct SubscriptWhere {
 struct OuterGeneric<T> {
   func contextuallyGenericMethod() where T == any HasAssoc {}
 }
+
+typealias HasAssocAlias = HasAssoc
+
+func testExistentialInCase(_ x: Any) {
+  switch x {
+  case is HasAssoc:
+    // expected-error@-1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}
+    break
+  default:
+    break
+  }
+  _ = {
+    switch x {
+    case is HasAssoc:
+      // expected-error@-1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}
+      break
+    default:
+      break
+    }
+  }
+  switch x {
+  case is HasAssocAlias:
+    // expected-error@-1 {{use of 'HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any HasAssocAlias' (aka 'any HasAssoc')}}
+    break
+  default:
+    break
+  }
+  _ = {
+    switch x {
+    case is HasAssocAlias:
+      // expected-error@-1 {{use of 'HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any HasAssocAlias' (aka 'any HasAssoc')}}
+      break
+    default:
+      break
+    }
+  }
+  switch x {
+  case is ~Copyable:
+    // expected-error@-1 {{constraint that suppresses conformance requires 'any'}}
+    // expected-warning@-2 {{'is' test is always true}}
+    break
+  default:
+    break
+  }
+  _ = {
+    switch x {
+    case is ~Copyable:
+      // expected-error@-1 {{constraint that suppresses conformance requires 'any'}}
+      // expected-warning@-2 {{'is' test is always true}}
+      break
+    default:
+      break
+    }
+  }
+}
+
+func throwingFn() throws {}
+
+// These are downgraded to warnings until Swift 7, see protocol_types_swift7.swift.
+// https://github.com/swiftlang/swift/issues/77553
+func testExistentialInCatch() throws {
+  do {
+    try throwingFn()
+  } catch is HasAssoc {}
+  // expected-warning@-1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}
+  _ = {
+    do {
+      try throwingFn()
+    } catch is HasAssoc {}
+    // expected-warning@-1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}
+  }
+  do {
+    try throwingFn()
+  } catch is HasAssocAlias {}
+  // expected-warning@-1 {{use of 'HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any HasAssocAlias' (aka 'any HasAssoc')}}
+  _ = {
+    do {
+      try throwingFn()
+    } catch is HasAssocAlias {}
+    // expected-warning@-1 {{use of 'HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any HasAssocAlias' (aka 'any HasAssoc')}}
+  }
+  do {
+    try throwingFn()
+  } catch is ~Copyable {}
+  // expected-warning@-1 {{constraint that suppresses conformance requires 'any'}}
+  // expected-warning@-2 {{'is' test is always true}}
+
+  // FIXME: We shouldn't emit a duplicate 'always true' warning here.
+  _ = {
+    do {
+      try throwingFn()
+    } catch is ~Copyable {}
+    // expected-warning@-1 {{constraint that suppresses conformance requires 'any'}}
+    // expected-warning@-2 2{{'is' test is always true}}
+  }
+}

test/type/protocol_types_swift7.swift

@@ -0,0 +1,49 @@
+// RUN: %target-typecheck-verify-swift -swift-version 7
+// REQUIRES: swift7
+
+protocol HasAssoc {
+  associatedtype Assoc
+}
+
+typealias HasAssocAlias = HasAssoc
+
+func throwingFn() throws {}
+
+// In Swift 6 we previously missed this diagnostic.
+// https://github.com/swiftlang/swift/issues/77553
+func testExistentialInCatch() throws {
+  do {
+    try throwingFn()
+  } catch is HasAssoc {}
+  // expected-error@-1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}
+  _ = {
+    do {
+      try throwingFn()
+    } catch is HasAssoc {}
+    // expected-error@-1 {{use of protocol 'HasAssoc' as a type must be written 'any HasAssoc'}}
+  }
+  do {
+    try throwingFn()
+  } catch is HasAssocAlias {}
+  // expected-error@-1 {{use of 'HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any HasAssocAlias' (aka 'any HasAssoc')}}
+  _ = {
+    do {
+      try throwingFn()
+    } catch is HasAssocAlias {}
+    // expected-error@-1 {{use of 'HasAssocAlias' (aka 'HasAssoc') as a type must be written 'any HasAssocAlias' (aka 'any HasAssoc')}}
+  }
+  do {
+    try throwingFn()
+  } catch is ~Copyable {}
+  // expected-error@-1 {{constraint that suppresses conformance requires 'any'}}
+  // expected-warning@-2 {{'is' test is always true}}
+
+  // FIXME: We shouldn't emit a duplicate 'always true' warning here.
+  _ = {
+    do {
+      try throwingFn()
+    } catch is ~Copyable {}
+    // expected-error@-1 {{constraint that suppresses conformance requires 'any'}}
+    // expected-warning@-2 2{{'is' test is always true}}
+  }
+}