Add NativeEnqueuer to work with the Enqueuer.

Add NativeEnqueuer to work with the Enqueuer.

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

[sra1, 2012-10-27 01:00:48]

For the native live class analysis I need to see all the JS code and
parse the types.  Pulling in the native types all starts at places like this:

LocalWindow get window => JS('LocalWindow', 'window');
_LocalWindowImpl get _window => JS('_LocalWindowImpl', 'window');
Document get document => JS('Document', 'document');

class .... {
  static Float32Array _F32(arg) =>
      JS('Float32Array', 'new Float32Array(#)', arg);
 
I need to treat each JS as a potential constructor.
How do I parse the type string?
It needs to resolve in the correct library (the one containing the JS).
Obviously, the types on JS are going to become much more important and everyone
writing that kind of code will need to be careful.

How do I convey that information back to the work queue?
This is what I have so far:

I'm creating a NativeEnqueuer that is available from the Enqueuer (aka world).
There is a different NativeEnqueuer for resolution and code-gen (I think I need
it but my opinion might change).
world.nativeEnqueuer will be available, and calling methods, e.g.

world.nativeEnqueuer.registerJS(type_string)   (called from resolution or
codeget)
could call methods on world, e.g.  world.registerInstantiatedClass for all
native subtypes of the declared type.

Where to I put these callbacks in the resolver and codegen?

Thanks!

https://codereview.chromium.org/11304021/diff/1/lib/compiler/implementation/n...
File lib/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11304021/diff/1/lib/compiler/implementation/n...
lib/compiler/implementation/native_handler.dart:59:
//nativeClasses.forEach(registerClass);
Uncomment to go back to registering everything.

https://codereview.chromium.org/11304021/diff/1/lib/compiler/implementation/n...
lib/compiler/implementation/native_handler.dart:153: // its supertypes. The code
emitter and the ssa builder use that
Nicolas: How and when does the ssa builder use this?

[ngeoffray, 2012-10-29 14:10:47]

About types in JS, I think we can start using type literals instead of strings.
We would then have the resolver find the type for you.

https://codereview.chromium.org/11304021/diff/1/lib/compiler/implementation/n...
File lib/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11304021/diff/1/lib/compiler/implementation/n...
lib/compiler/implementation/native_handler.dart:153: // its supertypes. The code
emitter and the ssa builder use that
On 2012/10/27 01:00:48, sra1 wrote:
> Nicolas: How and when does the ssa builder use this?

That's in handleSsaNative in this file: method isOverriddenMethod.

[sra1, 2012-10-29 16:12:46]

On 2012/10/29 14:10:47, ngeoffray wrote:
> About types in JS, I think we can start using type literals instead of
strings.
> We would then have the resolver find the type for you.

I think we want to use types that are not expressable in Dart.
For example, a serialized script value that you get from postMessage is one of
about 10 types (string, int, Map, List, ArrayBuffer, typed-array, ImageData,
Blob, but not a HTML element or SVG element or Event or AudioNode).
If we just say 'Object' then we will need something else to restrict the type so
that we don't infer all types are instantiated.


> 
>
https://codereview.chromium.org/11304021/diff/1/lib/compiler/implementation/n...
> File lib/compiler/implementation/native_handler.dart (right):
> 
>
https://codereview.chromium.org/11304021/diff/1/lib/compiler/implementation/n...
> lib/compiler/implementation/native_handler.dart:153: // its supertypes. The
code
> emitter and the ssa builder use that
> On 2012/10/27 01:00:48, sra1 wrote:
> > Nicolas: How and when does the ssa builder use this?
> 
> That's in handleSsaNative in this file: method isOverriddenMethod.

[ngeoffray, 2012-11-08 08:18:01]

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:43: class
NativeEnqueuerBase implements NativeEnqueuer {
A couple of high-level comments and alternatives about this class:

1) Could it be a subclass of the real Enqueuer, and override methods of
interest? For example, when a new Dart class is registered, this enqueuer would
check if it's native, do what it needs to do based on that information and then
call the super.

2) Alternatively, it could just contain top-level methods that are called by the
real enqueuer. In the case of dart2dart, it doesn't see these JS and native
thingies, so these methods would never be called.

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1739: if
(selector.name == const SourceString('JS')) {
Note that the equality check must be done on the element.

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1761: void
resolveJsTypeParameter(Node node, Node expression) {
I would move this method to the native enqueuer.

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1790:
world.nativeEnqueuer.registerJSDartType(type, expression);
Could this just be (native)enqueuer.registerInstantiatedClass ?

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1760:
//enqueuer.codegen.nativeEnqueuer.xxx
That is the right place to do it for the codegen enqueuer.

[sra1, 2012-11-12 20:28:03]

Still a work in progress.

I have factored the properties of a native feature (NativeBehavior) from its
application.

https://chromiumcodereview.appspot.com/11304021/diff/10001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/11304021/diff/10001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1761: void
resolveJsTypeParameter(Node node, Node expression) {
On 2012/11/08 08:18:01, ngeoffray wrote:
> I would move this method to the native enqueuer.

Done.  I've moved everything except the lookup of a string in the scope.

https://chromiumcodereview.appspot.com/11304021/diff/10001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1790:
world.nativeEnqueuer.registerJSDartType(type, expression);
On 2012/11/08 08:18:01, ngeoffray wrote:
> Could this just be (native)enqueuer.registerInstantiatedClass ?

No, the matching is much more complex (subtypes, and I will want to add special
rules for some things.)

[ahe, 2012-11-13 20:55:22]

Just skimmed the patch on my phone.  I really like like the direction you're
taking.

Regarding metadata, I should soon have a fix for @C(), and please let me know if
you find other problems.

Nicolas and I have been talking about creating a js_foreign.dart library where
we explicitly define all foreign elements, so it is possible to find
documentation.  I would like to hear your thoughts on that, or if you have any
ideas for stuff that needs an overhaul.

[sra1, 2012-11-14 05:04:26]

Ok, so I'm putting this out to review to make the most of the time-zone
difference.

By default it will be turned off at first, enabled by
--enable-native-live-class-analysis.

There are some commented-out prints.  I will remove them before checking in.

html_dart2js will not be checked in - those changes are a 'demo' of the kinds of
things I need to get the generator to produce.  That will be done in separate
CLs when the annotation declarations are checked in.  They are needed to 

I could check in the two annotation classes in a separate CL to unblock work on
annotating the library - that would give me something to work with while the
review is in progress.

[ahe, 2012-11-14 13:51:30]

So far so good.  A lot of nitpicking below, nothing really important. 
Unfortunately, I have go offline before I have completed the review, but here
are the comments so far.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:254: :
this.isEvaluatingConstant = isConst == null ? false : isConst;
I prefer the fix I have in https://codereview.chromium.org/11293244/.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:1: // Copyright (c)
2012, the Dart project authors.  Please see the AUTHORS file
All my comments in this file are nitpicking.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:80:
native.NativeEnqueuer nativeResolutionEnqueuer(world, bool liveTypeAnalysis) =>
Long line.  As a matter of style, I prefer to avoid => syntax for functions that
doesn't fit on one line.  But we are not consistent about that in dart2js.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:80:
native.NativeEnqueuer nativeResolutionEnqueuer(world, bool liveTypeAnalysis) =>
I don't think the option liveTypeAnalysis is necessary.  You are no using it,
and it can be obtained from compiler.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:80:
native.NativeEnqueuer nativeResolutionEnqueuer(world, bool liveTypeAnalysis) =>
An undocumented property of the library native_handler.dart, is that we want to
get rid of it.  It is used to implement language extensions that we want to
replace by metadata.  So we would like to keep it at arms length, and so it
would be better if these types came from a different library (in which case you
can drop the "native" prefix").  We are using the word "native" to mean
deprecated stuff that should go away :-)

For simplicity, you could start by defining a new library which simply export
NativeEnqueuer?  This library would then represent stuff that we want to have
long term.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:538:
enqueuer.resolution.nativeEnqueuer.logSummary(log);
I like this approach.  How about merging these two lines to:

enqueuer.resolution.logSummary(log);

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:553:
enqueuer.codegen.nativeEnqueuer.logSummary(log);
Ditto.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:666: throw 'Internal
error: unresolved element: $element.';
Remove this.  FYI: you can often use --throw-on-error instead of inserting
debug-throws.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:406:
--enable-native-live-type-analysis
It's great that you added documentation.  I forgot to do so myself in my
--analyze-all, so thank you for reminding me :-)

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:215: } else if
(identical(member.kind, ElementKind.FIELD)) {
I think we are slowly moving towards using == instead of identical for these
tests.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:216: if
(member.enclosingElement.isNative()) {
Assert the condition instead?

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:319:
nativeEnqueuer.registerFieldLoad(member);
Does order matter here?

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:320:
nativeEnqueuer.registerFieldStore(member);
Why also store?

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:323:
nativeEnqueuer.registerFieldLoad(member);
Why load?

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:47: JS('void',
r'#.push(#)', receiver, value);
I'm not sure what void means here.  I think Nicolas has asked me to not use void
in JS, but I never understood why.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:17: // This class
is a temporary work-around until we get a more powerful DartType.
Not a proper documentation comment.  That is, use /** or ///.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:18: class
SpecialType {
Can we use ConcreteType from
dart/sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart?

If not, what do we need to change in concrete_types_inferrer.dart?

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:22: // Object, but
no subtypes:
///

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:25: // The
specific implementation of List that is JavaScript Array:
///

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:30: // This could
be an abstract class but we use it as a stub for the dart_backend.
///

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:36: // Method is a
member of a native class.
///

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:39: // Field is a
member of a native class.
///

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:43: // JS-form
code can be an instantiation point for types.
This comment needs a bit of work to become a proper documentation comment.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:49: // Emits a
summary log line.
///

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:58: // Each class
is exactly one of these sets.
I think this comment applies to the following fields.  That generally doesn't
work well with documentation comments, so you may want to change this to a
class-level documentation comment.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:64: // no need to
match it again.
/// 

Perhaps also reword the comment.  For example, by prefixing with "Records
matched constraints, once ..."

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:67: final queue =
new Queue();  // Classes in pendingClasse have thunks in queue.
Move comment to previous line and use ///.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:78: // constructed
by backend.
///

Also, start sentence with an uppercase letter.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:83: Element e =
compiler.coreLibrary.find(name);
Should be compiler.findHelper

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:85:
compiler.cancel("Core library missing class '${name}'");
How about: "Could not find implementation class '$name'"?

[ngeoffray, 2012-11-14 21:17:39]

Like Peter a lot of nitpicking, but I like where this is heading.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:80:
native.NativeEnqueuer nativeResolutionEnqueuer(world, bool liveTypeAnalysis) =>
Line too long, and the style we used is not to use '=>' if the expression does
not fit in the same line as the method definition.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:666: throw 'Internal
error: unresolved element: $element.';
Remove debugging code.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:44: native.NativeEnqueuer
nativeEnqueuer;  // Set by compiler.
Why not by this class?

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:174: if
(!member.isInstanceMember()) return;
How about just doing nativeEnqueuer.processInstantiatedClassMember(cls, member),
and let that one deal with the isNative checks, field store and field load?

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:218: // print('  
$memberName:  ${instanceMembersByName[memberName]}');
Remove debugging code

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:315: if
(selector.applies(member, compiler)) {
Same comment, nativeEnqueuer.handleUnseenMember.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:816: bool
liveTypeAnalysis) =>
Use {} instead.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:820: bool
liveTypeAnalysis) =>
ditto

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:47: JS('void',
r'#.push(#)', receiver, value);
On 2012/11/14 13:51:30, ahe wrote:
> I'm not sure what void means here.  I think Nicolas has asked me to not use
void
> in JS, but I never understood why.

I can't remember if I  said not to use 'void', but in any case it's something
the backend can understand.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:84: return
JS('num', r'#.length', receiver);  // TODO(sra): Use 'int'?
yes

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:305: // return
JS('Object', r'#[#]', a, index);
Remove commented line.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:351: // var value =
JS('Object', r'#[#]', list, i);
ditto

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:591: // var date =
JS('Object', r'new Date(#)', value);
ditto

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:904: // var jsError
= JS('Object', r'new Error()');
ditto

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:920: //
toStringWrapper() => JS('Object', r'this.dartException').toString();
ditto

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:949: // return
JS('Object', r'#.dartException', ex);
ditto

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1509: //var
typeInfo = JS('Object', r'#.builtin$typeInfo', object);
ditto

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:67: final queue =
new Queue();  // Classes in pendingClasse have thunks in queue.
Explain what those thunks are.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:103: if
(element.kind == ElementKind.CLASS) {
too much indentation.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:107:
unusedClasses.add(classElement);
Add a comment why you're also adding it to 'unused'.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:116:
enqueueClass(ClassElement classElement, cause) {
String cause?

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:118: //
compiler.log(
Remove debugging code

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:142: //
print('processClass $classElement $cause');
Remove debugging code.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:160: //
print('Native method call $method');
Remove debugging code.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:179: //
print('Native load field $field');
Remove

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:187: //
print('Native store field $field');
Remove

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:194: void
registerJsCall(Send node, resolver) {
Add type annotation to resolver.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:207: // These two
special types are always instantiated.
What are these two? Add them in the comment.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:220: if (cause is
Node) {
What is the difference between this if and the next one?

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:221: // throw
'Eugh $cause';
Remove throw

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:224:
//compiler.cancel('Eugh!', token: cause.position());
Remove cancel

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:227:
compiler.log('All native types marked used due to $type.');
What is type here?

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:232:
enqueueUnusedClassesMatching(predicate, cause, [reason]) {
Types please

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:233: var matches =
unusedClasses.filter(predicate);
ditto

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:234: //
print('${matches.length} matches: $matches\n..   $reason  $world');
Remove

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:253: var name =
classElement.name.slowToString();
String name

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:344: // A method
might create and return instances of native subclasses its
of its

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:449: DartType
lookup(String name) {
You could move this method into _collect.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:467:
typesInstantiated..clear()..addAll(creates);
First use of cascaded calls in dart2js I see! Congratulations :)

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:470:
typesReturned..clear()..addAll(returns);
And second :)

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:508: void
_escape(DartType type, Compiler compiler) {
Please add comments on what this function does.

http://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:520: void
_capture(DartType type, Compiler compiler) {
ditto

[sra1, 2012-11-15 00:09:10]

I have worked through the code review comments.

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:43: class
NativeEnqueuerBase implements NativeEnqueuer {
On 2012/11/08 08:18:01, ngeoffray wrote:
> A couple of high-level comments and alternatives about this class:
> 
> 1) Could it be a subclass of the real Enqueuer, and override methods of
> interest? For example, when a new Dart class is registered, this enqueuer
would
> check if it's native, do what it needs to do based on that information and
then
> call the super.

Enqueuer already needs to be split into a resolution and codegen enqueuer.  Then
I would have to subclass two classes.  Composition is more flexible than
inhertitance.

> 
> 2) Alternatively, it could just contain top-level methods that are called by
the
> real enqueuer. In the case of dart2dart, it doesn't see these JS and native
> thingies, so these methods would never be called.

I think I'm already doing (2), but with class NativeEnqueuer

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11304021/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1739: if
(selector.name == const SourceString('JS')) {
On 2012/11/08 08:18:01, ngeoffray wrote:
> Note that the equality check must be done on the element.

Which element?

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:254: :
this.isEvaluatingConstant = isConst == null ? false : isConst;
On 2012/11/14 13:51:30, ahe wrote:
> I prefer the fix I have in https://codereview.chromium.org/11293244/.

I will merge your fix.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:80:
native.NativeEnqueuer nativeResolutionEnqueuer(world, bool liveTypeAnalysis) =>
On 2012/11/14 13:51:30, ahe wrote:
> An undocumented property of the library native_handler.dart, is that we want
to
> get rid of it.  It is used to implement language extensions that we want to
> replace by metadata.  So we would like to keep it at arms length, and so it
> would be better if these types came from a different library (in which case
you
> can drop the "native" prefix").  We are using the word "native" to mean
> deprecated stuff that should go away :-)

Almost all the functionality in here will be needed forever.

When we eliminate the 'native' keyword and replace it with an annotation, there
will still need to some code that tests to see if the annotation is present, and
to interpret the parameters to the annotation.  The call will move from the
parser to somewhere else.  This is a matter of changing one syntax for another,
one which allows the test to be deferred for a little bit.  Otherwise I don't
see what will change.

I am concerned that moving some things to attributes means that we need to
resolve the attributes to know if something is a special native thing.  For
example, we want to know which classes are native classes in order to add them
to a set prior to resolution.  But we need to resolve the attributes on all
classes now to know which ones are native classes, and to do that we need to
resolve the classes used by the annotations.

I'm sure it can be done, but special syntax is much more straightforward to
implement.

> 
> For simplicity, you could start by defining a new library which simply export
> NativeEnqueuer?  This library would then represent stuff that we want to have
> long term.

If you agree with my characterization, then it is reasonable to keep this
library as is?  We will switch the 10% of the library that is parser hooks for
other, annotation-based hooks.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:80:
native.NativeEnqueuer nativeResolutionEnqueuer(world, bool liveTypeAnalysis) =>
On 2012/11/14 13:51:30, ahe wrote:
> I don't think the option liveTypeAnalysis is necessary.  You are no using it,
> and it can be obtained from compiler.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:80:
native.NativeEnqueuer nativeResolutionEnqueuer(world, bool liveTypeAnalysis) =>
On 2012/11/14 13:51:30, ahe wrote:
> I don't think the option liveTypeAnalysis is necessary.  You are no using it,
> and it can be obtained from compiler.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:80:
native.NativeEnqueuer nativeResolutionEnqueuer(world, bool liveTypeAnalysis) =>
On 2012/11/14 21:17:39, ngeoffray wrote:
> Line too long, and the style we used is not to use '=>' if the expression does
> not fit in the same line as the method definition.

I see plenty of examples.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:538:
enqueuer.resolution.nativeEnqueuer.logSummary(log);
On 2012/11/14 13:51:30, ahe wrote:
> I like this approach.  How about merging these two lines to:
> 
> enqueuer.resolution.logSummary(log);

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:553:
enqueuer.codegen.nativeEnqueuer.logSummary(log);
On 2012/11/14 13:51:30, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:666: throw 'Internal
error: unresolved element: $element.';
On 2012/11/14 13:51:30, ahe wrote:
> Remove this.  FYI: you can often use --throw-on-error instead of inserting
> debug-throws.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:44: native.NativeEnqueuer
nativeEnqueuer;  // Set by compiler.
On 2012/11/14 21:17:39, ngeoffray wrote:
> Why not by this class?

This class has two instances.
I can use isResolutionQueue here, or set it outside.

isResolutionQueue feels like a hack to me,  The enqueuer also does not have the
backend on hand, and getting it also feels like a hack.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:174: if
(!member.isInstanceMember()) return;
On 2012/11/14 21:17:39, ngeoffray wrote:
> How about just doing nativeEnqueuer.processInstantiatedClassMember(cls,
member),
> and let that one deal with the isNative checks, field store and field load?

I think it will be too complicated.

Names need to be added to instanceMembersByName, so
processIntantiatedClassMembers would need to return a flag.

The logic for the cross product of selectors and members is all here, and I
think it will be confusing to move some of it elsewhere.

If I renamed NativeEnqueuer to NativeLivenessAnalysis, would that make it
clearer that this is the right place for the selector X member code?

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:218: // print('  
$memberName:  ${instanceMembersByName[memberName]}');
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove debugging code

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:315: if
(selector.applies(member, compiler)) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> Same comment, nativeEnqueuer.handleUnseenMember.

Same response - keep the cross-product as the responsibility of this class.

I would have put the field in the worklist, except we will eventually need to
distinguish loads from stores rather than treating them together.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:319:
nativeEnqueuer.registerFieldLoad(member);
On 2012/11/14 13:51:30, ahe wrote:
> Does order matter here?

Added comments to answer the questions here.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:320:
nativeEnqueuer.registerFieldStore(member);
On 2012/11/14 13:51:30, ahe wrote:
> Why also store?

See new comments.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:323:
nativeEnqueuer.registerFieldLoad(member);
On 2012/11/14 13:51:30, ahe wrote:
> Why load?

See new comments.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:816: bool
liveTypeAnalysis) =>
On 2012/11/14 21:17:39, ngeoffray wrote:
> Use {} instead.

OK, but I don't really agree with a style guideline that forces you to change
from => to {} depending on whether you add a parameter or change the name of a
parameter type.
That is causing required edits that the autoindenting in your favorite editor
cannot handle.

It is dangerous to do the change - I forgot 'return' and it cost me some time. 
It is much safer to adopt guidelines that don't force you to make error-prone
changes.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:47: JS('void',
r'#.push(#)', receiver, value);
On 2012/11/14 13:51:30, ahe wrote:
> I'm not sure what void means here.  I think Nicolas has asked me to not use
void
> in JS, but I never understood why.

I have been using 'void' to mean that the value of 'JS' is unused.
This is good for native live class analysis, since if a native class is returned
by JS but ignored, it did not really enter the Dart code.

I'd rather use 'void' than invent something new that means the same.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:84: return
JS('num', r'#.length', receiver);  // TODO(sra): Use 'int'?
On 2012/11/14 21:17:39, ngeoffray wrote:
> yes

I expect the interceptor work will take care of this.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:305: // return
JS('Object', r'#[#]', a, index);
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove commented line.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:351: // var value =
JS('Object', r'#[#]', list, i);
On 2012/11/14 21:17:39, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:591: // var date =
JS('Object', r'new Date(#)', value);
On 2012/11/14 21:17:39, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:904: // var jsError
= JS('Object', r'new Error()');
On 2012/11/14 21:17:39, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:920: //
toStringWrapper() => JS('Object', r'this.dartException').toString();
On 2012/11/14 21:17:39, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:949: // return
JS('Object', r'#.dartException', ex);
On 2012/11/14 21:17:39, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1509: //var
typeInfo = JS('Object', r'#.builtin$typeInfo', object);
On 2012/11/14 21:17:39, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:17: // This class
is a temporary work-around until we get a more powerful DartType.
On 2012/11/14 13:51:30, ahe wrote:
> Not a proper documentation comment.  That is, use /** or ///.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:18: class
SpecialType {
On 2012/11/14 13:51:30, ahe wrote:
> Can we use ConcreteType from
>
dart/sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart?
> 

Lets keep it separate for now.
It is not clear that ConcreteType will scale adequately.

We already have 3 kind of types
DartType
HType
ConcreteType

I need to be able to test  List<num> vs List<File>.
I think DartType will be the closest to that when implemented (and will be
needed for warnings).

I could rephrase this analysis as predicates on DartType:
Exactly(T)
Cone(T)
Union(T1, T2)

I just think that is overkill for the current needs, but would be the direction
I would take this if needed.

> If not, what do we need to change in concrete_types_inferrer.dart?

That would need to iterate over the the list of SpecialType and DartType in
'typesReturned' to construct the set of concrete types.
This should always be more precise that the declared type.
The type analysis will need some hooks for understanding that the native
elements can instantiate types.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:22: // Object, but
no subtypes:
On 2012/11/14 13:51:30, ahe wrote:
> ///

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:25: // The
specific implementation of List that is JavaScript Array:
On 2012/11/14 13:51:30, ahe wrote:
> ///

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:30: // This could
be an abstract class but we use it as a stub for the dart_backend.
On 2012/11/14 13:51:30, ahe wrote:
> ///

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:36: // Method is a
member of a native class.
On 2012/11/14 13:51:30, ahe wrote:
> ///

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:39: // Field is a
member of a native class.
On 2012/11/14 13:51:30, ahe wrote:
> ///

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:43: // JS-form
code can be an instantiation point for types.
On 2012/11/14 13:51:30, ahe wrote:
> This comment needs a bit of work to become a proper documentation comment.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:49: // Emits a
summary log line.
On 2012/11/14 13:51:30, ahe wrote:
> ///

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:58: // Each class
is exactly one of these sets.
On 2012/11/14 13:51:30, ahe wrote:
> I think this comment applies to the following fields.  That generally doesn't
> work well with documentation comments, so you may want to change this to a
> class-level documentation comment.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:64: // no need to
match it again.
On 2012/11/14 13:51:30, ahe wrote:
> /// 
> 
> Perhaps also reword the comment.  For example, by prefixing with "Records
> matched constraints, once ..."

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:67: final queue =
new Queue();  // Classes in pendingClasse have thunks in queue.
On 2012/11/14 13:51:30, ahe wrote:
> Move comment to previous line and use ///.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:67: final queue =
new Queue();  // Classes in pendingClasse have thunks in queue.
On 2012/11/14 21:17:39, ngeoffray wrote:
> Explain what those thunks are.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:78: // constructed
by backend.
On 2012/11/14 13:51:30, ahe wrote:
> ///
> 
> Also, start sentence with an uppercase letter.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:83: Element e =
compiler.coreLibrary.find(name);
On 2012/11/14 13:51:30, ahe wrote:
> Should be compiler.findHelper

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:85:
compiler.cancel("Core library missing class '${name}'");
On 2012/11/14 13:51:30, ahe wrote:
> How about: "Could not find implementation class '$name'"?

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:103: if
(element.kind == ElementKind.CLASS) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> too much indentation.

OK.

I think we should change the style, though.  It is pretty horrible when there
are additional arguments:

foo(() {
  print(1);
},
1);

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:107:
unusedClasses.add(classElement);
On 2012/11/14 21:17:39, ngeoffray wrote:
> Add a comment why you're also adding it to 'unused'.

I added a comment on nativeClasses.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:116:
enqueueClass(ClassElement classElement, cause) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> String cause?

It can be a member.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:118: //
compiler.log(
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove debugging code

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:142: //
print('processClass $classElement $cause');
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove debugging code.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:160: //
print('Native method call $method');
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove debugging code.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:179: //
print('Native load field $field');
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:187: //
print('Native store field $field');
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:194: void
registerJsCall(Send node, resolver) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> Add type annotation to resolver.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:207: // These two
special types are always instantiated.
On 2012/11/14 21:17:39, ngeoffray wrote:
> What are these two? Add them in the comment.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:220: if (cause is
Node) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> What is the difference between this if and the next one?

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:221: // throw
'Eugh $cause';
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove throw

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:224:
//compiler.cancel('Eugh!', token: cause.position());
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove cancel

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:227:
compiler.log('All native types marked used due to $type.');
On 2012/11/14 21:17:39, ngeoffray wrote:
> What is type here?

Old debugging code.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:232:
enqueueUnusedClassesMatching(predicate, cause, [reason]) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> Types please

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:233: var matches =
unusedClasses.filter(predicate);
On 2012/11/14 21:17:39, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:234: //
print('${matches.length} matches: $matches\n..   $reason  $world');
On 2012/11/14 21:17:39, ngeoffray wrote:
> Remove

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:253: var name =
classElement.name.slowToString();
On 2012/11/14 21:17:39, ngeoffray wrote:
> String name

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:344: // A method
might create and return instances of native subclasses its
On 2012/11/14 21:17:39, ngeoffray wrote:
> of its

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:449: DartType
lookup(String name) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> You could move this method into _collect.

But then I allocate two closures .. I think it is OK here.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:467:
typesInstantiated..clear()..addAll(creates);
On 2012/11/14 21:17:39, ngeoffray wrote:
> First use of cascaded calls in dart2js I see! Congratulations :)

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:470:
typesReturned..clear()..addAll(returns);
On 2012/11/14 21:17:39, ngeoffray wrote:
> And second :)

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:508: void
_escape(DartType type, Compiler compiler) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> Please add comments on what this function does.

Done.

https://codereview.chromium.org/11304021/diff/14004/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:520: void
_capture(DartType type, Compiler compiler) {
On 2012/11/14 21:17:39, ngeoffray wrote:
> ditto

Done.

[ngeoffray, 2012-11-15 12:49:57]

LGTM!

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:529:
backend.nativeResolutionEnqueuer(enqueuer.resolution);
The reason why I asked about why not initializing those in the enqueuer, is that
I don't like having this initialization code here (and that you need to
duplicate in the mirrors library). For me, it seems much cleaner to let the
enqueuer deal with it.

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:313: // print('unseen
$selector add $member   ${member.kind}');
Remove print

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:34: /// Initial
entry point to
Aborted comment.

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:42: /// [field], a
member of a native class, has been loaded
Not a sentence.

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:501: fields[0] is!
StringConstant) {
fits in one line?

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/11304021/diff/20017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1739: if
(selector.name == const SourceString('JS')) {
Add a target.kind == ElementKind.FOREIGN. Otherwise, user-defined JS would be
matched.

[ahe, 2012-12-04 09:15:19]

https://codereview.chromium.org/11304021/diff/24002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11304021/diff/24002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:227: for (var type
in behavior.typesInstantiated) {
When I see code like this, I assume this is equivalent:

Collection<DartType> types = behavior.typesInstantiated;
for (DartType type in types) {
  ...

However, SpecialType does not implement DartType.  So my version would fail.

I think that is problematic because a compiler will always have a natural
concept of "type", and the concept "type" should be reserved for expressing only
that concept.  Otherwise it will cause confusion.  There are a few ways to
accomplish this:

* Use different words to describe concepts that are type-like.  For example,
HType.  However, this is problematic as you then have to convert between
DartTypes and HTypes.  It is also problematic because people will accept
fallacious arguments like:

The JavaScript backend is a separate component from the front-end, and in the
backend, the concept of "type" is clearly HType.  So in the backend, we can call
fields "type", not "htype".

This argument is fallacious because nobody works exclusively on the backend or
the frontend, so everyone will get confused when there isn't agreement about a
core concept such as "type" throughout the compiler.

* Make sure that all types implement DartType.  This is actually easy.

In case you're counting, we now have at least four different kinds of types in
dart2js:

1. HType
2. DartType
3. SpecialType
4. ConcreteType

It would be great if we all could agree to stop this, and also not compound the
situation further.  As far as I can tell, a good first step would be to have
SpecialType and ConcreteType implement DartType.

https://codereview.chromium.org/11304021/diff/24002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:234: assert(type
is DartType);
There is a more natural way to express this in Dart:

DartType dartType = type;

That also gives you a considerably better error message than this:

Internal error:
'file:///mnt/data/b/build/slave/dart2js-linux-release-host-checked/build/dart/sdk/lib/_internal/compiler/implementation/native_handler.dart':
Failed assertion: line 312 pos 14: 'type is DartType' is not true.

I assume the problem is that "type" is null.