Skip to content

Commit

Permalink
[dart2js] Pass type arguments to callable properties even with
Browse files Browse the repository at this point in the history
--omit-implicit-checks.

When the default check policy is "trusted" instead of "emitted", we skip
checking every function during the RTI need computation. This works
because if the type arguments are needed for some other reason, like the
literal being used in the body, we see the type use and go back and
update the RTI need of the function. We can't do this for callable
properties because we don't know which function will be assigned at
runtime, so we need to conservatively provide type arguments no matter
what.

We could optimize this slightly by only providing type arguments if we
know a type use occurs in a function which is assignable to the type of
the property, but I expect this to save little for the amount of
overhead during compilation.

We also update the dependency computation to ensure that if a callable
property needs type arguments, then they're available for the enclosing
context to pass along.

Bug: #41449
Fixes: #41449
Change-Id: I98d9024dfa64cbfe33bd43172ffa905a8537649e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/154284
Reviewed-by: Stephen Adams <sra@google.com>
Commit-Queue: Mayank Patke <fishythefish@google.com>
  • Loading branch information
fishythefish authored and commit-bot@chromium.org committed Jul 24, 2020
1 parent 4c74ebb commit 5671730
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 4 deletions.
17 changes: 13 additions & 4 deletions pkg/compiler/lib/src/js_backend/runtime_types_resolution.dart
Expand Up @@ -167,11 +167,15 @@ class MethodNode extends CallableNode {
}
}

bool _isProperty(Entity entity) =>
entity is MemberEntity && (entity.isField || entity.isGetter);

class CallablePropertyNode extends CallableNode {
final MemberEntity property;
final DartType type;

CallablePropertyNode(this.property, this.type);
CallablePropertyNode(this.property, this.type)
: assert(_isProperty(property));

@override
Entity get entity => property;
Expand All @@ -181,8 +185,6 @@ class CallablePropertyNode extends CallableNode {

@override
bool selectorApplies(Selector selector, BuiltWorld world) {
if (world.annotationsData.getParameterCheckPolicy(property).isTrusted)
return false;
if (property.memberName != selector.memberName) return false;
if (type is FunctionType &&
!selector.callStructure
Expand Down Expand Up @@ -305,6 +307,8 @@ class TypeVariableTests {
Iterable<RtiNode> dependencies;
if (entity is ClassEntity) {
dependencies = _classes[entity]?.dependencies;
} else if (_isProperty(entity)) {
dependencies = _callableProperties[entity]?.dependencies;
} else {
dependencies = _methods[entity]?.dependencies;
}
Expand Down Expand Up @@ -1031,6 +1035,8 @@ class RuntimeTypesNeedBuilderImpl implements RuntimeTypesNeedBuilder {
});
} else if (entity is FunctionEntity) {
methodsNeedingTypeArguments.add(entity);
} else if (_isProperty(entity)) {
// Do nothing. We just need to visit the dependencies.
} else {
localFunctionsNeedingTypeArguments.add(entity);
}
Expand Down Expand Up @@ -1139,6 +1145,9 @@ class RuntimeTypesNeedBuilderImpl implements RuntimeTypesNeedBuilder {
localFunctionsUsingTypeVariableLiterals
.forEach(potentiallyNeedTypeArguments);

typeVariableTests._callableProperties.keys
.forEach(potentiallyNeedTypeArguments);

if (closedWorld.isMemberUsed(
closedWorld.commonElements.invocationTypeArgumentGetter)) {
// If `Invocation.typeArguments` is live, mark all user-defined
Expand Down Expand Up @@ -1317,7 +1326,7 @@ class RuntimeTypesNeedBuilderImpl implements RuntimeTypesNeedBuilder {
typeVariableTests
.forEachAppliedSelector((Selector selector, Set<Entity> targets) {
for (Entity target in targets) {
if (target is MemberEntity && (target.isField || target.isGetter) ||
if (_isProperty(target) ||
methodsNeedingTypeArguments.contains(target) ||
localFunctionsNeedingTypeArguments.contains(target)) {
selectorsNeedingTypeArguments.add(selector);
Expand Down
57 changes: 57 additions & 0 deletions tests/dart2js/41449c_test.dart
@@ -0,0 +1,57 @@
// Copyright (c) 2020, 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.
//
// dart2jsOptions=-O4

// Regression test for passing type parameters through call-through stub.
//
// We use an abstract class with two implementations to avoid the optimizer
// 'inlining' the call-through stub, so we are testing that the stub itself
// passes through the type parameters.

import 'package:expect/expect.dart';

abstract class AAA {
dynamic get foo;
}

class B1 implements AAA {
final dynamic foo;
B1(this.foo);
}

class B2 implements AAA {
final dynamic _arr;
B2(foo) : _arr = [foo];
dynamic get foo => _arr.first;
}

class B3 implements AAA {
final dynamic __foo;
B3(this.__foo);
dynamic get _foo => __foo;
dynamic get foo => _foo;
}

@pragma('dart2js:noInline')
test1<T>(AAA a, String expected) {
// call-through getter 'foo' with one type argument.
Expect.equals(expected, a.foo<T>());
}

@pragma('dart2js:noInline')
test2<U, V>(AAA a, String expected) {
// call-through getter 'foo' with two type arguments.
Expect.equals(expected, a.foo<U, V>());
}

main() {
test1<int>(B1(<P>() => '$P'), 'int');
test1<num>(B2(<Q>() => '$Q'), 'num');
test1<double>(B3(<R>() => '$R'), 'double');

test2<int, num>(B1(<A, B>() => '$A $B'), 'int num');
test2<num, int>(B2(<X, Y>() => '$X $Y'), 'num int');
test2<double, String>(B3(<C, D>() => '$C $D'), 'double String');
}
59 changes: 59 additions & 0 deletions tests/dart2js_2/41449c_test.dart
@@ -0,0 +1,59 @@
// Copyright (c) 2020, 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.
//
// dart2jsOptions=-O4

// @dart = 2.7

// Regression test for passing type parameters through call-through stub.
//
// We use an abstract class with two implementations to avoid the optimizer
// 'inlining' the call-through stub, so we are testing that the stub itself
// passes through the type parameters.

import 'package:expect/expect.dart';

abstract class AAA {
dynamic get foo;
}

class B1 implements AAA {
final dynamic foo;
B1(this.foo);
}

class B2 implements AAA {
final dynamic _arr;
B2(foo) : _arr = [foo];
dynamic get foo => _arr.first;
}

class B3 implements AAA {
final dynamic __foo;
B3(this.__foo);
dynamic get _foo => __foo;
dynamic get foo => _foo;
}

@pragma('dart2js:noInline')
test1<T>(AAA a, String expected) {
// call-through getter 'foo' with one type argument.
Expect.equals(expected, a.foo<T>());
}

@pragma('dart2js:noInline')
test2<U, V>(AAA a, String expected) {
// call-through getter 'foo' with two type arguments.
Expect.equals(expected, a.foo<U, V>());
}

main() {
test1<int>(B1(<P>() => '$P'), 'int');
test1<num>(B2(<Q>() => '$Q'), 'num');
test1<double>(B3(<R>() => '$R'), 'double');

test2<int, num>(B1(<A, B>() => '$A $B'), 'int num');
test2<num, int>(B2(<X, Y>() => '$X $Y'), 'num int');
test2<double, String>(B3(<C, D>() => '$C $D'), 'double String');
}

0 comments on commit 5671730

Please sign in to comment.