Eliminate multi-callback structure for LibraryLoader.

pkg/compiler/lib/src/library_loader.dart

Index: pkg/compiler/lib/src/library_loader.dart
diff --git a/pkg/compiler/lib/src/library_loader.dart b/pkg/compiler/lib/src/library_loader.dart
index 8865f1e6e28183e1a0f9e7178fa11c0e90e11908..c4c76e43ca8d5fdc47071b92a7be1579ca420983 100644
--- a/pkg/compiler/lib/src/library_loader.dart
+++ b/pkg/compiler/lib/src/library_loader.dart
@@ -29,15 +29,21 @@ import 'elements/modelx.dart'
         SyntheticImportElement;
 import 'enqueue.dart' show DeferredAction;
 import 'environment.dart';
+import 'io/source_file.dart' show SourceFile;
+import 'patch_parser.dart' show PatchParserTask;
 import 'resolved_uri_translator.dart';
 import 'script.dart';
 import 'serialization/serialization.dart' show LibraryDeserializer;
 import 'tree/tree.dart';
 import 'util/util.dart' show Link, LinkBuilder;
 
+import 'package:front_end/src/fasta/scanner.dart' show Token;
+
 typedef Future<Iterable<LibraryElement>> ReuseLibrariesFunction(
     Iterable<LibraryElement> libraries);
 
+typedef Uri PatchResolverFunction(String dartLibraryPath);
+
 /**
  * [CompilerTask] for loading libraries and setting up the import/export scopes.
  *
@@ -138,7 +144,8 @@ abstract class LibraryLoaderTask implements LibraryProvider, CompilerTask {
       ScriptLoader scriptLoader,
       ElementScanner scriptScanner,
       LibraryDeserializer deserializer,
-      LibraryLoaderListener listener,
+      PatchResolverFunction patchResolverFunc,
+      PatchParserTask patchParser,
       Environment environment,
       DiagnosticReporter reporter,
       Measurer measurer) = _LibraryLoaderTask;
@@ -146,8 +153,10 @@ abstract class LibraryLoaderTask implements LibraryProvider, CompilerTask {
   /// Returns all libraries that have been loaded.
   Iterable<LibraryElement> get libraries;
 
-  /// Loads the library specified by the [resolvedUri] and returns its
-  /// [LibraryElement].
+  /// Loads the library specified by the [resolvedUri] and returns the
+  /// [LoadedLibraries] that were loaded to load the specified uri. The
+  /// [LibraryElement] itself can be found by calling
+  /// `loadedLibraries.rootLibrary`

[Siggi Cherem (dart-lang), 2017/03/24 17:40:28]

nit: add a `.` at the end.

[Emily Fortuna, 2017/03/24 18:30:19]

Done.

   ///
   /// If the library is not already loaded, the method creates the
   /// [LibraryElement] for the library and computes the import/export scope,
@@ -157,7 +166,7 @@ abstract class LibraryLoaderTask implements LibraryProvider, CompilerTask {
   /// If [skipFileWithPartOfTag] is `true`, `null` is returned if the
   /// compilation unit for [resolvedUri] contains a `part of` tag. This is only
   /// used for analysis through [Compiler.analyzeUri].
-  Future<LibraryElement> loadLibrary(Uri resolvedUri,
+  Future<LoadedLibraries> loadLibrary(Uri resolvedUri,
       {bool skipFileWithPartOfTag: false});
 
   /// Reset the library loader task to prepare for compilation. If provided,
@@ -180,6 +189,31 @@ abstract class LibraryLoaderTask implements LibraryProvider, CompilerTask {
   /// Returns the deferred actions registered since the last call to
   /// [pullDeferredActions].
   Iterable<DeferredAction> pullDeferredActions();
+
+  /// The locations of js patch-files relative to the sdk-descriptors.
+  static const _patchLocations = const <String, String>{
+    "async": "_internal/js_runtime/lib/async_patch.dart",
+    "collection": "_internal/js_runtime/lib/collection_patch.dart",
+    "convert": "_internal/js_runtime/lib/convert_patch.dart",
+    "core": "_internal/js_runtime/lib/core_patch.dart",
+    "developer": "_internal/js_runtime/lib/developer_patch.dart",
+    "io": "_internal/js_runtime/lib/io_patch.dart",
+    "isolate": "_internal/js_runtime/lib/isolate_patch.dart",
+    "math": "_internal/js_runtime/lib/math_patch.dart",
+    "mirrors": "_internal/js_runtime/lib/mirrors_patch.dart",
+    "typed_data": "_internal/js_runtime/lib/typed_data_patch.dart",
+    "_internal": "_internal/js_runtime/lib/internal_patch.dart"
+  };
+
+  /// Returns the location of the patch-file associated with [libraryName]
+  /// resolved from [plaformConfigUri].
+  ///
+  /// Returns null if there is none.
+  static Uri resolvePatchUri(String libraryName, Uri platformConfigUri) {
+    String patchLocation = _patchLocations[libraryName];
+    if (patchLocation == null) return null;
+    return platformConfigUri.resolve(patchLocation);
+  }
 }
 
 /// Interface for an entity that provide libraries. For instance from normal
@@ -303,14 +337,19 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
   /// from a serialized form.
   final LibraryDeserializer deserializer;
 
-  /// Hooks to inform others about progress done by this loader.
-  // TODO(sigmund): move away from this.
-  final LibraryLoaderListener listener;
-
   /// Definitions provided via the `-D` command line flags. Used to resolve
   /// conditional imports.
   final Environment environment;
 
+  // TODO(efortuna): Don't pass PatchParserTask here.
+  final PatchParserTask _patchParserTask;
+
+  /// Function that accepts the string name of a library and returns the
+  /// path to the corresponding patch file. This is a function that is passed in
+  /// because our test mock_compiler subclasses compiler.
+  // TODO(efortuna): Refactor mock_compiler to not do this.

[Siggi Cherem (dart-lang), 2017/03/24 17:40:27]

thanks for documenting so nicely the issue :)

I have a couple ideas we can try, but we can totally delay these for a separate
CL:
 - idea 1: make this field non-final and public, then the mock compiler can set
it directly and we can set it's default implementation in a way that allows us
to remove the extra indirection with apiimpl.

 - idea 2: make patchLocations and platofmConfigUri a parameter of the loader
task instead. The algorithm in the mock-compiler is not that different than what
we have here, so if we pass a special value for configUri and the map, we might
have something clean to do.

 - idea 3: make the mock compiler use a different path for the PATCH_CORE, to
match exactly what's expected in the map we use here.


I like #2 or #3 - if #3 is simple enough, I'd go with that. If we weren't moving
to load from kernel, I'd make the compiler more configurable about where
patch-files are with #2, but seems unnecessary at this moment.

[Emily Fortuna, 2017/03/24 18:30:19]

makes sense. upcoming CL!

+  final PatchResolverFunction _patchResolverFunc;
+
   List<DeferredAction> _deferredActions = <DeferredAction>[];
 
   final DiagnosticReporter reporter;
@@ -320,7 +359,8 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
       this.scriptLoader,
       this.scanner,
       this.deserializer,
-      this.listener,
+      this._patchResolverFunc,
+      this._patchParserTask,
       this.environment,
       this.reporter,
       Measurer measurer)
@@ -335,8 +375,6 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
   final Map<String, LibraryElement> libraryNames =
       new Map<String, LibraryElement>();
 
-  LibraryDependencyHandler currentHandler;
-
   Iterable<LibraryElement> get libraries => libraryCanonicalUriMap.values;
 
   LibraryElement lookupLibrary(Uri canonicalUri) {
@@ -345,8 +383,6 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
 
   void reset({bool reuseLibrary(LibraryElement library)}) {
     measure(() {
-      assert(currentHandler == null);
-
       Iterable<LibraryElement> reusedLibraries = null;
       if (reuseLibrary != null) {
         reusedLibraries = measureSubtask(_reuseLibrarySubtaskName, () {
@@ -373,8 +409,6 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
 
   Future resetAsync(Future<bool> reuseLibrary(LibraryElement library)) {
     return measure(() {
-      assert(currentHandler == null);
-
       Future<LibraryElement> wrapper(LibraryElement library) {
         try {
           return reuseLibrary(library)
@@ -401,7 +435,6 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
   Future<Null> resetLibraries(
       Future<Iterable<LibraryElement>> reuseLibraries(
           Iterable<LibraryElement> libraries)) {
-    assert(currentHandler == null);
     return measureSubtask(_reuseLibrarySubtaskName, () {
       return new Future<Iterable<LibraryElement>>(() {
         // Wrap in Future to shield against errors in user code.
@@ -431,31 +464,20 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
     }
   }
 
-  Future<LibraryElement> loadLibrary(Uri resolvedUri,
-      {bool skipFileWithPartOfTag: false}) {
-    return measure(() {
-      assert(currentHandler == null);
-      // TODO(johnniwinther): Ensure that currentHandler correctly encloses the
-      // loading of a library cluster.
-      currentHandler = new LibraryDependencyHandler(this);
-      return createLibrary(
-              currentHandler, null, resolvedUri, NO_LOCATION_SPANNABLE,
-              skipFileWithPartOfTag: skipFileWithPartOfTag)
-          .then((LibraryElement library) {
-        if (library == null) {
-          currentHandler = null;
-          return null;
-        }
-        return reporter.withCurrentElement(library, () {
-          return measure(() {
-            currentHandler.computeExports();
-            LoadedLibraries loadedLibraries = new _LoadedLibraries(library,
-                currentHandler.newLibraries, currentHandler.nodeMap, this);
-            currentHandler = null;
-            return listener
-                .onLibrariesLoaded(loadedLibraries)
-                .then((_) => library);
-          });
+  Future<LoadedLibraries> loadLibrary(Uri resolvedUri,
+      {bool skipFileWithPartOfTag: false}) async {

[Siggi Cherem (dart-lang), 2017/03/24 17:40:28]

seems like you don't need async here

[Emily Fortuna, 2017/03/24 18:30:19]

Done.

+    return measure(() async {
+      LibraryDependencyHandler loader = new LibraryDependencyHandler(this);

[Siggi Cherem (dart-lang), 2017/03/24 17:40:27]

nit: maybe rename loader as handler?

[Emily Fortuna, 2017/03/24 18:30:19]

Done.

+      LibraryElement library = await createLibrary(
+          loader, null, resolvedUri, NO_LOCATION_SPANNABLE,
+          skipFileWithPartOfTag: skipFileWithPartOfTag);
+      if (library == null) {

[Siggi Cherem (dart-lang), 2017/03/24 17:40:28]

style nit: we don't use the {} on this fast-exit code, just:

  if (library == null) return null;

[Emily Fortuna, 2017/03/24 18:30:19]

Done.

+        return null;
+      }
+      return reporter.withCurrentElement(library, () {
+        return measure(() {
+          loader.computeExports();
+          return new _LoadedLibraries(library, loader.newLibraries, this);
         });
       });
     });
@@ -548,8 +570,6 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
         }
       });
     }).then((_) {
-      return listener.onLibraryScanned(library, handler);

[Siggi Cherem (dart-lang), 2017/03/24 17:40:27]

woo hoo!

-    }).then((_) {
       return reporter.withCurrentElement(library, () {
         checkDuplicatedLibraryName(library);
 
@@ -659,20 +679,18 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
       LibraryDependencyHandler handler, LibraryElement library) {
     libraryCanonicalUriMap[library.canonicalUri] = library;
     handler.registerNewLibrary(library);
-    return listener.onLibraryScanned(library, handler).then((_) {
-      return Future.forEach(library.imports, (ImportElement import) {
-        Uri resolvedUri = library.canonicalUri.resolveUri(import.uri);
+    return Future.forEach(library.imports, (ImportElement import) {
+      Uri resolvedUri = library.canonicalUri.resolveUri(import.uri);
+      return createLibrary(handler, library, resolvedUri, library);
+    }).then((_) {
+      return Future.forEach(library.exports, (ExportElement export) {
+        Uri resolvedUri = library.canonicalUri.resolveUri(export.uri);
         return createLibrary(handler, library, resolvedUri, library);
       }).then((_) {
-        return Future.forEach(library.exports, (ExportElement export) {
-          Uri resolvedUri = library.canonicalUri.resolveUri(export.uri);
-          return createLibrary(handler, library, resolvedUri, library);
-        }).then((_) {
-          // TODO(johnniwinther): Shouldn't there be an [ImportElement] for the
-          // implicit import of dart:core?
-          return createLibrary(handler, library, Uris.dart_core, library);
-        }).then((_) => library);
-      });
+        // TODO(johnniwinther): Shouldn't there be an [ImportElement] for the
+        // implicit import of dart:core?
+        return createLibrary(handler, library, Uris.dart_core, library);
+      }).then((_) => library);
     });
   }
 
@@ -693,57 +711,67 @@ class _LibraryLoaderTask extends CompilerTask implements LibraryLoaderTask {
    */
   Future<LibraryElement> createLibrary(LibraryDependencyHandler handler,
       LibraryElement importingLibrary, Uri resolvedUri, Spannable spannable,
-      {bool skipFileWithPartOfTag: false}) {
+      {bool skipFileWithPartOfTag: false}) async {
     Uri readableUri =
         uriTranslator.translate(importingLibrary, resolvedUri, spannable);
     LibraryElement library = libraryCanonicalUriMap[resolvedUri];
     if (library != null) {
       return new Future.value(library);
     }
-    return deserializer.readLibrary(resolvedUri).then((LibraryElement library) {
-      if (library != null) {
-        return loadDeserializedLibrary(handler, library);
-      }
-      return reporter.withCurrentElement(importingLibrary, () {
-        return _readScript(spannable, readableUri, resolvedUri)
-            .then((Script script) {
-          if (script == null) return null;
-          LibraryElement element =
-              createLibrarySync(handler, script, resolvedUri);
-          CompilationUnitElementX compilationUnit =
-              element.entryCompilationUnit;
-          if (compilationUnit.partTag != null) {
-            if (skipFileWithPartOfTag) {
-              // TODO(johnniwinther): Avoid calling [listener.onLibraryCreated]
-              // for this library.
-              libraryCanonicalUriMap.remove(resolvedUri);
-              return null;
-            }
-            if (importingLibrary == null) {
-              DiagnosticMessage error = reporter.withCurrentElement(
-                  compilationUnit,
-                  () => reporter.createMessage(
-                      compilationUnit.partTag, MessageKind.MAIN_HAS_PART_OF));
-              reporter.reportError(error);
-            } else {
-              DiagnosticMessage error = reporter.withCurrentElement(
-                  compilationUnit,
-                  () => reporter.createMessage(
-                      compilationUnit.partTag, MessageKind.IMPORT_PART_OF));
-              DiagnosticMessage info = reporter.withCurrentElement(
-                  importingLibrary,
-                  () => reporter.createMessage(
-                      spannable, MessageKind.IMPORT_PART_OF_HERE));
-              reporter.reportError(error, [info]);
-            }
+    library = await deserializer.readLibrary(resolvedUri);
+    if (library != null) {
+      return loadDeserializedLibrary(handler, library);
+    }
+    return reporter.withCurrentElement(importingLibrary, () {
+      return _readScript(spannable, readableUri, resolvedUri)
+          .then((Script script) async {
+        if (script == null) return null;
+        LibraryElement element =
+            createLibrarySync(handler, script, resolvedUri);
+        CompilationUnitElementX compilationUnit = element.entryCompilationUnit;
+        if (compilationUnit.partTag != null) {
+          if (skipFileWithPartOfTag) {
+            // TODO(johnniwinther): Avoid calling
+            // [compiler.processLoadedLibraries] with this library.
+            libraryCanonicalUriMap.remove(resolvedUri);
+            return null;
           }
-          return processLibraryTags(handler, element).then((_) {
-            reporter.withCurrentElement(element, () {
-              handler.registerLibraryExports(element);
-            });
-            return element;
-          });
+          if (importingLibrary == null) {
+            DiagnosticMessage error = reporter.withCurrentElement(
+                compilationUnit,
+                () => reporter.createMessage(
+                    compilationUnit.partTag, MessageKind.MAIN_HAS_PART_OF));
+            reporter.reportError(error);
+          } else {
+            DiagnosticMessage error = reporter.withCurrentElement(
+                compilationUnit,
+                () => reporter.createMessage(
+                    compilationUnit.partTag, MessageKind.IMPORT_PART_OF));
+            DiagnosticMessage info = reporter.withCurrentElement(
+                importingLibrary,
+                () => reporter.createMessage(
+                    spannable, MessageKind.IMPORT_PART_OF_HERE));
+            reporter.reportError(error, [info]);
+          }
+        }
+        await processLibraryTags(handler, element);
+        reporter.withCurrentElement(element, () {
+          handler.registerLibraryExports(element);
         });
+
+        if (element.isPlatformLibrary &&

[Siggi Cherem (dart-lang), 2017/03/24 17:40:28]

so nice to see patching done while loading, yay!

Minor suggestion: consider adding a tiny method with this logic to make it
self-documenting. 

  await patchLibraryIfNecessary(handler, element);

[Emily Fortuna, 2017/03/24 18:30:20]

done! I aaaaaalmost did that in my cleanup process, but then I was like "nah".
Done now!

+            // Don't patch library currently disallowed.
+            !element.isSynthesized &&
+            !element.isPatched &&
+            // Don't patch deserialized libraries.
+            !deserializer.isDeserialized(element)) {
+          // Apply patch, if any.
+          Uri patchUri = _patchResolverFunc(element.canonicalUri.path);
+          if (patchUri != null) {
+            await _patchParserTask.patchLibrary(handler, patchUri, element);
+          }
+        }
+        return element;
       });
     });
   }
@@ -1330,7 +1358,6 @@ class LibraryDependencyHandler implements LibraryLoader {
    * Registers [library] for the processing of its import/export scope.
    */
   void registerNewLibrary(LibraryElement library) {
-    task.listener.onLibraryCreated(library);
     _newLibraries.add(library);
     if (!library.exportsHandled) {
       nodeMap[library] = new LibraryDependencyNode(library);
@@ -1350,11 +1377,12 @@ class LibraryDependencyHandler implements LibraryLoader {
   }
 }
 
-/// Information on the bulk of newly loaded libraries through a call to
+/// Information on the set libraries loaded as a result of a call to
 /// [LibraryLoader.loadLibrary].
 abstract class LoadedLibraries {
-  /// The uri passed to [LibraryLoader.loadLibrary].
-  Uri get rootUri;
+  /// The accesss the library object created corresponding to the library
+  /// passed to [LibraryLoader.loadLibrary].
+  LibraryElement get rootLibrary;
 
   /// Returns `true` if a library with canonical [uri] was loaded in this bulk.
   bool containsLibrary(Uri uri);
@@ -1369,7 +1397,7 @@ abstract class LoadedLibraries {
   ///
   /// The argument [importChainReversed] to [callback] contains the chain of
   /// imports uris that lead to importing [uri] starting in [uri] and ending in
-  /// [rootUri].
+  /// the uri that was passed in with [loadLibrary].
   ///
   /// [callback] is called once for each chain of imports leading to [uri] until
   /// [callback] returns `false`.
@@ -1381,22 +1409,19 @@ class _LoadedLibraries implements LoadedLibraries {
   final _LibraryLoaderTask task;
   final LibraryElement rootLibrary;
   final Map<Uri, LibraryElement> loadedLibraries = <Uri, LibraryElement>{};
-  final Map<LibraryElement, LibraryDependencyNode> nodeMap;
+  final List<LibraryElement> _newLibraries;
 
-  _LoadedLibraries(this.rootLibrary, Iterable<LibraryElement> libraries,
-      this.nodeMap, this.task) {
-    libraries.forEach((LibraryElement loadedLibrary) {
+  _LoadedLibraries(this.rootLibrary, this._newLibraries, this.task) {
+    _newLibraries.forEach((LibraryElement loadedLibrary) {
       loadedLibraries[loadedLibrary.canonicalUri] = loadedLibrary;
     });
   }
 
-  Uri get rootUri => rootLibrary.canonicalUri;
-
   bool containsLibrary(Uri uri) => loadedLibraries.containsKey(uri);
 
   LibraryElement getLibrary(Uri uri) => loadedLibraries[uri];
 
-  void forEachLibrary(f(LibraryElement library)) => nodeMap.keys.forEach(f);
+  void forEachLibrary(f(LibraryElement library)) => _newLibraries.forEach(f);
 
   void forEachImportChain(Uri targetUri,
       {bool callback(Link<Uri> importChainReversed)}) {
@@ -1475,7 +1500,7 @@ class _LoadedLibraries implements LoadedLibraries {
     computeSuffixes(rootLibrary, const Link<Uri>());
   }
 
-  String toString() => 'root=$rootLibrary,libraries=${loadedLibraries.keys}';
+  String toString() => 'root=$rootLibrary,libraries=${_newLibraries}';
 }
 
 // TODO(sigmund): remove ScriptLoader & ElementScanner. Such abstraction seems
@@ -1528,21 +1553,7 @@ abstract class ScriptLoader {
 abstract class ElementScanner {
   void scanLibrary(LibraryElement library);
   void scanUnit(CompilationUnitElement unit);
-}
-
-/// TODO(sigmund): remove this abstraction. Ideally the loader can produce the
-/// LoadedLibraries results once, and the compiler and choose what to do with
-/// it instead.
-abstract class LibraryLoaderListener {
-  /// Called after a request to load a library. The [results] will include all
-  /// transitive libraries loaded as a result of the initial request.
-  Future onLibrariesLoaded(LoadedLibraries results);
-
-  /// Called whenever a library element is created.
-  void onLibraryCreated(LibraryElement library);
-
-  /// Called whenever a library is scanned from a script file.
-  Future onLibraryScanned(LibraryElement library, LibraryLoader loader);
+  Token scanFile(SourceFile file);

[Siggi Cherem (dart-lang), 2017/03/24 17:40:27]

looks like you don't need scanFile after all? could we remove it here and in
compiler.dart?

[Emily Fortuna, 2017/03/24 18:30:20]

yes thanks.

 }
 
 const _reuseLibrarySubtaskName = "Reuse library";