Skip to content

Commit

Permalink
[Sema] Downgrade implicit self diag to warning for macro args
Browse files Browse the repository at this point in the history
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
  • Loading branch information
hamishknight committed Jan 10, 2025
1 parent 7959485 commit e80f6bb
Show file tree
Hide file tree
Showing 5 changed files with 191 additions and 18 deletions.
78 changes: 63 additions & 15 deletions lib/Sema/MiscDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand All @@ -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()) {
Expand All @@ -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);
Expand All @@ -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
Expand Down
5 changes: 3 additions & 2 deletions lib/Sema/MiscDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
21 changes: 20 additions & 1 deletion test/Macros/macro_misc_diags.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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 }

Expand Down Expand Up @@ -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
}
}
}
44 changes: 44 additions & 0 deletions test/Macros/macro_misc_diags_swift5.swift
Original file line number Diff line number Diff line change
@@ -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
}
}
}
}
61 changes: 61 additions & 0 deletions test/Macros/macro_misc_diags_swift7.swift
Original file line number Diff line number Diff line change
@@ -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
}
}
}

0 comments on commit e80f6bb

Please sign in to comment.