From 5e9fa3967cf0288083638aec3b34328ab8a90a51 Mon Sep 17 00:00:00 2001
From: John Cater <jcater@google.com>
Date: Tue, 18 Apr 2023 16:38:39 -0400
Subject: [PATCH] Add attribute validation to IncompatibleTargetChecker.
 (#18135)

This causes errors with configurable `target_compatible_with` to be
reported as errors, rather than causing a Bazel crash.

Fixes bazelbuild#18021.

(This is functionally the same change as
ebfb529a78978c3cfd1d93141c7762f908698df9, but applied to the
release-6.2.0 branch)
---
 .../IncompatibleTargetChecker.java            |  4 +--
 .../packages/ConfiguredAttributeMapper.java   |  2 +-
 .../google/devtools/build/lib/skyframe/BUILD  |  1 +
 .../skyframe/ConfiguredTargetFunction.java    | 31 ++++++++++++++-----
 .../target_compatible_with_test.sh            | 27 ++++++++++++++++
 5 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java
index 2f4bb040daf303..23bb7ddbedaeba 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java
@@ -107,7 +107,7 @@ public static Optional<RuleConfiguredTargetValue> createDirectlyIncompatibleTarg
       Environment env,
       @Nullable PlatformInfo platformInfo,
       NestedSetBuilder<Package> transitivePackages)
-      throws InterruptedException {
+      throws ConfiguredAttributeMapper.ValidationException, InterruptedException {
     Target target = targetAndConfiguration.getTarget();
     Rule rule = target.getAssociatedRule();
 
@@ -124,7 +124,7 @@ public static Optional<RuleConfiguredTargetValue> createDirectlyIncompatibleTarg
     }
 
     // Resolve the constraint labels.
-    List<Label> labels = attrs.get("target_compatible_with", BuildType.LABEL_LIST);
+    List<Label> labels = attrs.getAndValidate("target_compatible_with", BuildType.LABEL_LIST);
     ImmutableList<ConfiguredTargetKey> constraintKeys =
         labels.stream()
             .map(
diff --git a/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java
index 631910d7a904be..e9785e7da95b85 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java
@@ -127,7 +127,7 @@ private ValidationException(String message) {
    * Variation of {@link #get} that throws an informative exception if the attribute can't be
    * resolved due to intrinsic contradictions in the configuration.
    */
-  private <T> T getAndValidate(String attributeName, Type<T> type) throws ValidationException {
+  public <T> T getAndValidate(String attributeName, Type<T> type) throws ValidationException {
     SelectorList<T> selectorList = getSelectorList(attributeName, type);
     if (selectorList == null) {
       // This is a normal attribute.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index beeaa75a9c5599..cf1800915830a8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -307,6 +307,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
         "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
         "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper",
         "//src/main/java/com/google/devtools/build/lib/packages:exec_group",
         "//src/main/java/com/google/devtools/build/lib/packages:globber",
         "//src/main/java/com/google/devtools/build/lib/packages:globber_utils",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index dca55e0a094f0c..017ac2ead97dec 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -71,6 +71,7 @@
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Aspect;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
 import com.google.devtools.build.lib.packages.ExecGroup;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
@@ -108,6 +109,7 @@
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
+import net.starlark.java.syntax.Location;
 
 /**
  * SkyFunction for {@link ConfiguredTargetValue}s.
@@ -335,13 +337,28 @@ public SkyValue compute(SkyKey key, Environment env)
                 getPrioritizedDetailedExitCode(causes)));
       }
 
-      Optional<RuleConfiguredTargetValue> incompatibleTarget =
-          IncompatibleTargetChecker.createDirectlyIncompatibleTarget(
-              targetAndConfiguration,
-              configConditions,
-              env,
-              platformInfo,
-              state.transitivePackages);
+      Optional<RuleConfiguredTargetValue> incompatibleTarget;
+      try {
+        incompatibleTarget =
+            IncompatibleTargetChecker.createDirectlyIncompatibleTarget(
+                targetAndConfiguration,
+                configConditions,
+                env,
+                platformInfo,
+                state.transitivePackages);
+      } catch (ConfiguredAttributeMapper.ValidationException e) {
+        BuildConfigurationValue configuration = targetAndConfiguration.getConfiguration();
+        Label label = targetAndConfiguration.getLabel();
+        Location location = targetAndConfiguration.getTarget().getLocation();
+        env.getListener().post(new AnalysisRootCauseEvent(configuration, label, e.getMessage()));
+        throw new DependencyEvaluationException(
+            new ConfiguredValueCreationException(
+                location, e.getMessage(), label, configuration.getEventId(), null, null),
+            // These errors occur within DependencyResolver, which is attached to the current
+            // target. i.e. no dependent ConfiguredTargetFunction call happens to report its own
+            // error.
+            /* depReportedOwnError= */ false);
+      }
       if (incompatibleTarget == null) {
         return null;
       }
diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh
index ca7c90d0609167..1aad0e721bd8a0 100755
--- a/src/test/shell/integration/target_compatible_with_test.sh
+++ b/src/test/shell/integration/target_compatible_with_test.sh
@@ -1551,4 +1551,31 @@ EOF
   expect_not_log "${debug_message5}"
 }
 
+# Regression test for b/277371822.
+function test_missing_default() {
+  cat >> target_skipping/BUILD <<'EOF'
+sh_test(
+    name = "pass_on_foo1_or_foo2_but_not_on_foo3",
+    srcs = [":pass.sh"],
+    target_compatible_with = select({
+        ":foo1": [],
+        # No default branch.
+    }),
+)
+EOF
+
+  cd target_skipping || fail "couldn't cd into workspace"
+
+  bazel test \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo3_platform \
+    --platforms=@//target_skipping:foo3_platform \
+    //target_skipping:pass_on_foo1_or_foo2_but_not_on_foo3 &> "${TEST_log}" \
+    && fail "Bazel passed unexpectedly."
+
+  expect_log 'ERROR:.*configurable attribute "target_compatible_with" in //target_skipping:pass_on_foo1_or_foo2_but_not_on_foo3'
+  expect_log 'FAILED: Build did NOT complete successfully'
+  expect_not_log 'FATAL: bazel crashed'
+}
+
 run_suite "target_compatible_with tests"