Redo how Type objects are exposed from DDC.

lib/src/compiler/code_generator.dart

Index: lib/src/compiler/code_generator.dart
diff --git a/lib/src/compiler/code_generator.dart b/lib/src/compiler/code_generator.dart
index bfc7dc95e960e65c47c14f4b018e088ee168da51..a874a168fe5b5c8d8d76a3265eb98ec922d590df 100644
--- a/lib/src/compiler/code_generator.dart
+++ b/lib/src/compiler/code_generator.dart
@@ -115,6 +115,12 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   String _buildRoot;
 
+  /// Whether the current compilation unit being compiled is within the Dart
+  /// SDK itself.
+  // TODO(rnystrom): Get rid of this if we can treat class literals inside
+  // the SDK the same way we do outside.
+  bool _compilingSdkLibrary = false;

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

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]

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
:)

+
   CodeGenerator(AnalysisContext c, this.options, this._extensionTypes)
       : context = c,
         types = c.typeProvider,
@@ -218,7 +224,6 @@ class CodeGenerator extends GeneralizingAstVisitor
     // Add implicit dart:core dependency so it is first.
     emitLibraryName(dartCoreLibrary);
 
-    //
     // Visit each compilation unit and emit its code.
     //
     // NOTE: declarations are not necessarily emitted in this order.
@@ -336,6 +341,8 @@ class CodeGenerator extends GeneralizingAstVisitor
   void visitCompilationUnit(CompilationUnit unit) {
     _constField = new ConstFieldVisitor(types, unit.element.source);
 
+    _compilingSdkLibrary = unit.element.source.uri.scheme == "dart";

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

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.)

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

Done.

+
     for (var declaration in unit.declarations) {
       var element = declaration.element;
       if (element != null) {
@@ -1965,7 +1972,25 @@ class CodeGenerator extends GeneralizingAstVisitor
 
     // type literal
     if (element is TypeDefiningElement) {
-      return _emitTypeName(fillDynamicTypeArgs(element.type));
+      // TODO(rnystrom): Treating core libraries specially is lame. The JS()
+      // code templates often refer to core lib classes and expect that to emit
+      // a reference directly to the JS class (i.e. its constructor), not a
+      // Type object. Figure out how to handle these better.
+      if (_compilingSdkLibrary ||

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

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)

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

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.

+          node.parent is MethodInvocation ||

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

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 :)

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

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.

+          node.parent is PrefixedIdentifier ||
+          node.parent is PropertyAccess) {
+        // A static member call. Emit the class itself.
+        return _emitTypeName(fillDynamicTypeArgs(element.type));
+      } else {
+        // A type literal expression. Emit the "Type" object that corresponds
+        // to that type.
+        var dynType = fillDynamicTypeArgs(element.type);
+        var isGeneric = dynType is ParameterizedType &&
+            dynType.typeArguments.isNotEmpty;
+        var genericInst = _emitTypeName(dynType, lowerGeneric: isGeneric);

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

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. :)

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

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.
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.

+        return js.call('dart.wrapType(#)', genericInst);
+      }
     }
 
     // library member
@@ -2112,7 +2137,8 @@ class CodeGenerator extends GeneralizingAstVisitor
       var parts = _emitFunctionTypeParts(type as FunctionType);
       return js.call('dart.functionType(#)', [parts]);
     }
-    // For now, reify generic method parameters as dynamic
+
+    // For now, reify generic method parameters as dynamic.
     bool _isGenericTypeParameter(DartType type) =>
         type is TypeParameterType &&
         type.element.enclosingElement is! TypeDefiningElement;
@@ -2127,7 +2153,7 @@ class CodeGenerator extends GeneralizingAstVisitor
 
     if (type is ParameterizedType) {
       var args = type.typeArguments;
-      Iterable jsArgs = null;
+      Iterable jsArgs;
       if (args.any((a) => !a.isDynamic && !_isGenericTypeParameter(a))) {
         jsArgs = args.map(_emitTypeName);
       } else if (lowerGeneric) {