Rework DillLoader to allow adding multiple dills.

pkg/front_end/lib/src/fasta/dill/dill_loader.dart

Index: pkg/front_end/lib/src/fasta/dill/dill_loader.dart
diff --git a/pkg/front_end/lib/src/fasta/dill/dill_loader.dart b/pkg/front_end/lib/src/fasta/dill/dill_loader.dart
index 281bfff376555d777d16df7d9cac507525a78ded..d70aa043703f8dd3580fef662155af2b209cf045 100644
--- a/pkg/front_end/lib/src/fasta/dill/dill_loader.dart
+++ b/pkg/front_end/lib/src/fasta/dill/dill_loader.dart
@@ -6,43 +6,49 @@ library fasta.dill_loader;
 
 import 'dart:async' show Future;
 
-import 'dart:io' show File;
-
-import 'package:kernel/kernel.dart' show loadProgramFromBinary;
-
 import 'package:kernel/ast.dart' show Library, Program;
 
 import '../loader.dart' show Loader;
-
 import '../target_implementation.dart' show TargetImplementation;
-
 import 'dill_library_builder.dart' show DillLibraryBuilder;
 
 class DillLoader extends Loader<Library> {
-  Uri input;
-
   Program program;

[Siggi Cherem (dart-lang), 2017/05/09 20:14:30]

doesn't have to be in this CL, but we probably only need to store List<Library>
and not Program here. It might be nuance, but it makes it clear to me that this
is just a collection of libraries, not an actual program

[scheglov, 2017/05/09 20:32:21]

Yeah, mostly we use libraries.
But lib/src/fasta/kernel/kernel_target.dart:371 KernelTarget.link() uses also
binary.uriToSource.

 
   DillLoader(TargetImplementation target) : super(target);
 
-  DillLibraryBuilder read(Uri uri, [Uri fileUri]) => super.read(uri, fileUri);
+  /// Add already compiled libraries from the given [dill].  If [uris] is

[Siggi Cherem (dart-lang), 2017/05/09 20:14:30]

nit: a couple suggestions on this comment:
 - I prefer to refer to the program as [program] rather than [dill]. I've been
thinking that maybe just the class should use the term dill, I might send you a
follow up CL with more docs :)
 - I think you are missing a "not" (if uris is *not* null)
 - I would remove the note about dependencies - it matters that all dependencies
are added before running the kernel target/builders, but I don't believe it
matters in what order you add programs here (e.g. a dependency can come before
or after). 

I have a couple more suggestions below about the API, so all-together this might
be more like:

/// Append compiled libraries from the given [program]. If [filter]
/// is provided, this only adds libraries whose Uri is accepted by
/// such filter.

[scheglov, 2017/05/09 20:32:21]

Done.

[Siggi Cherem (dart-lang), 2017/05/09 22:12:37]

After talking on VC I just realized something: is filter mainly used to exclude
dependencies?

For example:
// load sdk dill:
 appendLibraries(sdkDill);

// load library that uses the sdk:
 appendLibraries(packageFooDill, filter(uri) =>
'$uri'.startsWith('package:foo'))?

If we have a modular .dill for this package, all sdk dependencies will be marked
"external", so another option is to filter based on that. If you think that will
handle all cases where we need the filter, then we could remove the extra
argument and instead do:

   if (!library.isExternal) {
     ...
   }


what do you think?

[scheglov, 2017/05/10 00:47:40]

If the idea with serializing just individual strongly linked components of
libraries (without external libraries) works, we might get rid of the filter
here (it will be in BinaryPrinter instead).

+  /// `null`, then only libraries these URIs are added (e.g. to add a library
+  /// without all its already added dependencies).  It is up to the client to
+  /// make sure that the added libraries have all their dependencies added
+  /// before.
+  void addDill(Program dill, [Set<Uri> uris]) {

[Siggi Cherem (dart-lang), 2017/05/09 20:14:30]

some suggestions:
 - rename addDill => appendProgram or addLibrariesFromProgram
 - dill => program

change [Set<Uri> uris] into a filter callback: that way you don't force a
representation of the filter to the clients of this API:

   addLibrariesFromProgram(Program program, [bool filter(Uri uri)])

[scheglov, 2017/05/09 20:32:21]

Done.

+    program ??= new Program();
+    dill.unbindCanonicalNames();
+    for (Library library in dill.libraries) {
+      if (uris == null || uris.contains(library.importUri)) {
+        program.libraries.add(library);
+        read(library.importUri).library = library;
+      }
+    }
+  }
+
+  Future<Null> buildBody(DillLibraryBuilder builder) {
+    return buildOutline(builder);
+  }
 
   Future<Null> buildOutline(DillLibraryBuilder builder) async {
-    if (program == null) {
-      byteCount = await new File.fromUri(input).length();
-      setProgram(await loadProgramFromBinary(input.toFilePath()));
-    }
     builder.library.classes.forEach(builder.addClass);
     builder.library.procedures.forEach(builder.addMember);
     builder.library.fields.forEach(builder.addMember);
   }
 
-  Future<Null> buildBody(DillLibraryBuilder builder) {
-    return buildOutline(builder);
-  }
+  DillLibraryBuilder read(Uri uri, [Uri fileUri]) => super.read(uri, fileUri);
 
   void setProgram(Program program) {

[Siggi Cherem (dart-lang), 2017/05/09 20:14:30]

I'd delete this and use `addDill` in testing/suite.dart instead

[scheglov, 2017/05/09 20:32:21]

Ah... Right, we don't need it anymore!

-    assert(input != null);
+    if (this.program != null) {
+      throw new StateError('The full DILL can be set only once.');
+    }
     this.program = program;
     program.unbindCanonicalNames();
     for (Library library in program.libraries) {