From 87308e37f1e1794575d79d36d54ad3987cea8226 Mon Sep 17 00:00:00 2001 From: Allan Shortlidge Date: Wed, 15 Jan 2025 16:00:06 -0800 Subject: [PATCH] AST: Avoid creating duplicate AvailabilityScopes under SequenceExprs. Since availability scopes may be built at arbitrary times, the builder may encounter ASTs where SequenceExprs still exist and have not been folded, or it may encounter folded SequenceExprs that have not been removed from the AST. To avoid a double visit, track whether a SequenceExpr is folded and then customize how ASTVisitor handles folded sequences. Resolves rdar://142824799 and https://github.com/swiftlang/swift/issues/78567. --- include/swift/AST/ASTWalker.h | 20 ++++++++++++++++++++ include/swift/AST/Expr.h | 12 ++++++++++-- lib/AST/ASTWalker.cpp | 10 ++++++++++ lib/Sema/PreCheckTarget.cpp | 1 + lib/Sema/TypeCheckAvailability.cpp | 9 +++++++++ test/Sema/availability_scopes.swift | 16 ++++++++++++++++ 6 files changed, 66 insertions(+), 2 deletions(-) diff --git a/include/swift/AST/ASTWalker.h b/include/swift/AST/ASTWalker.h index e8f42599f1f15..dbc76128fe981 100644 --- a/include/swift/AST/ASTWalker.h +++ b/include/swift/AST/ASTWalker.h @@ -133,6 +133,19 @@ enum class QualifiedIdentTypeReprWalkingScheme { ASTOrderRecursive }; +/// Specifies the behavior for walking SequenceExprs. +enum class SequenceWalking { + /// Walk into every element of the sequence, regardless of what state the + /// sequence is in. + Default, + + /// If the sequence has been folded by type checking, only walk into the + /// elements that represent the operator nodes. This will ensure that the walk + /// does not visit the same AST nodes twice when it encounters a sequence that + /// has already been folded but hasn't been removed from the AST. + OnlyWalkFirstOperatorWhenFolded +}; + /// An abstract class used to traverse an AST. class ASTWalker { public: @@ -616,6 +629,13 @@ class ASTWalker { return MacroWalking::ArgumentsAndExpansion; } + /// This method configures how the walker should walk into SequenceExprs. + /// Needing to customize this behavior should be rare, as sequence expressions + /// are only encountered in un-typechecked ASTs. + virtual SequenceWalking getSequenceWalkingBehavior() const { + return SequenceWalking::Default; + } + /// This method determines whether the given declaration should be /// considered to be in a macro expansion context. It can be configured /// by subclasses. diff --git a/include/swift/AST/Expr.h b/include/swift/AST/Expr.h index 5afd1fd6235c8..f34ffc15db38e 100644 --- a/include/swift/AST/Expr.h +++ b/include/swift/AST/Expr.h @@ -368,9 +368,10 @@ class alignas(8) Expr : public ASTAllocated { IsObjC : 1 ); - SWIFT_INLINE_BITFIELD_FULL(SequenceExpr, Expr, 32, + SWIFT_INLINE_BITFIELD_FULL(SequenceExpr, Expr, 32+1, : NumPadBits, - NumElements : 32 + NumElements : 32, + IsFolded: 1 ); SWIFT_INLINE_BITFIELD(OpaqueValueExpr, Expr, 1, @@ -4002,6 +4003,13 @@ class SequenceExpr final : public Expr, getElements()[i] = e; } + bool isFolded() const { + return static_cast(Bits.SequenceExpr.IsFolded); + } + void setFolded(bool folded) { + Bits.SequenceExpr.IsFolded = static_cast(folded); + } + // Implement isa/cast/dyncast/etc. static bool classof(const Expr *E) { return E->getKind() == ExprKind::Sequence; diff --git a/lib/AST/ASTWalker.cpp b/lib/AST/ASTWalker.cpp index 0ef8d99e68264..ebc92ed6ec7b3 100644 --- a/lib/AST/ASTWalker.cpp +++ b/lib/AST/ASTWalker.cpp @@ -955,6 +955,16 @@ class Traversal : public ASTVisitorisFolded()) { + if (E->getNumElements() > 1) { + if (Expr *Elt = doIt(E->getElement(1))) + E->setElement(1, Elt); + } + return E; + } + for (unsigned i = 0, e = E->getNumElements(); i != e; ++i) if (Expr *Elt = doIt(E->getElement(i))) E->setElement(i, Elt); diff --git a/lib/Sema/PreCheckTarget.cpp b/lib/Sema/PreCheckTarget.cpp index 9e0eec2b94ab7..8edfe5e62825c 100644 --- a/lib/Sema/PreCheckTarget.cpp +++ b/lib/Sema/PreCheckTarget.cpp @@ -1166,6 +1166,7 @@ class PreCheckTarget final : public ASTWalker { if (!result) return Action::Stop(); // Already walked. + seqExpr->setFolded(true); return Action::SkipNode(result); } diff --git a/lib/Sema/TypeCheckAvailability.cpp b/lib/Sema/TypeCheckAvailability.cpp index 7cd3c31126993..38f1d8c5681a5 100644 --- a/lib/Sema/TypeCheckAvailability.cpp +++ b/lib/Sema/TypeCheckAvailability.cpp @@ -538,6 +538,15 @@ class AvailabilityScopeBuilder : private ASTWalker { return MacroWalking::Arguments; } + SequenceWalking getSequenceWalkingBehavior() const override { + // Since availability scopes may be built at arbitrary times, the builder + // may encounter ASTs where SequenceExprs still exist and have not been + // folded, or it may encounter folded SequenceExprs that have not been + // removed from the AST. When folded exprs are encountered, its important + // to avoid walking into the same AST nodes twice. + return SequenceWalking::OnlyWalkFirstOperatorWhenFolded; + } + /// Check whether this declaration is within a macro expansion buffer that /// will have its own availability scope that will be lazily expanded. bool isDeclInMacroExpansion(Decl *decl) const override { diff --git a/test/Sema/availability_scopes.swift b/test/Sema/availability_scopes.swift index c1040c8f0bc6f..69d1d672fb754 100644 --- a/test/Sema/availability_scopes.swift +++ b/test/Sema/availability_scopes.swift @@ -321,6 +321,22 @@ func testStringInterpolation() { """ } +// CHECK-NEXT: {{^}} (decl_implicit version=50 decl=result +// CHECK-NEXT: {{^}} (decl_implicit version=50 decl=unusedA +// CHECK-NEXT: {{^}} (decl_implicit version=50 decl=unusedB + +func testSequenceExprs(b: Bool, x: Int?) { + let result = b + ? x.map { + let unusedA: Int + return $0 + } + : x.map { + let unusedB: Int + return $0 + } +} + // CHECK-NEXT: {{^}} (decl version=50 unavailable=macOS decl=unavailableOnMacOS() // CHECK-NEXT: {{^}} (decl_implicit version=50 unavailable=macOS decl=x