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 bazelbuild#18021.

Closes bazelbuild#18027.

PiperOrigin-RevId: 523976899
Change-Id: I2602aa3d4febc4c486d610e19c3a61632b0519b5

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,15 @@ 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.acceptValidationException(e);
+        return DONE;
+      }
+      for (Label label : targetCompatibleWith) {
         tasks.lookUp(
             ConfiguredTargetKey.builder().setLabel(label).setConfiguration(configuration).build(),
             this);

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 +=

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.
@@ -297,8 +297,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());
     }

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -312,6 +312,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",

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,12 +126,23 @@ 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.
      *
-     * <p>Non-null after the {@link #incompatibleTargetProducer} completes.
+     * <p>Either this or {@link #validationException} will be non-null after the {@link
+     * #incompatibleTargetProducer} completes.
+     */
+    @Nullable private Optional<RuleConfiguredTargetValue> incompatibleTarget;
+
+    /**
+     * If this is set, an exception occurred during validation of the {@code target_compatible_with}
+     * attribute.
+     *
+     * <p>Either this or {@link #incompatibleTarget} will be non-null after the {@link
+     * #incompatibleTargetProducer} completes.
      */
-    private Optional<RuleConfiguredTargetValue> incompatibleTarget;
+    @Nullable private ValidationException validationException;
 
     /** Null if not yet computed or if {@link #resolveConfigurationsResult} is non-null. */
     @Nullable private OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMapResult;
@@ -171,6 +184,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 +493,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,6 +511,26 @@ private static boolean checkForIncompatibleTarget(
         return false;
       }
       state.incompatibleTargetProducer = null;
+      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);
+      }
       if (state.incompatibleTarget.isPresent()) {
         throw new IncompatibleTargetException(state.incompatibleTarget.get());
       }

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() {