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

pkg/front_end/lib/compiler_options.dart

Index: pkg/front_end/lib/compiler_options.dart
diff --git a/pkg/front_end/lib/compiler_options.dart b/pkg/front_end/lib/compiler_options.dart
index c360ee152c69c78286e090761366641a933a77b4..d82416887f54aff973fe17bb96f6f2391ba3ff52 100644
--- a/pkg/front_end/lib/compiler_options.dart
+++ b/pkg/front_end/lib/compiler_options.dart
@@ -6,6 +6,7 @@ library front_end.compiler_options;
 
 import 'package:front_end/src/base/performace_logger.dart';
 import 'package:front_end/src/incremental/byte_store.dart';
+import 'package:kernel/target/targets.dart' show Target;
 
 import 'compilation_error.dart';
 import 'file_system.dart';
@@ -25,13 +26,15 @@ class CompilerOptions {
   ///
   /// If `null`, the SDK will be searched for using
   /// [Platform.resolvedExecutable] as a starting point.
-  ///
-  /// This option is mutually exclusive with [sdkSummary].
   Uri sdkRoot;
 
   /// Map of `dart.xyz` libraries to URIs in the [fileSystem].

[ahe, 2017/06/29 11:09:15]

dart:xyz.

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

Done.

   /// E.g. {'core': 'file:///sdk/lib/core/core.dart'} (no `dart:` prefix).
-  Map<String, Uri> dartLibraries = {};
+  ///
+  /// If `null`, the default set of libraries will be loaded from
+  /// `sdkRoot/lib/libraries.json`.
+  // TODO(sigmund): also provide an option to specify the .json file.
+  Map<String, Uri> dartLibraries;

[ahe, 2017/06/29 11:09:15]

I think the only option should be a single URI that points to the .json file.

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

Happy to look into this in a follow-up CL and discuss further. I added a comment
in the TODO about this.

In particular:
 - I like having fewer ways to specify the sdk libraries if they are redundant
with each other, once I add the explicit .json URI option, there is potential
for redundancy here.

 - the Map is a very easy to use API in testing, but I commonly have to create a
memory-file system in those cases, adding a json file in that file system seems
as easy and not a big issue.

We can discuss more together with Paul about this - there might be other
motivations for also supporting a Map API.

 
   /// Callback to which compilation errors should be delivered.
   ///
@@ -49,36 +52,34 @@ class CompilerOptions {
   Uri packagesFileUri;
 
   /// URIs of input summary files (excluding the SDK summary; typically these
-  /// will be "file:" URIs).  These files should all be linked summaries.  They
-  /// should also be closed, in the sense that any libraries they reference
-  /// should also appear in [inputSummaries] or [sdkSummary].
-  List<Uri> inputSummaries = [];
-
-  /// URI of the SDK summary file (typically a "file:" URI).
+  /// will be "file:" URIs).
   ///
-  /// This should be a linked summary.  If `null`, the SDK summary will be
-  /// searched for at a default location within [sdkRoot].
+  /// These files should be summary files generated by this package (and not the
+  /// similarly named summary files from `package:analyzer`.)
   ///
-  /// This option is mutually exclusive with [sdkRoot].  TODO(paulberry): if the
-  /// VM does not contain a pickled copy of the SDK, we might need to change
-  /// this.
-  Uri sdkSummary;
+  /// Summaries may be provided in any order, but they should be acyclic and
+  /// closed: any libraries that they reference should be defined in either one
+  /// of [inputSummaries] or [sdkSummary].
+  List<Uri> inputSummaries = [];
 
-  /// URI override map.
+  /// URIs of other kernel programs to link.
   ///
-  /// This is a map from URIs that might appear in import/export/part statements
-  /// to URIs that should be used to locate the corresponding files in the
-  /// [fileSystem].  Any URI override listed in this map takes precedence over
-  /// the URI resolution that would be implied by the packages file (see
-  /// [packagesFileUri]) and/or [multiRoots].
+  /// Commonly used to link the code for the SDK libraries that was compiled
+  /// separately. Programs should be closed an acyclic: any libraries that they

[ahe, 2017/06/29 11:09:15]

an -> and.

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

Done.

+  /// reference should be defined in a program in [linkedDependencies] or any of
+  /// the [inputSummaries] or [sdkSummary].
+  /// CODE REVIEW COMMENT: this is used for the dart2js use case: they want to

[ahe, 2017/06/29 11:09:14]

This comment could be rewritten to something like:

For example, dart2js wants a linked version of the SDK, but the Dart VM ...

This way we can keep the information here.

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

thanks for the suggestion. Updated the doc.

+  /// link the SDK, not just use an outline to build the program.
+  List<Uri> linkedDependencies = [];
+
+  /// URI of the SDK summary file (typically a "file:" URI).
   ///
-  /// If a URI is not listed in this map, then the normal URI resolution
-  /// algorithm will be used.
+  /// This should should be a summary previosly generated by this package (and
+  /// not the similarly named summary files from `package:analyzer`.)
   ///
-  /// TODO(paulberry): transition analyzer and dev_compiler to use the
-  /// "multi-root:" mechanism, and then remove this.
-  @deprecated
-  Map<Uri, Uri> uriOverride = {};
+  /// If `null` and [compileSdk] is false, the SDK summary will be searched for
+  /// at a default location within [sdkRoot].
+  Uri sdkSummary;
 
   /// Multi-roots.
   ///
@@ -110,7 +111,10 @@ class CompilerOptions {
   /// The logger to report compilation progress.
   PerformanceLog logger = new PerformanceLog(new StringBuffer());
 
-  /// Whether to generate code for the SDK when compiling a whole-program.
+  /// Whether to generate code for the SDK.
+  ///
+  /// By default the front end resolves programs using a prebuilt SDK summary.
+  /// When this option is `true`, [sdkSummary] must be null.
   bool compileSdk = false;
 
   /// Whether a modular build compiles only the files listed explicitly or if it
@@ -128,12 +132,6 @@ class CompilerOptions {
   /// Whether to interpret Dart sources in strong-mode.
   bool strongMode = true;

[ahe, 2017/06/29 11:09:15]

This should be a per-package option, not a global option.

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

Not sure either - this might be a question for the language team actually. I
don't believe that's a mode that they intend to support.

 
-  // All options below are target-specific options.
-  //
-  // TODO(sigmund): revisit the right layout for these options. We might want to
-  // split them out into a separate bag of options or provide factories for
-  // common combinations of these options.
-
   /// Patch files to apply on the core libraries for a specific target platform.
   ///
   /// Keys on this map are expected to be `dart:*` URIs. The values can be

[ahe, 2017/06/29 11:09:14]

This is inconsistent with dartLibraries above, which removes the leading dart:.

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

Good point. Done.

@@ -141,9 +139,58 @@ class CompilerOptions {
   /// relative URIs are resolved from the [sdkRoot].
   Map<Uri, List<Uri>> targetPatches = {};

[ahe, 2017/06/29 11:09:15]

Think this should be single URI that points to a file instead.

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

I plan to add the .json input like with libraries.json.

As we discussed offline, I'm inclined in making both a single .json file
containing both, since most platforms have variations in both the libraries they
support and the patch-file location.

Added TODO to handle this separately.

 
-  /// Additional core libraries to be loaded when building a program.
-  // TODO(sigmund): delete. Ideally building a program only needs what's
-  // reachable and we can use kernelForBuildUnit when creating a snapshot of the
-  // SDK itself.
-  List<Uri> additionalLibraries = [];
+  /// The target platform that will consume the compiled code.
+  ///
+  /// Used to provide platform-specific details to the compiler like:
+  ///   * the set of libraries are part of a platform's SDK (e.g. dart:html for
+  ///     dart2js, dart:ui for flutter).
+  ///
+  ///   * what kernel transformations should be applied to the program
+  ///     (async/await, mixin inlining, etc).
+  ///
+  ///   * how to deal with non-standard features like `native` extensions.
+  ///
+  /// If not specified, the default target is the VM.
+  Target target;
+
+  /// Whether to show verbose messages (mainly for debugging and performance
+  /// tracking).

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

Clarify whether these are shown by print()ing them, or by some other reporting
API

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

Done and filed a bug that we want to change this.

+  bool verbose = false;
+
+  /// Whether to run extra verification steps to validate that compiled programs
+  /// are well formed.

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

If verification fails, how is the error reported?  Through onError or some other
mechanism?

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

Well.. both via onError and stdout (I even had a bug that I passed it to onError
when verifying programs, but not when verifying summaries, I just made it
consistent)

I added documentation here that we intend to report via onError, and created
Issue #30056 to track that we want to remove the stdout reporting.

+  bool verify = false;
+
+  /// Whether to dump generated programs in a text format (also mainly for
+  /// debugging).
+  bool debugDump = false;

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

Clarify whether the dump is shown by print() or through some other mechanism.

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

Thanks. Done

+
+  CompilerOptions();
+
+  /// Creates a new intance with options copied from [other].
+  CompilerOptions.from(CompilerOptions other)
+      : byteStore = other.byteStore,
+        chaseDependencies = other.chaseDependencies,
+        compileSdk = other.compileSdk,
+        dartLibraries = other.dartLibraries != null
+            ? new Map.from(other.dartLibraries)
+            : null,
+        debugDump = other.debugDump,
+        declaredVariables = other.declaredVariables != null
+            ? new Map.from(other.declaredVariables)
+            : null,
+        fileSystem = other.fileSystem,
+        inputSummaries = new List.from(other.inputSummaries),
+        linkedDependencies = new List.from(other.linkedDependencies),
+        logger = other.logger,
+        multiRoots = new List.from(other.multiRoots),
+        onError = other.onError,
+        packagesFileUri = other.packagesFileUri,
+        sdkRoot = other.sdkRoot,
+        sdkSummary = other.sdkSummary,
+        strongMode = other.strongMode,
+        target = other.target,
+        targetPatches = new Map.from(other.targetPatches),
+        verbose = other.verbose,
+        verify = other.verify;
 }