Merge pull request #77554 from hamishknight/statement-misc-diags

[Sema] A couple more MiscDiagnostics cleanups/fixes

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/CSSyntacticElement.cpp

@@ -1733,20 +1733,6 @@ class SyntacticElementSolutionApplication
     return Type();
   }
 
-  ASTNode visit(Stmt *S, bool performSyntacticDiagnostics = true) {
-    auto rewritten = ASTVisitor::visit(S);
-    if (!rewritten)
-      return {};
-
-    if (performSyntacticDiagnostics) {
-      if (auto *stmt = getAsStmt(rewritten)) {
-        performStmtDiagnostics(stmt, context.getAsDeclContext());
-      }
-    }
-
-    return rewritten;
-  }
-
   bool visitPatternBindingDecl(PatternBindingDecl *PBD) {
     // If this is a placeholder variable with an initializer, we just need to
     // set the inferred type.
@@ -2274,7 +2260,7 @@ class ResultBuilderRewriter : public SyntacticElementSolutionApplication {
 private:
   ASTNode visitDoStmt(DoStmt *doStmt) override {
     if (auto transformed = transformDo(doStmt)) {
-      return visit(transformed.get(), /*performSyntacticDiagnostics=*/false);
+      return visit(transformed.get());
     }
 
     auto newBody = visit(doStmt->getBody());

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/MiscDiagnostics.h

@@ -135,7 +135,11 @@ namespace swift {
 
   class BaseDiagnosticWalker : public ASTWalker {
     PreWalkAction walkToDeclPre(Decl *D) override {
-      return Action::VisitNodeIf(isa<PatternBindingDecl>(D));
+      // We don't walk into any nested local decls, except PatternBindingDecls,
+      // which are type-checked along with the parent, and MacroExpansionDecl,
+      // which needs to be visited to visit the macro arguments.
+      return Action::VisitNodeIf(isa<PatternBindingDecl>(D) ||
+                                 isa<MacroExpansionDecl>(D));
     }
 
     MacroWalking getMacroWalkingBehavior() const override {

lib/Sema/TypeCheckAvailability.cpp

@@ -3560,7 +3560,7 @@ bool diagnoseExplicitUnavailability(
 }
 
 namespace {
-class ExprAvailabilityWalker : public ASTWalker {
+class ExprAvailabilityWalker : public BaseDiagnosticWalker {
   /// Models how member references will translate to accessor usage. This is
   /// used to diagnose the availability of individual accessors that may be
   /// called by the expression being checked.
@@ -3595,11 +3595,6 @@ class ExprAvailabilityWalker : public ASTWalker {
   explicit ExprAvailabilityWalker(const ExportContext &Where)
     : Context(Where.getDeclContext()->getASTContext()), Where(Where) {}
 
-  MacroWalking getMacroWalkingBehavior() const override {
-    // Expanded source should be type checked and diagnosed separately.
-    return MacroWalking::Arguments;
-  }
-
   PreWalkAction walkToArgumentPre(const Argument &Arg) override {
     // Arguments should be walked in their own member access context which
     // starts out read-only by default.
@@ -3720,10 +3715,8 @@ class ExprAvailabilityWalker : public ASTWalker {
                                CE->getResultType(), E->getLoc(), Where);
     }
     if (AbstractClosureExpr *closure = dyn_cast<AbstractClosureExpr>(E)) {
-      if (shouldWalkIntoClosure(closure)) {
-        walkAbstractClosure(closure);
-        return Action::SkipChildren(E);
-      }
+      walkAbstractClosure(closure);
+      return Action::SkipChildren(E);
     }
 
     if (auto CE = dyn_cast<ExplicitCastExpr>(E)) {
@@ -3789,13 +3782,22 @@ class ExprAvailabilityWalker : public ASTWalker {
   }
 
   PreWalkResult<Stmt *> walkToStmtPre(Stmt *S) override {
-
-    // We end up here when checking the output of the result builder transform,
-    // which includes closures that are not "separately typechecked" and yet
-    // contain statements and declarations. We need to walk them recursively,
-    // since these availability for these statements is not diagnosed from
-    // typeCheckStmt() as usual.
-    diagnoseStmtAvailability(S, Where.getDeclContext(), /*walkRecursively=*/true);
+    // We need to recursively call diagnoseExprAvailability for any
+    // sub-expressions in the statement since the availability context may
+    // differ, e.g for things like `guard #available(...)`.
+    class StmtRecurseWalker : public BaseDiagnosticWalker {
+      DeclContext *DC;
+
+    public:
+      StmtRecurseWalker(DeclContext *DC) : DC(DC) {}
+
+      PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
+        diagnoseExprAvailability(E, DC);
+        return Action::SkipNode(E);
+      }
+    };
+    StmtRecurseWalker W(Where.getDeclContext());
+    S->walk(W);
     return Action::SkipNode(S);
   }
 
@@ -3942,10 +3944,6 @@ class ExprAvailabilityWalker : public ASTWalker {
     walkInContext(E->getSubExpr(), accessContext);
   }
 
-  bool shouldWalkIntoClosure(AbstractClosureExpr *closure) const {
-    return true;
-  }
-
   /// Walk an abstract closure expression, checking for availability
   void walkAbstractClosure(AbstractClosureExpr *closure) {
     // Do the walk with the closure set as the decl context of the 'where'
@@ -4395,26 +4393,29 @@ void swift::diagnoseExprAvailability(const Expr *E, DeclContext *DC) {
 namespace {
 
 class StmtAvailabilityWalker : public BaseDiagnosticWalker {
+  const Stmt *TopLevelStmt;
   DeclContext *DC;
-  bool WalkRecursively;
 
 public:
-  explicit StmtAvailabilityWalker(DeclContext *dc, bool walkRecursively)
-    : DC(dc), WalkRecursively(walkRecursively) {}
+  explicit StmtAvailabilityWalker(const Stmt *S, DeclContext *dc)
+    : TopLevelStmt(S), DC(dc) {}
 
   PreWalkResult<Stmt *> walkToStmtPre(Stmt *S) override {
-    if (!WalkRecursively && isa<BraceStmt>(S))
-      return Action::SkipNode(S);
-
-    return Action::Continue(S);
+    // `diagnoseStmtAvailability` is called for every statement, so we don't
+    // want to walk into any nested statements.
+    return Action::VisitNodeIf(S == TopLevelStmt, S);
   }
 
   PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
-    if (WalkRecursively)
-      diagnoseExprAvailability(E, DC);
+    // Handled by ExprAvailabilityWalker.
     return Action::SkipNode(E);
   }
 
+  PreWalkAction walkToDeclPre(Decl *D) override {
+    // Handled by DeclAvailabilityChecker.
+    return Action::SkipNode();
+  }
+
   PreWalkAction walkToTypeReprPre(TypeRepr *T) override {
     auto where = ExportContext::forFunctionBody(DC, T->getStartLoc());
     diagnoseTypeReprAvailability(T, where);
@@ -4433,13 +4434,8 @@ class StmtAvailabilityWalker : public BaseDiagnosticWalker {
 };
 }
 
-void swift::diagnoseStmtAvailability(const Stmt *S, DeclContext *DC,
-                                     bool walkRecursively) {
-  // We'll visit the individual statements when we check them.
-  if (!walkRecursively && isa<BraceStmt>(S))
-    return;
-
-  StmtAvailabilityWalker walker(DC, walkRecursively);
+void swift::diagnoseStmtAvailability(const Stmt *S, DeclContext *DC) {
+  StmtAvailabilityWalker walker(S, DC);
   const_cast<Stmt*>(S)->walk(walker);
 }
 

lib/Sema/TypeCheckAvailability.h

@@ -211,12 +211,8 @@ bool isExported(const Decl *D);
 void diagnoseExprAvailability(const Expr *E, DeclContext *DC);
 
 /// Diagnose uses of unavailable declarations in statements (via patterns, etc)
-/// but not expressions, unless \p walkRecursively was specified.
-///
-/// \param walkRecursively Whether nested statements and expressions should
-/// be visited, too.
-void diagnoseStmtAvailability(const Stmt *S, DeclContext *DC,
-                              bool walkRecursively=false);
+/// but not expressions.
+void diagnoseStmtAvailability(const Stmt *S, DeclContext *DC);
 
 /// Checks both a TypeRepr and a Type, but avoids emitting duplicate
 /// diagnostics by only checking the Type if the TypeRepr succeeded.

lib/Sema/TypeCheckConstraints.cpp

@@ -327,9 +327,10 @@ void ParentConditionalConformance::diagnoseConformanceStack(
 
 namespace {
 /// Produce any additional syntactic diagnostics for a SyntacticElementTarget.
-class SyntacticDiagnosticWalker final : public ASTWalker {
+class SyntacticDiagnosticWalker final : public BaseDiagnosticWalker {
   const SyntacticElementTarget &Target;
   bool IsTopLevelExprStmt;
+  unsigned ExprDepth = 0;
 
   SyntacticDiagnosticWalker(const SyntacticElementTarget &target,
                             bool isExprStmt)
@@ -341,27 +342,30 @@ class SyntacticDiagnosticWalker final : public ASTWalker {
     target.walk(walker);
   }
 
-  MacroWalking getMacroWalkingBehavior() const override {
-    // We only want to walk macro arguments. Expansions will be walked when
-    // they're type-checked, not as part of the surrounding code.
-    return MacroWalking::Arguments;
+  PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
+    // We only want to call the diagnostic logic for the top-level expression,
+    // since the underlying logic will visit each sub-expression. We want to
+    // continue walking however to diagnose any child statements in e.g
+    // closures.
+    if (ExprDepth == 0) {
+      auto isExprStmt = (E == Target.getAsExpr()) ? IsTopLevelExprStmt : false;
+      performSyntacticExprDiagnostics(E, Target.getDeclContext(), isExprStmt);
+    }
+    ExprDepth += 1;
+    return Action::Continue(E);
   }
 
-  PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {
-    auto isExprStmt = (expr == Target.getAsExpr()) ? IsTopLevelExprStmt : false;
-    performSyntacticExprDiagnostics(expr, Target.getDeclContext(), isExprStmt);
-    return Action::SkipNode(expr);
+  PostWalkResult<Expr *> walkToExprPost(Expr *E) override {
+    assert(ExprDepth > 0);
+    ExprDepth -= 1;
+    return Action::Continue(E);
   }
 
   PreWalkResult<Stmt *> walkToStmtPre(Stmt *stmt) override {
     performStmtDiagnostics(stmt, Target.getDeclContext());
     return Action::Continue(stmt);
   }
 
-  PreWalkAction walkToDeclPre(Decl *D) override {
-    return Action::VisitNodeIf(isa<PatternBindingDecl>(D));
-  }
-
   PreWalkAction walkToTypeReprPre(TypeRepr *typeRepr) override {
     return Action::SkipNode();
   }

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);
     }
   }