Skip to content

[Sema] Consistently run MiscDiagnostics on macro arguments #77534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 12, 2024
Merged

[Sema] Consistently run MiscDiagnostics on macro arguments #77534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
146e95c
[Sema] Use BaseDiagnosticWalker consistently in MiscDiagnostics
hamishknight Nov 9, 2024
1b5ed65
[Sema] Consistently run MiscDiagnostics on macro arguments
hamishknight Nov 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 10 additions & 70 deletions lib/Sema/MiscDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -1577,12 +1577,6 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) {
cast<VarDecl>(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<Expr *> walkToExprPre(Expr *E) override {
Expr *subExpr;
bool isStore = false;
Expand Down Expand Up @@ -4548,7 +4542,7 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
if (E == nullptr || isa<ErrorExpr>(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) {
Expand Down Expand Up @@ -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<ArgumentList *>
walkToArgumentListPre(ArgumentList *args) override {
// Don't walk into an explicit argument list, as trailing closures that
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<Expr *> walkToExprPre(Expr *expr) override {
auto *stringLiteral = dyn_cast<StringLiteralExpr>(expr);
bool fromStringLiteral = false;
Expand Down Expand Up @@ -5231,7 +5213,7 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E,
if (!E || isa<ErrorExpr>(E) || !E->getType())
return;

class UnintendedOptionalBehaviorWalker : public ASTWalker {
class UnintendedOptionalBehaviorWalker : public BaseDiagnosticWalker {
ASTContext &Ctx;
SmallPtrSet<Expr *, 16> IgnoredExprs;

Expand Down Expand Up @@ -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<Expr *> walkToExprPre(Expr *E) override {
if (!E || isa<ErrorExpr>(E) || !E->getType())
return Action::SkipNode(E);
Expand Down Expand Up @@ -5624,7 +5600,7 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
if (!E || isa<ErrorExpr>(E) || !E->getType())
return;

class DeprecatedWritableKeyPathWalker : public ASTWalker {
class DeprecatedWritableKeyPathWalker : public BaseDiagnosticWalker {
ASTContext &Ctx;
const DeclContext *DC;

Expand Down Expand Up @@ -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<Expr *> walkToExprPre(Expr *E) override {
if (!E || isa<ErrorExpr>(E) || !E->getType())
return Action::SkipNode(E);
Expand All @@ -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:
Expand Down Expand Up @@ -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<Expr *> walkToExprPre(Expr *E) override {
if (!E || isa<ErrorExpr>(E) || !E->getType())
return Action::SkipNode(E);
Expand All @@ -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:
Expand All @@ -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<Expr *> walkToExprPre(Expr *E) override {
if (!E || isa<ErrorExpr>(E) || !E->getType())
return Action::SkipNode(E);
Expand All @@ -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:
Expand Down Expand Up @@ -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<Expr *> walkToExprPre(Expr *E) override {
if (!E || isa<ErrorExpr>(E) || !E->getType())
return Action::SkipNode(E);
Expand All @@ -5959,19 +5911,13 @@ static void diagUnqualifiedAccessToMethodNamedSelf(const Expr *E,
if (!E || isa<ErrorExpr>(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<Expr *> walkToExprPre(Expr *E) override {
if (!E || isa<ErrorExpr>(E) || !E->getType())
return Action::SkipNode(E);
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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<Expr *> walkToExprPre(Expr *E) override {
const auto *DLE = dyn_cast_or_null<DictionaryExpr>(E);
if (!DLE)
Expand Down
6 changes: 3 additions & 3 deletions lib/Sema/MiscDiagnostics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
};

Expand Down
6 changes: 3 additions & 3 deletions lib/Sema/TypeCheckConstraints.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Expr *> walkToExprPre(Expr *expr) override {
Expand Down
14 changes: 13 additions & 1 deletion test/Macros/macro_misc_diags.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,16 @@ macro makeBinding<T>(_ 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())
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
}
}
Expand Down
10 changes: 10 additions & 0 deletions test/Sema/diag_unintended_optional_behavior.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
}
}