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

pkg/front_end/lib/compiler_options.dart

 // Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 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';
 import 'physical_file_system.dart';
 
 /// Default error handler used by [CompilerOptions.onError].
 void defaultErrorHandler(CompilationError error) => throw error;
 
 /// Callback used to report errors encountered during compilation.
 typedef void ErrorHandler(CompilationError error);

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

Use new syntax?

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

Is the syntax supported everywhere already? happy to do that in a follow up. I'm
also considering using a different API for diagnostics btw.

 
 /// Front-end options relevant to compiler back ends.
 ///
 /// Not intended to be implemented or extended by clients.
 class CompilerOptions {
   /// The URI of the root of the Dart SDK (typically a "file:" URI).
   ///
   /// 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.
   ///
   /// By default, the first error will be reported by throwing an exception of
   /// type [CompilationError].
   ErrorHandler onError = defaultErrorHandler;

[ahe, 2017/07/03 10:05:31]

I think we should provide a stream of diagnostics instead and offer to abort on
first error or warning.

[Siggi Cherem (dart-lang), 2017/07/05 03:39:30]

Yeah, I like the idea of using a stream. I also want a way to have good defaults
(so if no options are passed, we report errors or throw). I think we can come up
with a good API to achieve both. I'll start a thread separately to discuss
ideas.

 
   /// URI of the ".packages" file (typically a "file:" URI).
   ///
   /// If `null`, the ".packages" file will be found via the standard
   /// package_config search algorithm.

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

What is the "standard package_config search algorithm"? Where can one find more
information?

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

Basically the language team provided a default implementation for resolving
.packages in `package:package_config`. The code is here:

https://github.com/dart-lang/package_config

and the algorithm is described here:

https://github.com/lrhn/dep-pkgspec/blob/master/DEP-pkgspec.md#resolving-pack...

   ///
   /// If the URI's path component is empty (e.g. `new Uri()`), no packages file
   /// will be used.

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

This seems brittle. I think it would be better to define a custom URI scheme
that signals "use the algorithm," and have null mean "don't search."

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

Personally I'm not a fan of custom URI schemes acting as a sentinel.  If we want
to change the current behavior, I'd suggest adding an enum that specifies how
the .packages file should be found.

Regardless of what we do, I hope that we can make the default behavior be to use
the standard package_config search algorithm, because that's what I think a
client is most likely to expect.

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

Ha, I had not noticed the special behavior with the empty path.

Let's also put this on a separate CL. My inclination is similar to Paul's:

I'd like the default (`null`) to be the most common behavior of users of the
API, most likely that is to do the lookup and default to an empty map if nothing
is found - we wouldn't issue an immediate error if no .packages is found, but
only later if you try to load a `package:*` URI. 

Paul - do you feel we still need the other option ("pretend it is empty"?).
Because I didn't see this, I've been creating an empty file in the unit tests
already.

[Paul Berry, 2017/07/04 15:12:14]

IIRC, the "pretend it is empty" was added as an afterthought while writing tests
of something (can't remember what).  I'm happy removing this functionality for
now.  If we discover that creating an empty file in the tests becomes too
burdensome (which seems unlikely) we can always re-add it later.

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

Thanks - sounds good. I can remove it on a follow up CL.

   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].
+  /// will be "file:" URIs).
+  ///
+  /// These files should be summary files generated by this package (and not the
+  /// similarly named summary files from `package:analyzer`.)
+  ///
+  /// 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 = [];
 
+  /// URIs of other kernel programs to link.
+  ///
+  /// 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).
   ///
-  /// This should be a linked summary.  If `null`, the SDK summary will be
-  /// searched for at a default location within [sdkRoot].
+  /// This should should be a summary previosly 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.
+  /// If `null` and [compileSdk] is false, the SDK summary will be searched for
+  /// at a default location within [sdkRoot].
   Uri sdkSummary;
 
-  /// URI override map.
-  ///
-  /// 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].
-  ///
-  /// If a URI is not listed in this map, then the normal URI resolution
-  /// algorithm will be used.
-  ///
-  /// TODO(paulberry): transition analyzer and dev_compiler to use the
-  /// "multi-root:" mechanism, and then remove this.
-  @deprecated
-  Map<Uri, Uri> uriOverride = {};
-
   /// Multi-roots.
   ///
   /// Any Uri that resolves to "multi-root:///$rest" will be searched for

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

Is this scheme registered? If not, we should use "org.dartlang.multi-root".

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

No, it should be possible to change it, let's discuss in person. 

For background, this is not used yet, but we will this quarter.

We implemented something similar in dart2js with bazel last year. Instead of a
scheme, we used a prefix of the form:

  file:///bazel-root/

but I like the idea of switching to a scheme because it makes it easier to work
with the URIs afterwards (no need to manipulate the path, just drop the scheme).

Note: besides being useful to find multiple roots, this also helps work with
absolute URIs in a way that is agnostic of the underlying machine directory
layout. Like the `app:` scheme used in package:kernel. This is helpful for
distributed build systems.

   /// at "$root/$rest", where "$root" is drawn from this list.

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

We should use Uri.resolve, not string concatenation.

Also, what happens if $rest starts with "../"?

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

note that $rest was meant to be an absolute path. I switched "rest" to
"absolute-path" in the comment to make this clearer. In that context, resolve or
joining the paths should be equivalent, but I can reword it if you prefer.

   ///
   /// Intended use: if the user has a Bazel workspace located at path
   /// "$workspace", this could be set to the file URIs corresponding to the
   /// paths for "$workspace", "$workspace/bazel-bin",
   /// and "$workspace/bazel-genfiles", effectively overlaying source and
   /// generated files.
   List<Uri> multiRoots = [];
 
   /// The declared variables for use by configurable imports and constant
   /// evaluation.
   Map<String, String> declaredVariables;
 
   /// The [FileSystem] which should be used by the front end to access files.
   ///
   /// All file system access performed by the front end goes through this
   /// mechanism, with one exception: if no value is specified for
   /// [packagesFileUri], the packages file is located using the actual physical
   /// file system.  TODO(paulberry): fix this.
   FileSystem fileSystem = PhysicalFileSystem.instance;
 
   /// The byte storage to get and put serialized data.

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

"get and put" -> "access"

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

Done.

   ByteStore byteStore = new NullByteStore();
 
   /// 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
   /// compiles dependencies as well.
   ///
   /// This option is intended only for modular APIs like `kernelForBuildUnit`.
   /// These APIs by default ensure that builds are hermetic, where all files
   /// that will be compiled are listed explicitly and all other dependencies
   /// are covered by summary files.
   ///
   /// When this option is true, these APIs will treat any dependency that is
   /// not described in a summary as if it was explicitly listed as an input.

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

This reminds me of the completely broken default behavior in javac. This doesn't
work because people think it's a build system (but it isn't). We should revisit
this.

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

Not sure I follow. We can discuss more on our next sync up.

   bool chaseDependencies = false;
 
   /// 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.

   /// either absolute or relative URIs. Absolute URIs are read directly, while
   /// 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;
 }