[cfe/ffi] Fix Finalizable in for( in ) loops

The `Finalizable` visitor was visiting for-in loops in AST order:
(1) variable, (2) iterable, (3) body. This caused the `variable` to be
fenced in the `iterable` expression. The `variable` should only be
fenced in the `body`.

TEST=tests/ffi/regress_51538_test.dart
TEST=pkg/vm/test/transformations/ffi_test.dart
     with pkg/vm/testcases/transformations/ffi/regress_51538.dart

Closes: https://github.com/dart-lang/sdk/issues/51538
Change-Id: Idacf87b6de3ee0d2d5c6c5046060c55135593fed
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/286182
Auto-Submit: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>

pkg/vm/lib/transformations/ffi/finalizable.dart

@@ -149,11 +149,38 @@ mixin FinalizableTransformer on Transformer {
 
   @override
   TreeNode visitForInStatement(ForInStatement node) {
-    return inScope(
-      node,
-      () => super.visitForInStatement(node),
-      appendFencesToStatement: node.body,
-    );
+    // This does not use [inScope], because it would visit [iterable] with
+    // [variable] in scope.
+
+    // First, transform the iterable, which does not have variable in scope.
+    // ignore: unnecessary_null_comparison
+    if (node.iterable != null) {
+      node.iterable = transform(node.iterable);
+      node.iterable.parent = node;
+    }
+
+    final scope = _Scope(node, parent: _currentScope);
+    _currentScope = scope;
+
+    // Then, transform the variable, adding it to the new scope.
+    // ignore: unnecessary_null_comparison
+    if (node.variable != null) {
+      assert(node.variable.initializer == null);
+      node.variable = transform(node.variable);
+      node.variable.parent = node;
+    }
+
+    // Then transform the body, with the new variable in scope.
+    // ignore: unnecessary_null_comparison
+    if (node.body != null) {
+      node.body = transform(node.body);
+      node.body.parent = node;
+    }
+
+    _appendReachabilityFences(node.body, scope.toFenceThisScope);
+
+    _currentScope = _currentScope!.parent;
+    return node;
   }
 
   @override
@@ -625,6 +652,22 @@ class _Scope {
             (parent?.allDeclarationsIsEmpty ?? true) &&
                 !(declaresThis ?? false);
 
+  @override
+  String toString() => toStringIndented();
+
+  toStringIndented({int indentation = 0}) {
+    final nonIndented = '''node: $node
+declarations:${_declarations.map((e) => '''
+  $e''').join()}
+declaresThis: $declaresThis
+labels:${_labels.map((e) => '''
+  $e''').join()}
+parent:
+${parent?.toStringIndented(indentation: indentation + 2)}
+''';
+    return nonIndented.replaceAll('\n', (' ' * indentation) + '\n');
+  }
+
   void addDeclaration(VariableDeclaration declaration) {
     _declarations.add(declaration);
     allDeclarationsIsEmpty = false;

pkg/vm/testcases/transformations/ffi/regress_51538.dart

@@ -0,0 +1,17 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+class Foo implements Finalizable {}
+
+Future<Foo> bar() => Future.value(Foo());
+
+void main() async {
+  for (final element in [await bar()]) {
+    print(element);
+  }
+}

pkg/vm/testcases/transformations/ffi/regress_51538.dart.aot.expect

@@ -0,0 +1,28 @@
+library #lib /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+import "dart:ffi" as ffi;
+import "dart:async" as asy;
+import "dart:_internal" as _in;
+
+import "dart:ffi";
+
+class Foo extends core::Object implements ffi::Finalizable {
+  synthetic constructor •() → self::Foo
+    : super core::Object::•()
+    ;
+}
+static method bar() → asy::Future<self::Foo>
+  return [@vm.inferred-type.metadata=dart.async::_Future<#lib::Foo>] asy::Future::value<self::Foo>(new self::Foo::•());
+static method main() → void async /* futureValueType= void */ {
+  {
+    core::Iterator<self::Foo> :sync-for-iterator = [@vm.direct-call.metadata=dart.core::_GrowableList.iterator] [@vm.inferred-type.metadata=dart._internal::ListIterator<#lib::Foo>] [@vm.inferred-type.metadata=dart.core::_GrowableList<#lib::Foo>] core::_GrowableList::_literal1<self::Foo>(await self::bar()).{core::Iterable::iterator}{core::Iterator<self::Foo>};
+    for (; [@vm.direct-call.metadata=dart._internal::ListIterator.moveNext] [@vm.inferred-type.metadata=dart.core::bool (skip check)] :sync-for-iterator.{core::Iterator::moveNext}(){() → core::bool}; ) {
+      final self::Foo element = [@vm.direct-call.metadata=dart._internal::ListIterator.current] [@vm.inferred-type.metadata=#lib::Foo] :sync-for-iterator.{core::Iterator::current}{self::Foo};
+      {
+        core::print(element);
+        _in::reachabilityFence(element);
+      }
+    }
+  }
+}

pkg/vm/testcases/transformations/ffi/regress_51538.dart.expect

@@ -0,0 +1,28 @@
+library #lib /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+import "dart:ffi" as ffi;
+import "dart:async" as asy;
+import "dart:_internal" as _in;
+
+import "dart:ffi";
+
+class Foo extends core::Object implements ffi::Finalizable {
+  synthetic constructor •() → self::Foo
+    : super core::Object::•()
+    ;
+}
+static method bar() → asy::Future<self::Foo>
+  return asy::Future::value<self::Foo>(new self::Foo::•());
+static method main() → void async /* futureValueType= void */ {
+  {
+    core::Iterator<self::Foo> :sync-for-iterator = core::_GrowableList::_literal1<self::Foo>(await self::bar()).{core::Iterable::iterator}{core::Iterator<self::Foo>};
+    for (; :sync-for-iterator.{core::Iterator::moveNext}(){() → core::bool}; ) {
+      final self::Foo element = :sync-for-iterator.{core::Iterator::current}{self::Foo};
+      {
+        core::print(element);
+        _in::reachabilityFence(element);
+      }
+    }
+  }
+}

pkg/vm/testcases/transformations/ffi/regress_51538_2.dart

@@ -0,0 +1,17 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+class Foo implements Finalizable {}
+
+Future<Foo> bar() => Future.value(Foo());
+
+void main() async {
+  await for (final element in Stream<Foo>.fromIterable([await bar()])) {
+    print(element);
+  }
+}

pkg/vm/testcases/transformations/ffi/regress_51538_2.dart.aot.expect

@@ -0,0 +1,33 @@
+library #lib /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+import "dart:ffi" as ffi;
+import "dart:async" as asy;
+import "dart:_internal" as _in;
+
+import "dart:ffi";
+
+class Foo extends core::Object implements ffi::Finalizable {
+  synthetic constructor •() → self::Foo
+    : super core::Object::•()
+    ;
+}
+static method bar() → asy::Future<self::Foo>
+  return [@vm.inferred-type.metadata=dart.async::_Future<#lib::Foo>] asy::Future::value<self::Foo>(new self::Foo::•());
+static method main() → void async /* futureValueType= void */ {
+  {
+    asy::Stream<self::Foo> :stream = [@vm.inferred-type.metadata=dart.async::_MultiStream<#lib::Foo>] asy::Stream::fromIterable<self::Foo>([@vm.inferred-type.metadata=dart.core::_GrowableList<#lib::Foo>] core::_GrowableList::_literal1<self::Foo>(await self::bar()));
+    asy::_StreamIterator<self::Foo>? :for-iterator = new asy::_StreamIterator::•<self::Foo>(:stream);
+    try
+      while (let dynamic #t1 = asy::_asyncStarMoveNextHelper(:stream) in await [@vm.direct-call.metadata=dart.async::_StreamIterator.moveNext] [@vm.inferred-type.metadata=!? (skip check)] :for-iterator.{asy::_StreamIterator::moveNext}(){() → asy::Future<core::bool>}) {
+        final self::Foo element = [@vm.direct-call.metadata=dart.async::_StreamIterator.current] [@vm.inferred-type.metadata=#lib::Foo] :for-iterator.{asy::_StreamIterator::current}{self::Foo};
+        {
+          core::print(element);
+          _in::reachabilityFence(element);
+        }
+      }
+    finally
+      if(!([@vm.direct-call.metadata=dart.async::_StreamIterator._subscription] :for-iterator.{asy::_StreamIterator::_subscription}{asy::StreamSubscription<self::Foo>?} == null))
+        await [@vm.direct-call.metadata=dart.async::_StreamIterator.cancel] [@vm.inferred-type.metadata=!? (skip check)] :for-iterator.{asy::_StreamIterator::cancel}(){() → asy::Future<dynamic>};
+  }
+}

pkg/vm/testcases/transformations/ffi/regress_51538_2.dart.expect

@@ -0,0 +1,33 @@
+library #lib /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+import "dart:ffi" as ffi;
+import "dart:async" as asy;
+import "dart:_internal" as _in;
+
+import "dart:ffi";
+
+class Foo extends core::Object implements ffi::Finalizable {
+  synthetic constructor •() → self::Foo
+    : super core::Object::•()
+    ;
+}
+static method bar() → asy::Future<self::Foo>
+  return asy::Future::value<self::Foo>(new self::Foo::•());
+static method main() → void async /* futureValueType= void */ {
+  {
+    asy::Stream<self::Foo> :stream = asy::Stream::fromIterable<self::Foo>(core::_GrowableList::_literal1<self::Foo>(await self::bar()));
+    asy::_StreamIterator<self::Foo>? :for-iterator = new asy::_StreamIterator::•<self::Foo>(:stream);
+    try
+      while (let dynamic #t1 = asy::_asyncStarMoveNextHelper(:stream) in await :for-iterator.{asy::_StreamIterator::moveNext}(){() → asy::Future<core::bool>}) {
+        final self::Foo element = :for-iterator.{asy::_StreamIterator::current}{self::Foo};
+        {
+          core::print(element);
+          _in::reachabilityFence(element);
+        }
+      }
+    finally
+      if(!(:for-iterator.{asy::_StreamIterator::_subscription}{asy::StreamSubscription<self::Foo>?} == null))
+        await :for-iterator.{asy::_StreamIterator::cancel}(){() → asy::Future<dynamic>};
+  }
+}

pkg/vm/testcases/transformations/ffi/regress_51538_3.dart

@@ -0,0 +1,19 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+class Foo implements Finalizable {}
+
+Future<bool> hasMore() async => false;
+
+Future<Foo> nextElement() => Future.value(Foo());
+
+void main() async {
+  for (var element = Foo(); await hasMore(); element = await nextElement()) {
+    print(element);
+  }
+}

pkg/vm/testcases/transformations/ffi/regress_51538_3.dart.aot.expect

@@ -0,0 +1,33 @@
+library #lib /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+import "dart:ffi" as ffi;
+import "dart:async" as asy;
+import "dart:_internal" as _in;
+
+import "dart:ffi";
+
+class Foo extends core::Object implements ffi::Finalizable {
+  synthetic constructor •() → self::Foo
+    : super core::Object::•()
+    ;
+}
+static method hasMore() → asy::Future<core::bool> async /* futureValueType= core::bool */ 
+  return false;
+static method nextElement() → asy::Future<self::Foo>
+  return [@vm.inferred-type.metadata=dart.async::_Future<#lib::Foo>] asy::Future::value<self::Foo>(new self::Foo::•());
+static method main() → void async /* futureValueType= void */ {
+  for (self::Foo element = new self::Foo::•(); await block {
+    final asy::Future<core::bool> :expressionValueWrappedFinalizable = self::hasMore();
+    _in::reachabilityFence(element);
+  } =>:expressionValueWrappedFinalizable; element = block {
+    final self::Foo :expressionValueWrappedFinalizable = await block {
+      final asy::Future<self::Foo> :expressionValueWrappedFinalizable = self::nextElement();
+      _in::reachabilityFence(element);
+    } =>:expressionValueWrappedFinalizable;
+    _in::reachabilityFence(element);
+  } =>:expressionValueWrappedFinalizable) {
+    core::print(element);
+    _in::reachabilityFence(element);
+  }
+}

pkg/vm/testcases/transformations/ffi/regress_51538_3.dart.expect

@@ -0,0 +1,33 @@
+library #lib /*isNonNullableByDefault*/;
+import self as self;
+import "dart:core" as core;
+import "dart:ffi" as ffi;
+import "dart:async" as asy;
+import "dart:_internal" as _in;
+
+import "dart:ffi";
+
+class Foo extends core::Object implements ffi::Finalizable {
+  synthetic constructor •() → self::Foo
+    : super core::Object::•()
+    ;
+}
+static method hasMore() → asy::Future<core::bool> async /* futureValueType= core::bool */ 
+  return false;
+static method nextElement() → asy::Future<self::Foo>
+  return asy::Future::value<self::Foo>(new self::Foo::•());
+static method main() → void async /* futureValueType= void */ {
+  for (self::Foo element = new self::Foo::•(); await block {
+    final asy::Future<core::bool> :expressionValueWrappedFinalizable = self::hasMore();
+    _in::reachabilityFence(element);
+  } =>:expressionValueWrappedFinalizable; element = block {
+    final self::Foo :expressionValueWrappedFinalizable = await block {
+      final asy::Future<self::Foo> :expressionValueWrappedFinalizable = self::nextElement();
+      _in::reachabilityFence(element);
+    } =>:expressionValueWrappedFinalizable;
+    _in::reachabilityFence(element);
+  } =>:expressionValueWrappedFinalizable) {
+    core::print(element);
+    _in::reachabilityFence(element);
+  }
+}

tests/ffi/regress_51538_2_test.dart

@@ -0,0 +1,17 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+class Foo implements Finalizable {}
+
+Future<Foo> bar() => Future.value(Foo());
+
+void main() async {
+  await for (final element in Stream<Foo>.fromIterable([await bar()])) {
+    print(element);
+  }
+}

tests/ffi/regress_51538_3_test.dart

@@ -0,0 +1,19 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+class Foo implements Finalizable {}
+
+Future<bool> hasMore() async => false;
+
+Future<Foo> nextElement() => Future.value(Foo());
+
+void main() async {
+  for (var element = Foo(); await hasMore(); element = await nextElement()) {
+    print(element);
+  }
+}

tests/ffi/regress_51538_test.dart

@@ -0,0 +1,17 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+class Foo implements Finalizable {}
+
+Future<Foo> bar() => Future.value(Foo());
+
+void main() async {
+  for (final element in [await bar()]) {
+    print(element);
+  }
+}