Qualify library names in packages

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 2c19b39ef6f2c7ac0a9af6e10978145d953ec503..f3ae615334cd509fc4ff537b6b0086fcb8a7a755 100644
--- a/lib/src/compiler/code_generator.dart
+++ b/lib/src/compiler/code_generator.dart
@@ -10,6 +10,10 @@ import 'package:analyzer/dart/ast/token.dart' show Token, TokenType;
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/src/dart/ast/token.dart' show StringToken;
+
+// TODO(vsm): Delete
+import 'package:analyzer/src/context/source.dart' show SourceFactoryImpl;
+
 //TODO(leafp): Remove deprecated dependency
 //ignore: DEPRECATED_MEMBER_USE
 import 'package:analyzer/src/generated/element.dart'
@@ -190,7 +194,7 @@ class CodeGenerator extends GeneralizingAstVisitor
 
       var libraryTemp = _isDartRuntime(library)
           ? _runtimeLibVar
-          : new JS.TemporaryId(jsLibraryName(library));
+          : new JS.TemporaryId(jsLibraryName(context, library));
       _libraries[library] = libraryTemp;
       items.add(new JS.ExportDeclaration(
           js.call('const # = Object.create(null)', [libraryTemp])));
@@ -3684,7 +3688,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     // It's either one of the libraries in this module, or it's an import.
     return _libraries[library] ??
         _imports.putIfAbsent(
-            library, () => new JS.TemporaryId(jsLibraryName(library)));
+            library, () => new JS.TemporaryId(jsLibraryName(context, library)));
   }
 
   JS.Node annotate(JS.Node node, AstNode original, [Element element]) {
@@ -3760,7 +3764,21 @@ class CodeGenerator extends GeneralizingAstVisitor
 /// Choose a canonical name from the library element.
 /// This never uses the library's name (the identifier in the `library`
 /// declaration) as it doesn't have any meaningful rules enforced.
-String jsLibraryName(LibraryElement library) {
+String jsLibraryName(AnalysisContext context, LibraryElement library) {
+  var sourceFactory = context.sourceFactory as SourceFactoryImpl;

[vsm, 2016/04/27 18:24:35]

sourceFactory has it's own restoreAbsolute that walks the resolvers in order -
but it's preferring the file resolver to the package resolver.

[Jennifer Messerly, 2016/04/27 18:30:05]

Oh crazy! Is that hard coded? You could change the order we pass the resolvers
in. (I would guess we already pass in package resolver first though.... code is
in lib/src/analyzer/context.dart in DDC)

[Paul Berry, 2016/04/27 19:19:57]

Are you sure about this?  Looking at the definition in
package:analyzer/src/context/source.dart, it looks like it just visits the
resolvers in order.

[vsm, 2016/04/27 20:56:08]

yep, just need to set in the right order!  will fix and get rid of the unneeded
import.

+  for (var resolver in sourceFactory.resolvers) {
+    var uri = resolver.restoreAbsolute(library.source);

[Paul Berry, 2016/04/27 19:19:57]

Why not just:

var uri = library.source.uri;

This retrieves the URI that was used to find the package when it was first
resolved, which I think gives you the information you want and saves a lot of
legwork.

[Jennifer Messerly, 2016/04/27 19:24:21]

ah, thanks, I wondered that too! Figured maybe I was missing something.

Yeah in my experience, library.source.uri always has the right thing. :)

[vsm, 2016/04/27 20:56:08]

that should actually work for the bazel use case.  according to siggi, we'll
always be given a package:uri for anything that could be a package uri.

it is handy though for compiling from the command line - i can glob in a list of
files under packages/collection rather than listing each explicitly.

i can remove from this cl though if y'all would prefer.

[Paul Berry, 2016/04/27 21:13:49]

Ah, fair point.  Ok, I see why this is a use case that is worth supporting.  And
I see why in order to support it we need to implement the multi-package
resolver's restoreAbsolute() method.  However, I still believe we shouldn't be
calling that method from here (nor should we be calling
sourceFactory.restoreUri() from here).  Instead, we need to ensure that
library.source.uri is correct before passing the sources into the analyzer in
the first place (otherwise things will go wrong elsewhere in the analyzer).

For an example of how we do this in analyzer_cli, see
https://github.com/dart-lang/sdk/blob/master/pkg/analyzer_cli/lib/src/driver.....
 Note that we create the Source object, then use SourceFactory.restoreUri() to
see if a better URI can be found for it, and if so, we recreate the source based
on the better URI.  This is all done prior to feeding the sources into the
context, so the analyzer always sees correct URIs.

It's particularly important that we get this right for DDC, because DDC outputs
a summary, and the URIs contained in the summary are the URIs from
library.source.uri.  So if those URIs aren't correct, we'll run into trouble
when someone tries to use the summary as a dependency of another package.

[Jennifer Messerly, 2016/04/27 21:52:51]

yeah +1 to all that

@Vijay -- the DDC spot to fix this is probably
https://github.com/dart-lang/dev_compiler/blob/ec5180eb173c09b62109195025bc98...

that code there to map file relative paths to absolute file URIs never made me
happy so this may fix it :)

+    if (uri?.scheme == 'package') {
+      // Strip the package name.
+      // TODO(vsm): This is not unique if an escaped '/'appears in a filename.
+      // E.g., "foo/bar.dart" and "foo$47bar.dart" would collide.
+      var packageRelativePath = uri.pathSegments.skip(1).join('/');
+      return pathToJSIdentifier(packageRelativePath);
+    }
+  }
+  // TODO(vsm): This is not unique, but we're not in
+  // a package.  We need a mechanism for creating a unique library
+  // name.
   return pathToJSIdentifier(library.source.uri.pathSegments.last);
 }