Skip to content

Commit b2dcf1e

Browse files
hamishknighthamishknight
authored
Merge pull request #77534 from hamishknight/i-take-issue-with-your-argument
[Sema] Consistently run MiscDiagnostics on macro arguments
2 parents 36a9628 + 1b5ed65 commit b2dcf1e

File tree

5 files changed

+39
-77
lines changed

5 files changed

+39
-77
lines changed

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 10 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -1561,7 +1561,7 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) {
15611561
if (!var) // Ignore subscripts
15621562
return;
15631563

1564-
class DiagnoseWalker : public ASTWalker {
1564+
class DiagnoseWalker : public BaseDiagnosticWalker {
15651565
ASTContext &Ctx;
15661566
VarDecl *Var;
15671567
const AccessorDecl *Accessor;
@@ -1577,12 +1577,6 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) {
15771577
cast<VarDecl>(DRE->getDecl())->isSelfParameter();
15781578
}
15791579

1580-
MacroWalking getMacroWalkingBehavior() const override {
1581-
// Macro expansions will be walked when they're type-checked, not as
1582-
// part of the surrounding code.
1583-
return MacroWalking::None;
1584-
}
1585-
15861580
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
15871581
Expr *subExpr;
15881582
bool isStore = false;
@@ -4527,7 +4521,7 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
45274521
if (E == nullptr || isa<ErrorExpr>(E)) return;
45284522

45294523
// Walk into expressions which might have invalid trailing closures
4530-
class DiagnoseWalker : public ASTWalker {
4524+
class DiagnoseWalker : public BaseDiagnosticWalker {
45314525
ASTContext &Ctx;
45324526

45334527
void diagnoseIt(const CallExpr *E) {
@@ -4573,12 +4567,6 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
45734567
public:
45744568
DiagnoseWalker(ASTContext &ctx) : Ctx(ctx) { }
45754569

4576-
MacroWalking getMacroWalkingBehavior() const override {
4577-
// Macro expansions will be walked when they're type-checked, not as
4578-
// part of the surrounding code.
4579-
return MacroWalking::None;
4580-
}
4581-
45824570
PreWalkResult<ArgumentList *>
45834571
walkToArgumentListPre(ArgumentList *args) override {
45844572
// Don't walk into an explicit argument list, as trailing closures that
@@ -4648,7 +4636,7 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Stmt *S) {
46484636

46494637
namespace {
46504638

4651-
class ObjCSelectorWalker : public ASTWalker {
4639+
class ObjCSelectorWalker : public BaseDiagnosticWalker {
46524640
ASTContext &Ctx;
46534641
const DeclContext *DC;
46544642
Type SelectorTy;
@@ -4698,12 +4686,6 @@ class ObjCSelectorWalker : public ASTWalker {
46984686
ObjCSelectorWalker(const DeclContext *dc, Type selectorTy)
46994687
: Ctx(dc->getASTContext()), DC(dc), SelectorTy(selectorTy) { }
47004688

4701-
MacroWalking getMacroWalkingBehavior() const override {
4702-
// Macro expansions will be walked when they're type-checked, not as
4703-
// part of the surrounding code.
4704-
return MacroWalking::None;
4705-
}
4706-
47074689
PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {
47084690
auto *stringLiteral = dyn_cast<StringLiteralExpr>(expr);
47094691
bool fromStringLiteral = false;
@@ -5208,7 +5190,7 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E,
52085190
if (!E || isa<ErrorExpr>(E) || !E->getType())
52095191
return;
52105192

5211-
class UnintendedOptionalBehaviorWalker : public ASTWalker {
5193+
class UnintendedOptionalBehaviorWalker : public BaseDiagnosticWalker {
52125194
ASTContext &Ctx;
52135195
SmallPtrSet<Expr *, 16> IgnoredExprs;
52145196

@@ -5560,12 +5542,6 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E,
55605542
}
55615543
}
55625544

5563-
MacroWalking getMacroWalkingBehavior() const override {
5564-
// Macro expansions will be walked when they're type-checked, not as
5565-
// part of the surrounding code.
5566-
return MacroWalking::None;
5567-
}
5568-
55695545
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
55705546
if (!E || isa<ErrorExpr>(E) || !E->getType())
55715547
return Action::SkipNode(E);
@@ -5601,7 +5577,7 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
56015577
if (!E || isa<ErrorExpr>(E) || !E->getType())
56025578
return;
56035579

5604-
class DeprecatedWritableKeyPathWalker : public ASTWalker {
5580+
class DeprecatedWritableKeyPathWalker : public BaseDiagnosticWalker {
56055581
ASTContext &Ctx;
56065582
const DeclContext *DC;
56075583

@@ -5635,12 +5611,6 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
56355611
}
56365612
}
56375613

5638-
MacroWalking getMacroWalkingBehavior() const override {
5639-
// Macro expansions will be walked when they're type-checked, not as
5640-
// part of the surrounding code.
5641-
return MacroWalking::None;
5642-
}
5643-
56445614
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
56455615
if (!E || isa<ErrorExpr>(E) || !E->getType())
56465616
return Action::SkipNode(E);
@@ -5664,7 +5634,7 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
56645634

56655635
static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E,
56665636
const DeclContext *DC) {
5667-
class KVOObserveCallWalker : public ASTWalker {
5637+
class KVOObserveCallWalker : public BaseDiagnosticWalker {
56685638
const ASTContext &C;
56695639

56705640
public:
@@ -5722,12 +5692,6 @@ static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E,
57225692
}
57235693
}
57245694

5725-
MacroWalking getMacroWalkingBehavior() const override {
5726-
// Macro expansions will be walked when they're type-checked, not as
5727-
// part of the surrounding code.
5728-
return MacroWalking::None;
5729-
}
5730-
57315695
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
57325696
if (!E || isa<ErrorExpr>(E) || !E->getType())
57335697
return Action::SkipNode(E);
@@ -5748,7 +5712,7 @@ static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E,
57485712
static void diagnoseExplicitUseOfLazyVariableStorage(const Expr *E,
57495713
const DeclContext *DC) {
57505714

5751-
class ExplicitLazyVarStorageAccessFinder : public ASTWalker {
5715+
class ExplicitLazyVarStorageAccessFinder : public BaseDiagnosticWalker {
57525716
const ASTContext &C;
57535717

57545718
public:
@@ -5775,12 +5739,6 @@ static void diagnoseExplicitUseOfLazyVariableStorage(const Expr *E,
57755739
}
57765740
}
57775741

5778-
MacroWalking getMacroWalkingBehavior() const override {
5779-
// Macro expansions will be walked when they're type-checked, not as
5780-
// part of the surrounding code.
5781-
return MacroWalking::None;
5782-
}
5783-
57845742
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
57855743
if (!E || isa<ErrorExpr>(E) || !E->getType())
57865744
return Action::SkipNode(E);
@@ -5799,7 +5757,7 @@ static void diagnoseExplicitUseOfLazyVariableStorage(const Expr *E,
57995757
}
58005758

58015759
static void diagnoseComparisonWithNaN(const Expr *E, const DeclContext *DC) {
5802-
class ComparisonWithNaNFinder : public ASTWalker {
5760+
class ComparisonWithNaNFinder : public BaseDiagnosticWalker {
58035761
const ASTContext &C;
58045762

58055763
public:
@@ -5908,12 +5866,6 @@ static void diagnoseComparisonWithNaN(const Expr *E, const DeclContext *DC) {
59085866
}
59095867
}
59105868

5911-
MacroWalking getMacroWalkingBehavior() const override {
5912-
// Macro expansions will be walked when they're type-checked, not as
5913-
// part of the surrounding code.
5914-
return MacroWalking::None;
5915-
}
5916-
59175869
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
59185870
if (!E || isa<ErrorExpr>(E) || !E->getType())
59195871
return Action::SkipNode(E);
@@ -5936,19 +5888,13 @@ static void diagUnqualifiedAccessToMethodNamedSelf(const Expr *E,
59365888
if (!E || isa<ErrorExpr>(E) || !E->getType())
59375889
return;
59385890

5939-
class DiagnoseWalker : public ASTWalker {
5891+
class DiagnoseWalker : public BaseDiagnosticWalker {
59405892
ASTContext &Ctx;
59415893
const DeclContext *DC;
59425894

59435895
public:
59445896
DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()), DC(DC) {}
59455897

5946-
MacroWalking getMacroWalkingBehavior() const override {
5947-
// Macro expansions will be walked when they're type-checked, not as
5948-
// part of the surrounding code.
5949-
return MacroWalking::None;
5950-
}
5951-
59525898
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
59535899
if (!E || isa<ErrorExpr>(E) || !E->getType())
59545900
return Action::SkipNode(E);
@@ -6001,7 +5947,7 @@ static void diagUnqualifiedAccessToMethodNamedSelf(const Expr *E,
60015947
static void
60025948
diagnoseDictionaryLiteralDuplicateKeyEntries(const Expr *E,
60035949
const DeclContext *DC) {
6004-
class DiagnoseWalker : public ASTWalker {
5950+
class DiagnoseWalker : public BaseDiagnosticWalker {
60055951
ASTContext &Ctx;
60065952

60075953
private:
@@ -6100,12 +6046,6 @@ diagnoseDictionaryLiteralDuplicateKeyEntries(const Expr *E,
61006046
public:
61016047
DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()) {}
61026048

6103-
MacroWalking getMacroWalkingBehavior() const override {
6104-
// Macro expansions will be walked when they're type-checked, not as
6105-
// part of the surrounding code.
6106-
return MacroWalking::None;
6107-
}
6108-
61096049
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
61106050
const auto *DLE = dyn_cast_or_null<DictionaryExpr>(E);
61116051
if (!DLE)

‎lib/Sema/MiscDiagnostics.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,9 +139,9 @@ namespace swift {
139139
}
140140

141141
MacroWalking getMacroWalkingBehavior() const override {
142-
// Macro expansions will be walked when they're type-checked, not as
143-
// part of the surrounding code.
144-
return MacroWalking::None;
142+
// We only want to walk macro arguments. Expansions will be walked when
143+
// they're type-checked, not as part of the surrounding code.
144+
return MacroWalking::Arguments;
145145
}
146146
};
147147

‎lib/Sema/TypeCheckConstraints.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -342,9 +342,9 @@ class SyntacticDiagnosticWalker final : public ASTWalker {
342342
}
343343

344344
MacroWalking getMacroWalkingBehavior() const override {
345-
// Macro expansions will be walked when they're type-checked, not as
346-
// part of the surrounding code.
347-
return MacroWalking::None;
345+
// We only want to walk macro arguments. Expansions will be walked when
346+
// they're type-checked, not as part of the surrounding code.
347+
return MacroWalking::Arguments;
348348
}
349349

350350
PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {

‎test/Macros/macro_misc_diags.swift

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,16 @@ macro makeBinding<T>(_ x: T) = #externalMacro(module: "MacroPlugin", type: "Make
6161
@available(*, deprecated)
6262
func deprecatedFunc() -> Int { 0 }
6363

64-
// FIXME: We also ought to be diagnosing the macro argument
64+
// Make sure we do MiscDiagnostics passes for both macro arguments and expansions.
65+
6566
_ = #identity(Int)
6667
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf_.swift:1:1: error: expected member name or initializer call after type name
68+
// CHECK-DIAG: Client.swift:[[@LINE-2]]:15: error: expected member name or initializer call after type name
6769

6870
_ = {
6971
_ = #identity(Int)
7072
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf0_.swift:1:1: error: expected member name or initializer call after type name
73+
// CHECK-DIAG: Client.swift:[[@LINE-2]]:17: error: expected member name or initializer call after type name
7174
}
7275

7376
_ = #identity(deprecatedFunc())
@@ -99,6 +102,14 @@ _ = #trailingClosure {
99102
// 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
100103
}
101104

105+
func testOptionalToAny(_ y: Int?) {
106+
_ = #trailingClosure {
107+
let _: Any = y
108+
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}trailingClosurefMf_.swift:2:18: warning: expression implicitly coerced from 'Int?' to 'Any'
109+
// CHECK-DIAG: Client.swift:[[@LINE-2]]:18: warning: expression implicitly coerced from 'Int?' to 'Any'
110+
}
111+
}
112+
102113
// rdar://138997009 - Make sure we don't crash in MiscDiagnostics' implicit
103114
// self diagnosis.
104115
struct rdar138997009 {
@@ -119,6 +130,7 @@ class rdar138997009_Class {
119130
_ = #trailingClosure {
120131
foo()
121132
// 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
133+
// CHECK-DIAG: Client.swift:[[@LINE-2]]:9: error: call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit
122134
}
123135
}
124136
}

‎test/Sema/diag_unintended_optional_behavior.swift

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,3 +224,13 @@ func warnOptionalInStringInterpolationSegment(_ o : Int?) {
224224
// expected-note@-2 {{use 'String(describing:)' to silence this warning}} {{51-51=String(describing: }} {{55-55=)}}
225225
// expected-note@-3 {{provide a default value to avoid this warning}} {{55-55= ?? <#default value#>}}
226226
}
227+
228+
// Make sure we don't double diagnose
229+
_ = {
230+
func foo(_ y: Int?) {
231+
let _: Any = y // expected-warning {{expression implicitly coerced from 'Int?' to 'Any'}}
232+
// expected-note@-1 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
233+
// expected-note@-2 {{provide a default value to avoid this warning}}
234+
// expected-note@-3 {{force-unwrap the value to avoid this warning}}
235+
}
236+
}

0 commit comments

Comments
 (0)