Represent runtime type information as nested lists.

Represent runtime type information as nested lists.

The class of a type is represented either by its Javascript constructor or, if it is a native JavaScript type, by a string. A type without type arguments uses its class' representation. A types with type arguments is represented by a list, where the first element is the representation of the class and the rest are the representation of the arguments.

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

[kasperl, 2012-12-06 09:51:04]

LGTM.

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

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:32:
String getJsName(Element element) {
I wonder if this should return a JS AST node instead? At least long term that
would be nicer it seems. Maybe then you wouldn't have to invoke the namer
explicitly either?

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:43: if
(interface.element.typeVariables.isEmpty) return name;
Maybe cache interface.element.typeVariables in a local variable?

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:45:
interface.element.typeVariables.forEach((_) => arguments.add('dynamic'));
If we had an repeated add, it would be clearer to do something like:

   arguments.addRepeated('dynamic', count: typeVariables.length);

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:51: void
build(DartType t) {
I'd prefer a longer name than t -- but I guess type is already taken ;-)

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

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1678: //
TODO(karlklose): support suptyping.
suptyping -> subtyping. Maybe extend the comment to explain what's missing?

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

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2642:
List<HInstruction> buildRepresentations(DartType type) {
Maybe give this function a more telling name? Also consider if we could be
constructing the representations using instructions that are GVN'able. Whenever
you use foreign code we don't know anything about what it does (side-effects,
etc.) so using something simpler might be better. Calling "unknown" static
functions has the same problem.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2731: //
TODO(karlklose): remove this check when the backend can deal with it.
Expand the comment to explain what you're waiting for. It's not clear to me what
situation the backend doesn't handle nicely.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2739:
pushInvokeHelper1(interceptors.getGetRuntimeTypeInfo(), expression);
Could this code be put in a separate helper function? I think that would help
the readability and you wouldn't need to have an uninitialized instruction
variable.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2741: Element helper
= compiler.findHelper(const SourceString('isSubtype'));
Would it make sense to have a separate HIsSubtype node that we could potentially
GVN, hoist and share?

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2747:
representations.forEach((HInstruction representation) {
Add a comment that explains the code you're building here.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2749: HInstruction
typeArgument =
I wonder if we should be creating something that we could GVN? Nicolas has been
toying with simplifying HIndex so maybe we should be able to use that in more
places?

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3170:
createForeignString(template, inputs) {
Use createForeign?

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2542: int get checks =>
inputs.length - 1;
checkCount?

[karlklose, 2012-12-06 13:30:03]

Thanks, Kasper!

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

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:32:
String getJsName(Element element) {
The problem is that getRawTypeRepresentation uses the result to create another
string.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:43: if
(interface.element.typeVariables.isEmpty) return name;
On 2012/12/06 09:51:04, kasperl wrote:
> Maybe cache interface.element.typeVariables in a local variable?

Done.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:45:
interface.element.typeVariables.forEach((_) => arguments.add('dynamic'));
I agree. Currently, however, getting the length of  a Link already requires
traversing it.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:51: void
build(DartType t) {
I renamed it to part.

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

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1678: //
TODO(karlklose): support suptyping.
On 2012/12/06 09:51:04, kasperl wrote:
> suptyping -> subtyping. Maybe extend the comment to explain what's missing?

Done.

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

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2642:
List<HInstruction> buildRepresentations(DartType type) {
Renamed and added a comment for making the constructed representations GVN'able.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2731: //
TODO(karlklose): remove this check when the backend can deal with it.
On 2012/12/06 09:51:04, kasperl wrote:
> Expand the comment to explain what you're waiting for. It's not clear to me
what
> situation the backend doesn't handle nicely.

Done.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2739:
pushInvokeHelper1(interceptors.getGetRuntimeTypeInfo(), expression);
On 2012/12/06 09:51:04, kasperl wrote:
> Could this code be put in a separate helper function? I think that would help
> the readability and you wouldn't need to have an uninitialized instruction
> variable.

Done.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2741: Element helper
= compiler.findHelper(const SourceString('isSubtype'));
I am not sure how much work it is to do it, but I added a TODO and will
investigate making the builder output smaller.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2747:
representations.forEach((HInstruction representation) {
On 2012/12/06 09:51:04, kasperl wrote:
> Add a comment that explains the code you're building here.

Done.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2749: HInstruction
typeArgument =
As said above, I'll look into better code generation in a later CL.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3170:
createForeignString(template, inputs) {
On 2012/12/06 09:51:04, kasperl wrote:
> Use createForeign?

Done.

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/11448009/diff/1011/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2542: int get checks =>
inputs.length - 1;
On 2012/12/06 09:51:04, kasperl wrote:
> checkCount?

Done.

[ngeoffray, 2012-12-06 23:21:45]

LGTM

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

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:23: ///
Return the unique name for the element as an unqoted string.
unqoted -> unquoted

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:26:
Namer namer = backend.namer;
Why this local variable 'namer'?

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:65:
ClassElement cls = element;
Since the method is called getSomething, I would prefer not having this
resolving and registering in it. Could you move that in the callers?

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

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1636: if
(runtimeType.length = 1) return className;
== 1

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1681: // a function
is$s[0] on t[0] and call it with substitutes type arguments.
Please don't use JS in this method. The compiler knows or will know it's an
array.

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

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2636:
createForeign(String code, String type, List<HInstruction> inputs) {
HForeign createForeign

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2647: //
TODO(karlklose): change construction of the representations to be GVN'able.
You can also reference the bug number.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2650: return
createForeign(code, 'JSArray', inputs);
Should be '=List' but i plan on fixing that.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2743:
type.element.isTypeVariable()) {
I don't think this can happen (see line 2734 and 2738).

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2746: //
optimizations?
No, I think we need to put compiler flags on the method.

[karlklose, 2012-12-07 07:46:34]

Thanks Nicolas!

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

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:23: ///
Return the unique name for the element as an unqoted string.
On 2012/12/06 23:21:45, ngeoffray wrote:
> unqoted -> unquoted

Done.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:26:
Namer namer = backend.namer;
Done, removed.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:65:
ClassElement cls = element;
I changed it to use a string representation (like for the natives) for
uninstantiated classes for now.

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

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1636: if
(runtimeType.length = 1) return className;
Argh, this is bad. Good catch.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1681: // a function
is$s[0] on t[0] and call it with substitutes type arguments.
On 2012/12/06 23:21:45, ngeoffray wrote:
> Please don't use JS in this method. The compiler knows or will know it's an
> array.

Done.

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

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2636:
createForeign(String code, String type, List<HInstruction> inputs) {
On 2012/12/06 23:21:45, ngeoffray wrote:
> HForeign createForeign

Done.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2647: //
TODO(karlklose): change construction of the representations to be GVN'able.
On 2012/12/06 23:21:45, ngeoffray wrote:
> You can also reference the bug number.

Done.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2650: return
createForeign(code, 'JSArray', inputs);
I changed it to '=List'.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2743:
type.element.isTypeVariable()) {
You are right, this is a left-over from trying to handle both in this CL.
Removed.

https://codereview.chromium.org/11448009/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2746: //
optimizations?
I will remove the comment. Let's chat about dartbug.com/7182 next week.