Rework DillLoader to allow adding multiple dills.

pkg/front_end/lib/src/fasta/dill/dill_loader.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 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);
-
-  Future<Null> buildOutline(DillLibraryBuilder builder) async {
-    if (program == null) {
-      byteCount = await new File.fromUri(input).length();
-      setProgram(await loadProgramFromBinary(input.toFilePath()));
+  /// 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;
+      }
     }
-    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);
   }
 
+  Future<Null> buildOutline(DillLibraryBuilder builder) async {
+    builder.library.classes.forEach(builder.addClass);
+    builder.library.procedures.forEach(builder.addMember);
+    builder.library.fields.forEach(builder.addMember);
+  }
+
+  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) {
       read(library.importUri).library = library;
     }
   }
 }