[ASTScopes] Avoid relying on in SourceLoc for closures

Missed this in my previous patch, avoid relying on the `in` location for closures with parameters and capture lists. Instead, form scopes that start at the first element of the body. This fixes a crasher uncovered by the fuzzer.
2025-12-14 20:36:38 +01:00 · 2025-10-17 15:21:53 +01:00
parent 39a36d9216
commit 8232345b68
8 changed files with 57 additions and 63 deletions
--- a/include/swift/AST/ASTScope.h
+++ b/include/swift/AST/ASTScope.h
@@ -1855,7 +1855,6 @@ public:
  SourceRange
  getSourceRangeOfThisASTNode(bool omitAssertions = false) const override;

-  NullablePtr<AbstractClosureExpr> parentClosureIfAny() const; // public??
  BraceStmt *getStmt() const override { return stmt; }

 protected:
--- a/lib/AST/ASTScope.cpp
+++ b/lib/AST/ASTScope.cpp
@@ -224,15 +224,6 @@ Pattern *AbstractPatternEntryScope::getPattern() const {
  return getPatternEntry().getPattern();
 }

-NullablePtr<AbstractClosureExpr> BraceStmtScope::parentClosureIfAny() const {
-  if (auto parent = getParent()) {
-    if (auto closureScope = dyn_cast<ClosureParametersScope>(parent.get()))
-      return closureScope->closureExpr;
-  }
-
-  return nullptr;
-}
-
 std::string ASTScopeImpl::getClassName() const {
  // GenericTypeOrExtensionScope provides a custom implementation that deals
  // with declaration names and "portions".
--- a/lib/AST/ASTScopeSourceRange.cpp
+++ b/lib/AST/ASTScopeSourceRange.cpp
@@ -299,16 +299,27 @@ CaseStmtBodyScope::getSourceRangeOfThisASTNode(const bool omitAssertions) const
  return stmt->getBody()->getSourceRange();
 }

+/// Retrieve a SourceRange for a closure that covers the elements of its body,
+/// excluding its parameter list and captures if present.
+static SourceRange getClosureBodyContentRange(AbstractClosureExpr *ACE) {
+  // Autoclosures don't have explicit capture lists or parameters so we can
+  // just use the whole range.
+  if (auto *autoClosure = dyn_cast<AutoClosureExpr>(ACE))
+    return autoClosure->getSourceRange();
+
+  // Produce a range from the first body element to the end of the closure.
+  return SourceRange::combine(ACE->getBody()->getContentStartLoc(),
+                              ACE->getEndLoc());
+}
+
 SourceRange
 BraceStmtScope::getSourceRangeOfThisASTNode(const bool omitAssertions) const {
-  // The brace statements that represent closures start their scope at the
-  // 'in' keyword, when present.
-  if (auto anyClosure = parentClosureIfAny()) {
-    if (auto closure = dyn_cast<ClosureExpr>(parentClosureIfAny().get())) {
-      if (closure->getInLoc().isValid()) {
-        return SourceRange(closure->getInLoc(), endLoc);
-      }
-    }
+  // If we have a parent closure, the start location is given by the start
+  // of the first body element.
+  if (auto *closureParent = dyn_cast_or_null<ClosureParametersScope>(
+          getParent().getPtrOrNull())) {
+    auto closureRange = closureParent->getSourceRangeOfThisASTNode();
+    return SourceRange(closureRange.Start, endLoc);
  }
  return SourceRange(stmt->getStartLoc(), endLoc);
 }
@@ -325,28 +336,12 @@ SourceRange ConditionalClausePatternUseScope::getSourceRangeOfThisASTNode(

 SourceRange
 CaptureListScope::getSourceRangeOfThisASTNode(const bool omitAssertions) const {
-  if (auto autoClosure = dyn_cast<AutoClosureExpr>(expr->getClosureBody())) {
-    return autoClosure->getSourceRange();
-  }
-  auto closureExpr = cast<ClosureExpr>(expr->getClosureBody());
-  if (!omitAssertions)
-    ASTScopeAssert(closureExpr->getInLoc().isValid(),
-                   "We don't create these if no in loc");
-  return SourceRange(closureExpr->getInLoc(), closureExpr->getEndLoc());
+  return getClosureBodyContentRange(expr->getClosureBody());
 }

-SourceRange ClosureParametersScope::getSourceRangeOfThisASTNode(
-    const bool omitAssertions) const {
-  if (auto autoClosure = dyn_cast<AutoClosureExpr>(closureExpr)) {
-    return autoClosure->getSourceRange();
-  }
-  auto explicitClosureExpr = cast<ClosureExpr>(closureExpr);
-  if (explicitClosureExpr->getInLoc().isValid()) {
-    return SourceRange(explicitClosureExpr->getInLoc(),
-                       explicitClosureExpr->getEndLoc());
-  }
-
-  return explicitClosureExpr->getSourceRange();
+SourceRange
+ClosureParametersScope::getSourceRangeOfThisASTNode(bool omitAssertions) const {
+  return getClosureBodyContentRange(closureExpr);
 }

 SourceRange CustomAttributeScope::getSourceRangeOfThisASTNode(
--- a/lib/AST/Stmt.cpp
+++ b/lib/AST/Stmt.cpp
@@ -221,7 +221,11 @@ SourceLoc BraceStmt::getEndLoc() const {

 SourceLoc BraceStmt::getContentStartLoc() const {
  for (auto elt : getElements()) {
-    if (auto loc = elt.getStartLoc()) {
+    if (auto *D = elt.dyn_cast<Decl *>()) {
+      // FIXME: This should really be the default behavior of Decl::getStartLoc.
+      if (auto range = D->getSourceRangeIncludingAttrs())
+        return range.Start;
+    } else if (auto loc = elt.getStartLoc()) {
      return loc;
    }
  }
--- a/test/DebugInfo/guard-let-scope4.swift
+++ b/test/DebugInfo/guard-let-scope4.swift
@@ -6,8 +6,7 @@ open class C {
    { [weak self] in
      guard let self else { fatalError("cannot happen") }
      // CHECK: sil_scope [[LAMBDA:[0-9]+]] { loc "{{.*}}":6:5
-      // CHECK: sil_scope [[BODY:[0-9]+]] { loc "{{.*}}":6:19 parent [[LAMBDA]]
-      // CHECK: sil_scope [[LET:[0-9]+]] { loc "{{.*}}":7:7 parent [[BODY]]
+      // CHECK: sil_scope [[LET:[0-9]+]] { loc "{{.*}}":7:7 parent [[LAMBDA]]
      // CHECK-HAS-WEAK-LET: sil_scope [[TMP:[0-9]+]] { loc "{{.*}}":7:17 parent [[LET]]
      // CHECK: sil_scope [[GUARD:[0-9]+]] { loc "{{.*}}":7:17 parent [[LET]]
      // CHECK: debug_value {{.*}} : $C, let, name "self", {{.*}}, scope [[GUARD]]
--- a/test/NameLookup/scope_map-astscopelookup.swift
+++ b/test/NameLookup/scope_map-astscopelookup.swift
@@ -297,8 +297,8 @@ func HasLabeledDo() {
 // CHECK-EXPANDED-NEXT:               `-GuardStmtScope {{.*}}, [69:3 - 101:1] 
 // CHECK-EXPANDED-NEXT:                 `-ConditionalClausePatternUseScope, [69:18 - 101:1] let b1
 // CHECK-EXPANDED-NEXT:                   |-ConditionalClauseInitializerScope, [69:18 - 69:18] 
-// CHECK-EXPANDED-NEXT:                   |-ClosureParametersScope {{.*}}, [69:21 - 69:30] 
-// CHECK-EXPANDED-NEXT:                     `-BraceStmtScope {{.*}}, [69:21 - 69:30] 
+// CHECK-EXPANDED-NEXT:                   |-ClosureParametersScope {{.*}}, [69:23 - 69:30]
+// CHECK-EXPANDED-NEXT:                     `-BraceStmtScope {{.*}}, [69:23 - 69:30]
 // CHECK-EXPANDED-NEXT:                   `-ConditionalClausePatternUseScope, [69:46 - 101:1] let b2
 // CHECK-EXPANDED-NEXT:                     |-ConditionalClauseInitializerScope, [69:46 - 69:46] 
 // CHECK-EXPANDED-NEXT:                     |-GuardStmtBodyScope, [69:53 - 72:3] 
@@ -391,35 +391,35 @@ func HasLabeledDo() {
 // CHECK-EXPANDED-NEXT: |-AbstractFunctionDeclScope {{.*}}, [158:1 - 163:1] 'closures()'
 // CHECK-EXPANDED-NEXT:   `-FunctionBodyScope {{.*}}, [158:17 - 163:1] 
 // CHECK-EXPANDED-NEXT:     `-BraceStmtScope {{.*}}, [158:17 - 163:1] 
-// CHECK-EXPANDED-NEXT:       |-ClosureParametersScope {{.*}}, [159:10 - 161:3] 
-// CHECK-EXPANDED-NEXT:         `-BraceStmtScope {{.*}}, [159:10 - 161:3] 
-// CHECK-EXPANDED-NEXT:           `-ClosureParametersScope {{.*}}, [160:12 - 160:22] 
-// CHECK-EXPANDED-NEXT:             `-BraceStmtScope {{.*}}, [160:12 - 160:22] 
-// CHECK-EXPANDED-NEXT:       `-ClosureParametersScope {{.*}}, [162:10 - 162:19] 
-// CHECK-EXPANDED-NEXT:         `-BraceStmtScope {{.*}}, [162:10 - 162:19] 
-// CHECK-EXPANDED-NEXT: `-TopLevelCodeScope {{.*}}, [165:1 - 481:1] 
+// CHECK-EXPANDED-NEXT:       |-ClosureParametersScope {{.*}}, [160:5 - 161:3]
+// CHECK-EXPANDED-NEXT:         `-BraceStmtScope {{.*}}, [160:5 - 161:3]
+// CHECK-EXPANDED-NEXT:           `-ClosureParametersScope {{.*}}, [160:14 - 160:22]
+// CHECK-EXPANDED-NEXT:             `-BraceStmtScope {{.*}}, [160:14 - 160:22]
+// CHECK-EXPANDED-NEXT:       `-ClosureParametersScope {{.*}}, [162:13 - 162:19]
+// CHECK-EXPANDED-NEXT:         `-BraceStmtScope {{.*}}, [162:13 - 162:19]
+// CHECK-EXPANDED-NEXT: `-TopLevelCodeScope {{.*}}, [165:1 - 481:1]
 // CHECK-EXPANDED-NEXT:   `-BraceStmtScope {{.*}}, [165:1 - 481:1] 
-// CHECK-EXPANDED-NEXT:     |-ClosureParametersScope {{.*}}, [165:1 - 165:14] 
-// CHECK-EXPANDED-NEXT:       `-BraceStmtScope {{.*}}, [165:1 - 165:14] 
+// CHECK-EXPANDED-NEXT:     |-ClosureParametersScope {{.*}}, [165:3 - 165:14]
+// CHECK-EXPANDED-NEXT:       `-BraceStmtScope {{.*}}, [165:3 - 165:14]
 // CHECK-EXPANDED-NEXT:     |-AbstractFunctionDeclScope {{.*}}, [167:1 - 176:1] 'defaultArguments(i:j:)'
 // CHECK-EXPANDED-NEXT:       |-ParameterListScope {{.*}}, [167:22 - 168:49] 
 // CHECK-EXPANDED-NEXT:         |-DefaultArgumentInitializerScope {{.*}}, [167:32 - 167:32] 
-// CHECK-EXPANDED-NEXT:         `-DefaultArgumentInitializerScope {{.*}}, [168:32 - 168:48] 
-// CHECK-EXPANDED-NEXT:           `-ClosureParametersScope {{.*}}, [168:32 - 168:42] 
-// CHECK-EXPANDED-NEXT:             `-BraceStmtScope {{.*}}, [168:32 - 168:42] 
-// CHECK-EXPANDED-NEXT:       `-FunctionBodyScope {{.*}}, [168:51 - 176:1] 
+// CHECK-EXPANDED-NEXT:         `-DefaultArgumentInitializerScope {{.*}}, [168:32 - 168:48]
+// CHECK-EXPANDED-NEXT:           `-ClosureParametersScope {{.*}}, [168:34 - 168:42]
+// CHECK-EXPANDED-NEXT:             `-BraceStmtScope {{.*}}, [168:34 - 168:42]
+// CHECK-EXPANDED-NEXT:       `-FunctionBodyScope {{.*}}, [168:51 - 176:1]
 // CHECK-EXPANDED-NEXT:         `-BraceStmtScope {{.*}}, [168:51 - 176:1] 
 // CHECK-EXPANDED-NEXT:           |-AbstractFunctionDeclScope {{.*}}, [170:3 - 172:3] 'localWithDefaults(i:j:)'
 // CHECK-EXPANDED-NEXT:             |-ParameterListScope {{.*}}, [170:25 - 171:52] 
 // CHECK-EXPANDED-NEXT:               |-DefaultArgumentInitializerScope {{.*}}, [170:35 - 170:35] 
 // CHECK-EXPANDED-NEXT:               `-DefaultArgumentInitializerScope {{.*}}, [171:35 - 171:51] 
-// CHECK-EXPANDED-NEXT:                 `-ClosureParametersScope {{.*}}, [171:35 - 171:45] 
-// CHECK-EXPANDED-NEXT:                   `-BraceStmtScope {{.*}}, [171:35 - 171:45] 
-// CHECK-EXPANDED-NEXT:             `-FunctionBodyScope {{.*}}, [171:54 - 172:3] 
+// CHECK-EXPANDED-NEXT:                 `-ClosureParametersScope {{.*}}, [171:37 - 171:45]
+// CHECK-EXPANDED-NEXT:                   `-BraceStmtScope {{.*}}, [171:37 - 171:45]
+// CHECK-EXPANDED-NEXT:             `-FunctionBodyScope {{.*}}, [171:54 - 172:3]
 // CHECK-EXPANDED-NEXT:           `-PatternEntryDeclScope {{.*}}, [174:7 - 176:1] entry 0 'a'
 // CHECK-EXPANDED-NEXT:             `-PatternEntryInitializerScope {{.*}}, [174:11 - 175:11] entry 0 'a'
-// CHECK-EXPANDED-NEXT:               `-ClosureParametersScope {{.*}}, [175:3 - 175:8] 
-// CHECK-EXPANDED-NEXT:                 `-BraceStmtScope {{.*}}, [175:3 - 175:8] 
+// CHECK-EXPANDED-NEXT:               `-ClosureParametersScope {{.*}}, [175:5 - 175:8]
+// CHECK-EXPANDED-NEXT:                 `-BraceStmtScope {{.*}}, [175:5 - 175:8] 
 // CHECK-EXPANDED-NEXT:     |-NominalTypeDeclScope {{.*}}, [178:1 - 181:1] 'PatternInitializers'
 // CHECK-EXPANDED-NEXT:       `-NominalTypeBodyScope {{.*}}, [178:28 - 181:1] 'PatternInitializers'
 // CHECK-EXPANDED-NEXT:         |-PatternEntryDeclScope {{.*}}, [179:7 - 179:21] entry 0 'a' 'b'
--- a/test/NameLookup/scope_map_rdar147751795.swift
+++ b/test/NameLookup/scope_map_rdar147751795.swift
@@ -14,6 +14,6 @@ struct S {

 // rdar://147751795 - Make sure we don't end up with a duplicate closure scope.
 // CHECK:      PatternEntryInitializerScope {{.*}}, [8:15 - 10:3] entry 0 'x'
-// CHECK-NEXT:   `-ClosureParametersScope {{.*}}, [8:28 - 10:3]
-// CHECK-NEXT:     `-BraceStmtScope {{.*}}, [8:28 - 10:3]
+// CHECK-NEXT:   `-ClosureParametersScope {{.*}}, [9:5 - 10:3]
+// CHECK-NEXT:     `-BraceStmtScope {{.*}}, [9:5 - 10:3]
 // CHECK-EMPTY:
--- a/validation-test/compiler_crashers_2_fixed/2d2ef7abf5cfc8c5.swift
+++ b/validation-test/compiler_crashers_2_fixed/2d2ef7abf5cfc8c5.swift
@@ -0,0 +1,6 @@
+// {"kind":"typecheck","original":"1a6377c6","signature":"swift::ast_scope::ASTScopeImpl::getCharSourceRangeOfScope(swift::SourceManager&, bool) const","signatureAssert":"Assertion failed: ((closureExpr->getInLoc().isValid()) && \"We don't create these if no in loc\"), function getSourceRangeOfThisASTNode"}
+// RUN: not %target-swift-frontend -typecheck %s
+{
+  [a]
+  b -> c
+}