[Sema] A couple of fixes for MiscDiagnostics on macro expansions

lib/Sema/CSApply.cpp

@@ -408,13 +408,34 @@ namespace {
   /// Rewrites an expression by applying the solution of a constraint
   /// system to that expression.
   class ExprRewriter : public ExprVisitor<ExprRewriter, Expr *> {
+    // Delayed items to type-check.
+    SmallVector<Decl *, 4> LocalDeclsToTypeCheck;
+    SmallVector<MacroExpansionExpr *, 4> MacrosToExpand;
+
   public:
     ConstraintSystem &cs;
     DeclContext *dc;
     Solution &solution;
     std::optional<SyntacticElementTarget> target;
     bool SuppressDiagnostics;
 
+    ExprRewriter(ConstraintSystem &cs, Solution &solution,
+                 std::optional<SyntacticElementTarget> target,
+                 bool suppressDiagnostics)
+        : cs(cs), dc(target ? target->getDeclContext() : cs.DC),
+          solution(solution), target(target),
+          SuppressDiagnostics(suppressDiagnostics) {}
+
+    ConstraintSystem &getConstraintSystem() const { return cs; }
+
+    void addLocalDeclToTypeCheck(Decl *D) {
+      LocalDeclsToTypeCheck.push_back(D);
+    }
+
+    void addMacroToExpand(MacroExpansionExpr *E) {
+      MacrosToExpand.push_back(E);
+    }
+
     /// Coerce the given tuple to another tuple type.
     ///
     /// \param expr The expression we're converting.
@@ -2631,15 +2652,6 @@ namespace {
     }
 
   public:
-    ExprRewriter(ConstraintSystem &cs, Solution &solution,
-                 std::optional<SyntacticElementTarget> target,
-                 bool suppressDiagnostics)
-        : cs(cs), dc(target ? target->getDeclContext() : cs.DC),
-          solution(solution), target(target),
-          SuppressDiagnostics(suppressDiagnostics) {}
-
-    ConstraintSystem &getConstraintSystem() const { return cs; }
-
     /// Simplify the expression type and return the expression.
     ///
     /// This routine is used for 'simple' expressions that only need their
@@ -5528,16 +5540,19 @@ namespace {
 
       // FIXME: Expansion should be lazy.
       // i.e. 'ExpandMacroExpansionExprRequest' should be sinked into
-      // 'getRewritten()', and performed on-demand.
+      // 'getRewritten()', and performed on-demand. Unfortunately that requires
+      // auditing every ASTWalker's `getMacroWalkingBehavior` since
+      // `MacroWalking::Expansion` does not actually kick expansion.
       if (!cs.Options.contains(ConstraintSystemFlags::DisableMacroExpansions) &&
           // Do not expand macros inside macro arguments. For example for
           // '#stringify(#assert(foo))' when typechecking `#assert(foo)`,
           // we don't want to expand it.
           llvm::none_of(llvm::ArrayRef(ExprStack).drop_back(1),
                         [](Expr *E) { return isa<MacroExpansionExpr>(E); })) {
-        (void)evaluateOrDefault(cs.getASTContext().evaluator,
-                                ExpandMacroExpansionExprRequest{E},
-                                std::nullopt);
+        // We need to delay the expansion until we're done applying the solution
+        // since running MiscDiagnostics on the expansion may walk up and query
+        // the type of a parent closure, e.g `diagnoseImplicitSelfUseInClosure`.
+        addMacroToExpand(E);
       }
 
       cs.cacheExprTypes(E);
@@ -5604,6 +5619,18 @@ namespace {
                       diag::add_consume_to_silence)
             .fixItInsert(coercion->getStartLoc(), "consume ");
       }
+
+      // Type-check any local decls encountered.
+      for (auto *D : LocalDeclsToTypeCheck)
+        TypeChecker::typeCheckDecl(D);
+
+      // Expand any macros encountered.
+      // FIXME: Expansion should be lazy.
+      auto &eval = cs.getASTContext().evaluator;
+      for (auto *E : MacrosToExpand) {
+        (void)evaluateOrDefault(eval, ExpandMacroExpansionExprRequest{E},
+                                std::nullopt);
+      }
     }
 
     /// Diagnose an optional injection that is probably not what the
@@ -8805,22 +8832,15 @@ namespace {
 
   class ExprWalker : public ASTWalker, public SyntacticElementTargetRewriter {
     ExprRewriter &Rewriter;
-    SmallVector<Decl *, 4> LocalDeclsToTypeCheck;
 
   public:
     ExprWalker(ExprRewriter &Rewriter) : Rewriter(Rewriter) { }
 
-    ~ExprWalker() {
-      // Type-check any local decls encountered.
-      for (auto *D : LocalDeclsToTypeCheck)
-        TypeChecker::typeCheckDecl(D);
-    }
-
     Solution &getSolution() const override { return Rewriter.solution; }
     DeclContext *&getCurrentDC() const override { return Rewriter.dc; }
 
     void addLocalDeclToTypeCheck(Decl *D) override {
-      LocalDeclsToTypeCheck.push_back(D);
+      Rewriter.addLocalDeclToTypeCheck(D);
     }
 
     bool shouldWalkIntoPropertyWrapperPlaceholderValue() override {

lib/Sema/MiscDiagnostics.cpp

@@ -97,10 +97,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
     DiagnoseWalker(const DeclContext *DC, bool isExprStmt)
         : IsExprStmt(isExprStmt), Ctx(DC->getASTContext()), DC(DC) {}
 
-    MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
-    }
-
     PreWalkAction walkToTypeReprPre(TypeRepr *T) override {
       return Action::Continue();
     }
@@ -1581,7 +1577,9 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) {
     }
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -4576,7 +4574,9 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
     DiagnoseWalker(ASTContext &ctx) : Ctx(ctx) { }
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<ArgumentList *>
@@ -4699,7 +4699,9 @@ class ObjCSelectorWalker : public ASTWalker {
     : Ctx(dc->getASTContext()), DC(dc), SelectorTy(selectorTy) { }
 
   MacroWalking getMacroWalkingBehavior() const override {
-    return MacroWalking::Expansion;
+    // Macro expansions will be walked when they're type-checked, not as
+    // part of the surrounding code.
+    return MacroWalking::None;
   }
 
   PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {
@@ -5561,7 +5563,9 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E,
     }
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5634,7 +5638,9 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
     }
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5719,7 +5725,9 @@ static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E,
     }
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5770,7 +5778,9 @@ static void diagnoseExplicitUseOfLazyVariableStorage(const Expr *E,
     }
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5901,7 +5911,9 @@ static void diagnoseComparisonWithNaN(const Expr *E, const DeclContext *DC) {
     }
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5934,7 +5946,9 @@ static void diagUnqualifiedAccessToMethodNamedSelf(const Expr *E,
     DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()), DC(DC) {}
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -6089,7 +6103,9 @@ diagnoseDictionaryLiteralDuplicateKeyEntries(const Expr *E,
     DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()) {}
 
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
 
     PreWalkResult<Expr *> walkToExprPre(Expr *E) override {

lib/Sema/MiscDiagnostics.h

@@ -138,9 +138,10 @@ namespace swift {
       return Action::VisitNodeIf(isa<PatternBindingDecl>(D));
     }
 
-    // Only emit diagnostics in the expansion of macros.
     MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Expansion;
+      // Macro expansions will be walked when they're type-checked, not as
+      // part of the surrounding code.
+      return MacroWalking::None;
     }
   };
 

lib/Sema/TypeCheckConstraints.cpp

@@ -332,7 +332,9 @@ class SyntacticDiagnosticWalker final : public ASTWalker {
   }
 
   MacroWalking getMacroWalkingBehavior() const override {
-    return MacroWalking::Expansion;
+    // Macro expansions will be walked when they're type-checked, not as
+    // part of the surrounding code.
+    return MacroWalking::None;
   }
 
   PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {

test/Macros/macro_misc_diags.swift

@@ -0,0 +1,125 @@
+// REQUIRES: swift_swift_parser
+
+// RUN: %empty-directory(%t)
+// RUN: split-file --leading-lines %s %t
+
+// RUN: %host-build-swift -swift-version 5 -emit-library -o %t/%target-library-name(MacroPlugin) -module-name=MacroPlugin %t/MacroPlugin.swift -g -no-toolchain-stdlib-rpath
+
+// RUN: not %target-swift-frontend -typecheck -swift-version 6 -load-plugin-library %t/%target-library-name(MacroPlugin) %t/Client.swift -module-name Client -diagnostic-style=llvm 2> %t/diags
+// RUN: %FileCheck --check-prefix=CHECK-DIAG --implicit-check-not="{{error|warning}}: " -input-file=%t/diags %s
+
+//--- MacroPlugin.swift
+import SwiftSyntax
+import SwiftSyntaxMacros
+
+public struct IdentityMacro: ExpressionMacro {
+  public static func expansion(
+    of macro: some FreestandingMacroExpansionSyntax,
+    in context: some MacroExpansionContext
+  ) -> ExprSyntax {
+    guard let argument = macro.arguments.first else {
+      fatalError()
+    }
+    return "\(argument)"
+  }
+}
+
+public struct TrailingClosureMacro: ExpressionMacro {
+  public static func expansion(
+    of macro: some FreestandingMacroExpansionSyntax,
+    in context: some MacroExpansionContext
+  ) -> ExprSyntax {
+    guard let argument = macro.trailingClosure else {
+      fatalError()
+    }
+    return "\(argument)"
+  }
+}
+
+public struct MakeBinding : DeclarationMacro {
+  static public func expansion(
+    of node: some FreestandingMacroExpansionSyntax,
+    in context: some MacroExpansionContext
+  ) throws -> [DeclSyntax] {
+    guard let arg = node.arguments.first else {
+      fatalError()
+    }
+    return ["let x = \(arg)"]
+  }
+}
+
+//--- Client.swift
+@freestanding(expression)
+macro identity<T>(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "IdentityMacro")
+
+@freestanding(expression)
+macro trailingClosure<T>(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "TrailingClosureMacro")
+
+@freestanding(declaration, names: named(x))
+macro makeBinding<T>(_ x: T) = #externalMacro(module: "MacroPlugin", type: "MakeBinding")
+
+@available(*, deprecated)
+func deprecatedFunc() -> Int { 0 }
+
+// FIXME: We also ought to be diagnosing the macro argument
+_ = #identity(Int)
+// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf_.swift:1:1: error: expected member name or initializer call after type name
+
+_ = {
+  _ = #identity(Int)
+  // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf0_.swift:1:1: error: expected member name or initializer call after type name
+}
+
+_ = #identity(deprecatedFunc())
+// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf1_.swift:1:1: warning: 'deprecatedFunc()' is deprecated
+// CHECK-DIAG: Client.swift:[[@LINE-2]]:15: warning: 'deprecatedFunc()' is deprecated
+
+#makeBinding(deprecatedFunc())
+// CHECK-DIAG: Client.swift:[[@LINE-1]]:14: warning: 'deprecatedFunc()' is deprecated
+// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}makeBindingfMf_.swift:1:9: warning: 'deprecatedFunc()' is deprecated
+// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-4]]{{.*}}makeBindingfMf_.swift:1:5: warning: initialization of immutable value 'x' was never used
+
+struct S1 {
+  #makeBinding(deprecatedFunc())
+  // CHECK-DIAG: Client.swift:[[@LINE-1]]:16: warning: 'deprecatedFunc()' is deprecated
+  // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}makeBindingfMf_.swift:1:9: warning: 'deprecatedFunc()' is deprecated
+}
+
+struct S2 {
+  #makeBinding({deprecatedFunc()})
+  // CHECK-DIAG: Client.swift:[[@LINE-1]]:17: warning: 'deprecatedFunc()' is deprecated
+  // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}makeBindingfMf_.swift:2:5: warning: 'deprecatedFunc()' is deprecated
+}
+
+func takesClosure(_ fn: () -> Void) -> Int? { nil }
+
+_ = #trailingClosure {
+  if let _ = takesClosure {} {}
+  // CHECK-DIAG: Client.swift:[[@LINE-1]]:27: warning: trailing closure in this context is confusable with the body of the statement; pass as a parenthesized argument to silence this warning
+  // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-4]]{{.*}}trailingClosurefMf_.swift:2:27: warning: trailing closure in this context is confusable with the body of the statement
+}
+
+// rdar://138997009 - Make sure we don't crash in MiscDiagnostics' implicit
+// self diagnosis.
+struct rdar138997009 {
+  func foo() {}
+  func bar() {
+    _ = {
+      _ = #trailingClosure {
+        foo()
+      }
+    }
+  }
+}
+
+class rdar138997009_Class {
+  func foo() {}
+  func bar() {
+    _ = {
+      _ = #trailingClosure {
+        foo()
+        // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}trailingClosurefMf_.swift:2:9: error: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit
+      }
+    }
+  }
+}