Merge pull request swiftlang#76193 from hamishknight/miscing-diag

[Sema] Walk patterns for syntactic diagnostics

Author: hamishknight

include/swift/AST/ASTWalker.h

@@ -634,16 +634,6 @@ class ASTWalker {
     }
   }
 
-  /// This method configures whether the walker should visit the body of a
-  /// closure that was checked separately from its enclosing expression.
-  ///
-  /// For work that is performed for every top-level expression, this should
-  /// be overridden to return false, to avoid duplicating work or visiting
-  /// bodies of closures that have not yet been type checked.
-  virtual bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *) {
-    return true;
-  }
-
   /// This method configures whether the walker should visit the body of a
   /// TapExpr.
   virtual bool shouldWalkIntoTapExpression() { return true; }

lib/AST/ASTWalker.cpp

@@ -1012,12 +1012,6 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
         return nullptr;
     }
 
-    // If the closure was separately type checked and we don't want to
-    // visit separately-checked closure bodies, bail out now.
-    if (expr->isSeparatelyTypeChecked() &&
-        !Walker.shouldWalkIntoSeparatelyCheckedClosure(expr))
-      return expr;
-
     // Handle other closures.
     if (BraceStmt *body = cast_or_null<BraceStmt>(doIt(expr->getBody()))) {
       expr->setBody(body);

lib/Sema/MiscDiagnostics.cpp

@@ -59,32 +59,6 @@ static Expr *isImplicitPromotionToOptional(Expr *E) {
   return nullptr;
 }
 
-ASTWalker::PreWalkAction BaseDiagnosticWalker::walkToDeclPre(Decl *D) {
-  return Action::VisitNodeIf(isa<ClosureExpr>(D->getDeclContext()) &&
-                             shouldWalkIntoDeclInClosureContext(D));
-}
-
-bool BaseDiagnosticWalker::shouldWalkIntoDeclInClosureContext(Decl *D) {
-  auto *closure = dyn_cast<ClosureExpr>(D->getDeclContext());
-  assert(closure);
-
-  if (closure->isSeparatelyTypeChecked())
-    return false;
-
-  // Let's not walk into declarations contained in a multi-statement
-  // closure because they'd be handled via `typeCheckDecl` that runs
-  // syntactic diagnostics.
-  if (!closure->hasSingleExpressionBody()) {
-    // Since pattern bindings get their types through solution application,
-    // `typeCheckDecl` doesn't touch initializers (because they are already
-    // fully type-checked), so pattern bindings have to be allowed to be
-    // walked to diagnose syntactic issues.
-    return isa<PatternBindingDecl>(D);
-  }
-
-  return true;
-}
-
 /// Diagnose syntactic restrictions of expressions.
 ///
 ///   - Module values may only occur as part of qualification.
@@ -127,10 +101,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
       return MacroWalking::Expansion;
     }
 
-    PreWalkResult<Pattern *> walkToPatternPre(Pattern *P) override {
-      return Action::SkipNode(P);
-    }
-
     PreWalkAction walkToTypeReprPre(TypeRepr *T) override {
       return Action::Continue();
     }
@@ -1609,10 +1579,6 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) {
              cast<VarDecl>(DRE->getDecl())->isSelfParameter();
     }
 
-    bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-      return false;
-    }
-
     bool shouldWalkCaptureInitializerExpressions() override { return true; }
 
     MacroWalking getMacroWalkingBehavior() const override {
@@ -4594,10 +4560,6 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
   public:
     DiagnoseWalker(ASTContext &ctx) : Ctx(ctx) { }
 
-    bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-      return false;
-    }
-
     bool shouldWalkCaptureInitializerExpressions() override { return true; }
 
     MacroWalking getMacroWalkingBehavior() const override {
@@ -4723,10 +4685,6 @@ class ObjCSelectorWalker : public ASTWalker {
   ObjCSelectorWalker(const DeclContext *dc, Type selectorTy)
     : Ctx(dc->getASTContext()), DC(dc), SelectorTy(selectorTy) { }
 
-  bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-    return false;
-  }
-
   bool shouldWalkCaptureInitializerExpressions() override { return true; }
 
   MacroWalking getMacroWalkingBehavior() const override {
@@ -5589,10 +5547,6 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E,
       }
     }
 
-    bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-      return false;
-    }
-
     bool shouldWalkCaptureInitializerExpressions() override { return true; }
 
     MacroWalking getMacroWalkingBehavior() const override {
@@ -5668,10 +5622,6 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
       }
     }
 
-    bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-      return false;
-    }
-
     bool shouldWalkCaptureInitializerExpressions() override { return true; }
 
     MacroWalking getMacroWalkingBehavior() const override {
@@ -5974,10 +5924,6 @@ static void diagUnqualifiedAccessToMethodNamedSelf(const Expr *E,
   public:
     DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()), DC(DC) {}
 
-    bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-      return false;
-    }
-
     MacroWalking getMacroWalkingBehavior() const override {
       return MacroWalking::Expansion;
     }
@@ -6133,10 +6079,6 @@ diagnoseDictionaryLiteralDuplicateKeyEntries(const Expr *E,
   public:
     DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()) {}
 
-    bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-      return false;
-    }
-
     MacroWalking getMacroWalkingBehavior() const override {
       return MacroWalking::Expansion;
     }

lib/Sema/MiscDiagnostics.h

@@ -133,19 +133,14 @@ namespace swift {
                                              ForEachStmt *forEach);
 
   class BaseDiagnosticWalker : public ASTWalker {
-    PreWalkAction walkToDeclPre(Decl *D) override;
-
-    bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-      return false;
+    PreWalkAction walkToDeclPre(Decl *D) override {
+      return Action::VisitNodeIf(isa<PatternBindingDecl>(D));
     }
 
     // Only emit diagnostics in the expansion of macros.
     MacroWalking getMacroWalkingBehavior() const override {
       return MacroWalking::Expansion;
     }
-
-  private:
-    static bool shouldWalkIntoDeclInClosureContext(Decl *D);
   };
 
   // A simple, deferred diagnostic container.

lib/Sema/TypeCheckAvailability.cpp

@@ -3472,10 +3472,6 @@ class ExprAvailabilityWalker : public ASTWalker {
   explicit ExprAvailabilityWalker(const ExportContext &Where)
     : Context(Where.getDeclContext()->getASTContext()), Where(Where) {}
 
-  bool shouldWalkIntoSeparatelyCheckedClosure(ClosureExpr *expr) override {
-    return false;
-  }
-
   MacroWalking getMacroWalkingBehavior() const override {
     // Expanded source should be type checked and diagnosed separately.
     return MacroWalking::Arguments;

lib/Sema/TypeCheckConstraints.cpp

@@ -327,50 +327,40 @@ void ParentConditionalConformance::diagnoseConformanceStack(
 }
 
 namespace {
-/// Produce any additional syntactic diagnostics for the body of a function
-/// that had a result builder applied.
-class FunctionSyntacticDiagnosticWalker : public ASTWalker {
-  SmallVector<DeclContext *, 4> dcStack;
+/// Produce any additional syntactic diagnostics for a SyntacticElementTarget.
+class SyntacticDiagnosticWalker final : public ASTWalker {
+  const SyntacticElementTarget &Target;
+  bool IsTopLevelExprStmt;
+
+  SyntacticDiagnosticWalker(const SyntacticElementTarget &target,
+                            bool isExprStmt)
+      : Target(target), IsTopLevelExprStmt(isExprStmt) {}
 
 public:
-  FunctionSyntacticDiagnosticWalker(DeclContext *dc) { dcStack.push_back(dc); }
+  static void check(const SyntacticElementTarget &target, bool isExprStmt) {
+    auto walker = SyntacticDiagnosticWalker(target, isExprStmt);
+    target.walk(walker);
+  }
 
   MacroWalking getMacroWalkingBehavior() const override {
     return MacroWalking::Expansion;
   }
 
   PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {
-    performSyntacticExprDiagnostics(expr, dcStack.back(), /*isExprStmt=*/false);
-
-    if (auto closure = dyn_cast<ClosureExpr>(expr)) {
-      if (closure->isSeparatelyTypeChecked()) {
-        dcStack.push_back(closure);
-        return Action::Continue(expr);
-      }
-    }
-
+    auto isExprStmt = (expr == Target.getAsExpr()) ? IsTopLevelExprStmt : false;
+    performSyntacticExprDiagnostics(expr, Target.getDeclContext(), isExprStmt);
     return Action::SkipNode(expr);
   }
 
-  PostWalkResult<Expr *> walkToExprPost(Expr *expr) override {
-    if (auto closure = dyn_cast<ClosureExpr>(expr)) {
-      if (closure->isSeparatelyTypeChecked()) {
-        assert(dcStack.back() == closure);
-        dcStack.pop_back();
-      }
-    }
-
-    return Action::Continue(expr);
-  }
-
   PreWalkResult<Stmt *> walkToStmtPre(Stmt *stmt) override {
-    performStmtDiagnostics(stmt, dcStack.back());
+    performStmtDiagnostics(stmt, Target.getDeclContext());
     return Action::Continue(stmt);
   }
 
-  PreWalkResult<Pattern *> walkToPatternPre(Pattern *pattern) override {
-    return Action::SkipNode(pattern);
+  PreWalkAction walkToDeclPre(Decl *D) override {
+    return Action::VisitNodeIf(isa<PatternBindingDecl>(D));
   }
+
   PreWalkAction walkToTypeReprPre(TypeRepr *typeRepr) override {
     return Action::SkipNode();
   }
@@ -382,41 +372,7 @@ class FunctionSyntacticDiagnosticWalker : public ASTWalker {
 
 void constraints::performSyntacticDiagnosticsForTarget(
     const SyntacticElementTarget &target, bool isExprStmt) {
-  auto *dc = target.getDeclContext();
-  switch (target.kind) {
-  case SyntacticElementTarget::Kind::expression: {
-    // First emit diagnostics for the main expression.
-    performSyntacticExprDiagnostics(target.getAsExpr(), dc, isExprStmt);
-    return;
-  }
-
-  case SyntacticElementTarget::Kind::forEachPreamble: {
-    auto *stmt = target.getAsForEachStmt();
-
-    // First emit diagnostics for the main expression.
-    performSyntacticExprDiagnostics(stmt->getTypeCheckedSequence(), dc,
-                                    isExprStmt);
-
-    if (auto *whereExpr = stmt->getWhere())
-      performSyntacticExprDiagnostics(whereExpr, dc, /*isExprStmt*/ false);
-    return;
-  }
-
-  case SyntacticElementTarget::Kind::function: {
-    auto *body = target.getFunctionBody();
-    FunctionSyntacticDiagnosticWalker walker(dc);
-    body->walk(walker);
-    return;
-  }
-  case SyntacticElementTarget::Kind::closure:
-  case SyntacticElementTarget::Kind::stmtCondition:
-  case SyntacticElementTarget::Kind::caseLabelItem:
-  case SyntacticElementTarget::Kind::patternBinding:
-  case SyntacticElementTarget::Kind::uninitializedVar:
-    // Nothing to do for these.
-    return;
-  }
-  llvm_unreachable("Unhandled case in switch!");
+  SyntacticDiagnosticWalker::check(target, isExprStmt);
 }
 
 #pragma mark High-level entry points

test/Sema/issue-75389.swift

@@ -0,0 +1,55 @@
+// RUN: %target-typecheck-verify-swift
+
+// https://github.com/swiftlang/swift/issues/75389
+
+@available(*, unavailable)
+func unavailableFn() -> Int { 0 }
+// expected-note@-1 5{{'unavailableFn()' has been explicitly marked unavailable here}}
+
+if case unavailableFn() = 0 {}
+// expected-error@-1 {{'unavailableFn()' is unavailable}}
+
+switch Bool.random() {
+case true where unavailableFn() == 1:
+  // expected-error@-1 {{'unavailableFn()' is unavailable}}
+  break
+default:
+  break
+}
+switch 0 {
+case unavailableFn():
+  // expected-error@-1 {{'unavailableFn()' is unavailable}}
+  break
+default:
+  break
+}
+let _ = {
+  switch Bool.random() {
+  case true where unavailableFn() == 1:
+    // expected-error@-1 {{'unavailableFn()' is unavailable}}
+    break
+  default:
+    break
+  }
+  switch 0 {
+  case unavailableFn():
+    // expected-error@-1 {{'unavailableFn()' is unavailable}}
+    break
+  default:
+    break
+  }
+}
+
+struct S {}
+
+func ~= (lhs: S.Type, rhs: S.Type) -> Bool { true }
+
+if case (S) = S.self {}
+// expected-error@-1 {{expected member name or initializer call after type name}}
+// expected-note@-2 {{add arguments after the type to construct a value of the type}}
+// expected-note@-3 {{use '.self' to reference the type object}}
+
+if case ({ if case (S) = S.self { true } else { false } }()) = true {}
+// expected-error@-1 {{expected member name or initializer call after type name}}
+// expected-note@-2 {{add arguments after the type to construct a value of the type}}
+// expected-note@-3 {{use '.self' to reference the type object}}