Skip to content

Commit fab09c5

Browse files
hamishknighthamishknight
authored
Merge pull request #77479 from hamishknight/macro-misc-diag-fixes
[Sema] A couple of fixes for MiscDiagnostics on macro expansions
2 parents c4e44f0 + 9c3b8a6 commit fab09c5

File tree

5 files changed

+202
-38
lines changed

5 files changed

+202
-38
lines changed

‎lib/Sema/CSApply.cpp

Lines changed: 41 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -408,13 +408,34 @@ namespace {
408408
/// Rewrites an expression by applying the solution of a constraint
409409
/// system to that expression.
410410
class ExprRewriter : public ExprVisitor<ExprRewriter, Expr *> {
411+
// Delayed items to type-check.
412+
SmallVector<Decl *, 4> LocalDeclsToTypeCheck;
413+
SmallVector<MacroExpansionExpr *, 4> MacrosToExpand;
414+
411415
public:
412416
ConstraintSystem &cs;
413417
DeclContext *dc;
414418
Solution &solution;
415419
std::optional<SyntacticElementTarget> target;
416420
bool SuppressDiagnostics;
417421

422+
ExprRewriter(ConstraintSystem &cs, Solution &solution,
423+
std::optional<SyntacticElementTarget> target,
424+
bool suppressDiagnostics)
425+
: cs(cs), dc(target ? target->getDeclContext() : cs.DC),
426+
solution(solution), target(target),
427+
SuppressDiagnostics(suppressDiagnostics) {}
428+
429+
ConstraintSystem &getConstraintSystem() const { return cs; }
430+
431+
void addLocalDeclToTypeCheck(Decl *D) {
432+
LocalDeclsToTypeCheck.push_back(D);
433+
}
434+
435+
void addMacroToExpand(MacroExpansionExpr *E) {
436+
MacrosToExpand.push_back(E);
437+
}
438+
418439
/// Coerce the given tuple to another tuple type.
419440
///
420441
/// \param expr The expression we're converting.
@@ -2631,15 +2652,6 @@ namespace {
26312652
}
26322653

26332654
public:
2634-
ExprRewriter(ConstraintSystem &cs, Solution &solution,
2635-
std::optional<SyntacticElementTarget> target,
2636-
bool suppressDiagnostics)
2637-
: cs(cs), dc(target ? target->getDeclContext() : cs.DC),
2638-
solution(solution), target(target),
2639-
SuppressDiagnostics(suppressDiagnostics) {}
2640-
2641-
ConstraintSystem &getConstraintSystem() const { return cs; }
2642-
26432655
/// Simplify the expression type and return the expression.
26442656
///
26452657
/// This routine is used for 'simple' expressions that only need their
@@ -5528,16 +5540,19 @@ namespace {
55285540

55295541
// FIXME: Expansion should be lazy.
55305542
// i.e. 'ExpandMacroExpansionExprRequest' should be sinked into
5531-
// 'getRewritten()', and performed on-demand.
5543+
// 'getRewritten()', and performed on-demand. Unfortunately that requires
5544+
// auditing every ASTWalker's `getMacroWalkingBehavior` since
5545+
// `MacroWalking::Expansion` does not actually kick expansion.
55325546
if (!cs.Options.contains(ConstraintSystemFlags::DisableMacroExpansions) &&
55335547
// Do not expand macros inside macro arguments. For example for
55345548
// '#stringify(#assert(foo))' when typechecking `#assert(foo)`,
55355549
// we don't want to expand it.
55365550
llvm::none_of(llvm::ArrayRef(ExprStack).drop_back(1),
55375551
[](Expr *E) { return isa<MacroExpansionExpr>(E); })) {
5538-
(void)evaluateOrDefault(cs.getASTContext().evaluator,
5539-
ExpandMacroExpansionExprRequest{E},
5540-
std::nullopt);
5552+
// We need to delay the expansion until we're done applying the solution
5553+
// since running MiscDiagnostics on the expansion may walk up and query
5554+
// the type of a parent closure, e.g `diagnoseImplicitSelfUseInClosure`.
5555+
addMacroToExpand(E);
55415556
}
55425557

55435558
cs.cacheExprTypes(E);
@@ -5604,6 +5619,18 @@ namespace {
56045619
diag::add_consume_to_silence)
56055620
.fixItInsert(coercion->getStartLoc(), "consume ");
56065621
}
5622+
5623+
// Type-check any local decls encountered.
5624+
for (auto *D : LocalDeclsToTypeCheck)
5625+
TypeChecker::typeCheckDecl(D);
5626+
5627+
// Expand any macros encountered.
5628+
// FIXME: Expansion should be lazy.
5629+
auto &eval = cs.getASTContext().evaluator;
5630+
for (auto *E : MacrosToExpand) {
5631+
(void)evaluateOrDefault(eval, ExpandMacroExpansionExprRequest{E},
5632+
std::nullopt);
5633+
}
56075634
}
56085635

56095636
/// Diagnose an optional injection that is probably not what the
@@ -8807,22 +8834,15 @@ namespace {
88078834

88088835
class ExprWalker : public ASTWalker, public SyntacticElementTargetRewriter {
88098836
ExprRewriter &Rewriter;
8810-
SmallVector<Decl *, 4> LocalDeclsToTypeCheck;
88118837

88128838
public:
88138839
ExprWalker(ExprRewriter &Rewriter) : Rewriter(Rewriter) { }
88148840

8815-
~ExprWalker() {
8816-
// Type-check any local decls encountered.
8817-
for (auto *D : LocalDeclsToTypeCheck)
8818-
TypeChecker::typeCheckDecl(D);
8819-
}
8820-
88218841
Solution &getSolution() const override { return Rewriter.solution; }
88228842
DeclContext *&getCurrentDC() const override { return Rewriter.dc; }
88238843

88248844
void addLocalDeclToTypeCheck(Decl *D) override {
8825-
LocalDeclsToTypeCheck.push_back(D);
8845+
Rewriter.addLocalDeclToTypeCheck(D);
88268846
}
88278847

88288848
bool shouldWalkIntoPropertyWrapperPlaceholderValue() override {

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
9797
DiagnoseWalker(const DeclContext *DC, bool isExprStmt)
9898
: IsExprStmt(isExprStmt), Ctx(DC->getASTContext()), DC(DC) {}
9999

100-
MacroWalking getMacroWalkingBehavior() const override {
101-
return MacroWalking::Expansion;
102-
}
103-
104100
PreWalkAction walkToTypeReprPre(TypeRepr *T) override {
105101
return Action::Continue();
106102
}
@@ -1582,7 +1578,9 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) {
15821578
}
15831579

15841580
MacroWalking getMacroWalkingBehavior() const override {
1585-
return MacroWalking::Expansion;
1581+
// Macro expansions will be walked when they're type-checked, not as
1582+
// part of the surrounding code.
1583+
return MacroWalking::None;
15861584
}
15871585

15881586
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -4597,7 +4595,9 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
45974595
DiagnoseWalker(ASTContext &ctx) : Ctx(ctx) { }
45984596

45994597
MacroWalking getMacroWalkingBehavior() const override {
4600-
return MacroWalking::Expansion;
4598+
// Macro expansions will be walked when they're type-checked, not as
4599+
// part of the surrounding code.
4600+
return MacroWalking::None;
46014601
}
46024602

46034603
PreWalkResult<ArgumentList *>
@@ -4720,7 +4720,9 @@ class ObjCSelectorWalker : public ASTWalker {
47204720
: Ctx(dc->getASTContext()), DC(dc), SelectorTy(selectorTy) { }
47214721

47224722
MacroWalking getMacroWalkingBehavior() const override {
4723-
return MacroWalking::Expansion;
4723+
// Macro expansions will be walked when they're type-checked, not as
4724+
// part of the surrounding code.
4725+
return MacroWalking::None;
47244726
}
47254727

47264728
PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {
@@ -5582,7 +5584,9 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E,
55825584
}
55835585

55845586
MacroWalking getMacroWalkingBehavior() const override {
5585-
return MacroWalking::Expansion;
5587+
// Macro expansions will be walked when they're type-checked, not as
5588+
// part of the surrounding code.
5589+
return MacroWalking::None;
55865590
}
55875591

55885592
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5655,7 +5659,9 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
56555659
}
56565660

56575661
MacroWalking getMacroWalkingBehavior() const override {
5658-
return MacroWalking::Expansion;
5662+
// Macro expansions will be walked when they're type-checked, not as
5663+
// part of the surrounding code.
5664+
return MacroWalking::None;
56595665
}
56605666

56615667
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5740,7 +5746,9 @@ static void maybeDiagnoseCallToKeyValueObserveMethod(const Expr *E,
57405746
}
57415747

57425748
MacroWalking getMacroWalkingBehavior() const override {
5743-
return MacroWalking::Expansion;
5749+
// Macro expansions will be walked when they're type-checked, not as
5750+
// part of the surrounding code.
5751+
return MacroWalking::None;
57445752
}
57455753

57465754
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5791,7 +5799,9 @@ static void diagnoseExplicitUseOfLazyVariableStorage(const Expr *E,
57915799
}
57925800

57935801
MacroWalking getMacroWalkingBehavior() const override {
5794-
return MacroWalking::Expansion;
5802+
// Macro expansions will be walked when they're type-checked, not as
5803+
// part of the surrounding code.
5804+
return MacroWalking::None;
57955805
}
57965806

57975807
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5922,7 +5932,9 @@ static void diagnoseComparisonWithNaN(const Expr *E, const DeclContext *DC) {
59225932
}
59235933

59245934
MacroWalking getMacroWalkingBehavior() const override {
5925-
return MacroWalking::Expansion;
5935+
// Macro expansions will be walked when they're type-checked, not as
5936+
// part of the surrounding code.
5937+
return MacroWalking::None;
59265938
}
59275939

59285940
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -5955,7 +5967,9 @@ static void diagUnqualifiedAccessToMethodNamedSelf(const Expr *E,
59555967
DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()), DC(DC) {}
59565968

59575969
MacroWalking getMacroWalkingBehavior() const override {
5958-
return MacroWalking::Expansion;
5970+
// Macro expansions will be walked when they're type-checked, not as
5971+
// part of the surrounding code.
5972+
return MacroWalking::None;
59595973
}
59605974

59615975
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
@@ -6110,7 +6124,9 @@ diagnoseDictionaryLiteralDuplicateKeyEntries(const Expr *E,
61106124
DiagnoseWalker(const DeclContext *DC) : Ctx(DC->getASTContext()) {}
61116125

61126126
MacroWalking getMacroWalkingBehavior() const override {
6113-
return MacroWalking::Expansion;
6127+
// Macro expansions will be walked when they're type-checked, not as
6128+
// part of the surrounding code.
6129+
return MacroWalking::None;
61146130
}
61156131

61166132
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {

‎lib/Sema/MiscDiagnostics.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,10 @@ namespace swift {
138138
return Action::VisitNodeIf(isa<PatternBindingDecl>(D));
139139
}
140140

141-
// Only emit diagnostics in the expansion of macros.
142141
MacroWalking getMacroWalkingBehavior() const override {
143-
return MacroWalking::Expansion;
142+
// Macro expansions will be walked when they're type-checked, not as
143+
// part of the surrounding code.
144+
return MacroWalking::None;
144145
}
145146
};
146147

‎lib/Sema/TypeCheckConstraints.cpp

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

344344
MacroWalking getMacroWalkingBehavior() const override {
345-
return MacroWalking::Expansion;
345+
// Macro expansions will be walked when they're type-checked, not as
346+
// part of the surrounding code.
347+
return MacroWalking::None;
346348
}
347349

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

‎test/Macros/macro_misc_diags.swift

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
// REQUIRES: swift_swift_parser
2+
3+
// RUN: %empty-directory(%t)
4+
// RUN: split-file --leading-lines %s %t
5+
6+
// 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
7+
8+
// 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
9+
// RUN: %FileCheck --check-prefix=CHECK-DIAG --implicit-check-not="{{error|warning}}: " -input-file=%t/diags %s
10+
11+
//--- MacroPlugin.swift
12+
import SwiftSyntax
13+
import SwiftSyntaxMacros
14+
15+
public struct IdentityMacro: ExpressionMacro {
16+
public static func expansion(
17+
of macro: some FreestandingMacroExpansionSyntax,
18+
in context: some MacroExpansionContext
19+
) -> ExprSyntax {
20+
guard let argument = macro.arguments.first else {
21+
fatalError()
22+
}
23+
return "\(argument)"
24+
}
25+
}
26+
27+
public struct TrailingClosureMacro: ExpressionMacro {
28+
public static func expansion(
29+
of macro: some FreestandingMacroExpansionSyntax,
30+
in context: some MacroExpansionContext
31+
) -> ExprSyntax {
32+
guard let argument = macro.trailingClosure else {
33+
fatalError()
34+
}
35+
return "\(argument)"
36+
}
37+
}
38+
39+
public struct MakeBinding : DeclarationMacro {
40+
static public func expansion(
41+
of node: some FreestandingMacroExpansionSyntax,
42+
in context: some MacroExpansionContext
43+
) throws -> [DeclSyntax] {
44+
guard let arg = node.arguments.first else {
45+
fatalError()
46+
}
47+
return ["let x = \(arg)"]
48+
}
49+
}
50+
51+
//--- Client.swift
52+
@freestanding(expression)
53+
macro identity<T>(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "IdentityMacro")
54+
55+
@freestanding(expression)
56+
macro trailingClosure<T>(_ x: T) -> T = #externalMacro(module: "MacroPlugin", type: "TrailingClosureMacro")
57+
58+
@freestanding(declaration, names: named(x))
59+
macro makeBinding<T>(_ x: T) = #externalMacro(module: "MacroPlugin", type: "MakeBinding")
60+
61+
@available(*, deprecated)
62+
func deprecatedFunc() -> Int { 0 }
63+
64+
// FIXME: We also ought to be diagnosing the macro argument
65+
_ = #identity(Int)
66+
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf_.swift:1:1: error: expected member name or initializer call after type name
67+
68+
_ = {
69+
_ = #identity(Int)
70+
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf0_.swift:1:1: error: expected member name or initializer call after type name
71+
}
72+
73+
_ = #identity(deprecatedFunc())
74+
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-2]]{{.*}}identityfMf1_.swift:1:1: warning: 'deprecatedFunc()' is deprecated
75+
// CHECK-DIAG: Client.swift:[[@LINE-2]]:15: warning: 'deprecatedFunc()' is deprecated
76+
77+
#makeBinding(deprecatedFunc())
78+
// CHECK-DIAG: Client.swift:[[@LINE-1]]:14: warning: 'deprecatedFunc()' is deprecated
79+
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}makeBindingfMf_.swift:1:9: warning: 'deprecatedFunc()' is deprecated
80+
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-4]]{{.*}}makeBindingfMf_.swift:1:5: warning: initialization of immutable value 'x' was never used
81+
82+
struct S1 {
83+
#makeBinding(deprecatedFunc())
84+
// CHECK-DIAG: Client.swift:[[@LINE-1]]:16: warning: 'deprecatedFunc()' is deprecated
85+
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}makeBindingfMf_.swift:1:9: warning: 'deprecatedFunc()' is deprecated
86+
}
87+
88+
struct S2 {
89+
#makeBinding({deprecatedFunc()})
90+
// CHECK-DIAG: Client.swift:[[@LINE-1]]:17: warning: 'deprecatedFunc()' is deprecated
91+
// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}makeBindingfMf_.swift:2:5: warning: 'deprecatedFunc()' is deprecated
92+
}
93+
94+
func takesClosure(_ fn: () -> Void) -> Int? { nil }
95+
96+
_ = #trailingClosure {
97+
if let _ = takesClosure {} {}
98+
// 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
99+
// 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
100+
}
101+
102+
// rdar://138997009 - Make sure we don't crash in MiscDiagnostics' implicit
103+
// self diagnosis.
104+
struct rdar138997009 {
105+
func foo() {}
106+
func bar() {
107+
_ = {
108+
_ = #trailingClosure {
109+
foo()
110+
}
111+
}
112+
}
113+
}
114+
115+
class rdar138997009_Class {
116+
func foo() {}
117+
func bar() {
118+
_ = {
119+
_ = #trailingClosure {
120+
foo()
121+
// 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
122+
}
123+
}
124+
}
125+
}

0 commit comments

Comments
 (0)