Deduplicate incompatible target_compatible_with labels

The latest refactor unintentionally made it so you can list the same
incompatible constraint multiple times in the `target_compatible_with`
attribute.

It was unintentional, but actually greatly simplifies a discussion
point on bazelbuild/bazel-skylib#381. That skylib patch aims to make
it easier to write non-trivial `target_compatible_with` expressions.
For example, to express that something is compatible with everything
except for Windows, one can use the following:

    foo_binary(
        name = "bin",
        target_compatible_with = select({
            "@platforms//os:windows": ["@platforms//:incomaptible"],
            "//conditions:default: [],
        }),
    )

The skylib patch aims to reduce that to:

    foo_binary(
        name = "bin",
        target_compatible_with = compatibility.none_of("@platforms//os:windows"),
    )

This works fine on its own, but a problem arises when these
expressions are composed. For example, a macro author might write the
following:

    def foo_wrapped_binary(name, target_compatible_with = [], **kwargs):
        foo_binary(
            name = name,
            target_compatible_with = target_compatible_with + select({
                "@platforms//os:none": ["@platforms//:incompatible"],
                "//conditions:default": [],
            }),
        )

A macro author should be able to express their own incompatibility
while also honouring user-defined incompatibility.

It turns out that my latest refactor (#14096) unintentionally made
this work. This happened because we now check for incompatibility
before we perform a lot of sanity checks. That's also one of the
reasons that visibility restrictions are not honoured for incomaptible
targets at the moment (#16044).

Without the deduplicating behaviour, macro authors are stuck with
either not allowing composition, or having to create unique
incompatible constraints for every piece in a composed
`target_compatible_with` expression.

This patch adds a test to validate the deduplicating behaviour to
cement it as a feature.

Unfortunately, this means that `target_compatible_with` behaves
differently from other label list attributes. For other label list
attributes, bazel complains when labels are duplicated. For example,
the following BUILD file:

    py_library(
        name = "lib",
    )

    py_binary(
        name = "bin",
        srcs = ["bin.py"],
        deps = [
        |   ":lib",
        |   ":lib",
        ],
    )

will result in the following error:

    $ bazel build :bin
    INFO: Invocation ID: 3d8345a4-2e76-493e-8343-c96a36185b52
    ERROR: /home/jenkins/repos/test_repo/BUILD:41:10: Label '//:lib' is duplicated in the 'deps' attribute of rule 'bin'
    ERROR: error loading package '': Package '' contains errors
    INFO: Elapsed time: 2.514s
    INFO: 0 processes.
    FAILED: Build did NOT complete successfully (1 packages loaded)

Closes #16274.

PiperOrigin-RevId: 474747867
Change-Id: Iacc4de12ce90176d803e0bc9e695d4ec14603449

Author: philsc

src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java

@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
@@ -23,6 +25,7 @@
 import com.google.devtools.build.lib.packages.BuiltinProvider;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.starlarkbuildapi.platform.IncompatiblePlatformProviderApi;
+import java.util.Comparator;
 import javax.annotation.Nullable;
 
 /**
@@ -69,6 +72,15 @@ public static IncompatiblePlatformProvider incompatibleDueToConstraints(
       @Nullable Label targetPlatform, ImmutableList<ConstraintValueInfo> constraints) {
     Preconditions.checkNotNull(constraints);
     Preconditions.checkArgument(!constraints.isEmpty());
+
+    // Deduplicate and sort the list of incompatible constraints. Doing it here means that everyone
+    // inspecting this provider doesn't have to deal with it.
+    constraints =
+        constraints.stream()
+            .sorted(Comparator.comparing(ConstraintValueInfo::label))
+            .distinct()
+            .collect(toImmutableList());
+
     return new AutoValue_IncompatiblePlatformProvider(targetPlatform, null, constraints);
   }
 
@@ -95,6 +107,8 @@ public boolean isImmutable() {
    *
    * <p>This may be null. If it is null, then {@code getTargetsResponsibleForIncompatibility()} is
    * guaranteed to be non-null. It will have at least one element in it if it is not null.
+   *
+   * <p>The list is sorted based on the stringified label of each constraint.
    */
   @Nullable
   public abstract ImmutableList<ConstraintValueInfo> constraintsResponsibleForIncompatibility();

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

@@ -325,12 +325,9 @@ private static String reportOnIncompatibility(ConfiguredTarget target) {
 
     message += "s [";
 
-    // Print out a sorted list to make the output reproducible.
     boolean first = true;
     for (ConstraintValueInfo constraintValueInfo :
-        ImmutableList.sortedCopyOf(
-            (ConstraintValueInfo a, ConstraintValueInfo b) -> b.label().compareTo(a.label()),
-            provider.constraintsResponsibleForIncompatibility())) {
+        provider.constraintsResponsibleForIncompatibility()) {
       if (first) {
         first = false;
       } else {

src/test/shell/integration/target_compatible_with_test.sh

@@ -154,6 +154,15 @@ platform(
     ],
 )
 
+platform(
+    name = "foo3_bar2_platform",
+    parents = ["${default_host_platform}"],
+    constraint_values = [
+        ":foo3",
+        ":bar2",
+    ],
+)
+
 sh_test(
     name = "pass_on_foo1",
     srcs = ["pass.sh"],
@@ -779,7 +788,7 @@ EOF
   expect_log '^Dependency chain:$'
   expect_log '^    //target_skipping:generated_test (.*)$'
   expect_log '^    //target_skipping:generate_with_tool (.*)$'
-  expect_log "^    //target_skipping:generator_tool (.*)   <-- target platform (//target_skipping:foo2_bar1_platform) didn't satisfy constraints \[//target_skipping:foo1, //target_skipping:bar2\]"
+  expect_log "^    //target_skipping:generator_tool (.*)   <-- target platform (//target_skipping:foo2_bar1_platform) didn't satisfy constraints \[//target_skipping:bar2, //target_skipping:foo1\]"
   expect_log 'FAILED: Build did NOT complete successfully'
 }
 
@@ -880,6 +889,53 @@ EOF
   expect_log '^//target_skipping:pass_on_everything_but_foo1_and_foo2  *  PASSED in'
 }
 
+# Validates that we can reference the same incompatible constraint in several,
+# composed select() statements. This is useful for expressing compatibility for
+# orthogonal constraints. It's also useful when a macro author wants to express
+# incompatibility while also honouring the user-defined incompatibility.
+function test_composition() {
+  # The first select() statement might come from a macro. The second might come
+  # from the user who's calling that macro.
+  cat >> target_skipping/BUILD <<EOF
+sh_test(
+    name = "pass_on_foo3_and_bar2",
+    srcs = [":pass.sh"],
+    target_compatible_with = select({
+        ":foo3": [],
+        "//conditions:default": [":not_compatible"],
+    }) + select({
+        ":bar2": [],
+        "//conditions:default": [":not_compatible"],
+    }),
+)
+EOF
+
+  cd target_skipping || fail "couldn't cd into workspace"
+
+  bazel test \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo3_bar2_platform \
+    --platforms=@//target_skipping:foo3_bar2_platform \
+    --nocache_test_results \
+    //target_skipping:pass_on_foo3_and_bar2 &> "${TEST_log}" \
+    || fail "Bazel failed unexpectedly."
+  expect_log '^//target_skipping:pass_on_foo3_and_bar2  *  PASSED in'
+
+  # Validate that we get an error about the target being incompatible. Make
+  # sure that the ":not_compatible" constraint is only listed once even though
+  # it appears twice in the configured "target_compatible_with" attribute.
+  bazel test \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo1_bar1_platform \
+    --platforms=@//target_skipping:foo1_bar1_platform \
+    //target_skipping:pass_on_foo3_and_bar2 &> "${TEST_log}" \
+    && fail "Bazel passed unexpectedly."
+
+  expect_log 'ERROR: Target //target_skipping:pass_on_foo3_and_bar2 is incompatible and cannot be built, but was explicitly requested'
+  expect_log "^    //target_skipping:pass_on_foo3_and_bar2 (.*)   <-- target platform (//target_skipping:foo1_bar1_platform) didn't satisfy constraint //target_skipping:not_compatible$"
+  expect_log 'FAILED: Build did NOT complete successfully'
+}
+
 function test_incompatible_with_aliased_constraint() {
   cat >> target_skipping/BUILD <<'EOF'
 alias(