- Parse the return type of JS during resolution to know what can be instantiated there.

- Parse the return type of JS during resolution to know what can be instantiated there.
- Don't pass the number interceptor to the generic getInterceptor: double already covers it.
Committed: https://code.google.com/p/dart/source/detail?r=15637

[sra1, 2012-11-29 00:12:50]

https://codereview.chromium.org/11308257/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11308257/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:392: var argNodes =
node.arguments;
We should avoid parsing this string in two places.

This code should be moved into NativeEnqueuerBase.processNativeBehavior.
That runs after the native element has been 'parsed' into a set of instantiated
classes.
That way, things will work if the only String in the program comes from a DOM
call.

It also takes care of the fact that the instantiated types and returned types
are not always the same.

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

https://codereview.chromium.org/11308257/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1512: if
(declaredType.slowToString() == '=List') return HType.READABLE_ARRAY;
I have not figured out how to do it yet, but we should use the typesReturned set
from NativeBehavior.
We would need to compute a least upper bound here since HType does not do big
unions.

Using the NativeBehavior allows us to in future mark JS-calls and DOM methods
with the effects that they cause and are sensitive to.

Also, we might want to extend the annotation and JS-call type with nullability
annotations - I saw elsewhere that you desire the distinction between '=List'
and '=List|Null'.

[ngeoffray, 2012-11-29 11:10:43]

https://codereview.chromium.org/11308257/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11308257/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:392: var argNodes =
node.arguments;
On 2012/11/29 00:12:51, sra1 wrote:
> We should avoid parsing this string in two places.
> 
> This code should be moved into NativeEnqueuerBase.processNativeBehavior.
> That runs after the native element has been 'parsed' into a set of
instantiated
> classes.
> That way, things will work if the only String in the program comes from a DOM
> call.
> 
> It also takes care of the fact that the instantiated types and returned types
> are not always the same.

Done.

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

https://codereview.chromium.org/11308257/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1512: if
(declaredType.slowToString() == '=List') return HType.READABLE_ARRAY;
On 2012/11/29 00:12:51, sra1 wrote:
> I have not figured out how to do it yet, but we should use the typesReturned
set
> from NativeBehavior.
> We would need to compute a least upper bound here since HType does not do big
> unions.
> 
> Using the NativeBehavior allows us to in future mark JS-calls and DOM methods
> with the effects that they cause and are sensitive to.
> 
> Also, we might want to extend the annotation and JS-call type with nullability
> annotations - I saw elsewhere that you desire the distinction between '=List'
> and '=List|Null'.
> 

Agree. The current JS form is pretty limited and w'ere trying to do more and
more things with it. We should add more variants to it.

[ahe, 2012-11-29 14:48:43]

I'm sorry, but I'm really concerned where this is going from a maintenance
perspective.  In my opinion we have crossed a point where the amount of magic in
JS has become too much.

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

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:466: var match =
JS('=List|Null',
What does this mean?  Where is it documented what it means?

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:411: //  '=List'. 
      A JavaScript Array, no subtype.
This has *nothing* to do with legacy support for native frog code.

[sra1, 2012-11-29 16:45:50]

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:59: void
registerJsCall(Send node, Enqueuer enqueuer, ResolverVisitor resolver) {}
enqueuer already available in implementations as this.world.

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:435:
enqueuer.registerInstantiatedClass(compiler.listClass);
NativeBehavior is data.  There should be no side effect of creating a
description of something's native behavior.  We could add more information from
annotations, like side-effects.

The registering should be in processNativeBehavior, so that this annotation on a
native class also does the right thing:

class MessageEvent ... native ... {

    @Creates('=List')  @Returns('=List')
    final List ports;
}

NativeEnqueuerBase already had the main enqueuer as this.world.

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:439:
enqueuer.registerInstantiatedClass(type.element);
I assume this is how 'int' gets instantiated.

Is there a way to tell if the type is one of the small number (other than native
classes) with hidden calls to the constructors (int, bool, String, ...)

Does JS('List', ...) to instantiate compiler.listClass through this path?

I'm concerned that this path will lead to instantiating all the native types.

[ngeoffray, 2012-11-30 11:30:00]

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

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:466: var match =
JS('=List|Null',
On 2012/11/29 14:48:43, ahe wrote:
> What does this mean?  Where is it documented what it means?

The documentation is currently in NativeBehavior::ofJsCall (in
native_handler.dart). Moving JS (and its documentation) to its own library is
something I intend to address soon.

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:411: //  '=List'. 
      A JavaScript Array, no subtype.
On 2012/11/29 14:48:43, ahe wrote:
> This has *nothing* to do with legacy support for native frog code.

Yes, as discussed, native_handler got mixed into legacy support, and handling of
the html library. I'll clean that up.

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:435:
enqueuer.registerInstantiatedClass(compiler.listClass);
On 2012/11/29 16:45:50, sra1 wrote:
> NativeBehavior is data.  There should be no side effect of creating a
> description of something's native behavior.  We could add more information
from
> annotations, like side-effects.
> 
> The registering should be in processNativeBehavior, so that this annotation on
a
> native class also does the right thing:
> 
> class MessageEvent ... native ... {
> 
>     @Creates('=List')  @Returns('=List')
>     final List ports;
> }
> 
> NativeEnqueuerBase already had the main enqueuer as this.world.

Done.

https://codereview.chromium.org/11308257/diff/1009/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:439:
enqueuer.registerInstantiatedClass(type.element);
On 2012/11/29 16:45:50, sra1 wrote:
> I assume this is how 'int' gets instantiated.
> 
> Is there a way to tell if the type is one of the small number (other than
native
> classes) with hidden calls to the constructors (int, bool, String, ...)
> 
> Does JS('List', ...) to instantiate compiler.listClass through this path?
> 
> I'm concerned that this path will lead to instantiating all the native types.

Yeah, just unconditionally registering will lead to instantiating all the types.
I've changed the code to manually check for these primitive classes. There is no
easy way right now other than if/else, but we can think of creating a set just
for them.

[sra1, 2012-11-30 21:38:46]

lgtm

[ahe, 2012-12-03 12:10:27]

LGTM.  You are addressing my concerns about documentation in another CL, thank
you for that.