Implement subtype checks on type arguments.

Implement subtype checks on type arguments.

BUG=dartbug.com/5096

Committed: https://code.google.com/p/dart/source/detail?r=16116

[ngeoffray, 2012-12-12 12:09:11]

Initial comments.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:66:
RuntimeTypeInformation get rti {
I'd rather not have this getter.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:764: // that
are needed but do  not have a JS constructor.
do  not -> do not

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:764: // that
are needed but do  not have a JS constructor.
What are those? Please add a comment.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:7:
abstract class TypeChecks implements Iterable<ClassElement> {
Please add a high-level comment on what this is, and what this is used for.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:17:
final Set<ClassElement> requiredClasses;
I would rename this field to make it more obvious what it is:
requiredNonInstantiatedClasses?

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:33: void
referencedClass(ClassElement cls) {
I would change this method to return whether the class needs to be added to
requiredClasses, and let the caller add it to the set.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:40: //
list of classes for which we need to generate a holder for the
What will the holder be? How can it be referenced in JS code?

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3590:
compiler.codegenWorld.instantiatedTypes.add(type);
You should not register in builder.dart, bit in codegen.dart instead.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:56:
instantiatedTypes = new Set<DartType>(),
Should we remove instantiatedClasses?

[kasperl, 2012-12-12 12:09:45]

Comments:

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:9:
Set<ClassElement> operator[](ClassElement element);
Does this have to be a set? Would an iterable be enough?

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:20: :
requiredClasses = new Set<ClassElement>();
Move to declaration of requiredClasses.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:33: void
referencedClass(ClassElement cls) {
Maybe addReferencedClass? markClassAsReferenced? referenceClass?

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:35: if
(!universe.instantiatedClasses.contains(cls) ||
It's not completely obvious from this code that the set of instantiated classes
cannot change after we've made the decision to add a class to the
requiredClasses set. Add a comment that explains why this works?

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:91: if
(f != null) f(type);
Why would you call this with f == null? Nothing would happen right? No callbacks
-- and no side-effects.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:169:
Map<ClassElement, Set<ClassElement>> map;
final?

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:171:
TypeCheckMapping() : map = new Map<ClassElement, Set<ClassElement>>();
Initialize at declaration.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:175:
return result != null ? result : new Set<ClassElement>();
Seems a bit expensive to return a new empty set every time. If it was possible
to let TypeChecks return Iterable<ClassElement> it would be easy to return const
<ClassElement>[].

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1684: int len =
s.length;
Are s and t guaranteed to have the same length? Add a comment.

[karlklose, 2012-12-12 14:50:47]

I changed how we identify classes we need to generate a holder for: it is now
done in the emitter based on the result of the neededClasses calculation.

PTAL.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:66:
RuntimeTypeInformation get rti {
On 2012/12/12 12:09:11, ngeoffray wrote:
> I'd rather not have this getter.

Done.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:764: // that
are needed but do  not have a JS constructor.
On 2012/12/12 12:09:11, ngeoffray wrote:
> do  not -> do not

Done.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:764: // that
are needed but do  not have a JS constructor.
On 2012/12/12 12:09:11, ngeoffray wrote:
> What are those? Please add a comment.

Done.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:7:
abstract class TypeChecks implements Iterable<ClassElement> {
On 2012/12/12 12:09:11, ngeoffray wrote:
> Please add a high-level comment on what this is, and what this is used for.

Done.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:9:
Set<ClassElement> operator[](ClassElement element);
On 2012/12/12 12:09:45, kasperl wrote:
> Does this have to be a set? Would an iterable be enough?

Done.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:17:
final Set<ClassElement> requiredClasses;
I have removed this set.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:20: :
requiredClasses = new Set<ClassElement>();
I have removed this set.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:33: void
referencedClass(ClassElement cls) {
I have removed this method.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:35: if
(!universe.instantiatedClasses.contains(cls) ||
I changed the CL to actually check against the neededTypes in the emitter.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:40: //
list of classes for which we need to generate a holder for the
I have removed this method. The holder is described in the emitter.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:91: if
(f != null) f(type);
Removed.

(This function originally took two callbacks - one for interface types and one
for type variables - and both of them were allowed to be null.)

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:169:
Map<ClassElement, Set<ClassElement>> map;
On 2012/12/12 12:09:45, kasperl wrote:
> final?

Done.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:171:
TypeCheckMapping() : map = new Map<ClassElement, Set<ClassElement>>();
On 2012/12/12 12:09:45, kasperl wrote:
> Initialize at declaration.

Done.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:175:
return result != null ? result : new Set<ClassElement>();
On 2012/12/12 12:09:45, kasperl wrote:
> Seems a bit expensive to return a new empty set every time. If it was possible
> to let TypeChecks return Iterable<ClassElement> it would be easy to return
const
> <ClassElement>[].

Done.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1684: int len =
s.length;
No, they are not. Added a test.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3590:
compiler.codegenWorld.instantiatedTypes.add(type);
I cannot do this now, we need a way to encode the type in the instruction. Added
a TODO.

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11557010/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:56:
instantiatedTypes = new Set<DartType>(),
I am not sure. I'll add a comment.

[kasperl, 2012-12-13 09:48:55]

LGTM but you should probably wait for Nicolas' comments.

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:774:
buffer.add("$holder = {builtin\$cls: '$name'");
You should be able to use Erik's new whitespace saving helpers for minification.
Look for $n, $N, $_ and see if you can use them for all places where you emit
code.

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1201: /// Get
or Compute the classes that we need to emit.
Compute -> compute

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:45:
..addAll(checkedArguments);
I prefer 4 space indent of the .. lines.

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1678: var typeOfS =
isJsArray(s) ? s[0] : s;
Maybe add a few comments to this method. It isn't exactly easy to grok.

[karlklose, 2012-12-13 12:55:46]

Thanks for the review!

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:774:
buffer.add("$holder = {builtin\$cls: '$name'");
On 2012/12/13 09:48:55, kasperl wrote:
> You should be able to use Erik's new whitespace saving helpers for
minification.
> Look for $n, $N, $_ and see if you can use them for all places where you emit
> code.

Done.

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1201: /// Get
or Compute the classes that we need to emit.
On 2012/12/13 09:48:55, kasperl wrote:
> Compute -> compute

Done.

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:45:
..addAll(checkedArguments);
On 2012/12/13 09:48:55, kasperl wrote:
> I prefer 4 space indent of the .. lines.

Done.

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11557010/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1678: var typeOfS =
isJsArray(s) ? s[0] : s;
On 2012/12/13 09:48:55, kasperl wrote:
> Maybe add a few comments to this method. It isn't exactly easy to grok.

Done.

[ngeoffray, 2012-12-13 13:34:58]

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:778:
buffer.add("$holder$_=$_{builtin\$cls:$_'$name'");
I think you can share the following code with the one line 800 (that adds the
is$ properties to the holder), since holder is also a JS object.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:780:
buffer.add(',${_}is\$${namer.getName(check)}:${_}true');
Please use namer.operatorIs.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:785: // Create
representation objects for classes that we do not need a class
do not need -> do not have

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:800:
buffer.add('$holder.is\$${namer.getName(check)} = true;\n');
namer.operatorIs.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1225:
Set<ClassElement> get instantiatedClasses {
Do you need this? If it's only used by the neededClasses getter, I would inline
it.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1233:
Set<ClassElement> get neededClasses {
Instead of doing this lazily I would definitely prefer that the emitter just
computes it when it starts. The set will always be computed anyways.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:38: final
Set<DartType> instantiatedTypes;
TODO: merge these sets?

[karlklose, 2012-12-13 14:37:34]

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:778:
buffer.add("$holder$_=$_{builtin\$cls:$_'$name'");
I want to generate the checks for the synthetic holder inline. I could create a
shared helper, but I find the result pretty unreadable.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:780:
buffer.add(',${_}is\$${namer.getName(check)}:${_}true');
On 2012/12/13 13:34:58, ngeoffray wrote:
> Please use namer.operatorIs.

Done.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:785: // Create
representation objects for classes that we do not need a class
On 2012/12/13 13:34:58, ngeoffray wrote:
> do not need -> do not have

Done.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:800:
buffer.add('$holder.is\$${namer.getName(check)} = true;\n');
On 2012/12/13 13:34:58, ngeoffray wrote:
> namer.operatorIs.

Done.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1225:
Set<ClassElement> get instantiatedClasses {
It is also needed in emitClasses.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1233:
Set<ClassElement> get neededClasses {
On 2012/12/13 13:34:58, ngeoffray wrote:
> Instead of doing this lazily I would definitely prefer that the emitter just
> computes it when it starts. The set will always be computed anyways.

Done.

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/11557010/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:38: final
Set<DartType> instantiatedTypes;
On 2012/12/13 13:34:58, ngeoffray wrote:
> TODO: merge these sets?

Done.

[ngeoffray, 2012-12-13 14:52:12]

LGTM!

https://codereview.chromium.org/11557010/diff/12002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11557010/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:48:
Set<ClassElement> instantiatedClasses;
Please add a TODO(ngeoffray): to remove this field.

[karlklose, 2012-12-13 14:56:45]

Thanks, Nicolas!

https://codereview.chromium.org/11557010/diff/12002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11557010/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:48:
Set<ClassElement> instantiatedClasses;
On 2012/12/13 14:52:12, ngeoffray wrote:
> Please add a TODO(ngeoffray): to remove this field.

Done.