Some constraint enforcement tweaks:

1) Exclude host dependencies from constraint checking.
2) Check output files with the environment specs of their generating rules.
3) Provide a more generalized way to opt certain rule classes out of
   constraint checking (this fixes accidental checking on config_setting
   rules, which didn't really make sense).

--
MOS_MIGRATED_REVID=87963638

5 files changed, 70 insertions, 23 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index b6f4bfe1f1..9cd41c3609 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -141,15 +141,14 @@ public final class RuleConfiguredTargetBuilder {
    * publishes this rule's supported environments for the rules that depend on it.
    */
   private void checkConstraints() {
-    if (providers.get(SupportedEnvironmentsProvider.class) == null) {
-      // Note the "environment" rule sets its own SupportedEnvironmentProvider instance, so this
-      // logic is for "normal" rules that just want to apply default semantics.
-      EnvironmentCollection supportedEnvironments =
-          ConstraintSemantics.getSupportedEnvironments(ruleContext);
-      if (supportedEnvironments != null) {
-        add(SupportedEnvironmentsProvider.class, new SupportedEnvironments(supportedEnvironments));
-        ConstraintSemantics.checkConstraints(ruleContext, supportedEnvironments);
-      }
+    if (!ruleContext.getRule().getRuleClassObject().supportsConstraintChecking()) {
+      return;
+    }
+    EnvironmentCollection supportedEnvironments =
+        ConstraintSemantics.getSupportedEnvironments(ruleContext);
+    if (supportedEnvironments != null) {
+      add(SupportedEnvironmentsProvider.class, new SupportedEnvironments(supportedEnvironments));
+      ConstraintSemantics.checkConstraints(ruleContext, supportedEnvironments);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigRuleClasses.java
index e16a83d884..438f6234b4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigRuleClasses.java
@@ -51,6 +51,8 @@ public class ConfigRuleClasses {
                // No need to show up in ":all", etc. target patterns.
               .value(ImmutableList.of("manual"))
               .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON))
+          .exemptFromConstraintChecking(
+              "these rules don't include content that gets built into their dependers")
           .build();
     }
   }
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 103c796a2e..3527c31949 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
@@ -16,8 +16,10 @@ package com.google.devtools.build.lib.analysis.constraints;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.analysis.OutputFileConfiguredTarget;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
@@ -434,15 +436,9 @@ public class ConstraintSemantics {
    */
   public static void checkConstraints(RuleContext ruleContext,
       EnvironmentCollection ruleEnvironments) {
-
-    for (TransitiveInfoCollection dependency : getAllPrerequisites(ruleContext)) {
+    for (TransitiveInfoCollection dependency : getConstraintCheckedDependencies(ruleContext)) {
       SupportedEnvironmentsProvider depProvider =
           dependency.getProvider(SupportedEnvironmentsProvider.class);
-      if (depProvider == null) {
-        // Input files (InputFileConfiguredTarget) don't support environments. We may subsequently
-        // opt them into constraint checking, but for now just pass them by.
-        continue;
-      }
       Collection<Label> unsupportedEnvironments =
           getUnsupportedEnvironments(depProvider.getEnvironments(), ruleEnvironments);
 
@@ -495,20 +491,37 @@ public class ConstraintSemantics {
   /**
    * Returns all dependencies that should be constraint-checked against the current rule.
    */
-  private static Iterable<TransitiveInfoCollection> getAllPrerequisites(RuleContext ruleContext) {
-    Set<TransitiveInfoCollection> prerequisites = new LinkedHashSet<>();
+  private static Iterable<TransitiveInfoCollection> getConstraintCheckedDependencies(
+      RuleContext ruleContext) {
+    Set<TransitiveInfoCollection> depsToCheck = new LinkedHashSet<>();
     AttributeMap attributes = ruleContext.attributes();
 
     for (String attr : attributes.getAttributeNames()) {
       Type<?> attrType = attributes.getAttributeType(attr);
-      // TODO(bazel-team): support specifying which attributes are subject to constraint checking
       if ((attrType == Type.LABEL || attrType == Type.LABEL_LIST)
           && !RuleClass.isConstraintAttribute(attr)
           && !attr.equals("visibility")) {
-        prerequisites.addAll(
-            ruleContext.getPrerequisites(attr, RuleConfiguredTarget.Mode.DONT_CHECK));
+
+        for (TransitiveInfoCollection dep :
+            ruleContext.getPrerequisites(attr, RuleConfiguredTarget.Mode.DONT_CHECK)) {
+          // Output files inherit the environment spec of their generating rule.
+          if (dep instanceof OutputFileConfiguredTarget) {
+            // 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();
+          }
+          // Input files don't support environments. We may subsequently opt them into constraint
+          // checking, but for now just pass them by. Otherwise, we opt in anything that's not
+          // a host dependency.
+          // TODO(bazel-team): support choosing which attributes are subject to constraint checking
+          if (dep.getProvider(SupportedEnvironmentsProvider.class) != null
+              && !Verify.verifyNotNull(dep.getConfiguration()).isHostConfiguration()) {
+            depsToCheck.add(dep);
+          }
+        }
       }
     }
-    return prerequisites;
+
+    return depsToCheck;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java
index 0553af0686..ea0644ef82 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java
@@ -42,6 +42,7 @@ public final class EnvironmentRule implements RuleDefinition {
             .nonconfigurable("low-level attribute, used in TargetUtils without configurations"))
         .removeAttribute(RuleClass.COMPATIBLE_ENVIRONMENT_ATTR)
         .removeAttribute(RuleClass.RESTRICTED_ENVIRONMENT_ATTR)
+        .exemptFromConstraintChecking("this rule *defines* a constraint")
         .setUndocumented()
         .build();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 23f1c97f59..17866c552e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -432,6 +432,7 @@ public final class RuleClass {
     private SkylarkEnvironment ruleDefinitionEnvironment = null;
     private Set<Class<?>> configurationFragments = new LinkedHashSet<>();
     private boolean failIfMissingConfigurationFragment;
+    private boolean supportsConstraintChecking = true;
 
     private final Map<String, Attribute> attributes = new LinkedHashMap<>();
 
@@ -459,6 +460,7 @@ public final class RuleClass {
         }
         configurationFragments.addAll(parent.requiredConfigurationFragments);
         failIfMissingConfigurationFragment |= parent.failIfMissingConfigurationFragment;
+        supportsConstraintChecking = parent.supportsConstraintChecking;
 
         for (Attribute attribute : parent.getAttributes()) {
           String attrName = attribute.getName();
@@ -511,7 +513,7 @@ public final class RuleClass {
           configuredTargetFactory, validityPredicate, preferredDependencyPredicate,
           ImmutableSet.copyOf(advertisedProviders), configuredTargetFunction,
           ruleDefinitionEnvironment, configurationFragments, failIfMissingConfigurationFragment,
-          attributes.values().toArray(new Attribute[0]));
+          supportsConstraintChecking, attributes.values().toArray(new Attribute[0]));
     }
 
     /**
@@ -743,6 +745,19 @@ public final class RuleClass {
     }
 
     /**
+     * Exempts rules of this type from the constraint enforcement system. This should only be
+     * applied to rules that are intrinsically incompatible with constraint checking (any
+     * application of this method weakens the reach and strength of the system).
+     *
+     * @param reason user-informative message explaining the reason for exemption (not used)
+     */
+    public <TYPE> Builder exemptFromConstraintChecking(String reason) {
+      Preconditions.checkState(this.supportsConstraintChecking);
+      this.supportsConstraintChecking = false;
+      return this;
+    }
+
+    /**
      * Returns an Attribute.Builder object which contains a replica of the
      * same attribute in the parent rule if exists.
      *
@@ -841,6 +856,14 @@ public final class RuleClass {
    */
   private final boolean failIfMissingConfigurationFragment;
 
+
+  /**
+   * Determines whether instances of this rule should be checked for constraint compatibility
+   * with their dependencies and the rules that depend on them. This should be true for
+   * everything except for rules that are intrinsically incompatible with the constraint system.
+   */
+  private final boolean supportsConstraintChecking;
+
   /**
    * Constructs an instance of RuleClass whose name is 'name', attributes
    * are 'attributes'. The {@code srcsAllowedFiles} determines which types of
@@ -875,6 +898,7 @@ public final class RuleClass {
       @Nullable UserDefinedFunction configuredTargetFunction,
       @Nullable SkylarkEnvironment ruleDefinitionEnvironment,
       Set<Class<?>> allowedConfigurationFragments, boolean failIfMissingConfigurationFragment,
+      boolean supportsConstraintChecking,
       Attribute... attributes) {
     this.name = name;
     this.targetKind = name + " rule";
@@ -896,6 +920,7 @@ public final class RuleClass {
     this.outputsDefaultExecutable = outputsDefaultExecutable;
     this.requiredConfigurationFragments = ImmutableSet.copyOf(allowedConfigurationFragments);
     this.failIfMissingConfigurationFragment = failIfMissingConfigurationFragment;
+    this.supportsConstraintChecking = supportsConstraintChecking;
 
     // create the index:
     int index = 0;
@@ -1069,6 +1094,13 @@ public final class RuleClass {
   }
 
   /**
+   * Returns true if rules of this type can be used with the constraint enforcement system.
+   */
+  public boolean supportsConstraintChecking() {
+    return supportsConstraintChecking;
+  }
+
+  /**
    * Helper function for {@link RuleFactory#createAndAddRule}.
    */
   Rule createRuleWithLabel(Package.AbstractBuilder<?, ?> pkgBuilder, Label ruleLabel,