[Sema] Downgrade implicit self diag to warning for macro args

hamishknight · hamishknight · commit e80f6bb09478 · 2025-01-10T17:47:16.000Z
We previously missed diagnosing this for macro
args, fixing it turned out to be a bit more source
breaking than initially thought though, so downgrade
to a warning until Swift 7.

rdar://141963700
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -1721,13 +1721,18 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
   /// these diagnostics.
   SmallPtrSet<Expr *, 16> UnwrapStmtImplicitSelfExprs;
 
+  /// The number of parent macros.
+  unsigned MacroDepth = 0;
+
 public:
   explicit ImplicitSelfUsageChecker(ASTContext &Ctx, AbstractClosureExpr *ACE)
       : Ctx(Ctx), Closures() {
     if (ACE)
       Closures.push_back(ACE);
   }
 
+  bool isInMacro() const { return MacroDepth > 0; }
+
   static bool
   implicitWeakSelfReferenceIsValid510(const DeclRefExpr *DRE,
                                       const AbstractClosureExpr *inClosure) {
@@ -2219,7 +2224,35 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
     return isClosureRequiringSelfQualification(E);
   }
 
+  template <typename... ArgTypes>
+  InFlightDiagnostic diagnoseImplicitSelfUse(
+      SourceLoc loc, Expr *base, AbstractClosureExpr *closure,
+      Diag<ArgTypes...> ID,
+      typename detail::PassArgument<ArgTypes>::type... Args) {
+    std::optional<unsigned> warnUntilVersion;
+    // Prior to Swift 6, we may need to downgrade to a warning for compatibility
+    // with the 5.10 diagnostic behavior.
+    if (!Ctx.isSwiftVersionAtLeast(6) &&
+        invalidImplicitSelfShouldOnlyWarn510(base, closure)) {
+      warnUntilVersion.emplace(6);
+    }
+    // Prior to Swift 7, downgrade to a warning if we're in a macro to preserve
+    // compatibility with the Swift 6 diagnostic behavior where we previously
+    // skipped diagnosing.
+    if (!Ctx.isSwiftVersionAtLeast(7) && isInMacro())
+      warnUntilVersion.emplace(7);
+
+    auto diag = Ctx.Diags.diagnose(loc, ID, std::move(Args)...);
+    if (warnUntilVersion)
+      diag.warnUntilSwiftVersion(*warnUntilVersion);
+
+    return diag;
+  }
+
   PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
+    if (isa<MacroExpansionExpr>(E))
+      MacroDepth += 1;
+
     if (auto *CE = dyn_cast<AbstractClosureExpr>(E)) {
       if (shouldRecordClosure(CE))
         Closures.push_back(CE);
@@ -2249,12 +2282,10 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
         selfDRE = dyn_cast_or_null<DeclRefExpr>(MRE->getBase());
         auto baseName = MRE->getMember().getDecl()->getBaseName();
         memberLoc = MRE->getLoc();
-        Diags
-            .diagnose(memberLoc,
-                      diag::property_use_in_closure_without_explicit_self,
-                      baseName.getIdentifier())
-            .warnUntilSwiftVersionIf(
-                invalidImplicitSelfShouldOnlyWarn510(MRE->getBase(), ACE), 6);
+        diagnoseImplicitSelfUse(
+            memberLoc, MRE->getBase(), ACE,
+            diag::property_use_in_closure_without_explicit_self,
+            baseName.getIdentifier());
       }
 
     // Handle method calls with a specific diagnostic + fixit.
@@ -2264,12 +2295,10 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
         selfDRE = dyn_cast_or_null<DeclRefExpr>(DSCE->getBase());
         auto MethodExpr = cast<DeclRefExpr>(DSCE->getFn());
         memberLoc = DSCE->getLoc();
-        Diags
-            .diagnose(DSCE->getLoc(),
-                      diag::method_call_in_closure_without_explicit_self,
-                      MethodExpr->getDecl()->getBaseIdentifier())
-            .warnUntilSwiftVersionIf(
-                invalidImplicitSelfShouldOnlyWarn510(DSCE->getBase(), ACE), 6);
+        diagnoseImplicitSelfUse(
+            memberLoc, DSCE->getBase(), ACE,
+            diag::method_call_in_closure_without_explicit_self,
+            MethodExpr->getDecl()->getBaseIdentifier());
       }
 
     if (memberLoc.isValid()) {
@@ -2284,14 +2313,18 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
     }
 
     if (!selfDeclAllowsImplicitSelf(E, ACE)) {
-      Diags.diagnose(E->getLoc(), diag::implicit_use_of_self_in_closure)
-          .warnUntilSwiftVersionIf(invalidImplicitSelfShouldOnlyWarn510(E, ACE),
-                                   6);
+      diagnoseImplicitSelfUse(E->getLoc(), E, ACE,
+                              diag::implicit_use_of_self_in_closure);
     }
     return Action::Continue(E);
   }
 
   PostWalkResult<Expr *> walkToExprPost(Expr *E) override {
+    if (isa<MacroExpansionExpr>(E)) {
+      assert(isInMacro());
+      MacroDepth -= 1;
+    }
+
     auto *ACE = dyn_cast<AbstractClosureExpr>(E);
     if (!ACE) {
       return Action::Continue(E);
@@ -2304,6 +2337,21 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
     return Action::Continue(E);
   }
 
+  PreWalkAction walkToDeclPre(Decl *D) override {
+    if (isa<MacroExpansionDecl>(D))
+      MacroDepth += 1;
+
+    return BaseDiagnosticWalker::walkToDeclPre(D);
+  }
+
+  PostWalkAction walkToDeclPost(Decl *D) override {
+    if (isa<MacroExpansionDecl>(D)) {
+      assert(isInMacro());
+      MacroDepth -= 1;
+    }
+    return Action::Continue();
+  }
+
   PreWalkResult<Stmt *> walkToStmtPre(Stmt *S) override {
     /// Conditions like `if let self` or `guard let self`
     /// have an RHS 'self' decl that is implicit, but this is not
diff --git a/lib/Sema/MiscDiagnostics.h b/lib/Sema/MiscDiagnostics.h
@@ -134,12 +134,13 @@ namespace swift {
                                              ForEachStmt *forEach);
 
   class BaseDiagnosticWalker : public ASTWalker {
+  protected:
     PreWalkAction walkToDeclPre(Decl *D) override {
       // 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));
+      return Action::VisitChildrenIf(isa<PatternBindingDecl>(D) ||
+                                     isa<MacroExpansionDecl>(D));
     }
 
     MacroWalking getMacroWalkingBehavior() const override {
diff --git a/test/Macros/macro_misc_diags.swift b/test/Macros/macro_misc_diags.swift
@@ -48,6 +48,15 @@ public struct MakeBinding : DeclarationMacro {
   }
 }
 
+public struct MakeFunc : DeclarationMacro {
+  static public func expansion(
+    of node: some FreestandingMacroExpansionSyntax,
+    in context: some MacroExpansionContext
+  ) throws -> [DeclSyntax] {
+    ["func expansionFn() -> Int { 0 }"]
+  }
+}
+
 //--- Client.swift
 @freestanding(expression)
 macro identity<T>(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "IdentityMacro")
@@ -58,6 +67,9 @@ macro trailingClosure<T>(_ x: T) -> T = #externalMacro(module: "MacroPlugin", ty
 @freestanding(declaration, names: named(x))
 macro makeBinding<T>(_ x: T) = #externalMacro(module: "MacroPlugin", type: "MakeBinding")
 
+@freestanding(declaration, names: named(expansionFn))
+macro makeFunc<T>(_ x: T) = #externalMacro(module: "MacroPlugin", type: "MakeFunc")
+
 @available(*, deprecated)
 func deprecatedFunc() -> Int { 0 }
 
@@ -132,12 +144,19 @@ struct rdar138997009 {
 class rdar138997009_Class {
   func foo() {}
   func bar() {
+    // rdar://141963700 - Downgrade these to a warning for the macro argument,
+    // but is still an error in the expansion.
     _ = {
       _ = #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
-        // CHECK-DIAG: Client.swift:[[@LINE-2]]:9: error: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit
+        // CHECK-DIAG: Client.swift:[[@LINE-2]]:9: warning: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit; this will be an error in a future Swift language mode
       }
+      // Use an attribute to force a MacroExpansionDecl (otherwise we parse a
+      // MacroExpansionExpr)
+      @discardableResult
+      #makeFunc(foo())
+      // CHECK-DIAG: Client.swift:[[@LINE-1]]:17: warning: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit; this will be an error in a future Swift language mode
     }
   }
 }
diff --git a/test/Macros/macro_misc_diags_swift5.swift b/test/Macros/macro_misc_diags_swift5.swift
@@ -0,0 +1,44 @@
+// 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: %target-swift-frontend -typecheck -swift-version 5 -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 TrailingClosureMacro: ExpressionMacro {
+  public static func expansion(
+    of macro: some FreestandingMacroExpansionSyntax,
+    in context: some MacroExpansionContext
+  ) -> ExprSyntax {
+    guard let argument = macro.trailingClosure else {
+      fatalError()
+    }
+    return "\(argument)"
+  }
+}
+
+//--- Client.swift
+@freestanding(expression)
+macro trailingClosure<T>(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "TrailingClosureMacro")
+
+class rdar138997009_Class {
+  func foo() {}
+  func bar() {
+    // rdar://141963700 - This is downgraded to a warning for Swift 6 in the
+    // expansion, and Swift 7 for the argument.
+    _ = { [self] in
+      _ = #trailingClosure {
+        foo()
+        // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}trailingClosurefMf_.swift:2:9: warning: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in the Swift 6 language mode
+        // CHECK-DIAG: Client.swift:[[@LINE-2]]:9: warning: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit; this will be an error in a future Swift language mode
+      }
+    }
+  }
+}
diff --git a/test/Macros/macro_misc_diags_swift7.swift b/test/Macros/macro_misc_diags_swift7.swift
@@ -0,0 +1,61 @@
+// REQUIRES: swift_swift_parser
+// REQUIRES: swift7
+
+// 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 7 -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 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 MakeFunc : DeclarationMacro {
+  static public func expansion(
+    of node: some FreestandingMacroExpansionSyntax,
+    in context: some MacroExpansionContext
+  ) throws -> [DeclSyntax] {
+    ["func expansionFn() -> Int { 0 }"]
+  }
+}
+
+//--- Client.swift
+@freestanding(expression)
+macro trailingClosure<T>(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "TrailingClosureMacro")
+
+@freestanding(declaration, names: named(expansionFn))
+macro makeFunc<T>(_ x: T) = #externalMacro(module: "MacroPlugin", type: "MakeFunc")
+
+class rdar138997009_Class {
+  func foo() {}
+  func bar() {
+    // rdar://141963700 - In Swift 7 these are errors.
+    _ = {
+      _ = #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
+        // CHECK-DIAG: Client.swift:[[@LINE-2]]:9: error: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit
+      }
+      // Use an attribute to force a MacroExpansionDecl (otherwise we parse a
+      // MacroExpansionExpr)
+      @discardableResult
+      #makeFunc(foo())
+      // CHECK-DIAG: Client.swift:[[@LINE-1]]:17: error: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit
+    }
+  }
+}
