Change the deferred library check tool to allow unspecified packages to be in

lib/deferred_library_check.dart

Index: lib/deferred_library_check.dart
diff --git a/lib/deferred_library_check.dart b/lib/deferred_library_check.dart
index d472eae345809f2ad894f68a587478a8ec759cb2..59ac1d5a9863e6f97895d33e5d393333415b2a80 100644
--- a/lib/deferred_library_check.dart
+++ b/lib/deferred_library_check.dart
@@ -6,31 +6,36 @@
 /// given in a YAML file. The format of the YAML file is:
 ///
 ///     main:
-///       packages:
+///       include:
 ///         - some_package
 ///         - other_package
 ///
 ///     foo:
-///       packages:
+///       include:
 ///         - foo

[Siggi Cherem (dart-lang), 2015/12/18 02:29:41]

should we always implicitly include the foo package in a part named foo?

[Harry Terkelsen, 2015/12/18 21:22:21]

What I'm seeing is more like:

import 'package:foo.bar.baz/baz.dart' as baz;

and then in the specification file this part is named baz. So the name of the
part is not necessarily the same as the package that the deferred library is in.

 ///         - bar
 ///
 ///     baz:
-///       packages:
+///       include:
 ///         - baz
 ///         - quux
+///       exclude:
+///         - zardoz
 ///
 /// The YAML file consists of a list of declarations, one for each deferred
 /// part expected in the output. At least one of these parts must be named
 /// "main"; this is the main part that contains the program entrypoint. Each
 /// top-level part contains a list of package names that are expected to be
-/// contained in that part. Any package that is not explicitly listed is
-/// expected to be in the main part. For instance, in the example YAML above
-/// the part named "baz" is expected to contain the packages "baz" and "quux".
+/// contained in that part, a list of package names that are expected to be in
+/// another part, or both. For instance, in the example YAML above the part
+/// named "baz" is expected to contain the packages "baz" and "quux" and not to
+/// contain the package "zardoz".
 ///
 /// The names for parts given in the specification YAML file (besides "main")
-/// are arbitrary and just used for reporting when the output does not meet the
-/// specification.
+/// are the same as the name given to the deferred import in the dart file. For
+/// instance, if you have `import 'package:foo/foo.dart' deferred as foo;` in

[Siggi Cherem (dart-lang), 2015/12/18 02:29:41]

same here

[Harry Terkelsen, 2015/12/18 21:22:21]

Done.

+/// your dart file, then the corresponding name in the specification file is
+/// 'foo'.
 library dart2js_info.deferred_library_check;
 
 import 'info.dart';
@@ -38,51 +43,64 @@ import 'package:quiver/collection.dart';
 
 List<ManifestComplianceFailure> checkDeferredLibraryManifest(
     AllInfo info, Map manifest) {
-  // For each part in the manifest, record the expected "packages" for that
-  // part.
-  var packages = <String, String>{};
+  var includedPackages = new Multimap<String, String>();
+  var excludedPackages = new Multimap<String, String>();
   for (var part in manifest.keys) {
-    for (var package in manifest[part]['packages']) {
-      if (packages.containsKey(package)) {
-        throw new ArgumentError.value(
-            manifest,
-            'manifest',
-            'You cannot specify that package "$package" maps to both parts '
-            '"$part" and "${packages[package]}".');
-      }
-      packages[package] = part;
+    for (var package in manifest[part]['include'] ?? []) {
+      includedPackages.add(part, package);
+    }
+    for (var package in manifest[part]['exclude'] ?? []) {
+      excludedPackages.add(part, package);
+    }
+  }
+
+  // There are 2 types of parts that are valid to mention in the specification
+  // file. These are the main part and directly imported deferred parts. The
+  // main part is always named 'main'; the directly imported deferred parts are
+  // the outputUnits whose list of 'imports' contains a single import. If the
+  // part is shared, it will have more than one import since it will include the
+  // imports of all the top-level deferred parts that will load the shared part.
+  List<String> validParts = ['main']

[Siggi Cherem (dart-lang), 2015/12/18 02:29:41]

we might want to validate that there are no 2 parts with the same name (if 2
libraries on the same package are deferred separately).

Not yet needed by our customers, but I'm sure it wont be long until they hit
that issue.

[Harry Terkelsen, 2015/12/18 21:22:21]

Do you still think we should do this since the names of the parts are the part
after the "as" instead of the package name?

[Siggi Cherem (dart-lang), 2015/12/18 21:34:25]

Good point - nope, it makes sense to leave it as is.

+    ..addAll(info.outputUnits
+        .where((unit) => unit.imports.length == 1)
+        .map((unit) => unit.imports.single));

[Siggi Cherem (dart-lang), 2015/12/18 02:29:41]

do we need to get the package name from the library URI here? 

[Harry Terkelsen, 2015/12/18 21:22:21]

unit.imports is a list of the names of the deferred imports. For instance the
part created when there is "import 'package:foo/bar.dart' deferred as baz;" has
a length-1 list of imports ["baz"].

[Siggi Cherem (dart-lang), 2015/12/18 21:34:25]

interesting - I really thought that these were the import urls, not the
prefixes. 

What happens if two imports use the same prefix word? does that make our
dump-info ambiguous?

I wonder whether we should record the full thing (url + prefix) and validate
that the prefix is unique when we get here.

[Harry Terkelsen, 2015/12/18 21:42:53]

I think if there are two with the same name then one fill be "prefix" and the
other will be "prefix1"

+  List<String> mentionedParts = []
+    ..addAll(includedPackages.keys)
+    ..addAll(excludedPackages.keys);
+  var partNameFailures = <_InvalidPartName>[];
+  for (var part in mentionedParts) {
+    if (!validParts.contains(part)) {
+      partNameFailures.add(new _InvalidPartName(part, validParts));
     }
   }
+  if (partNameFailures.isNotEmpty) {
+    return partNameFailures;
+  }
 
-  var guessedPartMapping = new BiMap<String, String>();
-  guessedPartMapping['main'] = 'main';
+  var mentionedPackages = new Set()
+    ..addAll(includedPackages.values)
+    ..addAll(excludedPackages.values);
+  var actualIncludedPackages = new Multimap<String, String>();
 
   var failures = <ManifestComplianceFailure>[];
 
   checkInfo(BasicInfo info) {
+    if (info.size == 0) return;
     var lib = _getLibraryOf(info);
     if (lib != null && _isPackageUri(lib.uri)) {
       var packageName = _getPackageName(lib.uri);
-      var outputUnitName = info.outputUnit.name;
-      var expectedPart;
-      if (packages.containsKey(packageName)) {
-        expectedPart = packages[packageName];
+      if (!mentionedPackages.contains(packageName)) return;
+      var containingParts = <String>[];
+      if (info.outputUnit.name == 'main') {
+        containingParts.add('main');
       } else {
-        expectedPart = 'main';
+        containingParts.addAll(info.outputUnit.imports);
       }
-      var expectedOutputUnit = guessedPartMapping[expectedPart];
-      if (expectedOutputUnit == null) {
-        guessedPartMapping[expectedPart] = outputUnitName;
-      } else {
-        if (expectedOutputUnit != outputUnitName) {
-          // TODO(het): add options for how to treat unspecified packages
-          if (!packages.containsKey(packageName)) {
-            failures.add(new ManifestComplianceFailure(info.name, packageName));
-          } else {
-            var actualPart = guessedPartMapping.inverse[outputUnitName];
-            failures.add(new ManifestComplianceFailure(
-                info.name, packageName, expectedPart, actualPart));
-          }
+      for (var part in containingParts) {
+        actualIncludedPackages.add(part, packageName);
+        if (excludedPackages[part].contains(packageName)) {
+          failures
+              .add(new _PartContainedExcludedPackage(part, packageName, info));
         }
       }
     }
@@ -91,6 +109,12 @@ List<ManifestComplianceFailure> checkDeferredLibraryManifest(
   info.functions.forEach(checkInfo);
   info.fields.forEach(checkInfo);
 
+  includedPackages.forEach((part, package) {
+    if (!actualIncludedPackages.containsKey(part) ||
+        !actualIncludedPackages[part].contains(package)) {
+      failures.add(new _PartDidNotContainPackage(part, package));
+    }
+  });
   return failures;
 }
 
@@ -113,21 +137,40 @@ String _getPackageName(Uri uri) {
 }
 
 class ManifestComplianceFailure {
-  final String infoName;
-  final String packageName;
-  final String expectedPart;
-  final String actualPart;
+  const ManifestComplianceFailure();
+}
 
-  const ManifestComplianceFailure(this.infoName, this.packageName,
-      [this.expectedPart, this.actualPart]);
+class _InvalidPartName extends ManifestComplianceFailure {
+  final String part;
+  final List<String> validPartNames;
+  const _InvalidPartName(this.part, this.validPartNames);
 
   String toString() {
-    if (expectedPart == null && actualPart == null) {
-      return '"$infoName" from package "$packageName" was not declared '
-          'to be in an explicit part but was not in the main part';
-    } else {
-      return '"$infoName" from package "$packageName" was specified to '
-          'be in part $expectedPart but is in part $actualPart';
-    }
+    return 'Manifest file declares invalid part "$part". '
+        'Valid part names are: $validPartNames';
+  }
+}
+
+class _PartContainedExcludedPackage extends ManifestComplianceFailure {
+  final String part;
+  final String package;
+  final BasicInfo info;
+  const _PartContainedExcludedPackage(this.part, this.package, this.info);
+
+  String toString() {
+    return 'Part "$part" was specified to exclude package "$package" but it '
+        'actually contains ${kindToString(info.kind)} "${info.name}" which '
+        'is from package "$package"';
+  }
+}
+
+class _PartDidNotContainPackage extends ManifestComplianceFailure {
+  final String part;
+  final String package;
+  const _PartDidNotContainPackage(this.part, this.package);
+
+  String toString() {
+    return 'Part "$part" was specified to include package "$package" but it '
+        'does not contain any elements from that package.';
   }
 }