Eliminate multi-callback structure for LibraryLoader.

pkg/compiler/lib/src/compiler.dart

Index: pkg/compiler/lib/src/compiler.dart
diff --git a/pkg/compiler/lib/src/compiler.dart b/pkg/compiler/lib/src/compiler.dart
index 78a43e99b2fd9e23bdcc7f29d7d2eb337085ab29..87c7b9e4812e52c885c8cc3e55d0e3b24269af58 100644
--- a/pkg/compiler/lib/src/compiler.dart
+++ b/pkg/compiler/lib/src/compiler.dart
@@ -7,6 +7,7 @@ library dart2js.compiler_base;
 import 'dart:async' show EventSink, Future;
 
 import '../compiler_new.dart' as api;
+import 'io/source_file.dart' show SourceFile;

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

nit: move this further down to keep the alphabetical order

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

removed

 import 'closure.dart' as closureMapping show ClosureTask;
 import 'common/names.dart' show Selectors;
 import 'common/names.dart' show Identifiers, Uris;
@@ -51,7 +52,6 @@ import 'library_loader.dart'
         LibraryLoader,
         LibraryLoaderTask,
         LoadedLibraries,
-        LibraryLoaderListener,
         LibraryProvider,
         ScriptLoader;
 import 'mirrors_used.dart' show MirrorUsageAnalyzerTask;
@@ -84,7 +84,7 @@ import 'world.dart' show ClosedWorld, ClosedWorldRefiner, ClosedWorldImpl;
 typedef CompilerDiagnosticReporter MakeReporterFunction(
     Compiler compiler, CompilerOptions options);
 
-abstract class Compiler implements LibraryLoaderListener {
+abstract class Compiler {
   Measurer get measurer;
 
   final IdGenerator idGenerator = new IdGenerator();
@@ -213,6 +213,7 @@ abstract class Compiler implements LibraryLoaderListener {
       dietParser = new DietParserTask(idGenerator, backend, reporter, measurer),
       scanner = createScannerTask(),
       serialization = new SerializationTask(this),
+      patchParser = new PatchParserTask(this),
       libraryLoader = new LibraryLoaderTask(
           resolvedUriTranslator,
           options.compileOnly
@@ -220,12 +221,12 @@ abstract class Compiler implements LibraryLoaderListener {
               : new _ScriptLoader(this),
           new _ElementScanner(scanner),
           serialization,
-          this,
+          resolvePatchUri,
+          patchParser,
           environment,
           reporter,
           measurer),
       parser = new ParserTask(this),
-      patchParser = new PatchParserTask(this),
       resolver = createResolverTask(),
       closureToClassMapper = new closureMapping.ClosureTask(this),
       checker = new TypeCheckerTask(this),
@@ -317,29 +318,6 @@ abstract class Compiler implements LibraryLoaderListener {
         });
       });
 
-  /// This method is called immediately after the [LibraryElement] [library] has
-  /// been created.
-  ///
-  /// Use this callback method to store references to specific libraries.
-  /// Note that [library] has not been scanned yet, nor has its imports/exports
-  /// been resolved.
-  void onLibraryCreated(LibraryElement library) {
-    _commonElements.onLibraryCreated(library);
-  }
-
-  /// This method is called immediately after the [library] and its parts have
-  /// been scanned.
-  ///
-  /// Use this callback method to store references to specific member declared
-  /// in certain libraries. Note that [library] has not been patched yet, nor
-  /// has its imports/exports been resolved.
-  ///
-  /// Use [loader] to register the creation and scanning of a patch library
-  /// for [library].
-  Future onLibraryScanned(LibraryElement library, LibraryLoader loader) {
-    return backend.onLibraryScanned(library, loader);
-  }
-
   /// Compute the set of distinct import chains to the library at [uri] within
   /// [loadedLibraries].
   ///
@@ -350,7 +328,6 @@ abstract class Compiler implements LibraryLoaderListener {
   Set<String> computeImportChainsFor(LoadedLibraries loadedLibraries, Uri uri) {
     // TODO(johnniwinther): Move computation of dependencies to the library
     // loader.
-    Uri rootUri = loadedLibraries.rootUri;
     Set<String> importChains = new Set<String>();
     // The maximum number of full imports chains to process.
     final int chainLimit = 10000;
@@ -374,7 +351,8 @@ abstract class Compiler implements LibraryLoaderListener {
         }
       }
       String importChain = compactImportChain.map((CodeLocation codeLocation) {
-        return codeLocation.relativize(rootUri);
+        return codeLocation
+            .relativize(loadedLibraries.rootLibrary.canonicalUri);
       }).join(' => ');
 
       if (!importChains.contains(importChain)) {
@@ -405,25 +383,25 @@ abstract class Compiler implements LibraryLoaderListener {
   ///
   /// The method returns a [Future] allowing for the loading of additional
   /// libraries.
-  Future onLibrariesLoaded(LoadedLibraries loadedLibraries) {
-    return new Future.sync(() {
-      for (Uri uri in resolvedUriTranslator.disallowedLibraryUris) {
-        if (loadedLibraries.containsLibrary(uri)) {
-          Set<String> importChains =
-              computeImportChainsFor(loadedLibraries, uri);
-          reporter.reportInfo(
-              NO_LOCATION_SPANNABLE, MessageKind.DISALLOWED_LIBRARY_IMPORT, {
-            'uri': uri,
-            'importChain': importChains
-                .join(MessageTemplate.DISALLOWED_LIBRARY_IMPORT_PADDING)
-          });
-        }
-      }
+  LoadedLibraries processLoadedLibraries(LoadedLibraries loadedLibraries) {
+    loadedLibraries.forEachLibrary((LibraryElement library) {
+      _commonElements.registerCommonLibraries(library);
+      backend.setAnnotations(library);
+    });
 
-      if (!loadedLibraries.containsLibrary(Uris.dart_core)) {
-        return null;
+    for (Uri uri in resolvedUriTranslator.disallowedLibraryUris) {
+      if (loadedLibraries.containsLibrary(uri)) {
+        Set<String> importChains = computeImportChainsFor(loadedLibraries, uri);
+        reporter.reportInfo(
+            NO_LOCATION_SPANNABLE, MessageKind.DISALLOWED_LIBRARY_IMPORT, {
+          'uri': uri,
+          'importChain': importChains
+              .join(MessageTemplate.DISALLOWED_LIBRARY_IMPORT_PADDING)
+        });
       }
+    }
 
+    if (loadedLibraries.containsLibrary(Uris.dart_core)) {
       bool importsMirrorsLibrary =
           loadedLibraries.containsLibrary(Uris.dart_mirrors);
       if (importsMirrorsLibrary && !backend.supportsReflection) {
@@ -443,7 +421,9 @@ abstract class Compiler implements LibraryLoaderListener {
               .join(MessageTemplate.IMPORT_EXPERIMENTAL_MIRRORS_PADDING)
         });
       }
-    }).then((_) => backend.onLibrariesLoaded(loadedLibraries));
+    }
+    backend.onLibrariesLoaded(loadedLibraries);
+    return loadedLibraries;
   }
 
   // TODO(johnniwinther): Move this to [PatchParser] when it is moved to the
@@ -456,7 +436,7 @@ abstract class Compiler implements LibraryLoaderListener {
    */
   Uri resolvePatchUri(String dartLibraryPath);
 
-  Future runInternal(Uri uri) {
+  Future runInternal(Uri uri) async {
     // TODO(ahe): This prevents memory leaks when invoking the compiler
     // multiple times. Implement a better mechanism where we can store
     // such caches in the compiler and get access to them through a
@@ -472,27 +452,26 @@ abstract class Compiler implements LibraryLoaderListener {
     }
 
     assert(uri != null || options.analyzeOnly);
-    return new Future.sync(() {
-      if (librariesToAnalyzeWhenRun != null) {
-        return Future.forEach(librariesToAnalyzeWhenRun, (libraryUri) {
-          reporter.log('Analyzing $libraryUri (${options.buildId})');
-          return libraryLoader.loadLibrary(libraryUri);
-        });
-      }
-    }).then((_) {
-      if (uri != null) {
-        if (options.analyzeOnly) {
-          reporter.log('Analyzing $uri (${options.buildId})');
-        } else {
-          reporter.log('Compiling $uri (${options.buildId})');
-        }
-        return libraryLoader.loadLibrary(uri).then((LibraryElement library) {
-          mainApp = library;
-        });
+    // As far as I can tell, this branch is only used by test code.
+    if (librariesToAnalyzeWhenRun != null) {
+      await Future.forEach(librariesToAnalyzeWhenRun, (libraryUri) async {
+        reporter.log('Analyzing $libraryUri (${options.buildId})');
+        LoadedLibraries loadedLibraries =
+            await libraryLoader.loadLibrary(libraryUri);
+        processLoadedLibraries(loadedLibraries);
+      });
+    }
+    if (uri != null) {
+      if (options.analyzeOnly) {
+        reporter.log('Analyzing $uri (${options.buildId})');
+      } else {
+        reporter.log('Compiling $uri (${options.buildId})');
       }
-    }).then((_) {
-      compileLoadedLibraries();
-    });
+      LoadedLibraries libraries = await libraryLoader.loadLibrary(uri);
+      processLoadedLibraries(libraries);
+      mainApp = libraries.rootLibrary;
+    }
+    compileLoadedLibraries();
   }
 
   WorldImpact computeMain() {
@@ -569,19 +548,21 @@ abstract class Compiler implements LibraryLoaderListener {
   /// This operation assumes an unclosed resolution queue and is only supported
   /// when the '--analyze-main' option is used.
   Future<LibraryElement> analyzeUri(Uri libraryUri,
-      {bool skipLibraryWithPartOfTag: true}) {
+      {bool skipLibraryWithPartOfTag: true}) async {
     assert(options.analyzeMain);
     reporter.log('Analyzing $libraryUri (${options.buildId})');
-    return libraryLoader
-        .loadLibrary(libraryUri, skipFileWithPartOfTag: true)
-        .then((LibraryElement library) {
-      if (library == null) return null;
+    LoadedLibraries loadedLibraries = await libraryLoader
+        .loadLibrary(libraryUri, skipFileWithPartOfTag: true);
+    if (loadedLibraries != null) {

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

minor nit - we tend to prefer the fast-exit pattern that was here before:

if (loadedLibraries == null) return null;

processLoadedLIbraries(loadedLibraries);
...
return library;

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

Done.

+      processLoadedLibraries(loadedLibraries);
+      LibraryElement library = loadedLibraries.rootLibrary;

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

is it safe to say that the root library is not null at this point, or do we need
to check for that too?

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

As the code is currently written if you receive a LoadedLibraries object, its
rootLibrary cannot be null. I'll add an assert in the constructor though.

       ResolutionEnqueuer resolutionEnqueuer = startResolution();
       resolutionEnqueuer.applyImpact(computeImpactForLibrary(library));
       emptyQueue(resolutionEnqueuer, onProgress: showResolutionProgress);
       resolutionEnqueuer.logSummary(reporter.log);
       return library;
-    });
+    }
+    return null;
   }
 
   /// Starts the resolution phase, creating the [ResolutionEnqueuer] if not
@@ -1126,7 +1107,7 @@ class _CompilerCommonElements extends CommonElementsMixin {
   @override
   ResolutionDynamicType get dynamicType => const ResolutionDynamicType();
 
-  void onLibraryCreated(LibraryElement library) {
+  void registerCommonLibraries(LibraryElement library) {
     Uri uri = library.canonicalUri;
     if (uri == Uris.dart_core) {
       coreLibrary = library;
@@ -1932,6 +1913,7 @@ class _ElementScanner implements ElementScanner {
   _ElementScanner(this.scanner);
   void scanLibrary(LibraryElement library) => scanner.scanLibrary(library);
   void scanUnit(CompilationUnitElement unit) => scanner.scan(unit);
+  Token scanFile(SourceFile file) => scanner.scanFile(file);
 }
 
 class _EmptyEnvironment implements Environment {