Support lazy JS types.

pkg/dev_compiler/lib/src/compiler/code_generator.dart

Index: pkg/dev_compiler/lib/src/compiler/code_generator.dart
diff --git a/pkg/dev_compiler/lib/src/compiler/code_generator.dart b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
index b7f3d5a048c61f3eb83779eb8979cf64a025c04e..9073fd8c4e44b0dbe24626f9aabc3451e2aba334 100644
--- a/pkg/dev_compiler/lib/src/compiler/code_generator.dart
+++ b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
@@ -308,7 +308,8 @@ class CodeGenerator extends GeneralizingAstVisitor
   }
 
   List<String> _getJSName(Element e) {
-    if (findAnnotation(e.library, isPublicJSAnnotation) == null) {
+    if (e.library == null ||

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

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.

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

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.

+        findAnnotation(e.library, isPublicJSAnnotation) == null) {
       return null;
     }
 
@@ -322,6 +323,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     if (findAnnotation(e, isPublicJSAnnotation) != null) {
       elementJSName = getAnnotationName(e, isPublicJSAnnotation) ?? '';
     }
+
     if (e is TopLevelVariableElement &&
         e.getter != null &&
         (e.getter.isExternal ||
@@ -2705,11 +2707,8 @@ class CodeGenerator extends GeneralizingAstVisitor
       var type = element.enclosingElement.type;
       var member = _emitMemberName(name, isStatic: isStatic, type: type);
 
-      // For static methods, we add the raw type name, without generics or
-      // library prefix. We don't need those because static calls can't use
-      // the generic type.
       if (isStatic) {
-        var dynType = _emitType(fillDynamicTypeArgs(type));
+        var dynType = _emitStaticAccess(type);
         return new JS.PropertyAccess(dynType, member);
       }
 
@@ -2908,6 +2907,45 @@ class CodeGenerator extends GeneralizingAstVisitor
     return typeParts;
   }
 
+  /// Emits an expression that lets you access statics on a [[type]] from code.

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

comment nit: I don't think you want two brackets, [type] should be enough

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

[[[done]]]

+  ///
+  /// If [nameType] is true, then the type will be named.  In addition,
+  /// if [hoistType] is true, then the named type will be hoisted.
+  JS.Expression _emitConstructorAccess(DartType type,

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

just wanted to say, this is factored in a lovely way :)

_emitStaticAccess and _emitConstructorAccess read so nicely!

[Jacob, 2016/11/22 18:51:36]

yay!

+      {bool nameType: true, bool hoistType: true}) {
+    var interop = _emitJSInterop(type.element);

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

this could be:


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

not sure if better but something to consider :)

[Jacob, 2016/11/22 18:51:36]

Done.

+    if (interop != null) return interop;
+
+    // TODO(jacobr): we could consider duplicating logic instead of calling

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

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

[Jacob, 2016/11/22 18:51:36]

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.

+    // through to the somewhat unrelated _emitType mode. Calling _emitType here
+    // is acceptable as the type cannot be a JS interop type.
+    return _emitType(type, nameType: nameType, hoistType: hoistType);
+  }
+
+  /// Emits an expression that lets you access statics on a [[type]] from code.

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

comment nit again: [type] instead of [[type]]

[Jacob, 2016/11/22 18:51:36]

Done.

+  JS.Expression _emitStaticAccess(DartType type) {
+    // Make sure we aren't attempting to emit a static access path to a type
+    // that does not have a valid static access path.
+    assert(!type.isVoid &&
+        !type.isDynamic &&
+        !type.isBottom &&
+        type is! TypeParameterType);
+
+    // For statics, we add the raw type name, without generics or
+    // library prefix. We don't need those because static calls can't use
+    // the generic type.
+    type = fillDynamicTypeArgs(type);
+    var element = type.element;
+    _declareBeforeUse(element);
+
+    var interop = _emitJSInterop(element);
+    if (interop != null) return interop;
+
+    assert(type.name != '' && type.name != null);
+
+    return _emitTopLevelNameNoInterop(element);
+  }
+
   /// Emits a Dart [type] into code.
   ///
   /// If [lowerTypedef] is set, a typedef will be expanded as if it were a
@@ -2937,13 +2975,29 @@ class CodeGenerator extends GeneralizingAstVisitor
       return _callHelper('bottom');
     }
 
-    _declareBeforeUse(type.element);
+    var element = type.element;
+    _declareBeforeUse(element);
+
+    var interop = _emitJSInterop(element);
+    // Type parameters don't matter as JS interop types cannot be reified.

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

should we error out if the type is generic?

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

[Jacob, 2016/11/22 18:51:36]

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.

+    // We have to use lazy JS types because until we have proper module

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

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.

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

Done.

+    // loading for JS libraries bundled with Dart libraries, we will sometimes
+    // need to load Dart libraries before the corresponding JS libraries are
+    // actually loaded.
+    if (interop != null) {
+      if (_isObjectLiteral(element)) {
+        return _callHelper(
+            'lazyAnonymousJSType(#)', js.string(element.displayName));
+      } else {
+        return _callHelper('lazyJSType(() => #, #)',
+            [interop, js.string(_getJSName(element).join('.'))]);
+      }
+    }
 
     // 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) {
       // TODO(jmesserly): should we change how typedefs work? They currently
       // go through use similar logic as generic classes. This makes them
@@ -2957,9 +3011,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       return new JS.Identifier(name);
     }
 
-    if (type == subClass?.type) {
-      return className;
-    }
+    if (type == subClass?.type) return className;

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

nice!

[Jacob, 2016/11/22 18:51:36]

Acknowledged.

 
     if (type is ParameterizedType) {
       var args = type.typeArguments;
@@ -2982,12 +3034,16 @@ class CodeGenerator extends GeneralizingAstVisitor
       }
     }
 
-    return _emitTopLevelName(element);
+    return _emitTopLevelNameNoInterop(element);
   }
 
   JS.PropertyAccess _emitTopLevelName(Element e, {String suffix: ''}) {
     var interop = _emitJSInterop(e);
     if (interop != null) return interop;
+    return _emitTopLevelNameNoInterop(e, suffix: suffix);
+  }
+
+  JS.PropertyAccess _emitTopLevelNameNoInterop(Element e, {String suffix: ''}) {
     String name = getJSExportName(e) + suffix;
     return new JS.PropertyAccess(
         emitLibraryName(e.library), _propertyName(name));
@@ -3211,7 +3267,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     // the generic type.
     ClassElement classElement = element.enclosingElement;
     var type = classElement.type;
-    var dynType = _emitType(fillDynamicTypeArgs(type));
+    var dynType = _emitStaticAccess(type);
     var member = _emitMemberName(element.name, isStatic: true, type: type);
     return _visit(rhs).toAssignExpression(
         annotate(new JS.PropertyAccess(dynType, member), lhs));
@@ -3291,6 +3347,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       return _emitNullSafe(node);
     }
 
+    // SSS

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

remove this?

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

Done.

     var result = _emitForeignJS(node);
     if (result != null) return result;
 
@@ -3362,6 +3419,17 @@ class CodeGenerator extends GeneralizingAstVisitor
     return helperMethodName;
   }
 
+  JS.Expression _emitTarget(Expression target, Element element, bool isStatic) {
+    if (isStatic) {
+      if (element is ConstructorElement)
+        return _emitConstructorAccess(element.enclosingElement.type);

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

style nit: should have curly braces if it's more than one line

[Jacob, 2016/11/22 18:51:36]

Done.

+      if (element is ExecutableElement)
+        return _emitStaticAccess(

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

same here

[Jacob, 2016/11/22 18:51:36]

Done.

+            (element.enclosingElement as ClassElement).type);
+    }
+    return _visit(target);
+  }
+
   /// Emits a (possibly generic) instance method call.
   JS.Expression _emitMethodCallInternal(
       Expression target,
@@ -3374,7 +3442,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     bool isStatic = element is ExecutableElement && element.isStatic;
     var memberName = _emitMemberName(name, type: type, isStatic: isStatic);
 
-    JS.Expression jsTarget = _visit(target);
+    JS.Expression jsTarget = _emitTarget(target, element, isStatic);
     if (isDynamicInvoke(target) || isDynamicInvoke(node.methodName)) {
       if (_inWhitelistCode(target)) {
         var vars = <JS.MetaLetVariable, JS.Expression>{};
@@ -3899,7 +3967,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     var classElem = element.enclosingElement;
     var interop = _emitJSInterop(classElem);
     if (interop != null) return interop;
-    var typeName = _emitType(type);
+    var typeName = _emitConstructorAccess(type);
     if (name != null || element.isFactory) {
       var namedCtor = _constructorName(element);
       return new JS.PropertyAccess(typeName, namedCtor);
@@ -3925,7 +3993,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       if (element == null) {
         // TODO(jmesserly): this only happens if we had a static error.
         // Should we generate a throw instead?
-        ctor = _emitType(type,
+        ctor = _emitConstructorAccess(type,
             nameType: options.hoistInstanceCreation,
             hoistType: options.hoistInstanceCreation);
         if (name != null) {
@@ -4749,7 +4817,7 @@ class CodeGenerator extends GeneralizingAstVisitor
           [_emitDynamicOperationName('dload'), _visit(target), name]);
     }
 
-    var jsTarget = _visit(target);
+    var jsTarget = _emitTarget(target, member, isStatic);
     bool isSuper = jsTarget is JS.Super;
 
     if (isSuper && member is FieldElement && !member.isSynthetic) {
@@ -5123,7 +5191,8 @@ class CodeGenerator extends GeneralizingAstVisitor
       // TODO(vsm): When we canonicalize, we need to treat private symbols
       // correctly.
       var name = js.string(node.components.join('.'), "'");
-      return js.call('#.new(#)', [_emitType(types.symbolType), name]);
+      return js
+          .call('#.new(#)', [_emitConstructorAccess(types.symbolType), name]);
     }
 
     return _emitConst(emitSymbol);