Add more thorough error reporting behavior for when config_setting ha…

…s multiple constraint_values from the same constraint_setting (re: #350) PiperOrigin-RevId: 169577576
bazelbuild · Sep 22, 2017 · 5e2b0da · 5e2b0da
1 parent 60f3503
commit 5e2b0da
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 43 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
@@ -259,7 +259,7 @@ public PlatformInfo build() throws DuplicateConstraintException {
           label, validatedConstraints, ImmutableMap.copyOf(remoteExecutionProperties), location);
     }
 
-    private ImmutableList<ConstraintValueInfo> validateConstraints(
+    public static ImmutableList<ConstraintValueInfo> validateConstraints(
         Iterable<ConstraintValueInfo> constraintValues) throws DuplicateConstraintException {
 
       // Collect the constraints by the settings.
@@ -304,7 +304,7 @@ public DuplicateConstraintException(
       return duplicateConstraints;
     }
 
-    private static String formatError(
+    public static String formatError(
         ListMultimap<ConstraintSettingInfo, ConstraintValueInfo> duplicateConstraints) {
       return String.format(
           "Duplicate constraint_values detected: %s",

diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.rules.config;
 
+import static com.google.devtools.build.lib.analysis.platform.PlatformInfo.DuplicateConstraintException.formatError;
+
 import com.google.common.base.Joiner;
 import com.google.common.collect.HashMultiset;
 import com.google.common.collect.ImmutableList;
@@ -47,6 +49,7 @@
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
+import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.packages.RuleErrorConsumer;
 import com.google.devtools.build.lib.rules.config.ConfigRuleClasses.ConfigSettingRule;
 import com.google.devtools.build.lib.syntax.Type;
@@ -55,7 +58,6 @@
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -152,17 +154,18 @@ private boolean checkValidConditions(
       Map<Label, String> userDefinedFlagSettings,
       Iterable<ConstraintValueInfo> constraintValues,
       RuleErrorConsumer errors) {
-    // Check to make sure this config_setting contains and sets least one of {values, define_values,
-    // flag_value or constraint_values}.
     if (!valuesAreSet(nativeFlagSettings, userDefinedFlagSettings, constraintValues, errors)) {
       return false;
     }
 
     // The set of constraint_values in a config_setting should never contain multiple
-    // constraint_values that map to the same constraint_setting. This checks if there are
+    // constraint_values that map to the same constraint_setting. This method checks if there are
     // duplicates and records an error if so.
-    if (containsDuplicateSettings(constraintValues, errors)) {
-      return false;
+    try {
+      PlatformInfo.Builder.validateConstraints(constraintValues);
+    } catch (PlatformInfo.DuplicateConstraintException e) {
+        errors.ruleError(formatError(e.duplicateConstraints()));
+        return false;
     }
 
     return true;
@@ -173,6 +176,10 @@ private boolean checkValidConditions(
    */
   private static final String PARSE_ERROR_MESSAGE = "error while parsing configuration settings: ";
 
+  /**
+   * Check to make sure this config_setting contains and sets least one of {values, define_values,
+   * flag_value or constraint_values}.
+   */
   private boolean valuesAreSet(
       ImmutableMultimap<String, String> nativeFlagSettings,
       Map<Label, String> userDefinedFlagSettings,
@@ -192,34 +199,6 @@ private boolean valuesAreSet(
     return true;
   }
 
-
-  /**
-   * The set of constraint_values in a config_setting should never contain multiple
-   * constraint_values that map to the same constraint_setting. This method checks if there are
-   * duplicates and records an error if so.
-   */
-  private boolean containsDuplicateSettings(
-      Iterable<ConstraintValueInfo> constraintValues, RuleErrorConsumer errors) {
-    HashMap<ConstraintSettingInfo, ConstraintValueInfo> constraints = new HashMap<>();
-    for (ConstraintValueInfo constraint : constraintValues) {
-      ConstraintSettingInfo setting = constraint.constraint();
-      if (constraints.containsKey(setting)) {
-        errors.attributeError(
-            ConfigSettingRule.TARGET_PLATFORMS_ATTRIBUTE,
-            String.format(
-                PARSE_ERROR_MESSAGE
-                    + "the target platform contains multiple values '%s' "
-                    + "and '%s' that map to the same setting '%s'",
-                constraint.label(),
-                constraints.get(setting).label(),
-                setting.label()));
-        return true;
-      }
-      constraints.put(setting, constraint);
-    }
-    return false;
-  }
-
   /**
    * Given a list of [flagName, flagValue] pairs for native Blaze flags, returns true if
    * flagName == flagValue for every item in the list under this configuration, false otherwise.
@@ -450,4 +429,3 @@ public static ConfigFeatureFlagMatch fromAttributeValueAndPrerequisites(
     }
   }
 }
-
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
@@ -1237,17 +1237,31 @@ public void definesAndConstraints() throws Exception {
    */
   @Test
   public void multipleValuesPerSetting() throws Exception {
-    checkError("foo", "bad",
-        "in :target_platforms attribute of config_setting rule //foo:bad: "
-            + "error while parsing configuration settings: "
-            + "the target platform contains multiple values '//foo:space_needle' "
-            + "and '//foo:empire_state' that map to the same setting '//foo:notable_building'",
+    checkError(
+        "foo",
+        "bad",
+        "in config_setting rule //foo:bad: "
+            + "Duplicate constraint_values detected: "
+            + "constraint_setting //foo:notable_building has "
+            + "[//foo:empire_state, //foo:space_needle], "
+            + "constraint_setting //foo:museum has "
+            + "[//foo:moma, //foo:sam]",
         "constraint_setting(name = 'notable_building')",
         "constraint_value(name = 'empire_state', constraint_setting = 'notable_building')",
         "constraint_value(name = 'space_needle', constraint_setting = 'notable_building')",
+        "constraint_value(name = 'peace_arch', constraint_setting = 'notable_building')",
+        "constraint_setting(name = 'museum')",
+        "constraint_value(name = 'moma', constraint_setting = 'museum')",
+        "constraint_value(name = 'sam', constraint_setting = 'museum')",
         "config_setting(",
         "    name = 'bad',",
-        "    constraint_values = [':empire_state', ':space_needle'],",
+        "    constraint_values = [",
+        "        ':empire_state',",
+        "        ':space_needle',",
+        "        ':moma',",
+        "        ':sam',",
+        "    ],",
         ");");
   }
 }
+