Support lazy JS types.

Support lazy JS types.

BUG=
R=jmesserly@google.com

Committed: https://github.com/dart-lang/sdk/commit/1b339deab230a39d0bcb56e28fd8c6915e1c62a0

[Jacob, 2016-11-22 03:17:24]

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

[Jacob, 2016-11-22 03:17:24]

Need to do one final pass of polish but sending out now for early review to make
sure I am on the right track.
All tests pass except for the one commented out case which perhaps we shouldn't
support as it won't work in Dartium anyway.

[Jacob, 2016-11-22 16:26:22]

https://codereview.chromium.org/2515353003/diff/40001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (right):

https://codereview.chromium.org/2515353003/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:91: return
JS('bool', 'dart.as(#, #)', o, t._rawJSType);
I think this is a bit too strict. In reality JS types can duck type.
A reasonable option would be to log a warning in this case but still allow any
JS type.

[Jacob, 2016-11-22 17:15:37]

This code is now ready for review.

[Jennifer Messerly, 2016-11-22 18:18:19]

a few minor comments, otherwise LGTM!!! <3 the design here.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:311: if (e.library == null
||
nice fix. I'm curious, do you know how this happens? I would expect every
element to belong to a library, even in erroneous code.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2910: /// Emits an
expression that lets you access statics on a [[type]] from code.
comment nit: I don't think you want two brackets, [type] should be enough

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2914: JS.Expression
_emitConstructorAccess(DartType type,
just wanted to say, this is factored in a lovely way :)

_emitStaticAccess and _emitConstructorAccess read so nicely!

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2916: var interop =
_emitJSInterop(type.element);
this could be:


return _emitJSInterop(type.element) ??
    _emitType(type, nameType: nameType, hoistType: hoistType);

not sure if better but something to consider :)

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2919: // TODO(jacobr): we
could consider duplicating logic instead of calling
personally I'm not sure we need this TODO ... we can always split out the logic
when an actual divergence happens, until then it's nice to reuse as much code as
possible. Less copies == goodness :)

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2925: /// Emits an
expression that lets you access statics on a [[type]] from code.
comment nit again: [type] instead of [[type]]

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2982: // Type parameters
don't matter as JS interop types cannot be reified.
should we error out if the type is generic?

I'm guessing it won't be, because that wouldn't work.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2983: // We have to use
lazy JS types because until we have proper module
This comment is good, but maybe a code example would help? I'm thinking of a way
to make it more clear to someone new to the compiler codebase. Perhaps like
this:

// Given a JS type such as:
//     @JS('MyType')
//     class MyType { ... }
// We can't emit a reference to MyType because the JS file that defines it
// may be loaded after our code, and we can't import the JS module yet.
// So for now, we use a special lazy type object to represent MyType.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3014: if (type ==
subClass?.type) return className;
nice!

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3350: // SSS
remove this?

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3425: return
_emitConstructorAccess(element.enclosingElement.type);
style nit: should have curly braces if it's more than one line

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3427: return
_emitStaticAccess(
same here

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (right):

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:80:
_lazyJSType_is(o, LazyJSType t) {
style guide naming suggestion:

_isInstanceOfLazyJSType

(also matches how we name these things in Analyzer's strong type system class)

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:89:
_lazyJSType_as(o, LazyJSType t) {
same here:

_castToLazyJSType

(or _asInstanceOfLazyJSType)

[Jacob, 2016-11-22 18:51:37]

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:311: if (e.library == null
||
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> nice fix. I'm curious, do you know how this happens? I would expect every
> element to belong to a library, even in erroneous code.

I suspect one of dynamic or bottom doesn't show up as a specific library given
I'm now checking for interop before checking for those types.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2910: /// Emits an
expression that lets you access statics on a [[type]] from code.
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> comment nit: I don't think you want two brackets, [type] should be enough

[[[done]]]

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2914: JS.Expression
_emitConstructorAccess(DartType type,
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> just wanted to say, this is factored in a lovely way :)
> 
> _emitStaticAccess and _emitConstructorAccess read so nicely!

yay!

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2916: var interop =
_emitJSInterop(type.element);
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> this could be:
> 
> 
> return _emitJSInterop(type.element) ??
>     _emitType(type, nameType: nameType, hoistType: hoistType);
> 
> not sure if better but something to consider :)

Done.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2919: // TODO(jacobr): we
could consider duplicating logic instead of calling
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> personally I'm not sure we need this TODO ... we can always split out the
logic
> when an actual divergence happens, until then it's nice to reuse as much code
as
> possible. Less copies == goodness :)

sounds good. My only concern is that this is only alright because certain code
paths inside _emitType are never hit (the JS interop ones) which seems like a
code smell.
Perhaps slightly cleaner and a tiny optimization  would be to add an 
_emitTypeNoInterop helper that doesn't perform the interop checks.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2925: /// Emits an
expression that lets you access statics on a [[type]] from code.
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> comment nit again: [type] instead of [[type]]

Done.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2982: // Type parameters
don't matter as JS interop types cannot be reified.
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> should we error out if the type is generic?
> 
> I'm guessing it won't be, because that wouldn't work.

Would really like to allow it assuming JS interop generics are allowed but are
unreified. For example:

@JS()
class Foo<T> {
  external T get foo;
}

A type check for Foo<num> will with this code just become a type check for Foo
which is what you want as generics on JS types are not reified.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:2983: // We have to use
lazy JS types because until we have proper module
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> This comment is good, but maybe a code example would help? I'm thinking of a
way
> to make it more clear to someone new to the compiler codebase. Perhaps like
> this:
> 
> // Given a JS type such as:
> //     @JS('MyType')
> //     class MyType { ... }
> // We can't emit a reference to MyType because the JS file that defines it
> // may be loaded after our code, and we can't import the JS module yet.
> // So for now, we use a special lazy type object to represent MyType.

Done.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3014: if (type ==
subClass?.type) return className;
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> nice!

Acknowledged.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3350: // SSS
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> remove this?

Done.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3425: return
_emitConstructorAccess(element.enclosingElement.type);
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> style nit: should have curly braces if it's more than one line

Done.

https://codereview.chromium.org/2515353003/diff/60001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3427: return
_emitStaticAccess(
On 2016/11/22 18:18:19, Jennifer Messerly wrote:
> same here

Done.

[Jacob, 2016-11-22 18:55:42]

Description was changed from

==========
Support lazy JS types.

BUG=
==========

to

==========
Support lazy JS types.

BUG=
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1b339deab230a39d0bcb56e28fd8c6915e1c62a0
==========

[Jacob, 2016-11-22 18:55:44]

Committed patchset #5 (id:80001) manually as
1b339deab230a39d0bcb56e28fd8c6915e1c62a0 (presubmit successful).