Validate conflicting constraints in target_compatible_with

This makes sure a target's target_compatible_with doesn't include
impossible to satisfy constraints via 2 constraint_values for the same
constraint_setting

Fixes #27580

Closes #27583.

PiperOrigin-RevId: 839335518
Change-Id: I4d422ad7cb60662745fee176245acdb28a735136

Author: keith

src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.ConfigConditions;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
+import com.google.devtools.build.lib.analysis.platform.ConstraintCollection;
 import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
@@ -106,6 +107,8 @@ public static class IncompatibleTargetProducer implements StateMachine, Consumer
 
     private final StateMachine runAfter;
 
+    private final ImmutableList.Builder<ConstraintValueInfo> allConstraintValuesBuilder =
+        new ImmutableList.Builder<>();
     private final ImmutableList.Builder<ConstraintValueInfo> invalidConstraintValuesBuilder =
         new ImmutableList.Builder<>();
 
@@ -170,13 +173,26 @@ public StateMachine step(Tasks tasks) {
     public void accept(SkyValue value) {
       var configuredTarget = ((ConfiguredTargetValue) value).getConfiguredTarget();
       @Nullable ConstraintValueInfo info = PlatformProviderUtils.constraintValue(configuredTarget);
-      if (info == null || platformInfo.constraints().hasConstraintValue(info)) {
+      if (info == null) {
         return;
       }
-      invalidConstraintValuesBuilder.add(info);
+      allConstraintValuesBuilder.add(info);
+      if (!platformInfo.constraints().hasConstraintValue(info)) {
+        invalidConstraintValuesBuilder.add(info);
+      }
     }
 
     private StateMachine processResult(Tasks tasks) {
+      var allConstraintValues = allConstraintValuesBuilder.build();
+
+      // Validate that there are no duplicate constraint values from the same constraint setting
+      try {
+        ConstraintCollection.validateConstraints(allConstraintValues);
+      } catch (ConstraintCollection.DuplicateConstraintException e) {
+        sink.acceptValidationException(new ValidationException(e.getMessage()));
+        return runAfter;
+      }
+
       var invalidConstraintValues = invalidConstraintValuesBuilder.build();
       if (!invalidConstraintValues.isEmpty()) {
         sink.acceptIncompatibleTarget(

src/main/java/com/google/devtools/build/lib/analysis/platform/ConstraintCollection.java

@@ -284,6 +284,16 @@ public void addToFingerprint(Fingerprint fp) {
     constraints().values().forEach(constraintValue -> constraintValue.addTo(fp));
   }
 
+  /**
+   * Validates that the given constraints do not contain conflicting values.
+   *
+   * <p>Checks that no {@link ConstraintSettingInfo} has multiple different {@link
+   * ConstraintValueInfo} values. Multiple instances of the same constraint value are allowed.
+   *
+   * @param constraintValues the constraints to validate
+   * @throws DuplicateConstraintException if multiple different constraint values exist for the same
+   *     constraint setting
+   */
   public static void validateConstraints(Iterable<ConstraintValueInfo> constraintValues)
       throws DuplicateConstraintException {
     // Collect the constraints by the settings.
@@ -292,12 +302,14 @@ public static void validateConstraints(Iterable<ConstraintValueInfo> constraintV
             .collect(
                 toImmutableListMultimap(ConstraintValueInfo::constraint, Functions.identity()));
 
-    // Find settings with duplicate values.
+    // Find different constraint values targeting the same constraint setting.
+    // Ignore multiple instances of the same constraint value.
     ImmutableListMultimap<ConstraintSettingInfo, ConstraintValueInfo> duplicates =
         constraints.asMap().entrySet().stream()
-            .filter(e -> e.getValue().size() > 1)
+            .filter(e -> e.getValue().stream().distinct().count() > 1)
             .collect(
-                flatteningToImmutableListMultimap(Map.Entry::getKey, e -> e.getValue().stream()));
+                flatteningToImmutableListMultimap(
+                    Map.Entry::getKey, e -> e.getValue().stream().distinct()));
 
     if (!duplicates.isEmpty()) {
       throw new DuplicateConstraintException(duplicates);

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

@@ -126,7 +126,7 @@ public void validateAttributes() throws ValidationException {
 
   /** ValidationException indicates an error during attribute validation. */
   public static final class ValidationException extends Exception {
-    private ValidationException(String message) {
+    public ValidationException(String message) {
       super(message);
     }
   }

src/test/shell/integration/target_compatible_with_test.sh

@@ -289,6 +289,33 @@ EOF
   expect_log "'//target_skipping:foo3_config_setting' does not have mandatory providers: 'ConstraintValueInfo'"
 }
 
+# Validates that we get an error when target_compatible_with contains duplicate
+# constraint values from the same constraint setting. This is a regression test
+# for https://github.com/bazelbuild/bazel/issues/27580.
+function test_duplicate_constraint_values_in_target_compatible_with() {
+  cat >> target_skipping/BUILD <<'EOF'
+load("@rules_shell//shell:sh_binary.bzl", "sh_binary")
+
+sh_binary(
+    name = "target_with_duplicate_constraints",
+    srcs = ["pass.sh"],
+    target_compatible_with = [
+        ":foo1",
+        ":foo2",
+    ],
+)
+EOF
+
+  cd target_skipping || fail "couldn't cd into workspace"
+
+  bazel build \
+    //target_skipping:target_with_duplicate_constraints &> "${TEST_log}" \
+    && fail "Bazel succeeded unexpectedly."
+
+  expect_log "Duplicate constraint values detected: constraint_setting //target_skipping:foo_version has \[//target_skipping:foo1, //target_skipping:foo2\]"
+  expect_log "ERROR: Analysis of target '//target_skipping:target_with_duplicate_constraints' failed"
+}
+
 # Validates that the console log provides useful information to the user for
 # builds.
 function test_console_log_for_builds() {