Skip to content

Commit c8c080b

Browse files
committed
[AST] Always walk folded SequenceExpr if available
Expand the special-cased ASTWalker behavior for folded SequenceExprs such that we always walk the folded expression when available. This ensures that we don't attempt to add the same node multiple times when expanding ASTScopes during pre-checking. rdar://147751795
1 parent 83db811 commit c8c080b

File tree

7 files changed

+44
-44
lines changed

7 files changed

+44
-44
lines changed

include/swift/AST/ASTWalker.h

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -133,19 +133,6 @@ enum class QualifiedIdentTypeReprWalkingScheme {
133133
ASTOrderRecursive
134134
};
135135

136-
/// Specifies the behavior for walking SequenceExprs.
137-
enum class SequenceWalking {
138-
/// Walk into every element of the sequence, regardless of what state the
139-
/// sequence is in.
140-
Default,
141-
142-
/// If the sequence has been folded by type checking, only walk into the
143-
/// elements that represent the operator nodes. This will ensure that the walk
144-
/// does not visit the same AST nodes twice when it encounters a sequence that
145-
/// has already been folded but hasn't been removed from the AST.
146-
OnlyWalkFirstOperatorWhenFolded
147-
};
148-
149136
/// An abstract class used to traverse an AST.
150137
class ASTWalker {
151138
public:
@@ -629,13 +616,6 @@ class ASTWalker {
629616
return MacroWalking::ArgumentsAndExpansion;
630617
}
631618

632-
/// This method configures how the walker should walk into SequenceExprs.
633-
/// Needing to customize this behavior should be rare, as sequence expressions
634-
/// are only encountered in un-typechecked ASTs.
635-
virtual SequenceWalking getSequenceWalkingBehavior() const {
636-
return SequenceWalking::Default;
637-
}
638-
639619
/// This method determines whether the given declaration should be
640620
/// considered to be in a macro expansion context. It can be configured
641621
/// by subclasses.

include/swift/AST/Expr.h

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -368,10 +368,9 @@ class alignas(8) Expr : public ASTAllocated<Expr> {
368368
IsObjC : 1
369369
);
370370

371-
SWIFT_INLINE_BITFIELD_FULL(SequenceExpr, Expr, 32+1,
371+
SWIFT_INLINE_BITFIELD_FULL(SequenceExpr, Expr, 32,
372372
: NumPadBits,
373-
NumElements : 32,
374-
IsFolded: 1
373+
NumElements : 32
375374
);
376375

377376
SWIFT_INLINE_BITFIELD(OpaqueValueExpr, Expr, 1,
@@ -3970,6 +3969,9 @@ class SequenceExpr final : public Expr,
39703969
private llvm::TrailingObjects<SequenceExpr, Expr *> {
39713970
friend TrailingObjects;
39723971

3972+
/// The cached folded expression.
3973+
Expr *FoldedExpr = nullptr;
3974+
39733975
SequenceExpr(ArrayRef<Expr*> elements)
39743976
: Expr(ExprKind::Sequence, /*Implicit=*/false) {
39753977
Bits.SequenceExpr.NumElements = elements.size();
@@ -4005,11 +4007,14 @@ class SequenceExpr final : public Expr,
40054007
getElements()[i] = e;
40064008
}
40074009

4008-
bool isFolded() const {
4009-
return static_cast<bool>(Bits.SequenceExpr.IsFolded);
4010+
/// Retrieve the folded expression, or \c nullptr if the SequencExpr has
4011+
/// not yet been folded.
4012+
Expr *getFoldedExpr() const {
4013+
return FoldedExpr;
40104014
}
4011-
void setFolded(bool folded) {
4012-
Bits.SequenceExpr.IsFolded = static_cast<unsigned>(folded);
4015+
4016+
void setFoldedExpr(Expr *foldedExpr) {
4017+
FoldedExpr = foldedExpr;
40134018
}
40144019

40154020
// Implement isa/cast/dyncast/etc.

lib/AST/ASTWalker.cpp

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -950,13 +950,18 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
950950
}
951951

952952
Expr *visitSequenceExpr(SequenceExpr *E) {
953-
if (Walker.getSequenceWalkingBehavior() ==
954-
SequenceWalking::OnlyWalkFirstOperatorWhenFolded &&
955-
E->isFolded()) {
956-
if (E->getNumElements() > 1) {
957-
if (Expr *Elt = doIt(E->getElement(1)))
958-
E->setElement(1, Elt);
959-
}
953+
// After folding, the SequenceExpr elements can contain a broken mess of
954+
// partially folded and/or duplicate nodes (since folding can mutate nodes
955+
// in-place). We remove the SequenceExpr from the AST after folding, but
956+
// there's still a period of time during pre-checking when it's still in the
957+
// AST. To avoid issues for e.g ASTScope and availability scope
958+
// construction, only walk the folded expression if we have it.
959+
if (auto *foldedExpr = E->getFoldedExpr()) {
960+
auto *newExpr = doIt(foldedExpr);
961+
if (!newExpr)
962+
return nullptr;
963+
964+
E->setFoldedExpr(newExpr);
960965
return E;
961966
}
962967

lib/AST/AvailabilityScopeBuilder.cpp

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -176,15 +176,6 @@ class AvailabilityScopeBuilder : private ASTWalker {
176176
return MacroWalking::Arguments;
177177
}
178178

179-
SequenceWalking getSequenceWalkingBehavior() const override {
180-
// Since availability scopes may be built at arbitrary times, the builder
181-
// may encounter ASTs where SequenceExprs still exist and have not been
182-
// folded, or it may encounter folded SequenceExprs that have not been
183-
// removed from the AST. When folded exprs are encountered, its important
184-
// to avoid walking into the same AST nodes twice.
185-
return SequenceWalking::OnlyWalkFirstOperatorWhenFolded;
186-
}
187-
188179
/// Check whether this declaration is within a macro expansion buffer that
189180
/// will have its own availability scope that will be lazily expanded.
190181
bool isDeclInMacroExpansion(Decl *decl) const override {

lib/Sema/PreCheckTarget.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1168,7 +1168,6 @@ class PreCheckTarget final : public ASTWalker {
11681168
if (!result)
11691169
return Action::Stop();
11701170
// Already walked.
1171-
seqExpr->setFolded(true);
11721171
return Action::SkipNode(result);
11731172
}
11741173

lib/Sema/TypeCheckExpr.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,7 @@ Expr *TypeChecker::foldSequence(SequenceExpr *expr, DeclContext *dc) {
649649
Expr *Result = ::foldSequence(dc, LHS, Elts, PrecedenceBound());
650650
assert(Elts.empty());
651651

652+
expr->setFoldedExpr(Result);
652653
return Result;
653654
}
654655

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Note: test of the scope map. All of these tests are line- and
2+
// column-sensitive, so any additions should go at the end.
3+
4+
struct X {}
5+
var y: () -> X
6+
7+
struct S {
8+
let x: () = y = {() -> X in
9+
return .init()
10+
}
11+
}
12+
13+
// RUN: %target-swift-frontend -dump-scope-maps expanded %s 2>&1 | %FileCheck %s
14+
15+
// rdar://147751795 - Make sure we don't end up with a duplicate closure scope.
16+
// CHECK: PatternEntryInitializerScope {{.*}}, [8:15 - 10:3] entry 0 'x'
17+
// CHECK-NEXT: `-ClosureParametersScope {{.*}}, [8:28 - 10:3]
18+
// CHECK-NEXT: `-BraceStmtScope {{.*}}, [8:28 - 10:3]
19+
// CHECK-EMPTY:

0 commit comments

Comments
 (0)