Fix PlatformInfo to correctly report all duplicate constraints, not just the first.

Change-Id: I3d78418d2c51d09e3862fbab1854ae73a0b3253c
PiperOrigin-RevId: 168889865

4 files changed, 86 insertions, 51 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java
index be05d75e80..4b920ff38c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java
@@ -14,11 +14,16 @@
 
 package com.google.devtools.build.lib.analysis.platform;
 
-import com.google.common.collect.ArrayListMultimap;
+import static com.google.common.collect.ImmutableListMultimap.flatteningToImmutableListMultimap;
+import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap;
+import static java.util.stream.Collectors.joining;
+
+import com.google.common.base.Functions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Multimap;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Location;
@@ -32,6 +37,7 @@ import com.google.devtools.build.lib.syntax.FunctionSignature;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkType;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -255,21 +261,26 @@ public class PlatformInfo extends NativeInfo {
 
     private ImmutableList<ConstraintValueInfo> validateConstraints(
         Iterable<ConstraintValueInfo> constraintValues) throws DuplicateConstraintException {
-      Multimap<ConstraintSettingInfo, ConstraintValueInfo> constraints = ArrayListMultimap.create();
 
-      for (ConstraintValueInfo constraintValue : constraintValues) {
-        constraints.put(constraintValue.constraint(), constraintValue);
+      // Collect the constraints by the settings.
+      ImmutableListMultimap<ConstraintSettingInfo, ConstraintValueInfo> constraints =
+          Streams.stream(constraintValues)
+              .collect(
+                  toImmutableListMultimap(ConstraintValueInfo::constraint, Functions.identity()));
+
+      // Find settings with duplicate values.
+      ImmutableListMultimap<ConstraintSettingInfo, ConstraintValueInfo> duplicates =
+          constraints
+              .asMap()
+              .entrySet()
+              .stream()
+              .filter(e -> e.getValue().size() > 1)
+              .collect(
+                  flatteningToImmutableListMultimap(Map.Entry::getKey, e -> e.getValue().stream()));
+
+      if (!duplicates.isEmpty()) {
+        throw new DuplicateConstraintException(duplicates);
       }
-
-      // Are there any settings with more than one value?
-      for (ConstraintSettingInfo constraintSetting : constraints.keySet()) {
-        if (constraints.get(constraintSetting).size() > 1) {
-          // Only reports the first case of this error.
-          throw new DuplicateConstraintException(
-              constraintSetting, constraints.get(constraintSetting));
-        }
-      }
-
       return ImmutableList.copyOf(constraints.values());
     }
   }
@@ -279,32 +290,43 @@ public class PlatformInfo extends NativeInfo {
    * ConstraintSettingInfo} is added to a {@link Builder}.
    */
   public static class DuplicateConstraintException extends Exception {
-    private final ImmutableSet<ConstraintValueInfo> duplicateConstraints;
+    private final ImmutableListMultimap<ConstraintSettingInfo, ConstraintValueInfo>
+        duplicateConstraints;
 
     public DuplicateConstraintException(
-        ConstraintSettingInfo constraintSetting,
-        Iterable<ConstraintValueInfo> duplicateConstraintValues) {
-      super(formatError(constraintSetting, duplicateConstraintValues));
-      this.duplicateConstraints = ImmutableSet.copyOf(duplicateConstraintValues);
+        ListMultimap<ConstraintSettingInfo, ConstraintValueInfo> duplicateConstraints) {
+      super(formatError(duplicateConstraints));
+      this.duplicateConstraints = ImmutableListMultimap.copyOf(duplicateConstraints);
     }
 
-    public ImmutableSet<ConstraintValueInfo> duplicateConstraints() {
+    public ImmutableListMultimap<ConstraintSettingInfo, ConstraintValueInfo>
+        duplicateConstraints() {
       return duplicateConstraints;
     }
 
     private static String formatError(
-        ConstraintSettingInfo constraintSetting,
-        Iterable<ConstraintValueInfo> duplicateConstraints) {
-      StringBuilder constraintValuesDescription = new StringBuilder();
-      for (ConstraintValueInfo constraintValue : duplicateConstraints) {
-        if (constraintValuesDescription.length() > 0) {
-          constraintValuesDescription.append(", ");
-        }
-        constraintValuesDescription.append(constraintValue.label());
-      }
+        ListMultimap<ConstraintSettingInfo, ConstraintValueInfo> duplicateConstraints) {
+      return String.format(
+          "Duplicate constraint_values detected: %s",
+          duplicateConstraints
+              .asMap()
+              .entrySet()
+              .stream()
+              .map(e -> describeSingleDuplicateConstraintSetting(e))
+              .collect(joining(", ")));
+    }
+
+    private static String describeSingleDuplicateConstraintSetting(
+        Map.Entry<ConstraintSettingInfo, Collection<ConstraintValueInfo>> duplicate) {
       return String.format(
-          "Duplicate constraint_values for constraint_setting %s: %s",
-          constraintSetting.label(), constraintValuesDescription.toString());
+          "constraint_setting %s has [%s]",
+          duplicate.getKey().label(),
+          duplicate
+              .getValue()
+              .stream()
+              .map(ConstraintValueInfo::label)
+              .map(Label::toString)
+              .collect(joining(", ")));
     }
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD
index bc49178d2e..76904e997d 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD
@@ -26,6 +26,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/test/java/com/google/devtools/build/lib:analysis_testutil",
         "//src/test/java/com/google/devtools/build/lib:packages_testutil",
+        "//src/test/java/com/google/devtools/build/lib:testutil",
         "//src/test/java/com/google/devtools/build/lib/skylark:testutil",
         "//third_party:guava",
         "//third_party:guava-testlib",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformInfoTest.java b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformInfoTest.java
index a60ff1d4de..61b116c5ce 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformInfoTest.java
@@ -15,21 +15,19 @@
 package com.google.devtools.build.lib.analysis.platform;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.expectThrows;
 
 import com.google.common.testing.EqualsTester;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import org.junit.Before;
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 /** Tests of {@link PlatformInfo}. */
 @RunWith(JUnit4.class)
 public class PlatformInfoTest extends BuildViewTestCase {
-  @Rule public ExpectedException expectedException = ExpectedException.none();
 
   @Before
   public void createPlatform() throws Exception {
@@ -43,19 +41,32 @@ public class PlatformInfoTest extends BuildViewTestCase {
 
   @Test
   public void platformInfo_overlappingConstraintsError() throws Exception {
-    ConstraintSettingInfo setting = ConstraintSettingInfo.create(makeLabel("//constraint:basic"));
-
-    ConstraintValueInfo value1 = ConstraintValueInfo.create(setting, makeLabel("//constraint:foo"));
-    ConstraintValueInfo value2 = ConstraintValueInfo.create(setting, makeLabel("//constraint:bar"));
-
-    PlatformInfo.Builder builder =
-        PlatformInfo.builder().addConstraint(value1).addConstraint(value2);
-
-    expectedException.expect(PlatformInfo.DuplicateConstraintException.class);
-    expectedException.expectMessage(
-        "Duplicate constraint_values for constraint_setting //constraint:basic: "
-            + "//constraint:foo, //constraint:bar");
-    builder.build();
+    ConstraintSettingInfo setting1 = ConstraintSettingInfo.create(makeLabel("//constraint:basic"));
+    ConstraintSettingInfo setting2 =
+        ConstraintSettingInfo.create(makeLabel("//constraint:complex"));
+    ConstraintSettingInfo setting3 = ConstraintSettingInfo.create(makeLabel("//constraint:single"));
+
+    PlatformInfo.Builder builder = PlatformInfo.builder();
+
+    builder.addConstraint(ConstraintValueInfo.create(setting1, makeLabel("//constraint:value1")));
+    builder.addConstraint(ConstraintValueInfo.create(setting1, makeLabel("//constraint:value2")));
+
+    builder.addConstraint(ConstraintValueInfo.create(setting2, makeLabel("//constraint:value3")));
+    builder.addConstraint(ConstraintValueInfo.create(setting2, makeLabel("//constraint:value4")));
+    builder.addConstraint(ConstraintValueInfo.create(setting2, makeLabel("//constraint:value5")));
+
+    builder.addConstraint(ConstraintValueInfo.create(setting3, makeLabel("//constraint:value6")));
+
+    PlatformInfo.DuplicateConstraintException exception =
+        expectThrows(PlatformInfo.DuplicateConstraintException.class, () -> builder.build());
+    assertThat(exception)
+        .hasMessageThat()
+        .contains(
+            "Duplicate constraint_values detected: "
+                + "constraint_setting //constraint:basic has "
+                + "[//constraint:value1, //constraint:value2], "
+                + "constraint_setting //constraint:complex has "
+                + "[//constraint:value3, //constraint:value4, //constraint:value5]");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformTest.java b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformTest.java
index 8c71ae324b..52e9219b79 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformTest.java
@@ -126,8 +126,9 @@ public class PlatformTest extends BuildViewTestCase {
     checkError(
         "constraint/overlap",
         "plat_overlap",
-        "Duplicate constraint_values for constraint_setting //constraint:basic: "
-            + "//constraint:foo, //constraint/overlap:bar",
+        "Duplicate constraint_values detected: "
+            + "constraint_setting //constraint:basic has "
+            + "[//constraint:foo, //constraint/overlap:bar]",
         "constraint_value(name = 'bar',",
         "    constraint_setting = '//constraint:basic',",
         "    )",