Allow multiple matching select branches if they resolve to the same v…

…alue (#18066)

Select branches that map to the same value can unambiguosly be resolved even when multiple are true. This is currently not allowed and requires the use of constructs like `bazel-skylib`'s `selects.config_setting_group`. With this change, assuming A and B are true, the following is allowed:
```
select({
     "//:A": "Value",
     "//:B": "Value",
})
```

Closes #14874.

PiperOrigin-RevId: 523597091
Change-Id: I751809b1b9e11d20a86ae2af4bbdb0228fd3c670

Co-authored-by: Alessandro Patti <[email protected]>

site/en/configure/attributes.md

@@ -71,11 +71,13 @@ command line. Specifically, `deps` becomes:
 targets. By using `select()` in a configurable attribute, the attribute
 effectively adopts different values when different conditions hold.
 
-Matches must be unambiguous: either exactly one condition must match or, if
-multiple conditions match, one's `values` must be a strict superset of all
-others'. For example, `values = {"cpu": "x86", "compilation_mode": "dbg"}` is an
-unambiguous specialization of `values = {"cpu": "x86"}`. The built-in condition
-[`//conditions:default`](#default-condition) automatically matches when
+Matches must be unambiguous: if multiple conditions match then either
+*  They all resolve to the same value. For example, when running on linux x86, this is unambiguous
+   `{"@platforms//os:linux": "Hello", "@platforms//cpu:x86_64": "Hello"}` because both branches resolve to "hello".
+*  One's `values` is a strict superset of all others'. For example, `values = {"cpu": "x86", "compilation_mode": "dbg"}`
+   is an unambiguous specialization of `values = {"cpu": "x86"}`.
+
+The built-in condition [`//conditions:default`](#default-condition) automatically matches when
 nothing else does.
 
 While this example uses `deps`, `select()` works just as well on `srcs`,
@@ -516,7 +518,7 @@ Unlike `selects.with_or`, different targets can share `:config1_or_2` across
 different attributes.
 
 It's an error for multiple conditions to match unless one is an unambiguous
-"specialization" of the others. See [here](#configurable-build-example) for details.
+"specialization" of the others or they all resolve to the same value. See [here](#configurable-build-example) for details.
 
 ## AND chaining {:#and-chaining}
 

site/en/docs/configurable-attributes.md

@@ -71,11 +71,13 @@ command line. Specifically, `deps` becomes:
 targets. By using `select()` in a configurable attribute, the attribute
 effectively adopts different values when different conditions hold.
 
-Matches must be unambiguous: either exactly one condition must match or, if
-multiple conditions match, one's `values` must be a strict superset of all
-others'. For example, `values = {"cpu": "x86", "compilation_mode": "dbg"}` is an
-unambiguous specialization of `values = {"cpu": "x86"}`. The built-in condition
-[`//conditions:default`](#default-condition) automatically matches when
+Matches must be unambiguous: if multiple conditions match then either
+*  They all resolve to the same value. For example, when running on linux x86, this is unambiguous
+   `{"@platforms//os:linux": "Hello", "@platforms//cpu:x86_64": "Hello"}` because both branches resolve to "hello".
+*  One's `values` is a strict superset of all others'. For example, `values = {"cpu": "x86", "compilation_mode": "dbg"}`
+   is an unambiguous specialization of `values = {"cpu": "x86"}`.
+
+The built-in condition [`//conditions:default`](#default-condition) automatically matches when
 nothing else does.
 
 While this example uses `deps`, `select()` works just as well on `srcs`,
@@ -516,7 +518,7 @@ Unlike `selects.with_or`, different targets can share `:config1_or_2` across
 different attributes.
 
 It's an error for multiple conditions to match unless one is an unambiguous
-"specialization" of the others. See [here](#configurable-build-example) for details.
+"specialization" of the others or they all resolve to the same value. See [here](#configurable-build-example) for details.
 
 ## AND chaining {:#and-chaining}
 

src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm

@@ -672,7 +672,7 @@ sh_binary(
     demonstrated in Example 2 below.
   </li>
   <li>If multiple conditions match and one is not a specialization of all the
-    others, Bazel fails with an error.
+    others, Bazel fails with an error, unless all conditions resolve to the same value.
   </li>
   <li>The special pseudo-label <code>//conditions:default</code> is
     considered to match if no other condition matches. If this condition

src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java

@@ -176,7 +176,10 @@ private static class ConfigKeyAndValue<T> {
 
   private <T> ConfigKeyAndValue<T> resolveSelector(String attributeName, Selector<T> selector)
       throws ValidationException {
-    Map<Label, ConfigKeyAndValue<T>> matchingConditions = new LinkedHashMap<>();
+    // Use a LinkedHashMap to guarantee a deterministic branch selection when multiple branches
+    // matches but they
+    // resolve to the same value.
+    LinkedHashMap<Label, ConfigKeyAndValue<T>> matchingConditions = new LinkedHashMap<>();
     // Use a LinkedHashSet to guarantee deterministic error message ordering. We use a LinkedHashSet
     // vs. a more general SortedSet because the latter supports insertion-order, which should more
     // closely match how users see select() structures in BUILD files.
@@ -220,17 +223,19 @@ private <T> ConfigKeyAndValue<T> resolveSelector(String attributeName, Selector<
       }
     }
 
-    if (matchingConditions.size() > 1) {
+    if (matchingConditions.values().stream().map(s -> s.value).distinct().count() > 1) {
       throw new ValidationException(
           "Illegal ambiguous match on configurable attribute \""
               + attributeName
               + "\" in "
               + getLabel()
               + ":\n"
               + Joiner.on("\n").join(matchingConditions.keySet())
-              + "\nMultiple matches are not allowed unless one is unambiguously more specialized.");
-    } else if (matchingConditions.size() == 1) {
-      return Iterables.getOnlyElement(matchingConditions.values());
+              + "\nMultiple matches are not allowed unless one is unambiguously "
+              + "more specialized or they resolve to the same value. "
+              + "See https://bazel.build/reference/be/functions#select.");
+    } else if (matchingConditions.size() > 0) {
+      return Iterables.getFirst(matchingConditions.values(), null);
     }
 
     // If nothing matched, choose the default condition.

src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java

@@ -642,7 +642,8 @@ public void multipleMatches() throws Exception {
         "Illegal ambiguous match on configurable attribute \"srcs\" in //a:gen:\n"
             + "//conditions:dup1\n"
             + "//conditions:dup2\n"
-            + "Multiple matches are not allowed unless one is unambiguously more specialized.");
+            + "Multiple matches are not allowed unless one is unambiguously more specialized "
+            + "or they resolve to the same value.");
   }
 
   /**
@@ -688,6 +689,37 @@ public void multipleMatchesConditionAndSubcondition() throws Exception {
             "bin java/a/libgeneric.jar", "bin java/a/libprecise.jar"));
   }
 
+  /** Tests that multiple matches are allowed for conditions where the value is the same. */
+  @Test
+  public void multipleMatchesSameValue() throws Exception {
+    reporter.removeHandler(failFastHandler); // Expect errors.
+    scratch.file(
+        "conditions/BUILD",
+        "config_setting(",
+        "    name = 'dup1',",
+        "    values = {'compilation_mode': 'opt'})",
+        "config_setting(",
+        "    name = 'dup2',",
+        "    values = {'define': 'foo=bar'})");
+    scratch.file(
+        "a/BUILD",
+        "genrule(",
+        "    name = 'gen',",
+        "    cmd = '',",
+        "    outs = ['gen.out'],",
+        "    srcs = select({",
+        "        '//conditions:dup1': ['a.in'],",
+        "        '//conditions:dup2': ['a.in'],",
+        "        '" + BuildType.Selector.DEFAULT_CONDITION_KEY + "': [':default.in'],",
+        "    }))");
+    checkRule(
+        "//a:gen",
+        "srcs",
+        ImmutableList.of("-c", "opt", "--define", "foo=bar"),
+        /*expected:*/ ImmutableList.of("src a/a.in"),
+        /*not expected:*/ ImmutableList.of("src a/default.in"));
+  }
+
   /**
    * Tests that when multiple conditions match but one condition is more specialized than the
    * others, it is chosen and there is no error.