Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service

pkg/front_end/lib/src/base/processed_options.dart

Index: pkg/front_end/lib/src/base/processed_options.dart
diff --git a/pkg/front_end/lib/src/base/processed_options.dart b/pkg/front_end/lib/src/base/processed_options.dart
index a42d2b54caa8c554bb705ee450a79a3a61a8ab64..6681e2dc929a2f1df49f63cff9a2f434bdd1793a 100644
--- a/pkg/front_end/lib/src/base/processed_options.dart
+++ b/pkg/front_end/lib/src/base/processed_options.dart
@@ -4,14 +4,18 @@
 
 import 'dart:async';
 
+import 'package:kernel/target/targets.dart';
+import 'package:kernel/target/vm_fasta.dart';
 import 'package:front_end/compiler_options.dart';
 import 'package:front_end/file_system.dart';
 import 'package:front_end/src/fasta/translate_uri.dart';
+import 'package:front_end/src/fasta/ticker.dart';
 import 'package:front_end/src/base/performace_logger.dart';
 import 'package:front_end/src/incremental/byte_store.dart';
 import 'package:front_end/src/simple_error.dart';
 import 'package:package_config/packages_file.dart' as package_config;
-import 'package:kernel/kernel.dart' show Program, loadProgramFromBytes;
+import 'package:kernel/kernel.dart'
+    show Program, loadProgramFromBytes, CanonicalName;
 
 /// Wrapper around [CompilerOptions] which exposes the options in a form useful
 /// to the front end implementation.
@@ -34,17 +38,40 @@ class ProcessedOptions {
   TranslateUri _uriTranslator;
 
   /// The SDK summary, or `null` if it has not been read yet.
+  ///
+  /// A summary, also referred to as "outline" internally, is a [Program] where
+  /// all method bodies are left out. In essence, it contains just API
+  /// signatures and constants. When strong-mode is enabled, the summary already
+  /// includes inferred types.
   Program _sdkSummaryProgram;
 
   /// The summary for each uri in `options.inputSummaries`.
+  ///
+  /// A summary, also referred to as "outline" internally, is a [Program] where
+  /// all method bodies are left out. In essence, it contains just API
+  /// signatures and constants. When strong-mode is enabled, the summary already
+  /// includes inferred types.
   List<Program> _inputSummariesPrograms;
 
+  /// Other programs that are meant to be linked and compiled with the input
+  /// sources.
+  List<Program> _linkedDependencies;
+
   /// The location of the SDK, or `null` if the location hasn't been determined
   /// yet.
   Uri _sdkRoot;
-
   Uri get sdkRoot => _sdkRoot ??= _normalizeSdkRoot();
 
+  Uri _sdkSummary;
+  Uri get sdkSummary => _sdkSummary ??= _computeSdkSummaryUri();
+
+  Ticker _ticker;
+  Ticker get ticker => _ticker ??= new Ticker(isVerbose: _raw.verbose);

[ahe, 2017/07/05 13:29:39]

The ticker should be created eagerly. Otherwise it doesn't count from the
beginning.

[Siggi Cherem (dart-lang), 2017/07/05 18:42:08]

Done.

+
+  bool get verbose => _raw.verbose;
+  bool get verify => _raw.verify;
+  bool get debugDump => _raw.debugDump;
+
   /// Initializes a [ProcessedOptions] object wrapping the given [rawOptions].
   ProcessedOptions(CompilerOptions rawOptions) : this._raw = rawOptions;
 
@@ -62,24 +89,25 @@ class ProcessedOptions {
   /// if an option is a path to a file, it checks that the file exists.
   Future<bool> validateOptions() async {
     var fs = _raw.fileSystem;
-    var root = _raw.sdkRoot;
 
     bool _report(String msg) {
       _raw.onError(new SimpleError(msg));
       return false;
     }
 
-    if (root != null && !await fs.entityForUri(root).exists()) {
-      return _report("SDK root directory not found: ${_raw.sdkRoot}");
+    if (_raw.sdkRoot != null && !await fs.entityForUri(sdkRoot).exists()) {
+      return _report("SDK root directory not found: ${sdkRoot}");
     }
 
-    var summary = _raw.sdkSummary;
+    var summary = sdkSummary;
     if (summary != null && !await fs.entityForUri(summary).exists()) {
-      return _report("SDK summary not found: ${_raw.sdkSummary}");
+      return _report("SDK summary not found: ${summary}");
     }
 
-    // TODO(sigmund): add checks for options that are meant to be disjoint (like
-    // sdkRoot and sdkSummary).
+    if (compileSdk && summary != null) {
+      return _report(
+          "The compileSdk and sdkSummary options are mutually exclusive");
+    }
     return true;
   }
 
@@ -101,29 +129,51 @@ class ProcessedOptions {
   /// Whether to interpret Dart sources in strong-mode.
   bool get strongMode => _raw.strongMode;
 
-  /// Get an outline program that summarizes the SDK.
-  Future<Program> get sdkSummaryProgram async {
+  Target _target;
+  Target get target => _target ??=
+      _raw.target ?? new VmFastaTarget(new TargetFlags(strongMode: strongMode));
+
+  /// Get an outline program that summarizes the SDK, if any.
+  Future<Program> loadSdkSummary(CanonicalName nameRoot) async {

[ahe, 2017/07/05 13:29:39]

This doesn't feel like an option to me.

[Siggi Cherem (dart-lang), 2017/07/05 18:42:09]

Agree - I don't believe we want to put this separately though, maybe it is time
to rename this class instead?

Somehow I feel "CompilerContext" is the right name for this too. Which is not
surprising because I'd like to merge this with the existing `CompilerContext`
(or at least I wanted CompilerContext to have a reference to this object).


Possible next steps:
 (a) keep the CL as is with a TODO
 (b) move these 3 functions to CompilerContext, and replace
CompilerContxt.options with processedoptions
 (c) merge CompilerContext + ProcessedOptions entirely.

If you are OK, I'd like to start with (a) and discuss offline about whether we
want to do (b), (c) or something else.

     if (_sdkSummaryProgram == null) {
-      if (_raw.sdkSummary == null) return null;
-      _sdkSummaryProgram = await _loadProgram(_raw.sdkSummary);
+      if (sdkSummary == null) return null;
+      _sdkSummaryProgram = await loadProgram(sdkSummary, nameRoot);
     }
     return _sdkSummaryProgram;
   }
 
   /// Get the summary programs for each of the underlying `inputSummaries`
   /// provided via [CompilerOptions].
-  Future<List<Program>> get inputSummariesPrograms async {
+  Future<List<Program>> loadInputSummaries(CanonicalName nameRoot) async {

[ahe, 2017/07/05 13:29:39]

Ditto.

[Siggi Cherem (dart-lang), 2017/07/05 18:42:09]

Acknowledged.

     if (_inputSummariesPrograms == null) {
       var uris = _raw.inputSummaries;
       if (uris == null || uris.isEmpty) return const <Program>[];
-      _inputSummariesPrograms = await Future.wait(uris.map(_loadProgram));
+      _inputSummariesPrograms = <Program>[];
+      await Future.forEach(uris, (uri) async {
+        _inputSummariesPrograms.add(await loadProgram(uri, nameRoot));

[Paul Berry, 2017/06/29 18:51:07]

Previously, the use of Future.wait() would mean that multiple I/O operations got
scheduled concurrently.  With this change, we serialize the I/O.  Our experience
with analysis server is that such a change can really slow things down if you
are reading from a network filesystem.  Is this an intentional behavior change? 
If so, what's the advantage?

[Siggi Cherem (dart-lang), 2017/06/30 04:12:02]

It was initially deliberate, but I didn't need to go as far as making IO
operations in sequence.  PTAL.

The main reason I did this in order was to guarantee that we load into
[nameRoot] each library in the order that was provided. I was afraid of loading
out of order and risk that we would get the "wrong" definition for a library,
class, or member.

It appears that this is handled by the kernel deserialization, though (e.g. if
an existing definition is available, it merges the data with it), but I'm not
100% trusting that it is designed to be commutative. I didn't see any issues in
my tests, but I still don't trust it.

I decided instead to split this step in two: load bytes in parallel and then
deserialize the programs in sequence. This might be a good middleground for your
concern and mine.

[ahe, 2017/07/05 13:29:39]

We should throttle the number of concurrent operations. I've noticed that
starting too many asynchronous operations concurrently with Future.wait
completely locks up the system (on Mac OS X).

[Siggi Cherem (dart-lang), 2017/07/05 18:42:08]

Good to know. Added TODO for now.

+      });
     }
     return _inputSummariesPrograms;
   }
 
-  Future<Program> _loadProgram(Uri uri) async {
+  /// Load each of the [CompilerOptions.linkedDependencies] programs.
+  Future<List<Program>> loadLinkDependencies(CanonicalName nameRoot) async {

[ahe, 2017/07/05 13:29:39]

Also doesn't seem like an option.

[Siggi Cherem (dart-lang), 2017/07/05 18:42:08]

Acknowledged.

+    if (_linkedDependencies == null) {
+      var uris = _raw.linkedDependencies;
+      if (uris == null || uris.isEmpty) return const <Program>[];
+      _linkedDependencies = <Program>[];
+      await Future.forEach(uris, (uri) async {
+        _linkedDependencies.add(await loadProgram(uri, nameRoot));

[Paul Berry, 2017/06/29 18:51:07]

Similar concern here.

[Siggi Cherem (dart-lang), 2017/06/30 04:12:02]

Done.

+      });
+    }
+    return _linkedDependencies;
+  }
+
+  /// Helper to load a .dill file from [uri] using the existing [nameRoot].
+  Future<Program> loadProgram(Uri uri, CanonicalName nameRoot) async {
     var bytes = await fileSystem.entityForUri(uri).readAsBytes();
-    return loadProgramFromBytes(bytes)..unbindCanonicalNames();
+    var result = loadProgramFromBytes(bytes, new Program(nameRoot: nameRoot));

[Paul Berry, 2017/06/29 18:51:07]

Nit: why not just

return loadProgramFromBytes(...);

[Siggi Cherem (dart-lang), 2017/06/30 04:12:02]

Thanks - I had an extra debugging step earlier and forgot to undo the tmp
variable.

+    return result;
   }
 
   /// Get the [TranslateUri] which resolves "package:" and "dart:" URIs.
@@ -135,13 +185,19 @@ class ProcessedOptions {
       await _getPackages();
       // TODO(scheglov) Load SDK libraries from whatever format we decide.
       // TODO(scheglov) Remove the field "_raw.dartLibraries".
-      _uriTranslator = new TranslateUri(
-          _packages, _raw.dartLibraries, const <String, List<Uri>>{});
-      _uriTranslator.dartLibraries.addAll(_raw.dartLibraries);
+      var libraries = _raw.dartLibraries ?? await _parseLibraries();
+      _uriTranslator =
+          new TranslateUri(_packages, libraries, const <String, List<Uri>>{});
+      ticker.logMs("Read packages file");
     }
     return _uriTranslator;
   }
 
+  Future<Map<String, Uri>> _parseLibraries() async {
+    Uri librariesJson = _raw.sdkRoot?.resolve("lib/libraries.json");
+    return await computeLibraries(fileSystem, librariesJson);
+  }
+
   /// Get the package map which maps package names to URIs.
   ///
   /// This is an asynchronous getter since file system operations may be
@@ -176,8 +232,19 @@ class ProcessedOptions {
     }
     var root = _raw.sdkRoot;
     if (!root.path.endsWith('/')) {
-      root = root.replace(path: _sdkRoot.path + '/');
+      root = root.replace(path: root.path + '/');
     }
     return root;
   }
+
+  /// Get or infer the location of the SDK summary.
+  Uri _computeSdkSummaryUri() {
+    if (_raw.sdkSummary != null) return _raw.sdkSummary;
+
+    // Infer based on the sdkRoot, but only when `compileSdk` is false,
+    // otherwise the default intent was to compile the sdk from sources and not
+    // to load an sdk summary file.
+    if (_raw.compileSdk) return null;
+    return sdkRoot.resolve('outline.dill');
+  }
 }