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

pkg/front_end/lib/src/kernel_generator_impl.dart

+// Copyright (c) 2017, 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.
+
+/// Defines the front-end API for converting source code to Dart Kernel objects.
+library front_end.kernel_generator_impl;
+
+import 'dart:async' show Future;
+import 'dart:async';
+
+import 'package:kernel/kernel.dart' show Program, CanonicalName;
+
+import '../compiler_options.dart';
+import '../file_system.dart';
+import 'base/processed_options.dart';
+import 'fasta/dill/dill_target.dart' show DillTarget;
+import 'fasta/errors.dart' show InputError;
+import 'fasta/kernel/kernel_outline_shaker.dart';
+import 'fasta/kernel/kernel_target.dart' show KernelTarget;
+import 'fasta/kernel/verifier.dart';
+import 'fasta/kernel/utils.dart';
+import 'fasta/translate_uri.dart' show TranslateUri;
+import 'simple_error.dart';
+import 'fasta/errors.dart';
+
+/// CODE REVIEW COMMENT: Should we expose this in the public API as well (it is
+/// used to implement all 3 APIs, but also used directly in patched_sdk)
+
+/// Implementation for the `package:front_end/kernel_generator.dart` and
+/// `package:front_end/summary_generator.dart` APIs.
+Future<CompilerResult> generateKernel(
+    Iterable<Uri> sources, CompilerOptions options,
+    {bool buildSummary: false,
+    bool buildProgram: true,
+    bool trimDependencies: false}) async {
+  report(String msg) {
+    options.onError(new SimpleError(msg));

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

What is a SimpleError?

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

An error object that needs a better name? ;-)

The external API provides an interface for an error class, SimpleError was a
default implementation that only had a constructor with a string input (no error
location etc). 

For now, I renamed it to something more generic ("compilation-error") and all
the error reporting logic is handled behind the processed-options object, so
this class it now only used there.

+    return null;
+  }
+
+  var fs = options.fileSystem;
+
+  for (var source in sources) {
+    if (source.scheme == 'file' && !await fs.entityForUri(source).exists()) {
+      return report("Entry-point file not found: $source");
+    }
+  }
+
+  if (!options.chaseDependencies) {
+    fs = new HermeticFileSystem(sources.toSet(), fs);

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

It would be cool if we moved this logic into ProcessedOptions, so that we could
be confident that *all* of the front_end APIs are properly hermetic when
chaseDependencies is `true`.

Incidentally, I think in the long run we're going to have other reasons to wrap
the filesystem as well, e.g. to implement "multi-root" functionality.  That
logic should go into ProcessedOptions too, so that all the APIs get the benefit
of it.

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

Done.

I initially had discarded this idea because I thought the intent was for
ProcessedOptions to be just a wrapper on top of CompilerOptions and that we
wanted inputs to the APIs to be given explicitly here instead. Without the
inputs, we couldn't create the hermetic file system.

Thinking more about it, though, there is no reason why ProcessedOptions couldn't
have the inputs stored in it. Your other suggestion about computing a default
for chaseDependencies pushed me already in the direction of passing more data to
processed options, to the point that I think it makes sense to include there all
data that was provided from the user (including entry-points).

Let me know what you think of the new approach.

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

I like it!  Thanks :)

+  }
+
+  var pOptions = new ProcessedOptions(options);
+  pOptions.ticker.logMs("Parsed arguments");

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

This will always report that parsing arguments took 0ms. The Ticker needs to be
created on line 36.

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

Done - now ticker is created eagerly when allocating ProcessedOptions, which
happens before generateKernel gets called.

The "parsed arguments" event doesn't quite make sense anymore, so I got rid of
it.

+
+  if (!await pOptions.validateOptions()) return null;
+  pOptions.ticker.logMs("Validated arguments");
+
+  try {
+    TranslateUri uriTranslator = await pOptions.getUriTranslator();
+
+    var dillTarget =
+        new DillTarget(pOptions.ticker, uriTranslator, pOptions.target);
+
+    CanonicalName nameRoot = new CanonicalName.root();
+    Set<Uri> externalLibs(Program program) {
+      return program.libraries
+          .where((lib) => lib.isExternal)
+          .map((lib) => lib.importUri)
+          .toSet();
+    }
+
+    var sdkSummary = await pOptions.loadSdkSummary(nameRoot);
+    if (sdkSummary != null) {
+      var excluded = externalLibs(sdkSummary);
+      dillTarget.loader
+          .appendLibraries(sdkSummary, (uri) => !excluded.contains(uri));
+    }
+
+    // TODO(sigmund): provide better error reporting if input summaries or
+    // linked dependencies were listed out of order (or provide mechanism to
+    // sort them).
+    for (var inputSummary in await pOptions.loadInputSummaries(nameRoot)) {
+      var excluded = externalLibs(inputSummary);
+      dillTarget.loader
+          .appendLibraries(inputSummary, (uri) => !excluded.contains(uri));
+    }
+
+    // All summaries are considered external and shouldn't include source-info.
+    // TODO(sigmund): rather than clearing uriToSource here, it would be nice if
+    // the serializer could filter uriToSource as well.
+    dillTarget.loader.libraries.forEach((lib) => lib.isExternal = true);
+    dillTarget.loader.uriToSource.clear();

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

Why do we clear this? Have you made sure that stack traces are correct for
platform libraries?

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

Deleted this, but for a different reason :)

The background:

I was clearing this because we don't use it: none of the target platforms uses
source-info from external libraries. For instance, the VM loads the
platform.dill directly (which does contain the uriToSource data in it). The
script itself may contain references to platform APIs, but we don't even send
those external libraries to the VM. In contrast, Dart2js uses
`linkedDependencies` for the sdk, those are added further below and their
uriToSource remains intact.

A couple design questions that affect what we should do:
(a)- should summaries/outlines have uriToSource when we generate them?

I don't think they should. When I first wrote this `clear` line, the
`buildSummary` code-path below was not doing so. Now that it is, we shouldn't
need to clear it here if the input summary was built correctly.

(b.1)- should we support loading any .dill file and treat it as a summary, even
if it has bodies and uriToSource?

I don't know. This might depend on how the summary format evolves, maybe we
can't?

(b.2)- If yes, should this behave the same as if the input was an outline?

I don't know either. Clearing the uriToSource here was part of hiding the
differences, but there might be more. I think in scenarios where we really care
about how things are composed, we will use the API to generate summaries and
will not try to use full .dill files.


Given all of that, I think for now it is sufficient to clear the uriToSource
when we generate the summaries, so I've dropped it here.

+
+    // Linked dependencies are meant to be part of the program so they are not
+    // marked external.
+    for (var dependency in await pOptions.loadLinkDependencies(nameRoot)) {
+      var excluded = externalLibs(dependency);
+      dillTarget.loader
+          .appendLibraries(dependency, (uri) => !excluded.contains(uri));
+    }
+
+    await dillTarget.buildOutlines();
+
+    var kernelTarget = new KernelTarget(fs, dillTarget, uriTranslator);
+    sources.forEach(kernelTarget.read);
+    Program summaryProgram =
+        await kernelTarget.buildOutlines(nameRoot: nameRoot);
+    List<int> summary = null;
+    if (buildSummary) {
+      if (trimDependencies) {
+        // TODO(sigmund): see if it is worth supporting this.  Trimming the
+        // program here will affect how we build the program later. To support
+        // both we'd have to clone the program first.

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

To support both what?

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

buildSummary & buildProgram at once + trimming deps

Trimming is destructive, so if you do buildSummary + buildProgram, by the time
we run kernelTarget.buildProgram we have deleted stuff in the outline that we
might need later.

reworded.

+        assert(!buildProgram);
+        var excluded =
+            dillTarget.loader.libraries.map((lib) => lib.importUri).toSet();
+        trimProgram(summaryProgram, (uri) => !excluded.contains(uri));
+      }
+      if (options.verify) verifyProgram(summaryProgram);
+      if (kernelTarget.errors.isEmpty) {
+        summary = serializeProgram(summaryProgram, excludeUriToSource: true);
+      }
+      pOptions.ticker.logMs("Generated outline");
+    }
+
+    Program program;
+    if (buildProgram && kernelTarget.errors.isEmpty) {
+      program = await kernelTarget.buildProgram(verify: pOptions.verify);
+      if (trimDependencies) {
+        var excluded =
+            dillTarget.loader.libraries.map((lib) => lib.importUri).toSet();
+        trimProgram(program, (uri) => !excluded.contains(uri));
+      }
+      if (pOptions.debugDump) printProgramText(program);

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

libraryFilter option missing here.

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

Done.

+      pOptions.ticker.logMs("Generated program");
+    }
+
+    if (kernelTarget.errors.isNotEmpty) {
+      kernelTarget.errors.forEach(report);
+      return null;
+    }
+
+    return new CompilerResult(
+        summary: summary,
+        program: program,
+        deps: kernelTarget.loader.getDependencies());
+  } on InputError catch (e) {
+    report(e.format());
+    return null;
+  }
+}
+
+/// Result object of [generateKernel].
+class CompilerResult {
+  /// The generated summary bytes, if it was requested.
+  final List<int> summary;
+
+  /// The generated program, if it was requested.
+  final Program program;
+
+  /// Dependencies traversed by the compiler. Used only for generating
+  /// dependency .GN files in the dart-sdk build system.
+  /// Note this might be removed when we switch to compute depencencies without
+  /// using the compiler itself.
+  final List<Uri> deps;
+
+  CompilerResult({this.summary, this.program, this.deps});
+}
+
+/// A [FileSystem] that only allows access to files that have been explicitly
+/// whitelisted.
+class HermeticFileSystem implements FileSystem {
+  final Set<Uri> includedFiles;
+  final FileSystem _realFileSystem;
+
+  HermeticFileSystem(this.includedFiles, this._realFileSystem);
+
+  FileSystemEntity entityForUri(Uri uri) {
+    if (includedFiles.contains(uri)) return _realFileSystem.entityForUri(uri);
+    return inputError(

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

Throw FileSystemException instead, then Fasta can recover as from any other
file-system exception. Eventually, when I implement that (I have an open bug on
it).

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

Done. I like it.

+        null,

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

uri

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

n/a now that I use FileSystemException.

However, if I didn't change the exception, I thought I had to use `null` here.
Could you add a comment to InputError/inputError about what Uri is supposed to
be?

My understanding was that this uri would be used for the location where the
error is reported. For example, if we have `package:a/a.dart` with these
contents:

  import 'b.dart';
  ...

but `b.dart` doesn't. I expect `b.dart` to be mentioned in the message body, but
the Uri in input error would be `package:a/a.dart` and the offset would
highlight the import line above.

+        -1,
+        'Invalid access to $uri: '
+        'the file is accessed in a modular hermetic build '
+        '(where chaseDependencies is false), but it was not '
+        'explicitly listed as an input.');
+  }
+}