Fix an obscure crash scenario with static configs and BuildConfiguration.equals.

See OutputFileConfiguredTargetTest for deep details.

Also more strongly enforce the expectation that all output files have generating rules.

--
PiperOrigin-RevId: 143570028
MOS_MIGRATED_REVID=143570028

4 files changed, 16 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
index d7205a5bf5..8ba3211da1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
@@ -35,7 +35,7 @@ public class OutputFileConfiguredTarget extends FileConfiguredTarget
       TransitiveInfoCollection generatingRule, Artifact outputArtifact) {
     super(targetContext, outputArtifact);
     Preconditions.checkArgument(targetContext.getTarget() == outputFile);
-    this.generatingRule = generatingRule;
+    this.generatingRule = Preconditions.checkNotNull(generatingRule);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
index c8b26b97bf..343e69878f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
@@ -40,7 +40,7 @@ public final class PackageGroupConfiguredTarget extends AbstractConfiguredTarget
     NestedSetBuilder<PackageSpecification> builder =
         NestedSetBuilder.stableOrder();
     for (Label label : packageGroup.getIncludes()) {
-      TransitiveInfoCollection include = targetContext.findDirectPrerequisite(
+      TransitiveInfoCollection include = targetContext.maybeFindDirectPrerequisite(
           label, targetContext.getConfiguration());
       PackageSpecificationProvider provider = include == null ? null :
           include.getProvider(PackageSpecificationProvider.class);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java
index cf1afb2de6..fc263efbab 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -88,7 +89,20 @@ public class TargetContext {
     return visibility;
   }
 
+  /**
+   * Returns the prerequisite with the given label and configuration. Throws a RuntimeException if
+   * no such prerequisite exists.
+   */
   TransitiveInfoCollection findDirectPrerequisite(Label label, BuildConfiguration config) {
+    return Verify.verifyNotNull(maybeFindDirectPrerequisite(label, config),
+        "Could not find prerequisite %s in the expected configuration", label);
+  }
+
+  /**
+   * Returns the prerequisite with the given label and configuration, or null if no such
+   * prerequisite exists.
+   */
+  TransitiveInfoCollection maybeFindDirectPrerequisite(Label label, BuildConfiguration config) {
     for (ConfiguredTarget prerequisite : directPrerequisites) {
       if (prerequisite.getLabel().equals(label) && (prerequisite.getConfiguration() == config)) {
         return prerequisite;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
index a3c63cf2dd..86d736d60a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
@@ -763,12 +763,6 @@ public class ConstraintSemantics {
           // Note this reassignment means constraint violation errors reference the generating
           // rule, not the file. This makes the source of the environmental mismatch more clear.
           dep = ((OutputFileConfiguredTarget) dep).getGeneratingRule();
-          if (dep == null) {
-            // This shouldn't happen, but this is causing spurious Bazel test failures because the
-            // getProvider call below throws a NullPointerException. See b/33385302 for details.
-            // TODO(gregce): fix the underlying bug, then remove this condition.
-            continue;
-          }
         }
         // Input files don't support environments. We may subsequently opt them into constraint
         // checking, but for now just pass them by.