From 146e95c76ad9d1489854124f24bdaea6c2c45d58 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Sat, 9 Nov 2024 00:28:59 +0000 Subject: [PATCH 1/2] [Sema] Use BaseDiagnosticWalker consistently in MiscDiagnostics None of these walkers ought to be walking into non-PatternBinding decls, this avoids duplicate diagnostics in nested local decls in closures. --- lib/Sema/MiscDiagnostics.cpp | 80 +++---------------- .../diag_unintended_optional_behavior.swift | 10 +++ 2 files changed, 20 insertions(+), 70 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 789ec1fc36e00..827b547654e27 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1561,7 +1561,7 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) { if (!var) // Ignore subscripts return; - class DiagnoseWalker : public ASTWalker { + class DiagnoseWalker : public BaseDiagnosticWalker { ASTContext &Ctx; VarDecl *Var; const AccessorDecl *Accessor; @@ -1577,12 +1577,6 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) { cast(DRE->getDecl())->isSelfParameter(); } - MacroWalking getMacroWalkingBehavior() const override { - // 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 { Expr *subExpr; bool isStore = false; @@ -4548,7 +4542,7 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) { if (E == nullptr || isa(E)) return; // Walk into expressions which might have invalid trailing closures - class DiagnoseWalker : public ASTWalker { + class DiagnoseWalker : public BaseDiagnosticWalker { ASTContext &Ctx; void diagnoseIt(const CallExpr *E) { @@ -4594,12 +4588,6 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) { public: DiagnoseWalker(ASTContext &ctx) : Ctx(ctx) { } - MacroWalking getMacroWalkingBehavior() const override { - // Macro expansions will be walked when they're type-checked, not as - // part of the surrounding code. - return MacroWalking::None; - } - PreWalkResult walkToArgumentListPre(ArgumentList *args) override { // Don't walk into an explicit argument list, as trailing closures that @@ -4669,7 +4657,7 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Stmt *S) { namespace { -class ObjCSelectorWalker : public ASTWalker { +class ObjCSelectorWalker : public BaseDiagnosticWalker { ASTContext &Ctx; const DeclContext *DC; Type SelectorTy; @@ -4719,12 +4707,6 @@ class ObjCSelectorWalker : public ASTWalker { ObjCSelectorWalker(const DeclContext *dc, Type selectorTy) : Ctx(dc->getASTContext()), DC(dc), SelectorTy(selectorTy) { } - MacroWalking getMacroWalkingBehavior() const override { - // 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 { auto *stringLiteral = dyn_cast(expr); bool fromStringLiteral = false; @@ -5231,7 +5213,7 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E, if (!E || isa(E) || !E->getType()) return; - class UnintendedOptionalBehaviorWalker : public ASTWalker { + class UnintendedOptionalBehaviorWalker : public BaseDiagnosticWalker { ASTContext &Ctx; SmallPtrSet IgnoredExprs; @@ -5583,12 +5565,6 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E, } } - MacroWalking getMacroWalkingBehavior() const override { - // 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 { if (!E || isa(E) || !E->getType()) return Action::SkipNode(E); @@ -5624,7 +5600,7 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E, if (!E || isa(E) || !E->getType()) return; - class DeprecatedWritableKeyPathWalker : public ASTWalker { + class DeprecatedWritableKeyPathWalker : public BaseDiagnosticWalker { ASTContext &Ctx; const DeclContext *DC; @@ -5658,12 +5634,6 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E, } } - MacroWalking getMacroWalkingBehavior() const override { - // 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 { if (!E || isa(E) || !E->getType()) return Action::SkipNode(E); @@ -5687,7 +5657,7 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E, static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E, const DeclContext *DC) { - class KVOObserveCallWalker : public ASTWalker { + class KVOObserveCallWalker : public BaseDiagnosticWalker { const ASTContext &C; public: @@ -5745,12 +5715,6 @@ static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E, } } - MacroWalking getMacroWalkingBehavior() const override { - // 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 { if (!E || isa(E) || !E->getType()) return Action::SkipNode(E); @@ -5771,7 +5735,7 @@ static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E, static void diagnoseExplicitUseOfLazyVariableStorage(const Expr *E, const DeclContext *DC) { - class ExplicitLazyVarStorageAccessFinder : public ASTWalker { + class ExplicitLazyVarStorageAccessFinder : public BaseDiagnosticWalker { const ASTContext &C; public: @@ -5798,12 +5762,6 @@ static void diagnoseExplicitUseOfLazyVariableStorage(const Expr *E, } } - MacroWalking getMacroWalkingBehavior() const override { - // 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 { if (!E || isa(E) || !E->getType()) return Action::SkipNode(E); @@ -5822,7 +5780,7 @@ static void diagnoseExplicitUseOfLazyVariableStorage(const Expr *E, } static void diagnoseComparisonWithNaN(const Expr *E, const DeclContext *DC) { - class ComparisonWithNaNFinder : public ASTWalker { + class ComparisonWithNaNFinder : public BaseDiagnosticWalker { const ASTContext &C; public: @@ -5931,12 +5889,6 @@ static void diagnoseComparisonWithNaN(const Expr *E, const DeclContext *DC) { } } - MacroWalking getMacroWalkingBehavior() const override { - // 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 { if (!E || isa(E) || !E->getType()) return Action::SkipNode(E); @@ -5959,19 +5911,13 @@ static void diagUnqualifiedAccessToMethodNamedSelf(const Expr *E, if (!E || isa(E) || !E->getType()) return; - class DiagnoseWalker : public ASTWalker { + class DiagnoseWalker : public BaseDiagnosticWalker { ASTContext &Ctx; const DeclContext *DC; public: DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()), DC(DC) {} - MacroWalking getMacroWalkingBehavior() const override { - // 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 { if (!E || isa(E) || !E->getType()) return Action::SkipNode(E); @@ -6024,7 +5970,7 @@ static void diagUnqualifiedAccessToMethodNamedSelf(const Expr *E, static void diagnoseDictionaryLiteralDuplicateKeyEntries(const Expr *E, const DeclContext *DC) { - class DiagnoseWalker : public ASTWalker { + class DiagnoseWalker : public BaseDiagnosticWalker { ASTContext &Ctx; private: @@ -6123,12 +6069,6 @@ diagnoseDictionaryLiteralDuplicateKeyEntries(const Expr *E, public: DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()) {} - MacroWalking getMacroWalkingBehavior() const override { - // 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 { const auto *DLE = dyn_cast_or_null(E); if (!DLE) diff --git a/test/Sema/diag_unintended_optional_behavior.swift b/test/Sema/diag_unintended_optional_behavior.swift index e2cc7cb16c908..36a84226064fd 100644 --- a/test/Sema/diag_unintended_optional_behavior.swift +++ b/test/Sema/diag_unintended_optional_behavior.swift @@ -224,3 +224,13 @@ func warnOptionalInStringInterpolationSegment(_ o : Int?) { // expected-note@-2 {{use 'String(describing:)' to silence this warning}} {{51-51=String(describing: }} {{55-55=)}} // expected-note@-3 {{provide a default value to avoid this warning}} {{55-55= ?? <#default value#>}} } + +// Make sure we don't double diagnose +_ = { + func foo(_ y: Int?) { + let _: Any = y // expected-warning {{expression implicitly coerced from 'Int?' to 'Any'}} + // expected-note@-1 {{explicitly cast to 'Any' with 'as Any' to silence this warning}} + // expected-note@-2 {{provide a default value to avoid this warning}} + // expected-note@-3 {{force-unwrap the value to avoid this warning}} + } +} From 1b5ed65a57fb17ee3b5d4aaf5b4d39d362676b32 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Sat, 9 Nov 2024 00:28:59 +0000 Subject: [PATCH 2/2] [Sema] Consistently run MiscDiagnostics on macro arguments Previously we would only run MiscDiagnostics passes on macro arguments for some statement diagnostics, update the expression walkers that inherit from BaseDiagnosticWalker such that we consistently do MiscDiagnostics on macro arguments. --- lib/Sema/MiscDiagnostics.h | 6 +++--- lib/Sema/TypeCheckConstraints.cpp | 6 +++--- test/Macros/macro_misc_diags.swift | 14 +++++++++++++- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.h b/lib/Sema/MiscDiagnostics.h index 6f784ff0abae8..8d3608603e1b2 100644 --- a/lib/Sema/MiscDiagnostics.h +++ b/lib/Sema/MiscDiagnostics.h @@ -139,9 +139,9 @@ namespace swift { } MacroWalking getMacroWalkingBehavior() const override { - // Macro expansions will be walked when they're type-checked, not as - // part of the surrounding code. - return MacroWalking::None; + // We only want to walk macro arguments. Expansions will be walked when + // they're type-checked, not as part of the surrounding code. + return MacroWalking::Arguments; } }; diff --git a/lib/Sema/TypeCheckConstraints.cpp b/lib/Sema/TypeCheckConstraints.cpp index 6151c27b96d7b..6a06bcd0944d7 100644 --- a/lib/Sema/TypeCheckConstraints.cpp +++ b/lib/Sema/TypeCheckConstraints.cpp @@ -342,9 +342,9 @@ class SyntacticDiagnosticWalker final : public ASTWalker { } MacroWalking getMacroWalkingBehavior() const override { - // Macro expansions will be walked when they're type-checked, not as - // part of the surrounding code. - return MacroWalking::None; + // We only want to walk macro arguments. Expansions will be walked when + // they're type-checked, not as part of the surrounding code. + return MacroWalking::Arguments; } PreWalkResult walkToExprPre(Expr *expr) override { diff --git a/test/Macros/macro_misc_diags.swift b/test/Macros/macro_misc_diags.swift index d7207429eb450..9b88bf1a98d53 100644 --- a/test/Macros/macro_misc_diags.swift +++ b/test/Macros/macro_misc_diags.swift @@ -61,13 +61,16 @@ macro makeBinding(_ x: T) = #externalMacro(module: "MacroPlugin", type: "Make @available(*, deprecated) func deprecatedFunc() -> Int { 0 } -// FIXME: We also ought to be diagnosing the macro argument +// Make sure we do MiscDiagnostics passes for both macro arguments and expansions. + _ = #identity(Int) // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf_.swift:1:1: error: expected member name or initializer call after type name +// CHECK-DIAG: Client.swift:[[@LINE-2]]:15: 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 + // CHECK-DIAG: Client.swift:[[@LINE-2]]:17: error: expected member name or initializer call after type name } _ = #identity(deprecatedFunc()) @@ -99,6 +102,14 @@ _ = #trailingClosure { // 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 } +func testOptionalToAny(_ y: Int?) { + _ = #trailingClosure { + let _: Any = y + // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}trailingClosurefMf_.swift:2:18: warning: expression implicitly coerced from 'Int?' to 'Any' + // CHECK-DIAG: Client.swift:[[@LINE-2]]:18: warning: expression implicitly coerced from 'Int?' to 'Any' + } +} + // rdar://138997009 - Make sure we don't crash in MiscDiagnostics' implicit // self diagnosis. struct rdar138997009 { @@ -119,6 +130,7 @@ class rdar138997009_Class { _ = #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 } } }