dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

[dart2js] Fix list tracing treating first/last setters as type preserving.

Browse Source 
I suspect that when the list of type preserving selectors was created the first/last setters either didn't exist or were just overlooked. Any dynamic List could have its inferred type changed by these operations.

Due to Dart2js's type representation this bug affects more than just dynamic lists. We track some simple values as part of the type system. So an operation that modifies a value can technically modify the type, as Dart2js represents it, even if the "real" type is preserved.

In the added test we would represent the list literal's type as "List(length: 2, elementType: Bool(true))". Notice the type states the elementType is specifically true, not just bool. Thus the first/last operations are modifying that type. But since we aren't registering this type change, SSA optimizes away the call/index and inlines the element itself.

Interestingly, this primarily manifested for bools due to a check in SSA:
https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/ssa/optimize.dart#L475

In that code we are inlining constant-like expressions for the arguments of static invocations (such as the argument to a Expect.isTrue call). However, a few lines earlier you will see we only inline bool constants:
https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/ssa/optimize.dart#L467

So when the list element type is anything other than a bool, this inlining does not trigger thus avoiding the bug.

Bug: https://github.com/dart-lang/sdk/issues/53944
Change-Id: I4e893903f335fc99b13cf526736c27bb066a4bad
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/334420
Reviewed-by: Stephen Adams <sra@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
This commit is contained in:
Nate Biggs
2023-11-08 03:50:21 +00:00
committed by Commit Queue
parent 4559689665
commit 9eba05482b
3 changed files with 100 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/compiler/lib/src/inferrer/list_tracer.dart
+29 -26
View File
@@ -14,7 +14,8 @@ import 'type_graph_nodes.dart';
/// A set of selector names that [List] implements, that we know do not
/// change the element type of the list, or let the list escape to code
/// that might change the element type.
Set<String> okListSelectorsSet = Set<String>.from(const <String>[
Set<String> elementTypePreservingSelectorNames =
Set<String>.from(const <String>[
// From Object.
'==',
'hashCode',
@@ -43,8 +44,6 @@ Set<String> okListSelectorsSet = Set<String>.from(const <String>[
'takeWhile',
'skip',
'skipWhile',
'first',
'last',
'single',
'firstWhere',
'lastWhere',
@@ -53,7 +52,7 @@ Set<String> okListSelectorsSet = Set<String>.from(const <String>[
// From List.
'[]',
'length',
'length', // set:length is almost safe and handled specially
'reversed',
'sort',
'indexOf',
@@ -74,7 +73,7 @@ Set<String> okListSelectorsSet = Set<String>.from(const <String>[
'checkGrowable',
]);
Set<String> doNotChangeLengthSelectorsSet = Set<String>.from(const <String>[
Set<String> lengthPreservingSelectorNames = Set<String>.from(const <String>[
// From Object.
'==',
'hashCode',
@@ -96,7 +95,6 @@ Set<String> doNotChangeLengthSelectorsSet = Set<String>.from(const <String>[
'any',
'toList',
'toSet',
'length',
'isEmpty',
'isNotEmpty',
'take',
@@ -114,7 +112,6 @@ Set<String> doNotChangeLengthSelectorsSet = Set<String>.from(const <String>[
// From List.
'[]',
'[]=',
'length',
'reversed',
'sort',
'indexOf',
@@ -182,34 +179,40 @@ class ListTracerVisitor extends TracerVisitor {
String selectorName = selector.name;
final arguments = info.arguments;
if (currentUser == info.receiver) {
if (!okListSelectorsSet.contains(selectorName)) {
if (selector.isCall) {
int positionalLength = arguments!.positional.length;
if (selectorName == 'add') {
if (positionalLength == 1) {
if (!elementTypePreservingSelectorNames.contains(selectorName)) {
if (selectorName == 'add' && selector.isCall) {
if (arguments!.positional.length == 1) {
inputs.add(arguments.positional[0]);
}
} else if (selectorName == 'insert' && selector.isCall) {
if (arguments!.positional.length == 2) {
inputs.add(arguments.positional[1]);
}
} else if (selectorName == 'first' || selectorName == 'last') {
if (selector.isSetter) {
if (arguments!.positional.length == 1) {
inputs.add(arguments.positional[0]);
}
} else if (selectorName == 'insert') {
if (positionalLength == 2) {
inputs.add(arguments.positional[1]);
}
} else {
bailout('Used in a not-ok selector');
return;
}
// 'first' and 'last' getter are safe.
} else if (selector.isIndexSet) {
inputs.add(arguments!.positional[1]);
} else if (!selector.isIndex) {
} else if (selector.isIndex) {
// Index lookup is safe.
} else {
bailout('Used in a not-ok selector');
return;
}
}
if (!doNotChangeLengthSelectorsSet.contains(selectorName)) {
callsGrowableMethod = true;
}
if (selectorName == 'length' && selector.isSetter) {
callsGrowableMethod = true;
inputs.add(inferrer.types.nullType);
if (!lengthPreservingSelectorNames.contains(selectorName)) {
if (selectorName == 'length') {
if (selector.isSetter) {
callsGrowableMethod = true;
inputs.add(inferrer.types.nullType);
}
} else {
callsGrowableMethod = true;
}
}
} else if (selector.isCall &&
(info.hasClosureCallTargets || dynamicCallTargetsNonFunction(info))) {
pkg/compiler/test/inference/data/issue_53944.dart
+48
View File
@@ -0,0 +1,48 @@
// 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.
// Make sure first and last selectors properly update types.
/*member: testUnchangedFirst:[null]*/
void testUnchangedFirst(/*Value([exact=JSBool], value: true)*/ x) {}
/*member: testFirst1:[null]*/
void testFirst1(/*[exact=JSBool]*/ x) {}
/*member: testFirst2:[null]*/
void testFirst2(/*[exact=JSBool]*/ x) {}
/*member: testUnchangedLast:[null]*/
void testUnchangedLast(/*Value([exact=JSBool], value: true)*/ x) {}
/*member: testLast1:[null]*/
void testLast1(/*[exact=JSBool]*/ x) {}
/*member: testLast2:[null]*/
void testLast2(/*[exact=JSBool]*/ x) {}
/*member: main:[null]*/
main() {
final List<Object> x = [true, true];
testFirst1(x
. /*Container([exact=JSExtendableArray], element: [exact=JSBool], length: 2)*/ first);
x. /*update: Container([exact=JSExtendableArray], element: [exact=JSBool], length: 2)*/ first =
false;
testFirst2(x
. /*Container([exact=JSExtendableArray], element: [exact=JSBool], length: 2)*/ first);
final List<Object> y = [true, true];
testLast1(y
. /*Container([exact=JSExtendableArray], element: [exact=JSBool], length: 2)*/ first);
y. /*update: Container([exact=JSExtendableArray], element: [exact=JSBool], length: 2)*/ last =
false;
testLast2(y
. /*Container([exact=JSExtendableArray], element: [exact=JSBool], length: 2)*/ first);
final List<Object> z = [true, true];
testUnchangedFirst(z
. /*Container([exact=JSExtendableArray], element: Value([exact=JSBool], value: true), length: 2)*/ first);
testUnchangedLast(z
. /*Container([exact=JSExtendableArray], element: Value([exact=JSBool], value: true), length: 2)*/ last);
}
tests/web/regress/issue/53944_test.dart
+23
View File
@@ -0,0 +1,23 @@
// 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.
// Ensure list tracing recognizes List `first` and `last` setters as
// potentially modifying the type-value of the List.
import 'package:expect/expect.dart';
void main() {
List<bool> a = [true, true];
Expect.isTrue(a.first);
Expect.isTrue(a.last);
a.first = false;
Expect.isFalse(a.first);
Expect.isTrue(a.last);
a.last = false;
Expect.isFalse(a.first);
Expect.isFalse(a.last);
}