[dart2js] Include block statements as potentially captured scopes.

Some scopes that are capturable by closures are not getting marked as such. Instead the parent scope (e.g. the enclosing member) is being treated as the captured scope that owns all the variable declarations down to the closure. The scopes are then being shared across different closures and variables with the same name are able to be overwritten. This bug occurs for any block that isn't a loop or function (i.e. raw blocks, if blocks, try blocks). Loops are correctly handled as defining a scope and therefore those already work correctly. Note: This may cause some small code size increase. There may now be extra assignements reseting capture boxes in more scopes. Bug: #59843 Change-Id: Ic5009188795daabc1544f703fbeca01c0f1951d9 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/403263 Commit-Queue: Nate Biggs <natebiggs@google.com> Reviewed-by: Stephen Adams <sra@google.com>
dart-lang · Jan 7, 2025 · 057426e · 057426e
1 parent 2258ea7
commit 057426e
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 5 deletions.
diff --git a/pkg/compiler/lib/src/closure.dart b/pkg/compiler/lib/src/closure.dart
@@ -38,6 +38,10 @@ abstract class ClosureData {
   /// the SSA builder.
   CapturedScope getCapturedScope(MemberEntity entity);
 
+  /// Look up scope information about a block. Used by the SSA builder to box
+  /// variables if needed.
+  CapturedScope getCapturedBlockScope(ir.Block node);
+
   /// If [entity] is a closure call method or closure signature method, the
   /// original enclosing member is returned. Otherwise [entity] is returned.
   ///

diff --git a/pkg/compiler/lib/src/io/kernel_source_information.dart b/pkg/compiler/lib/src/io/kernel_source_information.dart
@@ -487,6 +487,11 @@ class KernelSourceInformationBuilder implements SourceInformationBuilder {
     return _buildTreeNode(node);
   }
 
+  @override
+  SourceInformation buildBlock(ir.TreeNode node) {
+    return _buildTreeNode(node);
+  }
+
   @override
   SourceInformation buildCall(
     covariant ir.TreeNode receiver,

diff --git a/pkg/compiler/lib/src/io/source_information.dart b/pkg/compiler/lib/src/io/source_information.dart
@@ -181,6 +181,9 @@ class SourceInformationBuilder {
   /// Generate [SourceInformation] for the if statement in [node].
   SourceInformation? buildIf(ir.TreeNode node) => null;
 
+  /// Generate [SourceInformation] for the block statement in [node].
+  SourceInformation? buildBlock(ir.TreeNode node) => null;
+
   /// Generate [SourceInformation] for the constructor invocation in [node].
   SourceInformation? buildNew(ir.TreeNode node) => null;
 

diff --git a/pkg/compiler/lib/src/ir/scope_visitor.dart b/pkg/compiler/lib/src/ir/scope_visitor.dart
@@ -205,10 +205,7 @@ class ScopeModelBuilder extends ir.VisitorDefault<EvaluationComplexity>
 
       KernelCapturedScope capturedScope;
       var nodeBox = NodeBox(getBoxName(), _executableContext!);
-      if (node is ir.ForStatement ||
-          node is ir.ForInStatement ||
-          node is ir.WhileStatement ||
-          node is ir.DoStatement) {
+      if (node is ir.LoopStatement) {
         capturedScope = KernelCapturedLoopScope(
           capturedVariablesForScope,
           nodeBox,
@@ -1373,7 +1370,17 @@ class ScopeModelBuilder extends ir.VisitorDefault<EvaluationComplexity>
 
   @override
   EvaluationComplexity visitBlock(ir.Block node) {
-    visitNodes(node.statements);
+    final parent = node.parent;
+    if (parent is ir.FunctionNode || parent is ir.LoopStatement) {
+      // Scoping for these blocks are handled by their parent nodes.
+      visitNodes(node.statements);
+    } else {
+      enterNewScope(node, () {
+        visitInVariableScope(node, () {
+          visitNodes(node.statements);
+        });
+      });
+    }
     return const EvaluationComplexity.lazy();
   }
 

diff --git a/pkg/compiler/lib/src/js_model/closure.dart b/pkg/compiler/lib/src/js_model/closure.dart
@@ -200,6 +200,10 @@ class ClosureDataImpl implements ClosureData {
     }
   }
 
+  @override
+  CapturedScope getCapturedBlockScope(ir.Block blockNode) =>
+      _capturedScopesMap.loaded()[blockNode] ?? const CapturedScope();
+
   @override
   // TODO(efortuna): Eventually capturedScopesMap[node] should always
   // be non-null, and we should just test that with an assert.

diff --git a/pkg/compiler/lib/src/ssa/builder.dart b/pkg/compiler/lib/src/ssa/builder.dart
@@ -2481,6 +2481,11 @@ class KernelSsaGraphBuilder extends ir.VisitorDefault<void>
     // a throwing prior subexpression, e.g. `[throw e, {...[]}]`.
     if (!_isReachable) return;
 
+    localsHandler.enterScope(
+      _closureDataLookup.getCapturedBlockScope(node),
+      _sourceInformationBuilder.buildBlock(node),
+    );
+
     for (ir.Statement statement in node.statements) {
       statement.accept(this);
       if (!_isReachable) {

diff --git a/tests/web/regress/issue/59843_test.dart b/tests/web/regress/issue/59843_test.dart
@@ -0,0 +1,64 @@
+import 'package:expect/expect.dart';
+
+void testBlock() {
+  List<void Function()> functions = [];
+  {
+    var sub;
+    functions.add(() {
+      Expect.isNull(sub);
+      sub = 1;
+    });
+  }
+  {
+    var sub;
+    functions.add(() {
+      Expect.equals(sub, 2);
+    });
+    sub = 2;
+  }
+  for (var function in functions) function();
+}
+
+void testIf() {
+  List<void Function()> functions = [];
+  if ('1234'.length > 2) {
+    var sub;
+    functions.add(() {
+      Expect.isNull(sub);
+      sub = 1;
+    });
+  }
+  if ('1234'.length > 2) {
+    var sub;
+    functions.add(() {
+      Expect.equals(sub, 2);
+    });
+    sub = 2;
+  }
+  for (var function in functions) function();
+}
+
+void testTry() {
+  List<void Function()> functions = [];
+  try {
+    var sub;
+    functions.add(() {
+      Expect.isNull(sub);
+      sub = 1;
+    });
+  } catch (e) {}
+  try {
+    var sub;
+    functions.add(() {
+      Expect.equals(sub, 2);
+    });
+    sub = 2;
+  } catch (e) {}
+  for (var function in functions) function();
+}
+
+void main() {
+  testBlock();
+  testIf();
+  testTry();
+}