From 8218ee084081eded31d35aa79ae819d6656cffd4 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Thu, 2 Mar 2023 09:05:19 +0000 Subject: [PATCH] [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 Commit-Queue: Daco Harkes Reviewed-by: Slava Egorov --- .../lib/transformations/ffi/finalizable.dart | 53 +++++++++++++++++-- .../transformations/ffi/regress_51538.dart | 17 ++++++ .../ffi/regress_51538.dart.aot.expect | 28 ++++++++++ .../ffi/regress_51538.dart.expect | 28 ++++++++++ .../transformations/ffi/regress_51538_2.dart | 17 ++++++ .../ffi/regress_51538_2.dart.aot.expect | 33 ++++++++++++ .../ffi/regress_51538_2.dart.expect | 33 ++++++++++++ .../transformations/ffi/regress_51538_3.dart | 19 +++++++ .../ffi/regress_51538_3.dart.aot.expect | 33 ++++++++++++ .../ffi/regress_51538_3.dart.expect | 33 ++++++++++++ tests/ffi/regress_51538_2_test.dart | 17 ++++++ tests/ffi/regress_51538_3_test.dart | 19 +++++++ tests/ffi/regress_51538_test.dart | 17 ++++++ 13 files changed, 342 insertions(+), 5 deletions(-) create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538.dart create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538.dart.aot.expect create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538.dart.expect create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538_2.dart create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538_2.dart.aot.expect create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538_2.dart.expect create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538_3.dart create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538_3.dart.aot.expect create mode 100644 pkg/vm/testcases/transformations/ffi/regress_51538_3.dart.expect create mode 100644 tests/ffi/regress_51538_2_test.dart create mode 100644 tests/ffi/regress_51538_3_test.dart create mode 100644 tests/ffi/regress_51538_test.dart diff --git a/pkg/vm/lib/transformations/ffi/finalizable.dart b/pkg/vm/lib/transformations/ffi/finalizable.dart index 3051332d5d1..cb904378117 100644 --- a/pkg/vm/lib/transformations/ffi/finalizable.dart +++ b/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; diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538.dart b/pkg/vm/testcases/transformations/ffi/regress_51538.dart new file mode 100644 index 00000000000..28eb5149a8b --- /dev/null +++ b/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 bar() => Future.value(Foo()); + +void main() async { + for (final element in [await bar()]) { + print(element); + } +} diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538.dart.aot.expect b/pkg/vm/testcases/transformations/ffi/regress_51538.dart.aot.expect new file mode 100644 index 00000000000..634b390fb8b --- /dev/null +++ b/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 + return [@vm.inferred-type.metadata=dart.async::_Future<#lib::Foo>] asy::Future::value(new self::Foo::•()); +static method main() → void async /* futureValueType= void */ { + { + core::Iterator :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(await self::bar()).{core::Iterable::iterator}{core::Iterator}; + 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); + } + } + } +} diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538.dart.expect b/pkg/vm/testcases/transformations/ffi/regress_51538.dart.expect new file mode 100644 index 00000000000..73f8b659795 --- /dev/null +++ b/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 + return asy::Future::value(new self::Foo::•()); +static method main() → void async /* futureValueType= void */ { + { + core::Iterator :sync-for-iterator = core::_GrowableList::_literal1(await self::bar()).{core::Iterable::iterator}{core::Iterator}; + 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); + } + } + } +} diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538_2.dart b/pkg/vm/testcases/transformations/ffi/regress_51538_2.dart new file mode 100644 index 00000000000..8201406a0fe --- /dev/null +++ b/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 bar() => Future.value(Foo()); + +void main() async { + await for (final element in Stream.fromIterable([await bar()])) { + print(element); + } +} diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538_2.dart.aot.expect b/pkg/vm/testcases/transformations/ffi/regress_51538_2.dart.aot.expect new file mode 100644 index 00000000000..0adc18c8ef3 --- /dev/null +++ b/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 + return [@vm.inferred-type.metadata=dart.async::_Future<#lib::Foo>] asy::Future::value(new self::Foo::•()); +static method main() → void async /* futureValueType= void */ { + { + asy::Stream :stream = [@vm.inferred-type.metadata=dart.async::_MultiStream<#lib::Foo>] asy::Stream::fromIterable([@vm.inferred-type.metadata=dart.core::_GrowableList<#lib::Foo>] core::_GrowableList::_literal1(await self::bar())); + asy::_StreamIterator? :for-iterator = new asy::_StreamIterator::•(: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}) { + 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?} == null)) + await [@vm.direct-call.metadata=dart.async::_StreamIterator.cancel] [@vm.inferred-type.metadata=!? (skip check)] :for-iterator.{asy::_StreamIterator::cancel}(){() → asy::Future}; + } +} diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538_2.dart.expect b/pkg/vm/testcases/transformations/ffi/regress_51538_2.dart.expect new file mode 100644 index 00000000000..db8611eac35 --- /dev/null +++ b/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 + return asy::Future::value(new self::Foo::•()); +static method main() → void async /* futureValueType= void */ { + { + asy::Stream :stream = asy::Stream::fromIterable(core::_GrowableList::_literal1(await self::bar())); + asy::_StreamIterator? :for-iterator = new asy::_StreamIterator::•(:stream); + try + while (let dynamic #t1 = asy::_asyncStarMoveNextHelper(:stream) in await :for-iterator.{asy::_StreamIterator::moveNext}(){() → asy::Future}) { + 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?} == null)) + await :for-iterator.{asy::_StreamIterator::cancel}(){() → asy::Future}; + } +} diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538_3.dart b/pkg/vm/testcases/transformations/ffi/regress_51538_3.dart new file mode 100644 index 00000000000..6bbe4da20e4 --- /dev/null +++ b/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 hasMore() async => false; + +Future nextElement() => Future.value(Foo()); + +void main() async { + for (var element = Foo(); await hasMore(); element = await nextElement()) { + print(element); + } +} diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538_3.dart.aot.expect b/pkg/vm/testcases/transformations/ffi/regress_51538_3.dart.aot.expect new file mode 100644 index 00000000000..2b8c730f036 --- /dev/null +++ b/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 async /* futureValueType= core::bool */ + return false; +static method nextElement() → asy::Future + return [@vm.inferred-type.metadata=dart.async::_Future<#lib::Foo>] asy::Future::value(new self::Foo::•()); +static method main() → void async /* futureValueType= void */ { + for (self::Foo element = new self::Foo::•(); await block { + final asy::Future :expressionValueWrappedFinalizable = self::hasMore(); + _in::reachabilityFence(element); + } =>:expressionValueWrappedFinalizable; element = block { + final self::Foo :expressionValueWrappedFinalizable = await block { + final asy::Future :expressionValueWrappedFinalizable = self::nextElement(); + _in::reachabilityFence(element); + } =>:expressionValueWrappedFinalizable; + _in::reachabilityFence(element); + } =>:expressionValueWrappedFinalizable) { + core::print(element); + _in::reachabilityFence(element); + } +} diff --git a/pkg/vm/testcases/transformations/ffi/regress_51538_3.dart.expect b/pkg/vm/testcases/transformations/ffi/regress_51538_3.dart.expect new file mode 100644 index 00000000000..f9520e3f5b2 --- /dev/null +++ b/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 async /* futureValueType= core::bool */ + return false; +static method nextElement() → asy::Future + return asy::Future::value(new self::Foo::•()); +static method main() → void async /* futureValueType= void */ { + for (self::Foo element = new self::Foo::•(); await block { + final asy::Future :expressionValueWrappedFinalizable = self::hasMore(); + _in::reachabilityFence(element); + } =>:expressionValueWrappedFinalizable; element = block { + final self::Foo :expressionValueWrappedFinalizable = await block { + final asy::Future :expressionValueWrappedFinalizable = self::nextElement(); + _in::reachabilityFence(element); + } =>:expressionValueWrappedFinalizable; + _in::reachabilityFence(element); + } =>:expressionValueWrappedFinalizable) { + core::print(element); + _in::reachabilityFence(element); + } +} diff --git a/tests/ffi/regress_51538_2_test.dart b/tests/ffi/regress_51538_2_test.dart new file mode 100644 index 00000000000..8201406a0fe --- /dev/null +++ b/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 bar() => Future.value(Foo()); + +void main() async { + await for (final element in Stream.fromIterable([await bar()])) { + print(element); + } +} diff --git a/tests/ffi/regress_51538_3_test.dart b/tests/ffi/regress_51538_3_test.dart new file mode 100644 index 00000000000..6bbe4da20e4 --- /dev/null +++ b/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 hasMore() async => false; + +Future nextElement() => Future.value(Foo()); + +void main() async { + for (var element = Foo(); await hasMore(); element = await nextElement()) { + print(element); + } +} diff --git a/tests/ffi/regress_51538_test.dart b/tests/ffi/regress_51538_test.dart new file mode 100644 index 00000000000..28eb5149a8b --- /dev/null +++ b/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 bar() => Future.value(Foo()); + +void main() async { + for (final element in [await bar()]) { + print(element); + } +}