Fix a couple of bugs with Incompatible Target Skipping

While adding `target_compatible_with` attributes in the FRC team 971's
code base I came across bug #12553. It wasn't possible to make a
Python target depend on another incompatible Python target.

This patch fixes that issue by teaching `RuleContext` about
incompatible prerequisites. This also fixes an issue with the
validation of file extensions.

It's possible that `validateDirectPrerequisite()` skips a bit too much
validation. It was unfortunately the cleanest way I could think of.

I struggled a bit to come up with what ended up becoming
`RuleContextConstraintSemantics.IncompatibleCheckResult`. The purpose
of that class is to centralize the logic for checking for
`OutputFileConfiguredTarget` dependencies. Such dependencies need a
bit of special logic in order to find `IncompatiblePlatformProvider`
instances. When I implemented this patch, I realized that the
`TopLevelConstraintSemantics` logic had a very similar problem. It
didn't deal with the `OutputFileConfiguredTarget` problem at all. I
took the liberty of fixing that issue in this patch also because it
nicely re-used the same new helper.

I could not figure out a good way to avoid making `RuleContext` depend
on `RuleContextConstraintSemantics`.

Fixes #12553

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

@@ -54,6 +54,7 @@
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics;
+import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics;
 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.stringtemplate.TemplateContext;
@@ -2306,6 +2307,14 @@ private boolean checkRuleDependencyMandatoryProviders(
 
     private void validateDirectPrerequisite(
         Attribute attribute, ConfiguredTargetAndData prerequisite) {
+      if (RuleContextConstraintSemantics.checkForIncompatibility(prerequisite.getConfiguredTarget())
+          .isIncompatible()) {
+        // If the prerequisite is incompatible (e.g. has an incompatible provider), we pretend that
+        // there is no further validation needed. Otherwise, it would be difficult to make the
+        // incompatible target satisfy things like required providers and file extensions.
+        return;
+      }
+
       validateDirectPrerequisiteType(prerequisite, attribute);
       validateDirectPrerequisiteFileTypes(prerequisite, attribute);
       if (attribute.performPrereqValidatorCheck()) {

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

@@ -17,6 +17,7 @@
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.Streams.stream;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Verify;
@@ -830,6 +831,45 @@ private static void addSelectValuesToSet(BuildType.Selector<?> select, final Set
     }
   }
 
+  /**
+   * Provides information about a target's incompatibility.
+   *
+   * <p>After calling {@code checkForIncompatibility()}, the {@code isIncompatible} getter tells you
+   * whether the target is incompatible. If the target is incompatible, then {@code
+   * underlyingTarget} tells you which underlying target provided the incompatibility. For the vast
+   * majority of targets this is the same one passed to {@code checkForIncompatibility()}. In some
+   * instances like {@link OutputFileConfiguredTarget}, however, the {@code underlyingTarget} is the
+   * rule that generated the file.
+   */
+  @AutoValue
+  public abstract static class IncompatibleCheckResult {
+    private static IncompatibleCheckResult create(
+        boolean isIncompatible, ConfiguredTarget underlyingTarget) {
+      return new AutoValue_RuleContextConstraintSemantics_IncompatibleCheckResult(
+          isIncompatible, underlyingTarget);
+    }
+
+    public abstract boolean isIncompatible();
+
+    public abstract ConfiguredTarget underlyingTarget();
+  };
+
+  /**
+   * Checks whether the target is incompatible.
+   *
+   * <p>See the documentation for {@link RuleContextConstraintSemantics.IncompatibleCheckResult} for
+   * more information.
+   */
+  public static IncompatibleCheckResult checkForIncompatibility(ConfiguredTarget target) {
+    if (target instanceof OutputFileConfiguredTarget) {
+      // For generated files, we want to query the generating rule for providers. genrule() for
+      // example doesn't attach providers like IncompatiblePlatformProvider to its outputs.
+      target = ((OutputFileConfiguredTarget) target).getGeneratingRule();
+    }
+    return IncompatibleCheckResult.create(
+        target.getProvider(IncompatiblePlatformProvider.class) != null, target);
+  }
+
   /**
    * Creates an incompatible {@link ConfiguredTarget} if the corresponding rule is incompatible.
    *
@@ -870,22 +910,12 @@ public static ConfiguredTarget incompatibleConfiguredTarget(
     }
 
     // This is incompatible if one of the dependencies is as well.
-    ImmutableList.Builder<ConfiguredTarget> incompatibleDependenciesBuilder =
-        ImmutableList.builder();
-    for (ConfiguredTargetAndData infoCollection : prerequisiteMap.values()) {
-      ConfiguredTarget dependency = infoCollection.getConfiguredTarget();
-      if (dependency instanceof OutputFileConfiguredTarget) {
-        // For generated files, we want to query the generating rule for providers. genrule() for
-        // example doesn't attach providers like IncompatiblePlatformProvider to its outputs.
-        dependency = ((OutputFileConfiguredTarget) dependency).getGeneratingRule();
-      }
-      if (dependency.getProvider(IncompatiblePlatformProvider.class) != null) {
-        incompatibleDependenciesBuilder.add(dependency);
-      }
-    }
-
     ImmutableList<ConfiguredTarget> incompatibleDependencies =
-        incompatibleDependenciesBuilder.build();
+        stream(prerequisiteMap.values())
+            .map(value -> checkForIncompatibility(value.getConfiguredTarget()))
+            .filter(result -> result.isIncompatible())
+            .map(result -> result.underlyingTarget())
+            .collect(toImmutableList());
     if (!incompatibleDependencies.isEmpty()) {
       return createIncompatibleConfiguredTarget(ruleContext, incompatibleDependencies, null);
     }
@@ -987,7 +1017,7 @@ private static ConfiguredTarget createIncompatibleConfiguredTarget(
           new FailAction(
               ruleContext.getActionOwner(),
               outputArtifacts,
-              "Can't build this. This target is incompatible.",
+              "Can't build this. This target is incompatible. Please file a bug upstream.",
               Code.CANT_BUILD_INCOMPATIBLE_TARGET));
     }
     return builder.build();

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

@@ -123,11 +123,11 @@ public PlatformRestrictionsResult checkPlatformRestrictions(
     ImmutableSet.Builder<ConfiguredTarget> incompatibleButRequestedTargets = ImmutableSet.builder();
 
     for (ConfiguredTarget target : topLevelTargets) {
-      IncompatiblePlatformProvider incompatibleProvider =
-          target.getProvider(IncompatiblePlatformProvider.class);
+      RuleContextConstraintSemantics.IncompatibleCheckResult result =
+          RuleContextConstraintSemantics.checkForIncompatibility(target);
 
       // Move on to the next target if this one is compatible.
-      if (incompatibleProvider == null) {
+      if (!result.isIncompatible()) {
         continue;
       }
 
@@ -143,7 +143,9 @@ public PlatformRestrictionsResult checkPlatformRestrictions(
               String.format(
                   TARGET_INCOMPATIBLE_ERROR_TEMPLATE,
                   target.getLabel().toString(),
-                  reportOnIncompatibility(target));
+                  // We need access to the provider so we pass in the underlying target here that is
+                  // responsible for the incompatibility.
+                  reportOnIncompatibility(result.underlyingTarget()));
           throw new ViewCreationFailedException(
               targetIncompatibleMessage,
               FailureDetail.newBuilder()

src/test/shell/integration/target_compatible_with_test.sh

@@ -447,6 +447,92 @@ EOF
   expect_log 'FAILED: Build did NOT complete successfully'
 }
 
+# Validates that rules with custom providers are skipped when incompatible.
+# This is valuable because we use providers to convey incompatibility.
+function test_dependencies_with_providers() {
+  cat > target_skipping/rules.bzl <<EOF
+DummyProvider = provider()
+
+def _dummy_rule_impl(ctx):
+    return [DummyProvider()]
+
+dummy_rule = rule(
+    implementation = _dummy_rule_impl,
+    attrs = {
+        "deps": attr.label_list(providers=[DummyProvider]),
+    },
+)
+EOF
+
+  cat >> target_skipping/BUILD <<EOF
+load("//target_skipping:rules.bzl", "dummy_rule")
+
+dummy_rule(
+    name = "dummy1",
+    target_compatible_with = [
+        "//target_skipping:foo1",
+    ],
+)
+
+dummy_rule(
+    name = "dummy2",
+    deps = [
+        ":dummy1",
+    ],
+)
+EOF
+
+  cd target_skipping || fail "couldn't cd into workspace"
+
+  pwd >&2
+  bazel build \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo3_platform \
+    --platforms=@//target_skipping:foo3_platform \
+    //target_skipping/... &> "${TEST_log}" || fail "Bazel failed unexpectedly."
+  expect_log '^Target //target_skipping:dummy2 was skipped'
+}
+
+function test_dependencies_with_extensions() {
+  cat > target_skipping/rules.bzl <<EOF
+def _dummy_rule_impl(ctx):
+    out = ctx.actions.declare_file(ctx.attr.name + ".cc")
+    ctx.actions.write(out, "Dummy content")
+    return DefaultInfo(files = depset([out]))
+
+dummy_rule = rule(
+    implementation = _dummy_rule_impl,
+)
+EOF
+
+  cat >> target_skipping/BUILD <<EOF
+load("//target_skipping:rules.bzl", "dummy_rule")
+
+# Generates a dummy.cc file.
+dummy_rule(
+    name = "dummy_file",
+    target_compatible_with = [":foo1"],
+)
+
+cc_library(
+    name = "dummy_cc_lib",
+    srcs = [
+        "dummy_file",
+    ],
+)
+EOF
+
+  cd target_skipping || fail "couldn't cd into workspace"
+
+  pwd >&2
+  bazel build \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo3_platform \
+    --platforms=@//target_skipping:foo3_platform \
+    //target_skipping/... &> "${TEST_log}" || fail "Bazel failed unexpectedly."
+  expect_log '^Target //target_skipping:dummy_cc_lib was skipped'
+}
+
 # Validates the same thing as test_non_top_level_skipping, but with a cc_test
 # and adding one more level of dependencies.
 function test_cc_test() {
@@ -480,6 +566,21 @@ EOF
 
   cd target_skipping || fail "couldn't cd into workspace"
 
+  # Validate the generated file that makes up the test.
+  bazel test \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo2_bar1_platform \
+    --platforms=@//target_skipping:foo2_bar1_platform \
+    //target_skipping:generated_test.cc &> "${TEST_log}" && fail "Bazel passed unexpectedly."
+  expect_log "ERROR: Target //target_skipping:generated_test.cc is incompatible and cannot be built, but was explicitly requested"
+
+  # Validate that we get the dependency chain printed out.
+  expect_log '^Dependency chain:$'
+  expect_log '^    //target_skipping:generate_with_tool$'
+  expect_log "^    //target_skipping:generator_tool   <-- target platform didn't satisfy constraint //target_skipping:foo1$"
+  expect_log 'FAILED: Build did NOT complete successfully'
+
+  # Validate the test.
   bazel test \
     --show_result=10 \
     --host_platform=@//target_skipping:foo2_bar1_platform \
@@ -727,7 +828,6 @@ EOF
 
   bazel test --show_result=10  \
     --host_platform=@//target_skipping:foo3_platform \
-    --toolchain_resolution_debug \
     --platforms=@//target_skipping:foo3_platform \
     //target_skipping:foo3_analysistest_test &> "${TEST_log}" \
     || fail "Bazel failed unexpectedly."