implement top-level JS annotated getters

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 05d603f8de8c428c91f543740d95d541006ad1c7..945945d0d38ecc2b8c2ed53b56d89b85ba4976b5 100644
--- a/lib/src/compiler/code_generator.dart
+++ b/lib/src/compiler/code_generator.dart
@@ -82,7 +82,8 @@ class CodeGenerator extends GeneralizingAstVisitor
   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>>();
@@ -90,6 +91,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       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');
 
@@ -226,8 +228,12 @@ class CodeGenerator extends GeneralizingAstVisitor
       }
 
       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.

+        }
       }
     }
 
@@ -290,6 +296,49 @@ class CodeGenerator extends GeneralizingAstVisitor
     }
   }
 
+  /// 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;
@@ -2346,10 +2395,17 @@ class CodeGenerator extends GeneralizingAstVisitor
   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(
@@ -3044,8 +3100,14 @@ class CodeGenerator extends GeneralizingAstVisitor
     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) {