From 6352b01635c8a0517666b8286c48ef8f72d86b0a Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Sat, 9 Nov 2024 00:24:52 +0000 Subject: [PATCH 1/4] [Sema] Avoid double-diagnosing in macro expansions Avoid walking into macro expansions for MiscDiagnostics, the expansions will instead be visited when they're type-checked on their own. --- lib/Sema/MiscDiagnostics.cpp | 44 +++++++++---- lib/Sema/MiscDiagnostics.h | 5 +- lib/Sema/TypeCheckConstraints.cpp | 4 +- test/Macros/macro_misc_diags.swift | 100 +++++++++++++++++++++++++++++ 4 files changed, 136 insertions(+), 17 deletions(-) create mode 100644 test/Macros/macro_misc_diags.swift diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 7c52a8d9403cb..d2484ea684de3 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/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 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 @@ -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 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 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 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 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 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 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 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 walkToExprPre(Expr *E) override { diff --git a/lib/Sema/MiscDiagnostics.h b/lib/Sema/MiscDiagnostics.h index deecbab75422b..6f784ff0abae8 100644 --- a/lib/Sema/MiscDiagnostics.h +++ b/lib/Sema/MiscDiagnostics.h @@ -138,9 +138,10 @@ namespace swift { return Action::VisitNodeIf(isa(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; } }; diff --git a/lib/Sema/TypeCheckConstraints.cpp b/lib/Sema/TypeCheckConstraints.cpp index f2524ef717f08..9ce41d049896d 100644 --- a/lib/Sema/TypeCheckConstraints.cpp +++ b/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 walkToExprPre(Expr *expr) override { diff --git a/test/Macros/macro_misc_diags.swift b/test/Macros/macro_misc_diags.swift new file mode 100644 index 0000000000000..2fe9e87042141 --- /dev/null +++ b/test/Macros/macro_misc_diags.swift @@ -0,0 +1,100 @@ +// 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(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "IdentityMacro") + +@freestanding(expression) +macro trailingClosure(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "TrailingClosureMacro") + +@freestanding(declaration, names: named(x)) +macro makeBinding(_ 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 +} From caceca6530fd0b91e76529c1fa8b0e39302ffd70 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Sat, 9 Nov 2024 00:24:53 +0000 Subject: [PATCH 2/4] [CS] NFC: Move some code Always bothered me the constructor for ExprRewriter is buried in the middle of the class. --- lib/Sema/CSApply.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp index 86a6e4c5539e8..69f9c95eded28 100644 --- a/lib/Sema/CSApply.cpp +++ b/lib/Sema/CSApply.cpp @@ -415,6 +415,15 @@ namespace { std::optional target; bool SuppressDiagnostics; + ExprRewriter(ConstraintSystem &cs, Solution &solution, + std::optional target, + bool suppressDiagnostics) + : cs(cs), dc(target ? target->getDeclContext() : cs.DC), + solution(solution), target(target), + SuppressDiagnostics(suppressDiagnostics) {} + + ConstraintSystem &getConstraintSystem() const { return cs; } + /// Coerce the given tuple to another tuple type. /// /// \param expr The expression we're converting. @@ -2631,15 +2640,6 @@ namespace { } public: - ExprRewriter(ConstraintSystem &cs, Solution &solution, - std::optional 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 From be94e5d30557b66a1b204094e3d9205946b5c8f5 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Sat, 9 Nov 2024 00:24:53 +0000 Subject: [PATCH 3/4] [CS] NFC: Sink `LocalDeclsToTypeCheck` into ExprRewriter --- lib/Sema/CSApply.cpp | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp index 69f9c95eded28..c061d762afe57 100644 --- a/lib/Sema/CSApply.cpp +++ b/lib/Sema/CSApply.cpp @@ -408,6 +408,9 @@ namespace { /// Rewrites an expression by applying the solution of a constraint /// system to that expression. class ExprRewriter : public ExprVisitor { + // Delayed items to type-check. + SmallVector LocalDeclsToTypeCheck; + public: ConstraintSystem &cs; DeclContext *dc; @@ -424,6 +427,10 @@ namespace { ConstraintSystem &getConstraintSystem() const { return cs; } + void addLocalDeclToTypeCheck(Decl *D) { + LocalDeclsToTypeCheck.push_back(D); + } + /// Coerce the given tuple to another tuple type. /// /// \param expr The expression we're converting. @@ -5604,6 +5611,10 @@ namespace { diag::add_consume_to_silence) .fixItInsert(coercion->getStartLoc(), "consume "); } + + // Type-check any local decls encountered. + for (auto *D : LocalDeclsToTypeCheck) + TypeChecker::typeCheckDecl(D); } /// Diagnose an optional injection that is probably not what the @@ -8805,22 +8816,15 @@ namespace { class ExprWalker : public ASTWalker, public SyntacticElementTargetRewriter { ExprRewriter &Rewriter; - SmallVector 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 { From 9c3b8a62563b6a1d00357b3a00de004507aaa7a3 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Sat, 9 Nov 2024 00:24:53 +0000 Subject: [PATCH 4/4] [CS] Delay macro expansion until end of CSApply Attempting to expand macros in the middle of CSApply can result in attempting to run MiscDiagnostics within a closure that hasn't yet had the solution applied to the AST, which can crash the implicit-self diagnostic logic. Move the expansion to the end of CSApply such that expansions are type-checked along with local decls, ensuring it's run after the solution has been applied to the AST. rdar://138997009 --- lib/Sema/CSApply.cpp | 24 ++++++++++++++++++++---- test/Macros/macro_misc_diags.swift | 25 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp index c061d762afe57..c08fa8aad9d98 100644 --- a/lib/Sema/CSApply.cpp +++ b/lib/Sema/CSApply.cpp @@ -410,6 +410,7 @@ namespace { class ExprRewriter : public ExprVisitor { // Delayed items to type-check. SmallVector LocalDeclsToTypeCheck; + SmallVector MacrosToExpand; public: ConstraintSystem &cs; @@ -431,6 +432,10 @@ namespace { 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. @@ -5535,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(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); @@ -5615,6 +5623,14 @@ namespace { // 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 diff --git a/test/Macros/macro_misc_diags.swift b/test/Macros/macro_misc_diags.swift index 2fe9e87042141..d7207429eb450 100644 --- a/test/Macros/macro_misc_diags.swift +++ b/test/Macros/macro_misc_diags.swift @@ -98,3 +98,28 @@ _ = #trailingClosure { // 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 + } + } + } +}