implement top-level JS annotated getters

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 'dart:math' show min, max;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart' show Token, TokenType;
@@ skipping 63 matching lines @@
   /// Usually a [SimpleIdentifier], but it can also be other expressions
   /// that are safe to evaluate multiple times, such as `this`.
   Expression _cascadeTarget;
 
   /// The variable for the current catch clause
   SimpleIdentifier _catchParameter;
 
   /// In an async* function, this represents the stream controller parameter.
   JS.TemporaryId _asyncStarController;
 
   /// The top-level reference to 'self' if this is a library tagged with @JS()

[Jennifer Messerly, 2016/05/18 22:20:26]

reword this comment?

[Harry Terkelsen, 2016/05/19 16:56:18]

Done.

-  JS.TemporaryId _self;
+  JS.Expression _jsPrefix;
+  bool libraryTaggedJS = false;

[Jennifer Messerly, 2016/05/18 22:20:27]

Perhaps call this "_isInteropLibrary"

Note: I have a comment below that might require removing this field

[Harry Terkelsen, 2016/05/19 16:56:18]

Done.

 
   final _privateNames =
       new HashMap<LibraryElement, HashMap<String, JS.TemporaryId>>();
   final _initializingFormalTemps =
       new HashMap<ParameterElement, JS.TemporaryId>();
 
   final _dartxVar = new JS.Identifier('dartx');
+  final _dartGlobal = js.call('dart.global');
   final _runtimeLibVar = new JS.Identifier('dart');
   final namedArgumentTemp = new JS.TemporaryId('opts');
 
   final _hasDeferredSupertype = new HashSet<ClassElement>();
 
   final _eagerTopLevelFields = new HashSet<Element>.identity();
 
   /// The  type provider from the current Analysis [context].
   final TypeProvider types;
 
@@ skipping 116 matching lines @@
           js.call('const # = Object.create(null)', [libraryTemp])));
 
       // dart:_runtime has a magic module that holds extenstion method symbols.
       // TODO(jmesserly): find a cleaner design for this.
       if (_isDartRuntime(library)) {
         items.add(new JS.ExportDeclaration(
             js.call('const # = Object.create(null)', [_dartxVar])));
       }
 
       if (findAnnotation(library, isPublicJSAnnotation) != null) {
-        _self = new JS.TemporaryId('self');
-        items.add(js.statement('const # = window;', [_self]));
+        libraryTaggedJS = true;

[Jennifer Messerly, 2016/05/18 22:20:27]

is this attribute per-library or is it global for the whole module?

So one important thing about DDC's CodeGenerator class, it doesn't necessarily
generating things in order. If you're compiling a library cycle, and you have
classes that depend on each other, it might need to generate things out of
order.

so it's not really safe to cache per-library state in this method. That's the
reason "currentLibrary" is used often.

What you could do is have a `HashMap<LibraryElement, String>` to track this
property and jsPrefix. But, I have a better suggestion below in
_emitTopLevelName.

[Harry Terkelsen, 2016/05/19 16:56:18]

It's per-library. I made the map.

+        var prefix = getAnnotationName(library, isPublicJSAnnotation);
+        if (prefix != null && !prefix.isEmpty) {

[Jennifer Messerly, 2016/05/18 22:20:27]

will we always have a name? in other words does it make sense to @JS() on the
library without a name?

[Harry Terkelsen, 2016/05/19 16:56:19]

A @JS annotation on the library is required for js interop

+          _jsPrefix = js.call(prefix);

[Jennifer Messerly, 2016/05/18 22:20:27]

I feel like we'd be better off validating our user-controlled input ("prefix")
before calling `js.call`. Our JS parser is an internal API, it can throw
exceptions for invalid input. We do call it directly from the SDK inline-JS but
that's restricted to only dart:* libraries.

As a simplification, can we just split on "." and store the list of strings?

Actually, since these are all properties off the global object, you don't even
need to validate that they are legal identifiers. Any string will work in JS.
We'll automagically quote it if necessary.

(if you did have to validate, it'd be good to unify with `toJSIdentifier` in
module_builder. Note, that function looks too conservative to me, it might be
worth redoing to match ES2015 spec. I'm fairly sure ES allows unicode
identifiers:
http://www.ecma-international.org/ecma-262/6.0/#sec-identifier-names)

[Harry Terkelsen, 2016/05/19 16:56:18]

I am just splitting on dots now without validation.

+          assert(_isValidJSName(_jsPrefix));

[Jennifer Messerly, 2016/05/18 22:20:26]

Should this be an error we issue?

I think of "assert" as for invariants that are under our control -- e.g. an
invariant in the compiler, or something Analyzer should guarantee, and they
should never happen. If they do it means we have a bug in our compiler. But if
the error is in user's input code, then we should issue a message and abort the
compile. I think this is because they control the JS builtin string.

For example this is how we deal with an import directive that is not part of the
inputs.

There isn't any infrastructure to help yet, because generally we try and issue
this sort of error from Analyzer, that way they show up in IDE/Analyzer CLI,
which is more user friendly.

That is a bit harder though. Maybe a simple approach we could do now is to
collect a list of AnalysisErrors as we compile inside CodeGenerator, then if we
have any, return a JSModuleFile.invalid from CodeGenerator.compile. Maybe like
ModuleCompiler does this for "missingPartErrorCode" and "unusedPartWarningCode".

[Harry Terkelsen, 2016/05/19 16:56:18]

I think you cannot just put arbitrary strings in the @JS annotation but I don't
think there's any documentation. All of the examples I've seen use dotted
identifiers.

+        }
       }
     }
 
     // Collect all Element -> Node mappings, in case we need to forward declare
     // any nodes.
     var nodes = new HashMap<Element, AstNode>.identity();
     var sdkBootstrappingFns = new List<FunctionElement>();
     for (var unit in compilationUnits) {
       if (_isDartRuntime(unit.element.library)) {
         sdkBootstrappingFns.addAll(unit.element.functions);
@@ skipping 42 matching lines @@
       List<JS.ModuleItem> result, Iterable<JS.ModuleItem> items) {
     for (var item in items) {
       if (item is JS.Block && !item.isScope) {
         _copyAndFlattenBlocks(result, item.statements);
       } else {
         result.add(item);
       }
     }
   }
 
+  /// Returns [true] if [jsName] is a simple identifier, a string with no dots,
+  /// or a property access where the receiver is a valid JS name and the
+  /// selector is a simple identifier.
+  bool _isValidJSName(JS.Expression jsName) {

[Jennifer Messerly, 2016/05/18 22:20:26]

based on suggestion above, I'm not sure you'll need this function

[Harry Terkelsen, 2016/05/19 16:56:19]

Done.

+    if (jsName is JS.Identifier) {
+      return true;
+    } else if (jsName is JS.LiteralString) {
+      return !jsName.value.contains('.');
+    } else if (jsName is JS.PropertyAccess) {
+      return (jsName.selector is JS.Identifier ||
+              jsName.selector is JS.LiteralString) &&
+          _isValidJSName(jsName.receiver);
+    } else {
+      return false;
+    }
+  }
+
+  /// Returns the equivalent of separating the given names by a '.'.
+  JS.PropertyAccess _mergeJSNames(Iterable<JS.Expression> names) {

[Jennifer Messerly, 2016/05/18 22:20:27]

same here, I don't think this will be needed

[Harry Terkelsen, 2016/05/19 16:56:18]

Done.

+    assert(names.every(_isValidJSName));

[Jennifer Messerly, 2016/05/18 22:20:27]

(this comment is probably moot based on my other ones)

I don't think this assert is needed again? Since we already know these names can
only flow to use from the _jsPrefix, and we already checked it.

[Harry Terkelsen, 2016/05/19 16:56:18]

Done.

+    Iterable<JS.LiteralString> extractParts(JS.Expression name) sync* {
+      if (name == null) return;
+      if (name is JS.LiteralString) {
+        yield name;
+        return;
+      }
+      if (name is JS.Identifier) {
+        yield js.string(name.name);
+        return;
+      }
+      var propertyAccess = name as JS.PropertyAccess;
+      yield* extractParts(propertyAccess.receiver);
+      yield propertyAccess.selector;
+    }
+    return names.expand(extractParts).fold(null, (merged, part) {
+      if (merged == null) {
+        return new JS.Identifier(
+            part.value.substring(1, part.value.length - 1));
+      }
+      return new JS.PropertyAccess(merged, part);
+    });
+  }
+
   String _libraryToModule(LibraryElement library) {
     assert(!_libraries.containsKey(library));
     var source = library.source;
     // TODO(jmesserly): we need to split out HTML.
     if (source.uri.scheme == 'dart') {
       return 'dart_sdk';
     }
     var moduleName = _buildUnit.libraryToModule(source);
     if (moduleName == null) {
       throw new StateError('Could not find module containing "$library".');
@@ skipping 2036 matching lines @@
         return js.call('#(#)', [genericName, jsArgs]);
       }
     }
 
     return _emitTopLevelName(element);
   }
 
   JS.PropertyAccess _emitTopLevelName(Element e, {String suffix: ''}) {
     if (e is TopLevelVariableElement &&
         e.getter != null &&
-        findAnnotation(e.getter, isPublicJSAnnotation) != null) {
+        (findAnnotation(e.getter, isPublicJSAnnotation) != null ||
+            (libraryTaggedJS && e.getter.isExternal))) {

[Jennifer Messerly, 2016/05/18 22:20:26]

`e.getter.isExternal && _libraryTaggedJS(e.library)`  would allow removing
libraryTaggedJS as noted above

[Harry Terkelsen, 2016/05/19 16:56:19]

Done.

       var annotationName = getAnnotationName(e.getter, isPublicJSAnnotation);
-      var name = js.string(annotationName ?? e.name);
-      return new JS.PropertyAccess(_self, name);
+      var name;
+      if (annotationName != null && annotationName.contains('.')) {
+        name = js.call(annotationName);
+      } else {
+        name = js.string(annotationName ?? e.name);
+      }
+      return _mergeJSNames(

[Jennifer Messerly, 2016/05/18 22:20:26]

rather than merging js_ast structures, IMO it would be easier to just make the
List<String> and build the dotted identifier:

List<String> names = ...;
var prop = _dartGlobal as JS.PropertyAccess; // type not needed but for clarity
for (var name in names) {
  prop = new JS.Access(prop, _propertyName(name));
}

[Harry Terkelsen, 2016/05/19 16:56:18]

Done.

+          [_dartGlobal, _jsPrefix, name].where((x) => x != null));

[Jennifer Messerly, 2016/05/18 22:20:27]

(probably a moot comment)  _jsPrefix is the only one that can be null, right? so
we don't need .where to check that.

[Harry Terkelsen, 2016/05/19 16:56:18]

Done.

     }
     String name = getJSExportName(e) + suffix;
     return new JS.PropertyAccess(
         emitLibraryName(e.library), _propertyName(name));
   }
 
   @override
   JS.Expression visitAssignmentExpression(AssignmentExpression node) {
     var left = node.leftHandSide;
     var right = node.rightHandSide;
@@ skipping 674 matching lines @@
           : parent.getSetter(element.name);
     }
     return null;
   }
 
   JS.Expression _emitConstructorName(
       ConstructorElement element, DartType type, SimpleIdentifier name) {
     var classElem = element.enclosingElement;
     if (findAnnotation(classElem, isPublicJSAnnotation) != null) {
       var annotationName = getAnnotationName(classElem, isPublicJSAnnotation);
-      var typeName = js.string(annotationName ?? classElem.name);
-      return new JS.PropertyAccess(_self, typeName);
+      var typeName;

[Jennifer Messerly, 2016/05/18 22:20:27]

this code seems duplicated with above.

(it sort of had that problem already, but the more code added the more obvious
it becomes :) )

[Harry Terkelsen, 2016/05/19 16:56:18]

Done.

+      if (annotationName != null && annotationName.contains('.')) {
+        typeName = js.call(annotationName);
+      } else {
+        typeName = js.string(annotationName ?? classElem.name);
+      }
+      return _mergeJSNames(
+          [_dartGlobal, _jsPrefix, typeName].where((x) => x != null));
     }
     var typeName = _emitType(type);
     if (name != null || element.isFactory) {
       var namedCtor = _constructorName(element);
       return new JS.PropertyAccess(typeName, namedCtor);
     }
     return typeName;
   }
 
   @override
@@ skipping 1435 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';