Redo how Type objects are exposed from DDC.

lib/src/compiler/code_generator.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:collection' show HashMap, HashSet;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart' show Token, TokenType;
 import 'package:analyzer/dart/element/element.dart';
@@ skipping 97 matching lines @@
   FunctionBody _currentFunction;
 
   /// Helper class for emitting elements in the proper order to allow
   /// JS to load the module.
   ElementLoader _loader;
 
   BuildUnit _buildUnit;
 
   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,
         _asyncStreamIterator =
             _getLibrary(c, 'dart:async').getType('StreamIterator').type,
         _jsArray = _getLibrary(c, 'dart:_interceptors').getType('JSArray'),
         dartCoreLibrary = _getLibrary(c, 'dart:core'),
         dartJSLibrary = _getLibrary(c, 'dart:js');
 
   LibraryElement get currentLibrary => _loader.currentElement.library;
@@ skipping 83 matching lines @@
 
     // Collect all Element -> Node mappings, in case we need to forward declare
     // any nodes.
     var nodes = new HashMap<Element, AstNode>.identity();
     compilationUnits.map(_collectElements).forEach(nodes.addAll);
     _loader = new ElementLoader(_emitModuleItem, nodes);
 
     // 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.
     // Order will be changed as needed so the resulting code can execute.
     // This is done by forward declaring items.
     compilationUnits.forEach(visitCompilationUnit);
 
     // Declare imports
     _finishImports(items);
 
@@ skipping 97 matching lines @@
     inferNullableTypes(node);
 
     var code = _visit(node);
     if (code != null) _moduleItems.add(code);
   }
 
   @override
   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) {
         _loader.emitDeclaration(element);
       } else {
         declaration.accept(this);
       }
     }
     for (var directive in unit.directives) {
       directive.accept(this);
@@ skipping 1609 matching lines @@
 
     // Get the original declaring element. If we had a property accessor, this
     // indirects back to a (possibly synthetic) field.
     var element = accessor;
     if (accessor is PropertyAccessorElement) element = accessor.variable;
 
     _loader.declareBeforeUse(element);
 
     // 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
     if (element.enclosingElement is CompilationUnitElement) {
       return _emitTopLevelName(element);
     }
 
     var name = element.name;
 
     // Unqualified class member. This could mean implicit-this, or implicit
@@ skipping 126 matching lines @@
 
     // TODO(jmesserly): like constants, should we hoist function types out of
     // methods? Similar issue with generic types. For all of these, we may want
     // to canonicalize them too, at least when inside the same library.
     var name = type.name;
     var element = type.element;
     if (name == '' || name == null || lowerTypedef) {
       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;
 
     if (_isGenericTypeParameter(type)) {
       return js.call('dart.dynamic');
     }
 
     if (type is TypeParameterType) {
       return new JS.Identifier(name);
     }
 
     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) {
         jsArgs = [];
       }
       if (jsArgs != null) {
         var genericName = _emitTopLevelName(element, suffix: '\$');
         return js.call('#(#)', [genericName, jsArgs]);
       }
     }
@@ skipping 1808 matching lines @@
 }
 
 bool isLibraryPrefix(Expression node) =>
     node is SimpleIdentifier && node.staticElement is PrefixElement;
 
 LibraryElement _getLibrary(AnalysisContext c, String uri) =>
     c.computeLibraryElement(c.sourceFactory.forUri(uri));
 
 bool _isDartRuntime(LibraryElement l) =>
     l.isInSdk && l.source.uri.toString() == 'dart:_runtime';