Wrapperless dart:html and friends

Wrapperless dart:html and friends

This reuses the dart2js code.

R=jacobr@google.com, jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/58e8be41b456ef47f3ab6628cbb14a45d9be9130

[vsm, 2016-02-16 17:11:56]

Description was changed from

==========
Wrapperless dart:html and friends

This reuses the dart2js code.
==========

to

==========
Wrapperless dart:html and friends

This reuses the dart2js code.
==========

[vsm, 2016-02-16 17:11:56]

vsm@google.com changed reviewers:
+ jacobr@google.com, jmesserly@google.com

[vsm, 2016-02-16 17:13:05]

This is not quite ready, but it passes all tests and enables a few more angular
apps.

Issues
- Clean up dodgy changes in classes.dart
- Clean up changes in js_codegen.dart

[Jennifer Messerly, 2016-02-16 17:46:38]

Nice! Honestly this looks pretty close to me :). Here are a few suggestions

https://codereview.chromium.org/1700153002/diff/1/lib/runtime/dart/_debugger.js
File lib/runtime/dart/_debugger.js (right):

https://codereview.chromium.org/1700153002/diff/1/lib/runtime/dart/_debugger....
lib/runtime/dart/_debugger.js:391: let keys = map[dartx.keys][dartx.toList]();
how did Maps end up as a native type?
I guess some native type in dart:html implements map?

Anyway, this is kind of interesting. Our current extension method story isn't
modular/incremental. So we'll want something like @JSPeerInterface to let
interfaces declare if JS native types can implement them.

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:673: // TODO(vsm): Refactor common logic out.
yeah if this was in a method like _emitRenamedNativeField, with a doc comment,
it'd be good. The logic makes sense.

BTW, in the past we've tended to care about code size & readability, so these
type of metaprogramming things are often generated via a helper:

    dart.renameField(HTMLElement, 'elementList', 'elements');

Whether that's the long term direction, I'm not sure. All of the existing
transpilers do use helpers to make the code more readable, though. Even though
it does make Closure advanced optimizations unhappy, I'd rather have
closure-advanced specific lowering.

Only other thing I'd check is if this can share any logic with existing
vitrualField code path.

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1007: if (_extensionTypes.contains(cls)) {
eeep. This is a little bit sketchy :). We try and make sure all extension names
are already defined by the library we're importing.

if the extension symbol isn't defined previously at the top-level, what
guarantee do we have that it's getting defined for the other code that uses it,
like method bodies?

my intuition here is that this might be papering over a problem you were seeing,
but there's still a bug that can be hit in other ways...

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:3035: if (!_isObjectProperty(memberName) ||
target is SuperExpression) {
nice fix.

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
File tool/input_sdk/private/classes.dart (right):

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
tool/input_sdk/private/classes.dart:241: let symbol = Symbol($subclass.name +
'.' + $fieldName.toString());
nice fix! yeah this bug has existed for a while (private fields didn't work in
extension methods/virtual fields). This is the right fix.

The only thing maybe worth double checking is how it looks in debugger.

We could save the original string "_foobar" name on the Symbol, if it helps.

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
tool/input_sdk/private/classes.dart:291: // TODO(vsm): Not all registered js
types are real.
yeah, it'd be really nice to avoid generating a call to this method if we can :)

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
tool/input_sdk/private/classes.dart:328: // TODO(vsm): Why?
yeah, this definitely needs further investigation. This looks like a compiler
bug, it asked us to generate "dartx.foo" for "foo", but it forgot to compile
"foo" in the first place. :)
 
What is "type" when this happens? I wonder if it's a native type? On native
types, it can be tricky to know where the method comes from, e.g.
HTMLElement/Element have gone through some refactoring over the last few years,
with methods moving "up" to Element so they can be shared by SVGElement.

That would suggest we could let ourself look up the proto chain. However, I
think we'd only want to do that for "native" types (not their subtypes).

[Jacob, 2016-02-16 17:47:35]

lgtm

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:676: FieldElement field = decl.element;
nit: change these to variables to var?

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1011: }
nit: consider

 var name = _extensionTypes.contains(cls) ?
    js.call('dart.getExtensionSymbol(#)', js.string(e.name)) :
    _emitMemberName(e.name, type: cls.type);

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:3035: if (!_isObjectProperty(memberName) ||
target is SuperExpression) {
Why does target is SuperExpression require static dispatch?
Naively would have thought 
super.dispatch could work dynamically by just following the JS prototype chain?

https://codereview.chromium.org/1700153002/diff/1/lib/src/server/server.dart
File lib/src/server/server.dart (left):

https://codereview.chromium.org/1700153002/diff/1/lib/src/server/server.dart#...
lib/src/server/server.dart:222: assert(options.inputs.length == 1);
perhaps this should change to 
assert(options.input.isNotEmpty)
?

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/lib/html/htm...
File tool/input_sdk/lib/html/html_common/conversions_dart2js.dart (right):

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/lib/html/htm...
tool/input_sdk/lib/html/html_common/conversions_dart2js.dart:96:
wrap_jso(jsObject) => jsObject;
add a TODO that we shouldn't really be exposing these methods.

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/lib/html/htm...
File tool/input_sdk/lib/html/html_common/html_common_ddc.dart (right):

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/lib/html/htm...
tool/input_sdk/lib/html/html_common/html_common_ddc.dart:28: class
DartHtmlObject {
delete this file?

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
File tool/input_sdk/private/classes.dart (right):

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
tool/input_sdk/private/classes.dart:241: let symbol = Symbol($subclass.name +
'.' + $fieldName.toString());
nit: replace
$subclass.name + '.' + $fieldName.toString()

with
'${$subclass.name}.${$fieldName}'
and similar cleanup elsewhere in the file.

[Jennifer Messerly, 2016-02-16 18:22:01]

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:3035: if (!_isObjectProperty(memberName) ||
target is SuperExpression) {
On 2016/02/16 17:47:35, Jacob wrote:
> Why does target is SuperExpression require static dispatch?
> Naively would have thought 
> super.dispatch could work dynamically by just following the JS prototype
chain?

Yup, that's what the CL is fixing, if I understand correctly. The bug was:

// Dart
class Foo extends Object {
  toString() => super.toString();
}

we were generating this incorrectly:

// JS
class Foo extends core.Object {
  toString() { return dart.toString(super); }
}

Vijay's change makes sure that "super." never uses static dispatch. So it ends
up as:

// JS
class Foo extends core.Object {
  toString() { return super.toString(); }
}

... which is correct. BTW, the reason "Object" methods generate like this is so
we can handle null correctly. But for "super." null is not possible.

BTW, Vijay -- I'm kind of surprised !_isNullable(target) below doesn't handle
this. We should know that "super" cannot be null (same with "this") ... so maybe
there is a bug here, although not necessarily one you should try to fix unless
you're motivated. Discussed more below.

thanks Jacob for making me take another look

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:3038: if (!type.isObject &&
wow, this code is confusing. Can we rewrite it like?

// In these cases, the target could be `null` so we need static dispatch.
return type.isObject ||
     _isJSBuiltinType(type) ||
     _extensionTypes.contains(type.element) ||
     _isNullable(target);


... now it's clearer, but it seems totally wrong :)

1. Why check for _isJSBuiltinType and _extensionTypes? That seems redundant.
2. Why do extension types always use static dispatch for object members?
3. Why is type.isObject a special case anyway?

Anyway, for 2, I think it's related to other brokenness in
https://github.com/dart-lang/dev_compiler/issues/451...

But yeah, what a mess. Here's what the code should be:

    return _isNullable(target);

Anyway I'm fine with the "super" workaround in the meantime. I guess because
extension methods fall into the wrong path, the existing _isNullable check
doesn't help.

[vsm, 2016-02-18 01:13:10]

PTAL

https://codereview.chromium.org/1700153002/diff/1/lib/runtime/dart/_debugger.js
File lib/runtime/dart/_debugger.js (right):

https://codereview.chromium.org/1700153002/diff/1/lib/runtime/dart/_debugger....
lib/runtime/dart/_debugger.js:391: let keys = map[dartx.keys][dartx.toList]();
On 2016/02/16 17:46:38, John Messerly wrote:
> how did Maps end up as a native type?
> I guess some native type in dart:html implements map?

Yes, I'm pretty sure we do.

> Anyway, this is kind of interesting. Our current extension method story isn't
> modular/incremental. So we'll want something like @JSPeerInterface to let
> interfaces declare if JS native types can implement them.

Yes!

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:673: // TODO(vsm): Refactor common logic out.
On 2016/02/16 17:46:38, John Messerly wrote:
> yeah if this was in a method like _emitRenamedNativeField, with a doc comment,
> it'd be good. The logic makes sense.
> 
> BTW, in the past we've tended to care about code size & readability, so these
> type of metaprogramming things are often generated via a helper:
> 
>     dart.renameField(HTMLElement, 'elementList', 'elements');
> 
> Whether that's the long term direction, I'm not sure. All of the existing
> transpilers do use helpers to make the code more readable, though. Even though
> it does make Closure advanced optimizations unhappy, I'd rather have
> closure-advanced specific lowering.
> 
> Only other thing I'd check is if this can share any logic with existing
> vitrualField code path.

Refactored and added a TODO to look into metaprogramming.  Another advantage of
your metaprogramming suggestion is code size.  These added methods are fed later
into dart.registerExtensions - perhaps it makes sense to just install there
directly?

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:676: FieldElement field = decl.element;
On 2016/02/16 17:47:35, Jacob wrote:
> nit: change these to variables to var?

Done.

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1007: if (_extensionTypes.contains(cls)) {
On 2016/02/16 17:46:38, John Messerly wrote:
> eeep. This is a little bit sketchy :). We try and make sure all extension
names
> are already defined by the library we're importing.
> 
> if the extension symbol isn't defined previously at the top-level, what
> guarantee do we have that it's getting defined for the other code that uses
it,
> like method bodies?
> 
> my intuition here is that this might be papering over a problem you were
seeing,
> but there's still a bug that can be hit in other ways...

Hmm, seems to work now if I revert.  I had probably temporarily messed this up
somehow.  Thanks for the catch!

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1011: }
On 2016/02/16 17:47:35, Jacob wrote:
> nit: consider
> 
>  var name = _extensionTypes.contains(cls) ?
>     js.call('dart.getExtensionSymbol(#)', js.string(e.name)) :
>     _emitMemberName(e.name, type: cls.type);

Removed change altogether.

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:3038: if (!type.isObject &&
On 2016/02/16 18:22:01, John Messerly wrote:
> wow, this code is confusing. Can we rewrite it like?
> 
> // In these cases, the target could be `null` so we need static dispatch.
> return type.isObject ||
>      _isJSBuiltinType(type) ||
>      _extensionTypes.contains(type.element) ||
>      _isNullable(target);
> 
> 
> ... now it's clearer, but it seems totally wrong :)
> 
> 1. Why check for _isJSBuiltinType and _extensionTypes? That seems redundant.
> 2. Why do extension types always use static dispatch for object members?
> 3. Why is type.isObject a special case anyway?
> 
> Anyway, for 2, I think it's related to other brokenness in
> https://github.com/dart-lang/dev_compiler/issues/451...
> 
> But yeah, what a mess. Here's what the code should be:
> 
>     return _isNullable(target);
> 
> Anyway I'm fine with the "super" workaround in the meantime. I guess because
> extension methods fall into the wrong path, the existing _isNullable check
> doesn't help.

Excellent point - cleaned up code!  The explicit super check was just hacking
around the problem you noted.  :-)

https://codereview.chromium.org/1700153002/diff/1/lib/src/server/server.dart
File lib/src/server/server.dart (left):

https://codereview.chromium.org/1700153002/diff/1/lib/src/server/server.dart#...
lib/src/server/server.dart:222: assert(options.inputs.length == 1);
On 2016/02/16 17:47:35, Jacob wrote:
> perhaps this should change to 
> assert(options.input.isNotEmpty)
> ?

Done.

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/lib/html/htm...
File tool/input_sdk/lib/html/html_common/conversions_dart2js.dart (right):

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/lib/html/htm...
tool/input_sdk/lib/html/html_common/conversions_dart2js.dart:96:
wrap_jso(jsObject) => jsObject;
On 2016/02/16 17:47:35, Jacob wrote:
> add a TODO that we shouldn't really be exposing these methods.

Done.

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
File tool/input_sdk/private/classes.dart (right):

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
tool/input_sdk/private/classes.dart:241: let symbol = Symbol($subclass.name +
'.' + $fieldName.toString());
On 2016/02/16 17:47:35, Jacob wrote:
> nit: replace
> $subclass.name + '.' + $fieldName.toString()
> 
> with
> '${$subclass.name}.${$fieldName}'
> and similar cleanup elsewhere in the file.

The double $ scares me.  :-)

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
tool/input_sdk/private/classes.dart:291: // TODO(vsm): Not all registered js
types are real.
On 2016/02/16 17:46:38, John Messerly wrote:
> yeah, it'd be really nice to avoid generating a call to this method if we can
:)

Acknowledged.

https://codereview.chromium.org/1700153002/diff/1/tool/input_sdk/private/clas...
tool/input_sdk/private/classes.dart:328: // TODO(vsm): Why?
On 2016/02/16 17:46:38, John Messerly wrote:
> yeah, this definitely needs further investigation. This looks like a compiler
> bug, it asked us to generate "dartx.foo" for "foo", but it forgot to compile
> "foo" in the first place. :)
>  
> What is "type" when this happens? I wonder if it's a native type? On native
> types, it can be tricky to know where the method comes from, e.g.
> HTMLElement/Element have gone through some refactoring over the last few
years,
> with methods moving "up" to Element so they can be shared by SVGElement.
> 
> That would suggest we could let ourself look up the proto chain. However, I
> think we'd only want to do that for "native" types (not their subtypes).

Yes, a native type.  The type is dart:html WrappedEvent, but the method is
actually on Event.  Suggestions?

[vsm, 2016-02-18 15:23:55]

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1700153002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:3038: if (!type.isObject &&
On 2016/02/18 01:13:09, vsm wrote:
> On 2016/02/16 18:22:01, John Messerly wrote:
> > wow, this code is confusing. Can we rewrite it like?
> > 
> > // In these cases, the target could be `null` so we need static dispatch.
> > return type.isObject ||
> >      _isJSBuiltinType(type) ||
> >      _extensionTypes.contains(type.element) ||
> >      _isNullable(target);
> > 
> > 
> > ... now it's clearer, but it seems totally wrong :)
> > 
> > 1. Why check for _isJSBuiltinType and _extensionTypes? That seems redundant.
> > 2. Why do extension types always use static dispatch for object members?
> > 3. Why is type.isObject a special case anyway?
> > 
> > Anyway, for 2, I think it's related to other brokenness in
> > https://github.com/dart-lang/dev_compiler/issues/451...
> > 
> > But yeah, what a mess. Here's what the code should be:
> > 
> >     return _isNullable(target);
> > 
> > Anyway I'm fine with the "super" workaround in the meantime. I guess because
> > extension methods fall into the wrong path, the existing _isNullable check
> > doesn't help.
> 
> Excellent point - cleaned up code!  The explicit super check was just hacking
> around the problem you noted.  :-)

Ugh, had to back off this a little bit.  We don't symbolize hashCode.  E.g., we
compile "hello".hashCode to "hello".hashCode with the intermediate change.  This
puts it back to dart.hashCode("hello").

[vsm, 2016-02-18 19:02:51]

Clarified comments.

PTAL

[Jennifer Messerly, 2016-02-18 21:56:54]

lgtm

[vsm, 2016-02-18 21:58:07]

Description was changed from

==========
Wrapperless dart:html and friends

This reuses the dart2js code.
==========

to

==========
Wrapperless dart:html and friends

This reuses the dart2js code.

R=jacobr@google.com, jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/58e8be41b456ef47f3ab6628cbb1...
==========

[vsm, 2016-02-18 21:58:09]

Committed patchset #4 (id:30001) manually as
58e8be41b456ef47f3ab6628cbb14a45d9be9130 (presubmit successful).