Added support for exports and re-exports.

lib/compiler/implementation/scanner/scanner_task.dart

Index: lib/compiler/implementation/scanner/scanner_task.dart
diff --git a/lib/compiler/implementation/scanner/scanner_task.dart b/lib/compiler/implementation/scanner/scanner_task.dart
index cd5e44f3e58c35ac1bc54d26ca0feebb0994f64b..f3342eec3f21e30c31289035b4fed4042e4b6e54 100644
--- a/lib/compiler/implementation/scanner/scanner_task.dart
+++ b/lib/compiler/implementation/scanner/scanner_task.dart
@@ -6,11 +6,16 @@ class ScannerTask extends CompilerTask {
   ScannerTask(Compiler compiler) : super(compiler);
   String get name => 'Scanner';
 
-  void scanLibrary(LibraryElement library) {
+  /**
+   * Scans [library] and registers the declared top level entities to [handler]
+   * for the computation of the library export scope.
+   */
+  void scanLibrary(ImportExportHandler handler, LibraryElement library) {
     var compilationUnit = library.entryCompilationUnit;
     compiler.log("scanning library ${compilationUnit.script.name}");
     scan(compilationUnit);
-    processLibraryTags(library);
+    processLibraryTags(handler, library);
+    handler.registerLibraryExports(library);
   }
 
   void scan(CompilationUnitElement compilationUnit) {
@@ -19,7 +24,13 @@ class ScannerTask extends CompilerTask {
     });
   }
 
-  void processLibraryTags(LibraryElement library) {
+  /**
+   * Processes the library tags in [library].
+   *
+   * The imported/exported libraries are loaded and processed recursively but
+   * the import/export scopes are not set up.
+   */
+  void processLibraryTags(ImportExportHandler handler, LibraryElement library) {
     int tagState = TagState.NO_TAG_SEEN;
 
     /**
@@ -35,7 +46,8 @@ class ScannerTask extends CompilerTask {
       return TagState.NEXT[value];
     }
 
-    LinkBuilder<Import> imports = new LinkBuilder<Import>();
+    bool importsDartCore = false;
+    var importsExports = new LinkBuilder<ImportExport>();

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

ImportExport is very descriptive, but not very explanatory.
What is the common concept that covers Import and Export and that makes you
combine them here? LibraryDependency?

[Johnni Winther, 2012/10/09 09:47:10]

Done.

     Uri base = library.entryCompilationUnit.script.uri;
     for (LibraryTag tag in library.tags.reverse()) {
       if (tag.isImport) {
@@ -44,7 +56,13 @@ class ScannerTask extends CompilerTask {
         // another library could then observe the current library
         // before it fully declares all the members that are sourced
         // in.
-        imports.addLast(tag);
+        if (tag.uri.dartString.slowToString() == 'dart:core') {

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Using something called slowToString smells bad.
How about:
 Uri tagUri = tag.uri;
 if (tagUri.scheme == "dart" && tagUri.path == "core") {
?
(Or add an operator== on Uri)

[Johnni Winther, 2012/10/09 09:47:10]

[uri] is a LiteralString, not a Uri.

+          importsDartCore = true;
+        }
+        importsExports.addLast(tag);
+      } else if (tag.isExport) {
+        tagState = checkTag(TagState.IMPORT, tag);
+        importsExports.addLast(tag);
       } else if (tag.isLibraryName) {
         tagState = checkTag(TagState.LIBRARY, tag);
         if (library.libraryTag !== null) {
@@ -58,24 +76,27 @@ class ScannerTask extends CompilerTask {
         tagState = checkTag(TagState.SOURCE, tag);
         loadPart(tag, resolved, library);
       } else {
-        compiler.cancel("illegal script tag: ${tag.tag}", node: tag);
+        compiler.cancel("illegal script tag: ${tag}", node: tag);
       }
     }
 
     // Apply patch, if any.
     if (library.uri.scheme == 'dart') {
-      compiler.patchDartLibrary(library, library.uri.path);
+      compiler.patchDartLibrary(handler, library, library.uri.path);
     }
 
     // Now that we have processed all the source tags, it is safe to
     // start loading other libraries.
 
-    if (library.uri.scheme != 'dart' || library.uri.path != 'core') {
-      compiler.importCoreLibrary(library);
+    if (!importsDartCore ||
+        (library.uri.scheme != 'dart' || library.uri.path != 'core')) {

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

I really can't read this. Please use
  if (!importsDartCore ||
      !(library.uri.scheme == 'dart' && library.uri.path == 'core')) {
instead. And then, shouldn't the first '||' be an '&&'?

You may want a utility function:
 bool isDartCore(Uri uri) => uri.scheme == "dart" && uri.path == "core";
It seems to crop up often enough.
Or maybe just do
 .. && library != compiler.coreLibrary
?

[Johnni Winther, 2012/10/09 09:47:10]

Done.

+      handler.registerDependency(library, null,
+                                 compiler.importCoreLibrary(handler),
+                                 library.entryCompilationUnit);
     }
 
-    for (Import tag in imports.toLink()) {
-      importLibraryFromTag(tag, library.entryCompilationUnit);
+    for (ImportExport tag in importsExports.toLink()) {

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

We even have a class called ImportExport? Please change it to something less
"bag of x and y" like. LibraryDependency is ok but not great.

[Johnni Winther, 2012/10/09 09:47:10]

Done.

+      loadLibraryFromTag(handler, library, tag, library.entryCompilationUnit);
     }
   }
 
@@ -91,20 +112,26 @@ class ScannerTask extends CompilerTask {
   }
 
   /**
-   * Handle an import script tag by importing the referenced library into the
-   * current library.
-   * Returns the resolved library [Uri].
+   * Handle an import/export tag by loading the referenced library and
+   * registering its dependency in [handler] for the computation of the import/
+   * export scope.
    */
-  Uri importLibraryFromTag(Import tag,
-                           CompilationUnitElement compilationUnit) {
+  void loadLibraryFromTag(ImportExportHandler handler,
+                          LibraryElement library,
+                          ImportExport tag,
+                          CompilationUnitElement compilationUnit) {
     Uri base = compilationUnit.script.uri;
     Uri resolved = base.resolve(tag.uri.dartString.slowToString());
-    LibraryElement importedLibrary = loadLibrary(resolved, tag.uri, resolved);
-    importLibrary(compilationUnit.getLibrary(),
-                  importedLibrary,
-                  tag,
-                  compilationUnit);
-    return resolved;
+    LibraryElement loadedLibrary =
+        loadLibraryInternal(handler, resolved, tag.uri, resolved);
+    handler.registerDependency(library, tag, loadedLibrary, compilationUnit);
+

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Why are we passing both tag and compilationUnit?
I *think* it's in order to give good error messages for the library loading by
referring to the tag in its source.
How about combining Tag and compilationUnit into a single object, if they have
no independent uses?

[Johnni Winther, 2012/10/09 09:47:10]

CompilationUnit not needed anymore due to patch refactoring.

+    if (!loadedLibrary.hasLibraryName()) {
+      compiler.withCurrentElement(library, () {
+        compiler.reportError(tag === null ? null : tag.uri,
+            'no #library tag found in ${loadedLibrary.uri}');
+      });
+    }
   }
 
   void scanElements(CompilationUnitElement compilationUnit) {
@@ -113,37 +140,214 @@ class ScannerTask extends CompilerTask {
     compiler.dietParser.dietParse(compilationUnit, tokens);
   }
 
+  /**
+   * Loads the library located at [uri] and returns its [LibraryElement].
+   *
+   * If the library is not already loaded, the method creates the
+   * [LibraryElement] for the library and computes the import/export scope,
+   * loading and computing the import/export scopes of all required libraries in
+   * the process. The method handles cyclic dependency between libraries.
+   *
+   * This is the main entry point for [ScannerTask].
+   */
   LibraryElement loadLibrary(Uri uri, Node node, Uri canonicalUri) {
+    var handler = new ImportExportHandler(compiler);
+    LibraryElement library =
+        loadLibraryInternal(handler, uri, node, canonicalUri);
+    handler.computeExports();
+    return library;
+  }
+
+  LibraryElement loadLibraryInternal(ImportExportHandler handler,
+                                     Uri uri, Node node, Uri canonicalUri) {
     bool newLibrary = false;
     LibraryElement library =
       compiler.libraries.putIfAbsent(uri.toString(), () {
           newLibrary = true;
           Script script = compiler.readScript(uri, node);
           LibraryElement element = new LibraryElement(script, canonicalUri);
+          handler.registerNewLibrary(element);
           native.maybeEnableNative(compiler, element, uri);
           return element;
         });
     if (newLibrary) {
       compiler.withCurrentElement(library, () {
-        scanLibrary(library);
+        scanLibrary(handler, library);
         compiler.onLibraryLoaded(library, uri);
       });
     }
     return library;
   }
+}
 
-  void importLibrary(LibraryElement library, LibraryElement imported,
-                     Import tag, [CompilationUnitElement compilationUnit]) {
-    if (!imported.hasLibraryName()) {
-      compiler.withCurrentElement(library, () {
-        compiler.reportError(tag === null ? null : tag.uri,
-                             'no #library tag found in ${imported.uri}');
+class DietParserTask extends CompilerTask {
+  DietParserTask(Compiler compiler) : super(compiler);
+  final String name = 'Diet Parser';
+
+  dietParse(CompilationUnitElement compilationUnit, Token tokens) {
+    measure(() {
+      Function idGenerator = compiler.getNextFreeClassId;
+      ElementListener listener =
+          new ElementListener(compiler, compilationUnit, idGenerator);
+      PartialParser parser = new PartialParser(listener);
+      parser.parseUnit(tokens);
+    });
+  }
+}
+
+/**
+ * The fields of this class models a state machine for checking script
+ * tags come in the correct order.
+ */
+class TagState {
+  static const int NO_TAG_SEEN = 0;
+  static const int LIBRARY = 1;
+  static const int IMPORT = 2;

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Includes export?

[Johnni Winther, 2012/10/09 09:47:10]

Changed to IMPORT_OR_EXPORT.

+  static const int SOURCE = 3;
+  static const int RESOURCE = 4;
+
+  /** Next state. */
+  static const List<int> NEXT =
+      const <int>[NO_TAG_SEEN,
+                  IMPORT, // Only one library tag is allowed.
+                  IMPORT,
+                  SOURCE,
+                  RESOURCE];
+}
+
+/**
+ * Data class for handling import tags in [ImportExportHandler].

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Comment isn't helping me understand it. This is just a way to say "you don't
want to know". A "Data class" suggests that it has no meaning in itself.
"Handling" is so vague it's useless. So this is something to do with "import
tags" (the syntax) used inside some other class.

If it deserves a class, find something that the collection of data has in
commmon - even if it's just a common use.

I guess that this combines data related to a single library import. The data is
used by ImportExportHandler to schedule and perform the actual inclusion of
declarations from one library in the other library's import scope.

If I knew that, I would understand why the data is needed and why it's combined.

If it was called ImportTask, you wouldn't think of it as a "data class" (and you
should rarely have data classes!), and would be more inclind to put meaningful
methods on the class.
Even if it's just 'performTask()'.

+ */
+class ImportData {
+  final Import tag;
+  final LibraryElement library;

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Add documentation for fields.
What is this library? The importer or the importee?
What is it used for?

+  final CompilationUnitElement compilationUnit;

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

What is it a compilationUnit of? What is it used for?

[Johnni Winther, 2012/10/09 09:47:10]

Not needed anymore due to patch refactoring.

+
+  ImportData(this.tag, this.library, this.compilationUnit);
+}
+
+/**
+ * Data class used for the computation of import/export scopes by
+ * [ImportExportHandler].

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Also bad explanation. I'm not any wiser, and I question the use of a "data
class".

+ */
+class ImportExportData {
+  final LibraryElement library;
+  Link<ImportData> imports = const EmptyLink<ImportData>();

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Explain fields.
Is this a work-list of import tasks that needs to be performed?

+  Map<Export, ImportExportData> dependencyMap =

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Just "dependencies". I can see it's a map, and if it stopped being a map, but
still worked the same, I shouldn't need to care.
And please encapsulate access to the fields.

+      new Map<Export, ImportExportData>();

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

This maps syntax to a data object. That seems odd. When do you need to look up
by export tag again?

+  Map<SourceString, Element> exportScope = new Map<SourceString, Element>();

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

This, I guess, is the export scope that is being built for a library. For
[library], presumably.

Have you considered making letting the Library create the map, and just use a
bool to inhibit access to it until it's fully populated, instead of making the
map non-final?
And preferably make the library only accept access to the map through specific
functions like addToExportScope?

+  Set<Element> pendingExportSet = new Set<Element>();
+
+  ImportExportData(LibraryElement this.library);
+}
+
+/**
+ * Helper class used for computing the possibly cyclic import/export scopes of
+ * a set of libraries.

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Are you sure this should not be a CompilerTask? It even has a compiler reference
:)
It's the linking phase of the compiler, not just some obscure helper function.
It seems it does significant work in the computeExports method - is this work
measured on any Task?

+ *
+ * This class is used by [ScannerTask.loadLibrary] to collect all newly loaded
+ * libraries and to compute their import/export scopes through a fixed-point
+ * algorithm.
+ */
+class ImportExportHandler {

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Again: "ImportExport" is a bad name. (I'll stop now :)

+  final Compiler compiler;
+
+  Map<LibraryElement,ImportExportData> dataMap =

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Plase give this a better name that describes how it is used.
And a descriptive comment.

+      new Map<LibraryElement,ImportExportData>();
+
+  ImportExportHandler(Compiler this.compiler);
+
+  /**
+   * Performs a fixed-point computation on the export scopes of all registered
+   * libraries and creates the import/export of the libraries based on the
+   * fixed-point.
+   */
+  void computeExports() {
+    bool changed = true;
+    while (changed) {
+      changed = false;
+      dataMap.forEach((LibraryElement library, ImportExportData data) {
+        var pendingExportSet = new Set<Element>.from(data.pendingExportSet);

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Why a Set? I'd think a list would have lower overhead and faster iteration.

[Johnni Winther, 2012/10/09 09:47:10]

Done.

+        data.pendingExportSet.clear();
+        pendingExportSet.forEach((Element element) {
+          SourceString name = element.name;
+          Element existingElement =
+              data.exportScope.putIfAbsent(name, () => element);
+          if (existingElement !== null && existingElement !== element) {

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

If you export the same library twice, should it be an error?
If so, make a comment here that you are not following the specification.

+            element = data.exportScope[name] = new ErroneousElement(
+                    MessageKind.DUPLICATE_EXPORT, [name], name, library);
+          }
+          data.dependencyMap.forEach((Export export,
+                                      ImportExportData exportData) {
+            // TODO(johnniwinther): Handle show and hide combinators.
+            if (exportData.exportScope[name] !== element) {
+              exportData.pendingExportSet.add(element);
+              changed = true;
+            }
+          });
+        });
       });
     }
+
+    // Setup export scopes.
+    dataMap.forEach((LibraryElement library, ImportExportData data) {
+      data.library.exportScope = data.exportScope;
+    });
+
+    // Setup import scopes.
+    dataMap.forEach((LibraryElement library, ImportExportData data) {

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Join loops?

[Johnni Winther, 2012/10/09 09:47:10]

Can't do. We need the completion of exports before handling of import. Sort the
whole point in this deferred handling of the imports/exports.

+      for (ImportData importData in data.imports) {
+        importLibrary(library, importData);
+      }
+    });
+  }
+
+  /**
+   * Registers that [importingLibrary] depends on [importedLibrary] through
+   * [tag].
+   */
+  void registerDependency(LibraryElement importingLibrary,
+                          ImportExport tag,
+                          LibraryElement importedLibrary,
+                          CompilationUnitElement compilationUnit) {
+    if (tag is Export) {
+      dataMap[importedLibrary].dependencyMap[tag] = dataMap[importingLibrary];

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Because dataMap says nothing about what it does, this line means nothing.
Better names, all around.
I want to be able to read that this adds this library as an export dependency on
the imported library. Even if it takes named variables for intermediate values.

Also, don't access the map on the dataMap[importedLibrary], make a method:
  dataMap[importedLibrary].addDependency(tag, dataMap[importingLibrary]);

You are crippling your style by accepting that something is just a data class
that doesn't correspond to any concept.

+    } else {
+      ImportExportData data = dataMap[importingLibrary];
+      var importData = new ImportData(tag, importedLibrary, compilationUnit);
+      data.imports = data.imports.prepend(importData);
+    }
+  }
+
+  /**
+   * Registers [library] for the processing of its import/export scope.
+   */
+  void registerNewLibrary(LibraryElement library) {
+    dataMap[library] = new ImportExportData(library);

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

So the ImportExportData is the data associated with library by this class. It
collects dependencies and it builds the exportMap. These are all data that could
have been on LibraryElement, but is only used in this class.

+  }
+
+  /**
+   * Registers all top-level entities of [library] as starting point for the
+   * fixed-point computation of the import/export scopes.
+   */
+  void registerLibraryExports(LibraryElement library) {
+    dataMap[library].pendingExportSet.addAll(
+        library.localScope.getValues().filter((Element element) {
+          return !element.name.isPrivate();
+        }));

[Lasse Reichstein Nielsen, 2012/09/28 07:45:36]

Ick.
How about:
  ImportExportData libraryData = dataMap[library];
  library.localScope.forEachPublic(libraryData.addExport);
Add methods as necessary to avoid these long chains of
dots. Or aim even higher and never have more than one '.'
in a "chain".

+  }
+
+  /**
+   * Imports the export scope of [import] to the import scope of [library].
+   */
+  void importLibrary(LibraryElement library, ImportData import) {
+    Import tag = import.tag;
+    LibraryElement imported = import.library;
     if (tag !== null && tag.prefix !== null) {
       SourceString prefix = tag.prefix.source;
       Element e = library.find(prefix);
       if (e === null) {
+        CompilationUnitElement compilationUnit = import.compilationUnit;
         if (compilationUnit === null) {
           compilationUnit = library.entryCompilationUnit;
         }
@@ -159,6 +363,7 @@ class ScannerTask extends CompilerTask {
       }
       PrefixElement prefixElement = e;
       imported.forEachExport((Element element) {
+        // TODO(johnniwinther): Handle show and hide combinators.
         Element existing =
             prefixElement.imported.putIfAbsent(element.name, () => element);
         if (existing !== element) {
@@ -175,44 +380,10 @@ class ScannerTask extends CompilerTask {
     } else {
       imported.forEachExport((Element element) {
         compiler.withCurrentElement(element, () {
+          // TODO(johnniwinther): Handle show and hide combinators.
           library.addImport(element, compiler);
         });
       });
     }
   }
-}
-
-class DietParserTask extends CompilerTask {
-  DietParserTask(Compiler compiler) : super(compiler);
-  final String name = 'Diet Parser';
-
-  dietParse(CompilationUnitElement compilationUnit, Token tokens) {
-    measure(() {
-      Function idGenerator = compiler.getNextFreeClassId;
-      ElementListener listener =
-          new ElementListener(compiler, compilationUnit, idGenerator);
-      PartialParser parser = new PartialParser(listener);
-      parser.parseUnit(tokens);
-    });
-  }
-}
-
-/**
- * The fields of this class models a state machine for checking script
- * tags come in the correct order.
- */
-class TagState {
-  static const int NO_TAG_SEEN = 0;
-  static const int LIBRARY = 1;
-  static const int IMPORT = 2;
-  static const int SOURCE = 3;
-  static const int RESOURCE = 4;
-
-  /** Next state. */
-  static const List<int> NEXT =
-      const <int>[NO_TAG_SEEN,
-                  IMPORT, // Only one library tag is allowed.
-                  IMPORT,
-                  SOURCE,
-                  RESOURCE];
-}
+}