From ebfb529a78978c3cfd1d93141c7762f908698df9 Mon Sep 17 00:00:00 2001
From: John Cater <jcater@google.com>
Date: Mon, 10 Apr 2023 16:04:03 -0400
Subject: [PATCH] Add attribute validation to IncompatibleTargetChecker.

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

Fixes #18021.
---
 .../IncompatibleTargetChecker.java            | 16 +++++++--
 .../TopLevelConstraintSemantics.java          |  2 +-
 .../packages/ConfiguredAttributeMapper.java   |  6 ++--
 .../google/devtools/build/lib/skyframe/BUILD  |  1 +
 .../lib/skyframe/PrerequisiteProducer.java    | 36 +++++++++++++++++--
 .../target_compatible_with_test.sh            | 27 ++++++++++++++
 6 files changed, 79 insertions(+), 9 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 0519a1f0f897f9..1f49e9a7be74e4 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
@@ -45,6 +45,7 @@
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
+import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper.ValidationException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.PackageSpecification;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
@@ -56,6 +57,7 @@
 import com.google.devtools.build.lib.util.OrderedSetMultimap;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.state.StateMachine;
+import java.util.List;
 import java.util.Optional;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
@@ -115,6 +117,8 @@ public static class IncompatibleTargetProducer implements StateMachine, Consumer
     /** Sink for the output of this state machine. */
     public interface ResultSink {
       void accept(Optional<RuleConfiguredTargetValue> incompatibleTarget);
+
+      void acceptValidationException(ValidationException e);
     }
 
     public IncompatibleTargetProducer(
@@ -148,8 +152,16 @@ public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
         return DONE;
       }
 
-      // Resolves the constraint labels.
-      for (Label label : attrs.get("target_compatible_with", BuildType.LABEL_LIST)) {
+      // Resolves the constraint labels, checking for invalid configured attributes.
+      List<Label> targetCompatibleWith;
+      try {
+        targetCompatibleWith = attrs.getAndValidate("target_compatible_with", BuildType.LABEL_LIST);
+      } catch (ValidationException e) {
+        sink.accept(Optional.empty());
+        sink.acceptValidationException(e);
+        return DONE;
+      }
+      for (Label label : targetCompatibleWith) {
         tasks.lookUp(
             ConfiguredTargetKey.builder().setLabel(label).setConfiguration(configuration).build(),
             this);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java
index 75baaddf1abf52..e9db8215b5bbcd 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java
@@ -303,7 +303,7 @@ private static String reportOnIncompatibility(ConfiguredTarget target) {
     String message = "\nDependency chain:";
     IncompatiblePlatformProvider provider = null;
 
-    // TODO(austinschuh): While the first eror is helpful, reporting all the errors at once would
+    // TODO(austinschuh): While the first error is helpful, reporting all the errors at once would
     // save the user bazel round trips.
     while (target != null) {
       message +=
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 768d6a920e2eb5..b6f5955f7bb4b9 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
@@ -125,7 +125,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.
@@ -292,8 +292,8 @@ public <T> T get(String attributeName, Type<T> type) {
       return getAndValidate(attributeName, type);
     } catch (ValidationException e) {
       // Callers that reach this branch should explicitly validate the attribute through an
-      // appropriate call and handle the exception directly. This method assumes
-      // pre-validated attributes.
+      // appropriate call (either {@link #validateAttributes} or {@link #getAndValidate}) and handle
+      // the exception directly. This method assumes pre-validated attributes.
       throw new IllegalStateException(
           "lookup failed on attribute " + attributeName + ": " + e.getMessage());
     }
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 758da7b6f95f79..53a9a95f926381 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -313,6 +313,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/PrerequisiteProducer.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java
index 1bff75cd12dfc9..0237f5bb48c3bc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java
@@ -59,6 +59,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.ValidationException;
 import com.google.devtools.build.lib.packages.ExecGroup;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
@@ -92,6 +93,7 @@
 import java.util.Set;
 import java.util.function.Predicate;
 import javax.annotation.Nullable;
+import net.starlark.java.syntax.Location;
 
 /**
  * Helper logic for {@link ConfiguredTargetFunction}: performs the analysis phase through
@@ -124,6 +126,7 @@ static class State implements SkyKeyComputeState, IncompatibleTargetProducer.Res
      * <p>Non-null only while the computation is in-flight.
      */
     @Nullable private Driver incompatibleTargetProducer;
+
     /**
      * If a value is present, it means the target was directly incompatible.
      *
@@ -131,6 +134,8 @@ static class State implements SkyKeyComputeState, IncompatibleTargetProducer.Res
      */
     private Optional<RuleConfiguredTargetValue> incompatibleTarget;
 
+    private ValidationException validationException;
+
     /** Null if not yet computed or if {@link #resolveConfigurationsResult} is non-null. */
     @Nullable private OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMapResult;
 
@@ -171,6 +176,11 @@ static class State implements SkyKeyComputeState, IncompatibleTargetProducer.Res
     public void accept(Optional<RuleConfiguredTargetValue> incompatibleTarget) {
       this.incompatibleTarget = incompatibleTarget;
     }
+
+    @Override
+    public void acceptValidationException(ValidationException e) {
+      this.validationException = e;
+    }
   }
 
   /**
@@ -475,14 +485,15 @@ private static boolean checkForIncompatibleTarget(
       @Nullable ConfigConditions configConditions,
       @Nullable PlatformInfo targetPlatformInfo,
       @Nullable NestedSetBuilder<Package> transitivePackages)
-      throws InterruptedException, IncompatibleTargetException {
+      throws InterruptedException, IncompatibleTargetException, DependencyEvaluationException {
     if (state.incompatibleTarget == null) {
+      BuildConfigurationValue configuration = targetAndConfiguration.getConfiguration();
       if (state.incompatibleTargetProducer == null) {
         state.incompatibleTargetProducer =
             new Driver(
                 new IncompatibleTargetProducer(
                     targetAndConfiguration.getTarget(),
-                    targetAndConfiguration.getConfiguration(),
+                    configuration,
                     configConditions,
                     targetPlatformInfo,
                     transitivePackages,
@@ -492,7 +503,26 @@ private static boolean checkForIncompatibleTarget(
         return false;
       }
       state.incompatibleTargetProducer = null;
-      if (state.incompatibleTarget.isPresent()) {
+      if (state.validationException != null) {
+        Label label = targetAndConfiguration.getLabel();
+        Location location = targetAndConfiguration.getTarget().getLocation();
+        env.getListener()
+            .post(
+                new AnalysisRootCauseEvent(
+                    configuration, label, state.validationException.getMessage()));
+        throw new DependencyEvaluationException(
+            new ConfiguredValueCreationException(
+                location,
+                state.validationException.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);
+      } else if (state.incompatibleTarget.isPresent()) {
         throw new IncompatibleTargetException(state.incompatibleTarget.get());
       }
     }
diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh
index 8f0f074576c2db..25b43d238ba24f 100755
--- a/src/test/shell/integration/target_compatible_with_test.sh
+++ b/src/test/shell/integration/target_compatible_with_test.sh
@@ -892,6 +892,33 @@ EOF
   expect_log 'ERROR: Build did NOT complete successfully'
 }
 
+# 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 'ERROR: Build did NOT complete successfully'
+  expect_not_log 'FATAL: bazel crashed'
+}
+
 # Validates that we can express targets being compatible with everything _but_
 # A and B.
 function test_inverse_logic() {