Dart2JS conflates variables with same name in different blocks

[lrhn]

Example:

void main() {
  List<void Function()> functions = [];
  {
    var sub;
    functions.add(() {
      print("1: $sub"); // 1: 2
      sub = 1;
    });
  }
  {
    var sub;
    functions.add(() {
      print("2: $sub"); // 2: 1
    });
    sub = 2;
  }
  for (var function in functions) function();
}

The variable sub inside the closure in the first block actually refers to the sub variable of the second block.
This can be seen from the program printing 1: 2 and 2: 1 where it should print 1: null and 2: 2.

Marking as soundness-issue too, since the variables don't have to have the same type, so the sub inside the first closure will have the static type of the first variable, and the runtime value of the second, which can be completely unrelated.

To trigger this the variables need to have the same name (rename one to sub1 and the problem goes away),
they have to be in the same function body (convert one block to () { ... }(); and the problem goes away),
both variables have to be captured in a closure, and there has to be at least one assignment to both variables (I guess otherwise that variable is eliminated and the value inlined).
Problem goes away with optimization level -O2 or above.

The generated code has a main starting with:

   main() {
      var t1, _i, _box_0 = {},
        functions = A._setArrayType([], type$.JSArray_of_void_Function);
      _box_0.sub = null;
      B.JSArray_methods.add$1(functions, new A.main_closure(_box_0));
      _box_0.sub = null;
      B.JSArray_methods.add$1(functions, new A.main_closure0(_box_0));
      _box_0.sub = 2;

which shows that it does use the same box object for both closures, and the same variable name.
(That also suggests that it would use the same box for differently named variables too, for different closures, keeping each variable alive longer than it needs.)

At -O2 or above, the individiual variables are renamed, so one is a and the other b. The main code (if I guess correctly) starts with:

c6(){var t,s,r={},q=A.ao([],u.u)
r.a=null
B.a.j(q,new A.af(r))
r.b=null
B.a.j(q,new A.ag(r))
r.b=2

That solves the problem (and still uses the same box object, r, for both closures, even if they share no captured variables).

---

I hit this issue when converting async_minitest.dart-using tests to simple blocks for each test. Some test runners use -O<2, and the tests failed only for dart2js, so the problem gets hit.
I'll add () {...}(); around the blocks for now, with a TODO to remove them when this issue is done.

[biggs0125]

It seems like our ScopeVisitor, which decides what nodes are the root of scopes, isn't set up to treat Block statements as defining their own scope. So in the provided example both sub declarations are treated as being owned by main's scope. Meaning we only allocate a single box for them since we allocate one box per captured scope. We then assume that a given Dart scope can't have two variables with the same name and so we use the Dart name to access both boxed variables leading to the collision.

This is fixed at -O2 and above because minification kicks in and we minify the name of each sub to a different name in the box rather than using the Dart name.

We only treat functions and loop statements as defining a scope. Given that, this is actually reproducible with try blocks and if blocks as well. This also faces the same issue:

void main() {
  List<void Function()> functions = [];
  if (opaqueTrueBool) {
    var sub;
    functions.add(() {
      print("1: $sub"); // 1: 2
      sub = 1;
    });
  }
   if (opaqueTrueBool) {
    var sub;
    functions.add(() {
      print("2: $sub"); // 2: 1
    });
    sub = 2;
  }
  for (var function in functions) function();
}

The fix here is to update the ScopeVisitor (and any downstream dependencies) to treat Block statements as defining a scope, therefore giving them each their own box. This may cause a small code size increase since we may include assignments like t1 = {} in more places to reset boxes as we enter the new scopes. This is necessary for soundness though.

[biggs0125]

It knows the variables are different but it's assigning them to the same box. If the boxes are set up correctly it should be safe to assume that two variables declared in the same box can't have the same name since a box represents a Dart scope and Dart semantics don't allow declaring two variables with the same name.

…

On Tue, Jan 7, 2025 at 2:04 PM Stephen Adams ***@***.***> wrote: The fact that the captured variables are renamed differently when minified indicates that the compiler knows the variables are different, and the bug might be in naming the closed-over slots, rather than the environment modelling. — Reply to this email directly, view it on GitHub <#59843 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AART2EJ3AIVAGYD5RACIVJ32JQQLXAVCNFSM6AAAAABUVLKZYOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZWGAZDKMBUG4> . You are receiving this because you were assigned.Message ID: ***@***.***>