Use sdk summaries in front_end/kernel_generator.

pkg/front_end/lib/kernel_generator.dart

Index: pkg/front_end/lib/kernel_generator.dart
diff --git a/pkg/front_end/lib/kernel_generator.dart b/pkg/front_end/lib/kernel_generator.dart
index ce9eee88a23557ccf57f2018013505886939a734..67bd3c17b3f4446f0a4d3559acd0599153c71815 100644
--- a/pkg/front_end/lib/kernel_generator.dart
+++ b/pkg/front_end/lib/kernel_generator.dart
@@ -9,6 +9,9 @@ import 'compilation_error.dart';
 import 'compiler_options.dart';
 import 'dart:async';
 
+import 'package:analyzer/src/generated/source.dart' show SourceKind;
+import 'package:analyzer/src/summary/package_bundle_reader.dart'
+    show InSummarySource;
 // TODO(sigmund): move loader logic under front_end/lib/src/kernel/
 import 'package:kernel/analyzer/loader.dart';
 import 'package:kernel/kernel.dart';
@@ -30,7 +33,8 @@ import 'package:source_span/source_span.dart' show SourceSpan;
 /// TODO(paulberry): will the VM have a pickled version of the SDK inside it? If
 /// so, then maybe this method should not convert SDK libraries to kernel.
 Future<Program> kernelForProgram(Uri source, CompilerOptions options) async {
-  var loader = await _createLoader(options);
+  var loader = await _createLoader(options, entry: source);
+  // TODO(sigmund): consider adding support for sdk summaries here as well.
   Program program = loader.loadProgram(source);
   _reportErrors(loader.errors, options.onError);
   return program;
@@ -63,20 +67,48 @@ Future<Program> kernelForProgram(Uri source, CompilerOptions options) async {
 /// caller to match up referenced elements to the summary files they were
 /// obtained from?
 Future<Program> kernelForBuildUnit(
-        List<Uri> sources, CompilerOptions options) async {
+    List<Uri> sources, CompilerOptions options) async {
   var repository = new Repository();
-  var loader = await _createLoader(options, repository: repository);
-  // TODO(sigmund): add special handling for part files.
-  sources.forEach(loader.loadLibrary);
+  var loader = await _createLoader(options,
+      repository: repository, entry: sources.first);

[Paul Berry, 2016/12/09 13:41:40]

It seems weird to treat the first source any differently than the others.

Would it be better if we just said that the caller is required to set
CompilerOptions.packagesFilePath when calling `kernelForBuildUnit`?  Then we
wouldn't need to make an arbitrary decision about which file to use for package
discovery.

[Siggi Cherem (dart-lang), 2016/12/09 16:58:21]

Good point, will do once we decide on the other aspects below

[Siggi Cherem (dart-lang), 2016/12/09 20:46:40]

Done.

+  var context = loader.context;
+
+  // Process every library in the build unit.
+  for (var uri in sources) {
+    var source = context.sourceFactory.forUri2(uri);
+    //  We ignore part files, those are handled by their enclosing library.
+    if (context.computeKindOf(source) == SourceKind.PART) continue;
+    loader.loadLibrary(uri);
+  }
+
+  // Process any library that has been referenced and that was not loaded from a
+  // summary file.

[Paul Berry, 2016/12/09 13:41:40]

This is actually undesirable behavior IMHO.  It contradicts the comment above
("The compilation process is hermetic, meaning that the only files which will be
read are those listed in [sources], [CompilerOptions.inputSummaries], and
[CompilerOptions.sdkSummary].")  Keeping the build process hermetic makes the
code simpler, has desirable properties for Bazel integration, and is consistent
with what we're doing for DDC.

I think we should replace this loop with some error checking that verifies that
all referenced libraries either come from summaries or are included in
`sources`.

On a related note, we should also have some error checking to verify that for
each "part" declaration in a library in `sources`,   the corresponding part file
is also in `sources`.

[Siggi Cherem (dart-lang), 2016/12/09 16:58:21]

Let's chat more about this.

I initially was kin to keeping it hermetic too, but then realized this makes it
much easier to use outside of the context of blaze, e.g. from the command line.
I believe DDC also supports this mode of operation on the command line after
some user feedback. IIUC, there is a preference to list just a few entry-points
and not all the reachable files.

For this specific CL, I'm happy to change it back, but I'd like to figure out
how to support the following use cases:
(a) generate kernel for whole program (including sdk)
(b) generate kernel for whole program user code (no sdk is compiled)
(c) generate kernel for whole program user code + attach kernel for the sdk (sdk
is not compiled, but it is attached)
(d) generate kernel for a build unit.
(e) generate kernel for the sdk

Currently we do (a) via generateForProgram, and (d) via generateForBuildUnit.

This CL is sort of doing (b) by changing generateForBuildUnit to make it
non-hermetic. An alternative is to do it like (a) with an option to exclude the
SDK. I'd be happy to do that for now. Let's chat offline about what we do with
(c) and (e) as well. (c) is needed short term to test how to run a program until
the VM supports loading the sdk from a snapshot.

[Siggi Cherem (dart-lang), 2016/12/09 20:46:40]

Updated the code based on an offline discussion. Here is the summary:
- I made it hermetic by default.
- I added CompilerOptions.chaseDependencies to let you override the behavior and
include extra files. 
- kernelForProgram chases dependencies by default, it handles use-case #a and #b
right now.
- use case #c is expected to be temporary and we might not submit that to the
code (it wont be necessary as soon as the VM can load kernel with external
references to the sdk)

I agree about checking that [part] files are covered, but made it a TODO for
now.

+  for (int i = 0; i < repository.libraries.length; ++i) {

[Paul Berry, 2016/12/09 13:41:40]

Nit: since `i` is not used, this would be clearer as:

for (var lib in repository.libraries)

But I guess it's moot if we get rid of this loop :)

[asgerf, 2016/12/09 13:56:53]

I think the number of libraries will grow during iteration as more libraries are
loaded, so maybe that should be clarified in a comment.

[Siggi Cherem (dart-lang), 2016/12/09 16:58:21]

Asger is correct - the list is growing as more libraries are discovered, so the
list of libraries behaves like a work-queue.

[Siggi Cherem (dart-lang), 2016/12/09 20:46:40]

Done, added some documentation.

+    var lib = repository.libraries[i];
+    var source = context.sourceFactory.forUri2(lib.importUri);
+    if (source is InSummarySource) continue;
+    // Depending on the context we can allow or report an error when a library
+    // is loaded here. For command-line tools we want to allow it to
+    // make it easy to define build units in terms of a few entry point files.
+    // For a closed build system like bazel, we can report an error.
+    // TODO(sigmund): expose an option to indicate that this should be an error.
+    loader.ensureLibraryIsLoaded(lib);
+  }
+
   Program program = new Program(repository.libraries);
   _reportErrors(loader.errors, options.onError);
   return program;
 }
 
+/// Create a [DartLoader] using the provided [options].
+///
+/// If [options] contain no configuration to resolve `.packages`, the [entry]
+/// file will be used to search for a `.packages` file.
 Future<DartLoader> _createLoader(CompilerOptions options,
-    {Repository repository}) async {
+    {Repository repository, Uri entry}) async {
   var kernelOptions = _convertOptions(options);
-  var packages = await createPackages(options.packagesFilePath);
+  var packages = await createPackages(options.packagesFilePath,
+      discoveryPath: entry?.path);
   return new DartLoader(
       repository ?? new Repository(), kernelOptions, packages);
 }
@@ -84,6 +116,7 @@ Future<DartLoader> _createLoader(CompilerOptions options,
 DartOptions _convertOptions(CompilerOptions options) {
   return new DartOptions(
       sdk: options.sdkPath,
+      sdkSummary: options.sdkSummary,
       packagePath: options.packagesFilePath,
       declaredVariables: options.declaredVariables);
 }