[dart2js] Address some feedback on constraints API.

* Changes default namer to use the always unique uri#prefix of a
deferred import.
* Changes builder api for combiner nodes to take sets.

Change-Id: Iab23db94a166560682a8c2bd4a78ebfd3e734353
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/218880
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Joshua Litt <joshualitt@google.com>

pkg/compiler/lib/src/deferred_load/program_split_constraints/nodes.dart

@@ -44,8 +44,7 @@ class ReferenceNode extends NamedNode {
   Uri get uri => _uriAndPrefix.uri;
   String get prefix => _uriAndPrefix.prefix;
 
-  ReferenceNode(this._uriAndPrefix, {name})
-      : super(name ?? _uriAndPrefix.prefix);
+  ReferenceNode(String name, this._uriAndPrefix) : super(name);
 
   @override
   Map<String, dynamic> toJson() {
@@ -60,8 +59,8 @@ class ReferenceNode extends NamedNode {
     if (nodeJson['type'] != 'reference') {
       throw 'Unrecognized type for reference node: ${nodeJson['type']}.';
     }
-    return ReferenceNode(UriAndPrefix.fromJson(nodeJson['import']),
-        name: nodeJson['name']);
+    return ReferenceNode(
+        nodeJson['name'], UriAndPrefix.fromJson(nodeJson['import']));
   }
 
   @override
@@ -193,17 +192,27 @@ class ProgramSplitBuilder {
   final Map<String, NamedNode> namedNodes = {};
   ReferenceNodeNamer _referenceNodeNamer;
 
-  /// The prefix in the 'uri#prefix' string will become a key to reference this
-  /// node in other builder calls.
-  String _prefixNamer(UriAndPrefix uriAndPrefix) => uriAndPrefix.prefix;
+  /// 'uri#prefix' will become a key to reference this node in other builder
+  /// calls.
+  String _uriAndPrefixNamer(UriAndPrefix uriAndPrefix) =>
+      uriAndPrefix.toString();
 
   /// Override the default reference node namer.
   set referenceNodeNamer(ReferenceNodeNamer namer) =>
       _referenceNodeNamer = namer;
 
   /// Returns the [ReferenceNodeNamer] to use for naming.
   ReferenceNodeNamer get referenceNodeNamer =>
-      _referenceNodeNamer ?? _prefixNamer;
+      _referenceNodeNamer ?? _uriAndPrefixNamer;
+
+  NamedNode _addNamedNode(NamedNode node) {
+    if (namedNodes.containsKey(node.name)) {
+      throw 'Node with name ${node.name} already exists: '
+          '${namedNodes[node.name]}';
+    }
+    namedNodes[node.name] = node;
+    return node;
+  }
 
   /// Returns a [ReferenceNode] referencing [importUriAndPrefix].
   /// [ReferenceNode]s are typically created in bulk, by mapping over a list of
@@ -212,10 +221,8 @@ class ProgramSplitBuilder {
   /// [referenceNodeNamer] per [ReferenceNode].
   ReferenceNode referenceNode(String importUriAndPrefix) {
     var uriAndPrefix = UriAndPrefix.fromJson(importUriAndPrefix);
-    var referenceNode = ReferenceNode(uriAndPrefix);
     var name = referenceNodeNamer(uriAndPrefix);
-    namedNodes[name] = referenceNode;
-    return referenceNode;
+    return _addNamedNode(ReferenceNode(name, uriAndPrefix));
   }
 
   /// Creates an unnamed [RelativeOrderNode] referencing two [NamedNode]s.
@@ -226,29 +233,37 @@ class ProgramSplitBuilder {
 
   /// Creates a [CombinerNode] which can be referenced by [name] in further
   /// calls to the builder.
-  CombinerNode combinerNode(
-      String name, List<String> nodes, CombinerType type) {
-    var combinerNode = CombinerNode(name, type,
-        nodes.map((name) => namedNodes[name] as ReferenceNode).toSet());
-    namedNodes[name] = combinerNode;
-    return combinerNode;
+  CombinerNode combinerNode(String name, Set<String> nodes, CombinerType type) {
+    ReferenceNode _lookup(String nodeName) {
+      if (!namedNodes.containsKey(nodeName)) {
+        throw 'Missing reference node for $nodeName';
+      }
+      var node = namedNodes[nodeName];
+      if (node is! ReferenceNode) {
+        // TODO(joshualitt): Implement nested combiners.
+        throw '$name references node $nodeName which is not a ReferenceNode.';
+      }
+      return node as ReferenceNode;
+    }
+
+    return _addNamedNode(CombinerNode(name, type, nodes.map(_lookup).toSet()));
   }
 
   /// Creates an 'and' [CombinerNode] which can be referenced by [name] in
   /// further calls to the builder.
-  CombinerNode andNode(String name, List<String> nodes) {
+  CombinerNode andNode(String name, Set<String> nodes) {
     return combinerNode(name, nodes, CombinerType.and);
   }
 
   /// Creates a 'fuse' [CombinerNode] which can be referenced by [name] in
   /// further calls to the builder.
-  CombinerNode fuseNode(String name, List<String> nodes) {
+  CombinerNode fuseNode(String name, Set<String> nodes) {
     return combinerNode(name, nodes, CombinerType.fuse);
   }
 
   /// Creates an 'or' [CombinerNode] which can be referenced by [name] in
   /// further calls to the builder.
-  CombinerNode orNode(String name, List<String> nodes) {
+  CombinerNode orNode(String name, Set<String> nodes) {
     return combinerNode(name, nodes, CombinerType.or);
   }
 }

pkg/compiler/test/custom_split/custom_split_test.dart

@@ -83,17 +83,30 @@ Future<String> constraintsToJson(
   return json;
 }
 
+Uri getFileInTestFolder(String test, String file) =>
+    Platform.script.resolve('data/$test/$file');
+
+Future<String> compileConstraintsToJson(String test, Compiler compiler) async {
+  var constraints = getFileInTestFolder(test, 'constraints.dart');
+  var component = compiler.componentForTesting;
+  return constraintsToJson(component, constraints);
+}
+
+File getConstraintsJsonFile(String test) {
+  var constraintsJsonUri = getFileInTestFolder(test, 'constraints.json');
+  return File(constraintsJsonUri.toFilePath());
+}
+
 /// Verifies the programmatic API produces the expected JSON.
 Future<void> verifyCompiler(String test, Compiler compiler) async {
-  var constraints = Platform.script.resolve('data/$test/constraints.dart');
-  var constraintsJsonUri =
-      Platform.script.resolve('data/$test/constraints.json');
-  var component = compiler.componentForTesting;
-  var json = await constraintsToJson(component, constraints);
-  var constraintsJson =
-      File(constraintsJsonUri.toFilePath()).readAsStringSync();
-  constraintsJson = constraintsJson.substring(0, constraintsJson.length - 1);
-  Expect.equals(json, constraintsJson);
+  var json = await compileConstraintsToJson(test, compiler);
+  Expect.equals(getConstraintsJsonFile(test).readAsStringSync(), json);
+}
+
+/// Generates constraint JSON.
+Future<void> generateJSON(String test, Compiler compiler) async {
+  var json = await compileConstraintsToJson(test, compiler);
+  getConstraintsJsonFile(test).writeAsStringSync(json);
 }
 
 /// Compute the [OutputUnit]s for all source files involved in the test, and
@@ -102,13 +115,16 @@ Future<void> verifyCompiler(String test, Compiler compiler) async {
 /// or all supporting libraries to be in the `libs` folder, starting with the
 /// same name as the original file in `data`.
 main(List<String> args) {
+  bool generateGoldens = args.contains('-g');
   asyncTest(() async {
     Directory dataDir = Directory.fromUri(Platform.script.resolve('data'));
     await checkTests(dataDir, const OutputUnitDataComputer(),
         options: compilerOptions,
         perTestOptions: createPerTestOptions(),
         args: args, setUpFunction: () {
       importPrefixes.clear();
-    }, testedConfigs: allSpecConfigs, verifyCompiler: verifyCompiler);
+    },
+        testedConfigs: allSpecConfigs,
+        verifyCompiler: generateGoldens ? generateJSON : verifyCompiler);
   });
 }

pkg/compiler/test/custom_split/data/diamond/constraints.dart

@@ -12,12 +12,16 @@ void main(List<String> args, SendPort sendPort) {
 }
 
 List<Node> processDeferredImports(List<String> imports) {
+  var step1 = 'memory:sdk/tests/web/native/main.dart#step1';
+  var step2a = 'memory:sdk/tests/web/native/main.dart#step2a';
+  var step2b = 'memory:sdk/tests/web/native/main.dart#step2b';
+  var step3 = 'memory:sdk/tests/web/native/main.dart#step3';
   var builder = ProgramSplitBuilder();
   return [
     ...imports.map(builder.referenceNode),
-    builder.orderNode('step1', 'step2a'),
-    builder.orderNode('step1', 'step2b'),
-    builder.orderNode('step2a', 'step3'),
-    builder.orderNode('step2b', 'step3'),
+    builder.orderNode(step1, step2a),
+    builder.orderNode(step1, step2b),
+    builder.orderNode(step2a, step3),
+    builder.orderNode(step2b, step3),
   ];
 }

pkg/compiler/test/custom_split/data/diamond/constraints.json

@@ -1,42 +1,42 @@
 [
   {
     "type": "reference",
-    "name": "step1",
+    "name": "memory:sdk/tests/web/native/main.dart#step1",
     "import": "memory:sdk/tests/web/native/main.dart#step1"
   },
   {
     "type": "reference",
-    "name": "step2a",
+    "name": "memory:sdk/tests/web/native/main.dart#step2a",
     "import": "memory:sdk/tests/web/native/main.dart#step2a"
   },
   {
     "type": "reference",
-    "name": "step2b",
+    "name": "memory:sdk/tests/web/native/main.dart#step2b",
     "import": "memory:sdk/tests/web/native/main.dart#step2b"
   },
   {
     "type": "reference",
-    "name": "step3",
+    "name": "memory:sdk/tests/web/native/main.dart#step3",
     "import": "memory:sdk/tests/web/native/main.dart#step3"
   },
   {
     "type": "order",
-    "predecessor": "step1",
-    "successor": "step2a"
+    "predecessor": "memory:sdk/tests/web/native/main.dart#step1",
+    "successor": "memory:sdk/tests/web/native/main.dart#step2a"
   },
   {
     "type": "order",
-    "predecessor": "step1",
-    "successor": "step2b"
+    "predecessor": "memory:sdk/tests/web/native/main.dart#step1",
+    "successor": "memory:sdk/tests/web/native/main.dart#step2b"
   },
   {
     "type": "order",
-    "predecessor": "step2a",
-    "successor": "step3"
+    "predecessor": "memory:sdk/tests/web/native/main.dart#step2a",
+    "successor": "memory:sdk/tests/web/native/main.dart#step3"
   },
   {
     "type": "order",
-    "predecessor": "step2b",
-    "successor": "step3"
+    "predecessor": "memory:sdk/tests/web/native/main.dart#step2b",
+    "successor": "memory:sdk/tests/web/native/main.dart#step3"
   }
-]
+]

pkg/compiler/test/custom_split/data/diamond_and/constraints.dart

@@ -12,11 +12,15 @@ void main(List<String> args, SendPort sendPort) {
 }
 
 List<Node> processDeferredImports(List<String> imports) {
+  var step1 = 'memory:sdk/tests/web/native/main.dart#step1';
+  var step2a = 'memory:sdk/tests/web/native/main.dart#step2a';
+  var step2b = 'memory:sdk/tests/web/native/main.dart#step2b';
+  var step3 = 'memory:sdk/tests/web/native/main.dart#step3';
   var builder = ProgramSplitBuilder();
   return [
     ...imports.map(builder.referenceNode),
-    builder.andNode('step2', ['step2a', 'step2b']),
-    builder.orderNode('step1', 'step2'),
-    builder.orderNode('step2', 'step3'),
+    builder.andNode('step2', {step2a, step2b}),
+    builder.orderNode(step1, 'step2'),
+    builder.orderNode('step2', step3),
   ];
 }

pkg/compiler/test/custom_split/data/diamond_and/constraints.json

@@ -1,40 +1,40 @@
 [
   {
     "type": "reference",
-    "name": "step1",
+    "name": "memory:sdk/tests/web/native/main.dart#step1",
     "import": "memory:sdk/tests/web/native/main.dart#step1"
   },
   {
     "type": "reference",
-    "name": "step2a",
+    "name": "memory:sdk/tests/web/native/main.dart#step2a",
     "import": "memory:sdk/tests/web/native/main.dart#step2a"
   },
   {
     "type": "reference",
-    "name": "step2b",
+    "name": "memory:sdk/tests/web/native/main.dart#step2b",
     "import": "memory:sdk/tests/web/native/main.dart#step2b"
   },
   {
     "type": "reference",
-    "name": "step3",
+    "name": "memory:sdk/tests/web/native/main.dart#step3",
     "import": "memory:sdk/tests/web/native/main.dart#step3"
   },
   {
     "type": "and",
     "name": "step2",
     "nodes": [
-      "step2a",
-      "step2b"
+      "memory:sdk/tests/web/native/main.dart#step2a",
+      "memory:sdk/tests/web/native/main.dart#step2b"
     ]
   },
   {
     "type": "order",
-    "predecessor": "step1",
+    "predecessor": "memory:sdk/tests/web/native/main.dart#step1",
     "successor": "step2"
   },
   {
     "type": "order",
     "predecessor": "step2",
-    "successor": "step3"
+    "successor": "memory:sdk/tests/web/native/main.dart#step3"
   }
-]
+]

pkg/compiler/test/custom_split/data/diamond_fuse/constraints.dart

@@ -12,11 +12,15 @@ void main(List<String> args, SendPort sendPort) {
 }
 
 List<Node> processDeferredImports(List<String> imports) {
+  var step1 = 'memory:sdk/tests/web/native/main.dart#step1';
+  var step2a = 'memory:sdk/tests/web/native/main.dart#step2a';
+  var step2b = 'memory:sdk/tests/web/native/main.dart#step2b';
+  var step3 = 'memory:sdk/tests/web/native/main.dart#step3';
   var builder = ProgramSplitBuilder();
   return [
     ...imports.map(builder.referenceNode),
-    builder.fuseNode('step2', ['step2a', 'step2b']),
-    builder.orderNode('step1', 'step2'),
-    builder.orderNode('step2', 'step3'),
+    builder.fuseNode('step2', {step2a, step2b}),
+    builder.orderNode(step1, 'step2'),
+    builder.orderNode('step2', step3),
   ];
 }

pkg/compiler/test/custom_split/data/diamond_fuse/constraints.json

@@ -1,40 +1,40 @@
 [
   {
     "type": "reference",
-    "name": "step1",
+    "name": "memory:sdk/tests/web/native/main.dart#step1",
     "import": "memory:sdk/tests/web/native/main.dart#step1"
   },
   {
     "type": "reference",
-    "name": "step2a",
+    "name": "memory:sdk/tests/web/native/main.dart#step2a",
     "import": "memory:sdk/tests/web/native/main.dart#step2a"
   },
   {
     "type": "reference",
-    "name": "step2b",
+    "name": "memory:sdk/tests/web/native/main.dart#step2b",
     "import": "memory:sdk/tests/web/native/main.dart#step2b"
   },
   {
     "type": "reference",
-    "name": "step3",
+    "name": "memory:sdk/tests/web/native/main.dart#step3",
     "import": "memory:sdk/tests/web/native/main.dart#step3"
   },
   {
     "type": "fuse",
     "name": "step2",
     "nodes": [
-      "step2a",
-      "step2b"
+      "memory:sdk/tests/web/native/main.dart#step2a",
+      "memory:sdk/tests/web/native/main.dart#step2b"
     ]
   },
   {
     "type": "order",
-    "predecessor": "step1",
+    "predecessor": "memory:sdk/tests/web/native/main.dart#step1",
     "successor": "step2"
   },
   {
     "type": "order",
     "predecessor": "step2",
-    "successor": "step3"
+    "successor": "memory:sdk/tests/web/native/main.dart#step3"
   }
-]
+]