Skip to content

Commit 0cdaeb5

Browse files
hborlahborla
authored
Merge pull request swiftlang#29649 from hborla/function-builder-syntactic-diags
[MiscDiagnostics] Walk into the body of a non single-statement closure if the closure had a function builder transform applied.
2 parents bca5511 + 778f894 commit 0cdaeb5

File tree

6 files changed

+88
-26
lines changed

6 files changed

+88
-26
lines changed

‎include/swift/AST/ASTWalker.h

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ namespace swift {
2020

2121
class Decl;
2222
class Expr;
23+
class ClosureExpr;
2324
class ModuleDecl;
2425
class Stmt;
2526
class Pattern;
@@ -219,7 +220,13 @@ class ASTWalker {
219220
/// For work that is performed for every top-level expression, this should
220221
/// be overridden to return false, to avoid duplicating work or visiting
221222
/// bodies of closures that have not yet been type checked.
222-
virtual bool shouldWalkIntoNonSingleExpressionClosure() { return true; }
223+
virtual bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *) {
224+
return true;
225+
}
226+
227+
/// This method configures whether the walker should visit the body of a
228+
/// TapExpr.
229+
virtual bool shouldWalkIntoTapExpression() { return true; }
223230

224231
/// This method configures whether the walker should exhibit the legacy
225232
/// behavior where accessors appear as peers of their storage, rather

‎lib/AST/ASTWalker.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -774,7 +774,7 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
774774
return nullptr;
775775
}
776776

777-
if (!Walker.shouldWalkIntoNonSingleExpressionClosure())
777+
if (!Walker.shouldWalkIntoNonSingleExpressionClosure(expr))
778778
return expr;
779779

780780
// Handle other closures.
@@ -1104,7 +1104,7 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
11041104
}
11051105
}
11061106

1107-
if (!Walker.shouldWalkIntoNonSingleExpressionClosure())
1107+
if (!Walker.shouldWalkIntoTapExpression())
11081108
return E;
11091109

11101110
if (auto oldBody = E->getBody()) {

‎lib/Sema/BuilderTransform.cpp

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -627,7 +627,7 @@ class BuilderClosureRewriter
627627
const Solution &solution;
628628
DeclContext *dc;
629629
AppliedBuilderTransform builderTransform;
630-
std::function<Expr *(Expr *)> rewriteExprFn;
630+
std::function<Expr *(Expr *)> rewriteExpr;
631631
std::function<Expr *(Expr *, Type, ConstraintLocator *)> coerceToType;
632632

633633
/// Retrieve the temporary variable that will be used to capture the
@@ -729,13 +729,6 @@ class BuilderClosureRewriter
729729
elements.push_back(pbd);
730730
}
731731

732-
Expr *rewriteExpr(Expr *expr) {
733-
Expr *result = rewriteExprFn(expr);
734-
if (result)
735-
performSyntacticExprDiagnostics(expr, dc, /*isExprStmt=*/false);
736-
return result;
737-
}
738-
739732
public:
740733
BuilderClosureRewriter(
741734
const Solution &solution,
@@ -745,7 +738,7 @@ class BuilderClosureRewriter
745738
std::function<Expr *(Expr *, Type, ConstraintLocator *)> coerceToType
746739
) : ctx(solution.getConstraintSystem().getASTContext()),
747740
solution(solution), dc(dc), builderTransform(builderTransform),
748-
rewriteExprFn(rewriteExpr),
741+
rewriteExpr(rewriteExpr),
749742
coerceToType(coerceToType){ }
750743

751744
Stmt *visitBraceStmt(BraceStmt *braceStmt, FunctionBuilderTarget target,

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -83,17 +83,23 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
8383
DiagnoseWalker(const DeclContext *DC, bool isExprStmt)
8484
: IsExprStmt(isExprStmt), Ctx(DC->getASTContext()), DC(DC) {}
8585

86-
// Not interested in going outside a basic expression.
87-
std::pair<bool, Stmt *> walkToStmtPre(Stmt *S) override {
88-
return { false, S };
89-
}
9086
std::pair<bool, Pattern*> walkToPatternPre(Pattern *P) override {
9187
return { false, P };
9288
}
93-
bool walkToDeclPre(Decl *D) override { return false; }
89+
90+
bool walkToDeclPre(Decl *D) override {
91+
if (auto *closure = dyn_cast<ClosureExpr>(D->getDeclContext()))
92+
return closure->hasAppliedFunctionBuilder();
93+
return false;
94+
}
95+
9496
bool walkToTypeReprPre(TypeRepr *T) override { return true; }
9597

96-
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
98+
bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *expr) override {
99+
return expr->hasAppliedFunctionBuilder();
100+
}
101+
102+
bool shouldWalkIntoTapExpression() override { return false; }
97103

98104
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
99105
// See through implicit conversions of the expression. We want to be able
@@ -1314,7 +1320,11 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) {
13141320
cast<VarDecl>(DRE->getDecl())->isSelfParameter();
13151321
}
13161322

1317-
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
1323+
bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *expr) override {
1324+
return expr->hasAppliedFunctionBuilder();
1325+
}
1326+
1327+
bool shouldWalkIntoTapExpression() override { return false; }
13181328

13191329
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
13201330
Expr *subExpr;
@@ -1478,10 +1488,16 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
14781488

14791489
// Don't walk into nested decls.
14801490
bool walkToDeclPre(Decl *D) override {
1491+
if (auto *closure = dyn_cast<ClosureExpr>(D->getDeclContext()))
1492+
return closure->hasAppliedFunctionBuilder();
14811493
return false;
14821494
}
14831495

1484-
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
1496+
bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *expr) override {
1497+
return expr->hasAppliedFunctionBuilder();
1498+
}
1499+
1500+
bool shouldWalkIntoTapExpression() override { return false; }
14851501

14861502
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
14871503
if (auto *CE = dyn_cast<AbstractClosureExpr>(E)) {
@@ -3302,7 +3318,11 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) {
33023318
public:
33033319
DiagnoseWalker(ASTContext &ctx) : Ctx(ctx) { }
33043320

3305-
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
3321+
bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *expr) override {
3322+
return expr->hasAppliedFunctionBuilder();
3323+
}
3324+
3325+
bool shouldWalkIntoTapExpression() override { return false; }
33063326

33073327
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
33083328
switch (E->getKind()) {
@@ -3447,7 +3467,11 @@ class ObjCSelectorWalker : public ASTWalker {
34473467
ObjCSelectorWalker(const DeclContext *dc, Type selectorTy)
34483468
: Ctx(dc->getASTContext()), DC(dc), SelectorTy(selectorTy) { }
34493469

3450-
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
3470+
bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *expr) override {
3471+
return expr->hasAppliedFunctionBuilder();
3472+
}
3473+
3474+
bool shouldWalkIntoTapExpression() override { return false; }
34513475

34523476
std::pair<bool, Expr *> walkToExprPre(Expr *expr) override {
34533477
auto *stringLiteral = dyn_cast<StringLiteralExpr>(expr);
@@ -4164,7 +4188,11 @@ static void diagnoseUnintendedOptionalBehavior(const Expr *E,
41644188
}
41654189
}
41664190

4167-
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
4191+
bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *expr) override {
4192+
return expr->hasAppliedFunctionBuilder();
4193+
}
4194+
4195+
bool shouldWalkIntoTapExpression() override { return false; }
41684196

41694197
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
41704198
if (!E || isa<ErrorExpr>(E) || !E->getType())
@@ -4236,7 +4264,11 @@ static void diagnoseDeprecatedWritableKeyPath(const Expr *E,
42364264
}
42374265
}
42384266

4239-
bool shouldWalkIntoNonSingleExpressionClosure() override { return false; }
4267+
bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *expr) override {
4268+
return expr->hasAppliedFunctionBuilder();
4269+
}
4270+
4271+
bool shouldWalkIntoTapExpression() override { return false; }
42404272

42414273
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
42424274
if (!E || isa<ErrorExpr>(E) || !E->getType())

‎lib/Sema/TypeCheckAvailability.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2353,10 +2353,12 @@ class AvailabilityWalker : public ASTWalker {
23532353
return true;
23542354
}
23552355

2356-
bool shouldWalkIntoNonSingleExpressionClosure() override {
2357-
return false;
2356+
bool shouldWalkIntoNonSingleExpressionClosure(ClosureExpr *expr) override {
2357+
return expr->hasAppliedFunctionBuilder();
23582358
}
23592359

2360+
bool shouldWalkIntoTapExpression() override { return false; }
2361+
23602362
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
23612363
ExprStack.push_back(E);
23622364

‎test/Constraints/function_builder_diags.swift

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,3 +299,31 @@ func checkSingleReturn(cond: Bool) {
299299
($0, 17)
300300
}
301301
}
302+
303+
// rdar://problem/59116520
304+
func checkImplicitSelfInClosure() {
305+
@_functionBuilder
306+
struct Builder {
307+
static func buildBlock(_ children: String...) -> Element { Element() }
308+
}
309+
310+
struct Element {
311+
static func nonEscapingClosure(@Builder closure: (() -> Element)) {}
312+
static func escapingClosure(@Builder closure: @escaping (() -> Element)) {}
313+
}
314+
315+
class C {
316+
let identifier: String = ""
317+
318+
func testImplicitSelf() {
319+
Element.nonEscapingClosure {
320+
identifier // okay
321+
}
322+
323+
Element.escapingClosure { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}}
324+
identifier // expected-error {{reference to property 'identifier' in closure requires explicit use of 'self' to make capture semantics explicit}}
325+
// expected-note@-1 {{reference 'self.' explicitly}}
326+
}
327+
}
328+
}
329+
}

0 commit comments

Comments
 (0)