Redo how Type objects are exposed from DDC.

Redo how Type objects are exposed from DDC.

- Instead of using the raw runtime type that DDC uses for its type
  checks, use a WrappedType that correctly implements Type's interface.
- Compile class literals to wrap the type in a WrappedType.
- Make Object.runtimeType() do the same thing.

Fixes #488. Fixes #511.

R=jmesserly@google.com

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

[Bob Nystrom, 2016-05-02 22:17:48]

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

[Bob Nystrom, 2016-05-02 22:19:09]

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:122: bool _compilingSdkLibrary = false;
This is kind of nasty, but it was the most expedient solution I could come up
with. The JS patch files in DDC's SDK assume that class literals are compiled
directly to references to the underlying type object, and not the
Type-implementing wrapper. So, to preserve that behavior, this just treats
"dart:" libraries as special.

Let me know if you have a better idea for this.

[Jennifer Messerly, 2016-05-02 23:04:20]

Wow this is really nice! Like the wrapType idea. A few small comments but this
is really close

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:122: bool _compilingSdkLibrary = false;
On 2016/05/02 22:19:09, Bob Nystrom wrote:
> This is kind of nasty, but it was the most expedient solution I could come up
> with. The JS patch files in DDC's SDK assume that class literals are compiled
> directly to references to the underlying type object, and not the
> Type-implementing wrapper. So, to preserve that behavior, this just treats
> "dart:" libraries as special.
> 
> Let me know if you have a better idea for this.

We already have another way to get this info, explained more below. However you
may want to repurpose this flag "_compilingInlineJS" based on my comment below
:)

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:344: _compilingSdkLibrary =
unit.element.source.uri.scheme == "dart";
This won't work because we don't necessarily visit things in order. It probably
happens to work now because we compile all dart:* URIs together, and always
exclusive from other code. So it's really a global flag. But you don't need this
line. Suggestion below.

(and FWIW, ConstFieldVisitor above is a bit sketchy for the same reason, but
it's "relatively harmless" because that "source" is unused. It's for printing
error messages in constants, which code should not have by the time it gets
here, unless folks are abusing --unsafe-force-compile flag.)

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:1979: if (_compilingSdkLibrary ||
Here's how this can be done better:

See where we call _isInlineJSFunction. Then the flag can be moved into there.
Set a flag, then clear it. Now the fix is perfectly targeted at the right place:
only JS macros :)


... if for some reason that doesn't work ... and also some FYI's regarding SDK
libs:

You can check for "dart:runtime" using `_isDartRuntime(currentLibrary)`. That
probably would be enough. That one is the "bootstrapping" code under
tool/input_sdk/private/ddc_runtime.

You can also use `currentLibrary.source.isInSystemLibrary` if you ever want to
catch all "dart:" libs.


(Aside: avoid Analyzer API `currentLibrary.isInSdk`. That's a trap: any library
whose name starts with "dart." will be treated as if it's in the SDK. So
`library dart.foo.bar` is in the SDK. Badness. That is why we always use
source.isInSystemLibrary. isInSdk is okay as a quick check too, but only if you
also validate the URI, see how _isDartRuntime works)

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:1980: node.parent is MethodInvocation ||
FYI: at some point I'd love to ditch parent pointers and instead we can be more
careful in the calling code (_emitMethodCall, _emitAccess)... that matches a
more top-down traversal. But that said I think this is okay. If we're paying for
parent pointers, might as well get some benefit :)

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:1991: var genericInst =
_emitTypeName(dynType, lowerGeneric: isGeneric);
you should never need to pass `lowerGeneric` explicitly. This is handled
automatically by emitTypeName. It is only used when we are defining the "all
dynamic" convenience variable:

`let Foo = Foo$()`;

We need "lowerGeneric" so we don't get `let Foo = Foo;`  =)

If you wanted to force `Foo$()` for some reason over `Foo` (which you shouldn't
need to do), then `lowerGeneric: true` would do it. It won't kick in if the type
isn't generic, so no worries there. :)

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
File tool/input_sdk/private/ddc_runtime/rtti.dart (right):

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/rtti.dart:105: // Delegate to the actual
method on the object.
it might be worth a comment/TODO here, this issue has background:
https://github.com/dart-lang/dev_compiler/issues/451

If I run `cd tool/input_sdk; git grep "get runtimeType"` there are a bunch of
hits in the SDK, mostly in native_typed_data ... those won't work. So if we've
got an extension type, we need to call into the extension method.

This issue affects all Object members (toString, nSM, hashCode, runtimeType) not
just runtimeType.

But might be worth leaving a note, because otherwise the code looks wrong (well,
it is, but the problem already existed)

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/rtti.dart:116: objectRuntimeType(obj) =>
JS('', '''(() => {
Hey so I've been trying to clean these up. You can write this as just:

    objectRuntimeType(obj) => wrapType(getReifiedType(obj));

Not all Dart code works in this magic library, because we haven't bootstrapped
the runtime by the point it executes, but I'm trying to bring sanity where I can
:)

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/rtti.dart:125: getReifiedType(obj) => JS('',
'''(() => {
if you feel like it -- cleanup idea :)

getReifiedType(obj) {
  var result = checkPrimitiveType(obj);
  if (result != null) return result;
  return _nonPrimitiveRuntimeType(obj);
}

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/rtti.dart:156: wrapType(type) => JS('',
'''(() => {
nowadays we'd write this more like:

wrapType(type) {
  // If we've already wrapped this type once, use the previous wrapper. This
  // way, multiple references to the same type return an identical Type.
  if (JS('bool', '#.hasOwnProperty(#)', type, _typeObject)) {
    return JS('', '#[#]', type, _typeObject);
  }
  return JS('', '#[#] = new #(#)', type, _typeObject, WrappedType, type);
}


Same code, but implemented more consistently with our dart2js inherited SDK.
It's kind of like inline ASM. Also matches how code_generator.dart looks.

[Bob Nystrom, 2016-05-03 20:50:47]

Thanks for the review!

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:344: _compilingSdkLibrary =
unit.element.source.uri.scheme == "dart";
On 2016/05/02 23:04:20, John Messerly wrote:
> This won't work because we don't necessarily visit things in order. It
probably
> happens to work now because we compile all dart:* URIs together, and always
> exclusive from other code. So it's really a global flag. But you don't need
this
> line. Suggestion below.
> 
> (and FWIW, ConstFieldVisitor above is a bit sketchy for the same reason, but
> it's "relatively harmless" because that "source" is unused. It's for printing
> error messages in constants, which code should not have by the time it gets
> here, unless folks are abusing --unsafe-force-compile flag.)

Done.

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:1979: if (_compilingSdkLibrary ||
On 2016/05/02 23:04:20, John Messerly wrote:
> Here's how this can be done better:
> 
> See where we call _isInlineJSFunction. Then the flag can be moved into there.
> Set a flag, then clear it. Now the fix is perfectly targeted at the right
place:
> only JS macros :)
> 
> 
> ... if for some reason that doesn't work ... and also some FYI's regarding SDK
> libs:
> 
> You can check for "dart:runtime" using `_isDartRuntime(currentLibrary)`. That
> probably would be enough. That one is the "bootstrapping" code under
> tool/input_sdk/private/ddc_runtime.
> 
> You can also use `currentLibrary.source.isInSystemLibrary` if you ever want to
> catch all "dart:" libs.
> 
> 
> (Aside: avoid Analyzer API `currentLibrary.isInSdk`. That's a trap: any
library
> whose name starts with "dart." will be treated as if it's in the SDK. So
> `library dart.foo.bar` is in the SDK. Badness. That is why we always use
> source.isInSystemLibrary. isInSdk is okay as a quick check too, but only if
you
> also validate the URI, see how _isDartRuntime works)
> 

Took some doing, but done. Scoping it to just within JS macros is definitely the
right approach. Doing that brought up a couple of issues where I was not
wrapping things that should have been wrapped. There are a few places where the
core libraries *do* intend to produce a Type.

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:1980: node.parent is MethodInvocation ||
On 2016/05/02 23:04:20, John Messerly wrote:
> FYI: at some point I'd love to ditch parent pointers and instead we can be
more
> careful in the calling code (_emitMethodCall, _emitAccess)... that matches a
> more top-down traversal. But that said I think this is okay. If we're paying
for
> parent pointers, might as well get some benefit :)

Yeah, this is gross. Doing something more explicitly top down would be better.

Part of this reflects the fact that Dart's behavior is strange. It really is
that a type name has different semantics based on the context where it appears.
If we had, say, something like metaclasses, we would need to distinguish class
literals from other uses of classes, and then all of this wrapping business
would go away.

https://codereview.chromium.org/1944483002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:1991: var genericInst =
_emitTypeName(dynType, lowerGeneric: isGeneric);
On 2016/05/02 23:04:20, John Messerly wrote:
> you should never need to pass `lowerGeneric` explicitly. This is handled
> automatically by emitTypeName. It is only used when we are defining the "all
> dynamic" convenience variable:
> 
> `let Foo = Foo$()`;
> 
> We need "lowerGeneric" so we don't get `let Foo = Foo;`  =)

Done. I think at first I wasn't filling in the dynamic type args which meant it
did the wrong thing if I didn't pass lowerGeneric.

> 
> If you wanted to force `Foo$()` for some reason over `Foo` (which you
shouldn't
> need to do), then `lowerGeneric: true` would do it. It won't kick in if the
type
> isn't generic, so no worries there. :)

It does, actually, I think. That's why I had to do the annoying isGeneric check
here. The "is ParameterizedType" test passes even if it happens to be an
interface with no type parameters, so you end up getting `Foo$()` if Foo isn't
generic at all instead of Foo.

The other places where lowerGeneric is passed are carefully to only call it when
it knows the type is generic.

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
File tool/input_sdk/private/ddc_runtime/rtti.dart (right):

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/rtti.dart:116: objectRuntimeType(obj) =>
JS('', '''(() => {
On 2016/05/02 23:04:20, John Messerly wrote:
> Hey so I've been trying to clean these up. You can write this as just:
> 
>     objectRuntimeType(obj) => wrapType(getReifiedType(obj));
> 
> Not all Dart code works in this magic library, because we haven't bootstrapped
> the runtime by the point it executes, but I'm trying to bring sanity where I
can
> :)

Done.

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/rtti.dart:125: getReifiedType(obj) => JS('',
'''(() => {
On 2016/05/02 23:04:20, John Messerly wrote:
> if you feel like it -- cleanup idea :)
> 
> getReifiedType(obj) {
>   var result = checkPrimitiveType(obj);
>   if (result != null) return result;
>   return _nonPrimitiveRuntimeType(obj);
> }

Done.

https://codereview.chromium.org/1944483002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/rtti.dart:156: wrapType(type) => JS('',
'''(() => {
On 2016/05/02 23:04:20, John Messerly wrote:
> nowadays we'd write this more like:
> 
> wrapType(type) {
>   // If we've already wrapped this type once, use the previous wrapper. This
>   // way, multiple references to the same type return an identical Type.
>   if (JS('bool', '#.hasOwnProperty(#)', type, _typeObject)) {
>     return JS('', '#[#]', type, _typeObject);
>   }
>   return JS('', '#[#] = new #(#)', type, _typeObject, WrappedType, type);
> }
> 
> 
> Same code, but implemented more consistently with our dart2js inherited SDK.
> It's kind of like inline ASM. Also matches how code_generator.dart looks.

Done.

[Jennifer Messerly, 2016-05-03 20:59:54]

a few tiny nits otherwise LGTM!!!

https://codereview.chromium.org/1944483002/diff/20001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1944483002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:2022: // A type literal expression in use.
Wrap the runtime type in a "Type" object.
long line.

https://codereview.chromium.org/1944483002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:2496: // HTML library where an argument to
JS() is itself a JS() call. If those
haha, wow. The crazy stuff you find sometimes :)

https://codereview.chromium.org/1944483002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:2499: try {
nit: try/finally is not needed. We will never try and reuse this visitor
instance if something goes wrong and throws (that means an internal compiler
error). So handling an exception is not required.

[Bob Nystrom, 2016-05-03 21:24:11]

Description was changed from

==========
Redo how Type objects are exposed from DDC.

- Instead of using the raw runtime type that DDC uses for its type
  checks, use a WrappedType that correctly implements Type's interface.
- Compile class literals to wrap the type in a WrappedType.
- Make Object.runtimeType() do the same thing.

Fixes #488. Fixes #511.
==========

to

==========
Redo how Type objects are exposed from DDC.

- Instead of using the raw runtime type that DDC uses for its type
  checks, use a WrappedType that correctly implements Type's interface.
- Compile class literals to wrap the type in a WrappedType.
- Make Object.runtimeType() do the same thing.

Fixes #488. Fixes #511.

R=jmesserly@google.com

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

[Bob Nystrom, 2016-05-03 21:24:13]

Committed patchset #3 (id:40001) manually as
d8cce189dcdfa51491830c8d11b2f9baa4853bb9 (presubmit successful).

[Bob Nystrom, 2016-05-03 21:24:32]

https://codereview.chromium.org/1944483002/diff/20001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1944483002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:2022: // A type literal expression in use.
Wrap the runtime type in a "Type" object.
On 2016/05/03 20:59:54, John Messerly wrote:
> long line.

Oops. Meant to delete that comment (I moved it outside of the if()). Fixed.

https://codereview.chromium.org/1944483002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:2496: // HTML library where an argument to
JS() is itself a JS() call. If those
On 2016/05/03 20:59:54, John Messerly wrote:
> haha, wow. The crazy stuff you find sometimes :)

Acknowledged.

https://codereview.chromium.org/1944483002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:2499: try {
On 2016/05/03 20:59:54, John Messerly wrote:
> nit: try/finally is not needed. We will never try and reuse this visitor
> instance if something goes wrong and throws (that means an internal compiler
> error). So handling an exception is not required.

Done.