Make compatible_with = ["all", "foo"] the same as compatible_with = ["all"].

Assuming "all" fulfills "foo", these should be exactly the same (and
maybe we should trigger a redundant listing error). In practice, it's
possible to make the first case succeed while the second fails because of
environment refining and lack of static constraint checking for selects.

See changes for details.

Also refactor ConstraintSemantics.checkConstraints to divide and conquer more clearly.

PiperOrigin-RevId: 158047217

2 files changed, 129 insertions, 35 deletions

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 bc24ae9862..664d3feda4 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
@@ -535,53 +535,103 @@ public class ConstraintSemantics {
     DepsToCheck depsToCheck = getConstraintCheckedDependencies(ruleContext);
 
     for (TransitiveInfoCollection dep : depsToCheck.allDeps()) {
-      SupportedEnvironmentsProvider depEnvironments =
-          dep.getProvider(SupportedEnvironmentsProvider.class);
       if (!depsToCheck.isSelectOnly(dep)) {
         // TODO(bazel-team): support static constraint checking for selects. A selectable constraint
         // is valid if the union of all deps in the select includes all of this rule's static
         // environments. Determining that requires following the select paths that don't get chosen,
         // which means we won't have ConfiguredTargets for those deps and need to find another
         // way to get their environments.
-        Collection<Label> unsupportedEnvironments =
-            getUnsupportedEnvironments(depEnvironments.getStaticEnvironments(), staticEnvironments);
-
-        if (!unsupportedEnvironments.isEmpty()) {
-          ruleContext.ruleError("dependency " + dep.getLabel()
-              + " doesn't support expected environment"
-              + (unsupportedEnvironments.size() == 1 ? "" : "s")
-              + ": " + Joiner.on(", ").join(unsupportedEnvironments));
-        }
+        checkStaticConstraints(ruleContext, staticEnvironments, dep);
       }
+      refineEnvironmentsForDep(ruleContext, staticEnvironments, dep, labelsToEnvironments,
+          refinedEnvironmentsSoFar, groupsWithEnvironmentsRemoved, removedEnvironmentCulprits);
+    }
 
-      // Refine this rule's environments by intersecting with the dep's refined environments:
-      for (Label refinedEnvironmentToPrune : getUnsupportedEnvironments(
-          depEnvironments.getRefinedEnvironments(), staticEnvironments)) {
-        EnvironmentWithGroup envToPrune = labelsToEnvironments.get(refinedEnvironmentToPrune);
-        if (envToPrune == null) {
-          // If we have no record of this environment, that means the current rule implicitly uses
-          // the defaults for this group. So explicitly opt that group's defaults into the refined
-          // set before trying to remove specific items.
-          for (EnvironmentWithGroup defaultEnv :
-              getDefaults(refinedEnvironmentToPrune, depEnvironments.getRefinedEnvironments())) {
-            refinedEnvironmentsSoFar.add(defaultEnv);
-            labelsToEnvironments.put(defaultEnv.environment(), defaultEnv);
-          }
-          envToPrune = Verify.verifyNotNull(labelsToEnvironments.get(refinedEnvironmentToPrune));
+    checkRefinedConstraints(ruleContext, groupsWithEnvironmentsRemoved,
+        refinedEnvironmentsSoFar, refinedEnvironments, removedEnvironmentCulprits);
+  }
+
+  /**
+   * Performs static constraint checking against the given dep.
+   *
+   * @param ruleContext the rule being analyzed
+   * @param staticEnvironments the static environments of the rule being analyzed
+   * @param dep the dep to check
+   */
+  private static void checkStaticConstraints(RuleContext ruleContext,
+      EnvironmentCollection staticEnvironments, TransitiveInfoCollection dep) {
+    SupportedEnvironmentsProvider depEnvironments =
+        dep.getProvider(SupportedEnvironmentsProvider.class);
+    Collection<Label> unsupportedEnvironments =
+        getUnsupportedEnvironments(depEnvironments.getStaticEnvironments(), staticEnvironments);
+    if (!unsupportedEnvironments.isEmpty()) {
+      ruleContext.ruleError("dependency " + dep.getLabel()
+          + " doesn't support expected environment"
+          + (unsupportedEnvironments.size() == 1 ? "" : "s")
+          + ": " + Joiner.on(", ").join(unsupportedEnvironments));
+    }
+  }
+
+  /**
+   * Helper method for {@link #checkConstraints}: refines a rule's environments with the given dep.
+   *
+   * <p>A rule's <b>complete</b> refined set applies this process to every dep.
+   */
+  private static void refineEnvironmentsForDep(
+      RuleContext ruleContext,
+      EnvironmentCollection staticEnvironments,
+      TransitiveInfoCollection dep,
+      Map<Label, EnvironmentWithGroup> labelsToEnvironments,
+      Set<EnvironmentWithGroup> refinedEnvironmentsSoFar,
+      Set<EnvironmentGroup> groupsWithEnvironmentsRemoved,
+      Map<Label, Target> removedEnvironmentCulprits) {
+
+    SupportedEnvironmentsProvider depEnvironments =
+        dep.getProvider(SupportedEnvironmentsProvider.class);
+
+    // Stores the environments that are pruned from the refined set because of this dep. Even
+    // though they're removed, some subset of the environments they fulfill may belong in the
+    // refined set. For example, if environment "both" fulfills "a" and "b" and "lib" statically
+    // sets restricted_to = ["both"] and "dep" sets restricted_to = ["a"], then lib's refined set
+    // excludes "both". But rather than be emptied out it can be reduced to "a".
+    Set<Label> prunedEnvironmentsFromThisDep = new LinkedHashSet<>();
+
+    // Refine this rule's environments by intersecting with the dep's refined environments:
+    for (Label refinedEnvironmentToPrune : getUnsupportedEnvironments(
+        depEnvironments.getRefinedEnvironments(), staticEnvironments)) {
+      EnvironmentWithGroup envToPrune = labelsToEnvironments.get(refinedEnvironmentToPrune);
+      if (envToPrune == null) {
+        // If we have no record of this environment, that means the current rule implicitly uses
+        // the defaults for this group. So explicitly opt that group's defaults into the refined
+        // set before trying to remove specific items.
+        for (EnvironmentWithGroup defaultEnv :
+            getDefaults(refinedEnvironmentToPrune, depEnvironments.getRefinedEnvironments())) {
+          refinedEnvironmentsSoFar.add(defaultEnv);
+          labelsToEnvironments.put(defaultEnv.environment(), defaultEnv);
+        }
+        envToPrune = Verify.verifyNotNull(labelsToEnvironments.get(refinedEnvironmentToPrune));
+      }
+      refinedEnvironmentsSoFar.remove(envToPrune);
+      groupsWithEnvironmentsRemoved.add(envToPrune.group());
+      removedEnvironmentCulprits.put(envToPrune.environment(),
+          findOriginalRefiner(ruleContext, depEnvironments, envToPrune));
+      prunedEnvironmentsFromThisDep.add(envToPrune.environment());
+    }
+
+    // Add in any dep environment that one of the environments we removed fulfills. In other
+    // words, the removed environment is no good, but some subset of it may be.
+    for (EnvironmentWithGroup depEnv :
+        depEnvironments.getRefinedEnvironments().getGroupedEnvironments()) {
+      for (Label fulfiller : depEnv.group().getFulfillers(depEnv.environment())) {
+        if (prunedEnvironmentsFromThisDep.contains(fulfiller)) {
+          refinedEnvironmentsSoFar.add(depEnv);
         }
-        refinedEnvironmentsSoFar.remove(envToPrune);
-        groupsWithEnvironmentsRemoved.add(envToPrune.group());
-        removedEnvironmentCulprits.put(envToPrune.environment(),
-            findOriginalRefiner(ruleContext, depEnvironments, envToPrune));
       }
     }
-
-    checkRefinedEnvironmentConstraints(ruleContext, groupsWithEnvironmentsRemoved,
-        refinedEnvironmentsSoFar, refinedEnvironments, removedEnvironmentCulprits);
   }
 
   /**
-   * Helper method for checkConstraints: performs refined environment constraint checking.
+   * Helper method for {@link #checkConstraints}: performs refined environment constraint checking.
    *
    * <p>Refined environment expectations: no environment group should be emptied out due to
    * refining. This reflects the idea that some of the static declared environments get pruned
@@ -589,7 +639,7 @@ public class ConstraintSemantics {
    *
    * <p>Violations of this expectation trigger rule analysis errors.
    */
-  private static void checkRefinedEnvironmentConstraints(
+  private static void checkRefinedConstraints(
       RuleContext ruleContext,
       Set<EnvironmentGroup> groupsWithEnvironmentsRemoved,
       Set<EnvironmentWithGroup> refinedEnvironmentsSoFar,
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
index fac4f006b6..1bf3b0f27a 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
@@ -1266,5 +1266,49 @@ public class ConstraintsTest extends AbstractConstraintsTest {
         + "\nenvironment group: //buildenv/bar:bar:\n"
         + " environment: //buildenv/bar:c removed by: //hello:lib (/workspace/hello/BUILD:9:1)");
   }
-}
 
+  private void writeRulesForRefiningSubsetTests(String topLevelRestrictedTo) throws Exception {
+    new EnvironmentGroupMaker("buildenv/foo")
+        .setEnvironments("a", "b", "all")
+        .setFulfills("all", "a")
+        .setFulfills("all", "b")
+        .setDefaults()
+        .make();
+    scratch.file("hello/BUILD",
+        "cc_library(",
+        "    name = 'lib',",
+        "    srcs = [],",
+        "    deps = [':dep1'],",
+        "    restricted_to = ['//buildenv/foo:" + topLevelRestrictedTo + "'])",
+        "cc_library(",
+        "    name = 'dep1',",
+        "    srcs = [],",
+        // This is technically illegal because "dep1" declares support for both "a" and "b" but
+        // no dependency under the select can provide "b". This is known as "static select
+        // constraint checking" and is currently an unimplemented Bazel TODO.
+        "    deps = select({",
+        "        '//config:a': [':dep2'],",
+        "        '//conditions:default': [':dep2'],",
+        "    }),",
+        "    restricted_to = ['//buildenv/foo:all'])",
+        "cc_library(",
+        "    name = 'dep2',",
+        "    srcs = [],",
+        "    compatible_with = ['//buildenv/foo:a'])");
+  }
+
+  @Test
+  public void refiningReplacesRemovedEnvironmentWithValidFulfillingSubset() throws Exception {
+    writeRulesForRefiningSubsetTests("a");
+    assertThat(getConfiguredTarget("//hello:lib")).isNotNull();
+  }
+
+  @Test
+  public void refiningReplacesRemovedEnvironmentWithInvalidFulfillingSubset() throws Exception {
+    writeRulesForRefiningSubsetTests("b");
+    reporter.removeHandler(failFastHandler);
+    assertThat(getConfiguredTarget("//hello:lib")).isNull();
+    assertContainsEvent("//hello:lib: the current command-line flags disqualify all supported "
+        + "environments because of incompatible select() paths");
+  }
+}