From 2f5048d3992b0815dcce2dda8ceceb1147e78e8f Mon Sep 17 00:00:00 2001
From: gregce <gregce@google.com>
Date: Mon, 22 Nov 2021 17:31:03 -0800
Subject: [PATCH 1/3] Support select() on constraint_value for aliases.

This implements approach #4 of
https://github.com/bazelbuild/bazel/issues/13047#issuecomment-805309450.

The basic change adds a new toolchain resolution mode: "resolve iff the target
has a select()". It then sets alias() to that mode.

We could remove this special casing if we ever ubiquitously provide platfrom
info to *all* rules
(https://github.com/bazelbuild/bazel/issues/12899#issuecomment-767759147).

RELNOTES: alias() can now select() directly on constraint_value()

Fixes https://github.com/bazelbuild/bazel/issues/13047.

PiperOrigin-RevId: 411684872
Change-Id: I998ef9bba3226871651fc14bd9ed268e9a3de82c
---
 .../devtools/build/lib/packages/Rule.java     | 26 +++++++++++
 .../build/lib/packages/RuleClass.java         | 27 ++++++++++--
 .../cquery/ConfiguredTargetAccessor.java      |  2 +-
 .../devtools/build/lib/rules/Alias.java       |  5 ++-
 .../skyframe/ConfiguredTargetFunction.java    |  2 +-
 .../lib/skyframe/PlatformLookupUtil.java      | 12 +++---
 .../analysis/ConfigurableAttributesTest.java  | 43 +++++++++++++++++++
 7 files changed, 105 insertions(+), 12 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index 1c9ba345b37619..59598f282b89e2 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -35,6 +35,7 @@
 import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.StarlarkImplicitOutputsFunction;
 import com.google.devtools.build.lib.packages.License.DistributionType;
 import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
+import com.google.devtools.build.lib.packages.RuleClass.ToolchainResolutionMode;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
@@ -942,6 +943,31 @@ public Collection<Label> getAspectLabelsSuperset(DependencyFilter predicate) {
     return labels.values();
   }
 
+  /**
+   * Should this rule instance resolve toolchains?
+   *
+   * <p>This may happen for two reasons:
+   *
+   * <ol>
+   *   <li>The rule uses toolchains by definition ({@link
+   *       RuleClass.Builder#useToolchainResolution(ToolchainResolutionMode)}
+   *   <li>The rule instance has a select(), which means it may depend on target platform properties
+   *       that are only provided when toolchain resolution is enabled.
+   * </ol>
+   */
+  public boolean useToolchainResolution() {
+    ToolchainResolutionMode mode = ruleClass.useToolchainResolution();
+    if (mode.isActive()) {
+      return true;
+    } else if (mode == ToolchainResolutionMode.HAS_SELECT) {
+      RawAttributeMapper attr = RawAttributeMapper.of(this);
+      return (attr.has(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)
+          && !attr.get(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE, BuildType.LABEL_LIST).isEmpty());
+    } else {
+      return false;
+    }
+  }
+
   /**
    * @return The repository name.
    */
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 9b784c7fed0af9..88270be29baf6a 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
@@ -221,6 +221,19 @@ public enum ToolchainResolutionMode {
     ENABLED,
     /** The rule should not use toolchain resolution. */
     DISABLED,
+    /**
+     * The rule instance uses toolchain resolution if it has a select().
+     *
+     * <p>This is for rules that don't intrinsically use toolchains but have select()s on {@link
+     * com.google.devtools.build.lib.rules.platform.ConstraintValue}, which are part of the build's
+     * platform. Such instances need to know what platform the build is targeting, which Bazel won't
+     * provide unless toolchain resolution is enabled.
+     *
+     * <p>Ultimately, we should remove this when <a
+     * href="https://github.com/bazelbuild/bazel/issues/12899#issuecomment-767759147}#12899</a>is
+     * addressed, so platforms are unconditionally provided for all rules.
+     */
+    HAS_SELECT,
     /** The rule should inherit the value from its parent rules. */
     INHERIT;
 
@@ -245,6 +258,7 @@ boolean isActive() {
         case ENABLED:
           return true;
         case DISABLED:
+        case HAS_SELECT: // Not true for RuleClass, but Rule may enable it.
           return false;
         default:
       }
@@ -380,7 +394,7 @@ public RuleErrorException(String message, Throwable cause) {
    * normal dependency resolution because they're needed to determine other dependencies. So there's
    * no intrinsic reason why we need an extra attribute to store them.
    *
-   * <p>There are three reasons why we still create this attribute:
+   * <p>There are four reasons why we still create this attribute:
    *
    * <ol>
    *   <li>Collecting them once in {@link #populateRuleAttributeValues} instead of multiple times in
@@ -390,6 +404,9 @@ public RuleErrorException(String message, Throwable cause) {
    *       we need to make sure its coverage remains complete.
    *   <li>Manual configuration trimming uses the normal dependency resolution process to work
    *       correctly and config_setting keys are subject to this trimming.
+   *   <li>{@link Rule#useToolchainResolution() supports conditional toolchain resolution for
+   *      targets with non-empty select()s. This requirement would go away if platform info was
+   *      prepared for all rules regardless of toolchain needs.
    * </ol>
    *
    * <p>It should be possible to clean up these issues if we decide we don't want an artificial
@@ -2753,8 +2770,12 @@ public ImmutableSet<Label> getRequiredToolchains() {
     return requiredToolchains;
   }
 
-  public boolean useToolchainResolution() {
-    return this.useToolchainResolution.isActive();
+  /**
+   * Public callers should use {@link Rule#useToolchainResolution()}, which also takes into account
+   * target-specific information.
+   */
+  ToolchainResolutionMode useToolchainResolution() {
+    return this.useToolchainResolution;
   }
 
   public boolean useToolchainTransition() {
diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java
index 86e99bcdec4e25..6d6fe69b3ddd1e 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java
@@ -211,7 +211,7 @@ private static ToolchainCollection<ToolchainContext> getToolchainContexts(
     }
 
     Rule rule = ((Rule) target);
-    if (!rule.getRuleClassObject().useToolchainResolution()) {
+    if (!rule.useToolchainResolution()) {
       return null;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/Alias.java b/src/main/java/com/google/devtools/build/lib/rules/Alias.java
index e4cfd47404f652..14cef921130e57 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/Alias.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/Alias.java
@@ -81,8 +81,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
           .canHaveAnyProvider()
           // Aliases themselves do not need toolchains or an execution platform, so this is fine.
           // The actual target will resolve platforms and toolchains with no issues regardless of
-          // this setting.
-          .useToolchainResolution(ToolchainResolutionMode.DISABLED)
+          // this setting. The only time an alias directly needs the platform is when it has a
+          // select() on a constraint_setting, so special-case enable those instances too.
+          .useToolchainResolution(ToolchainResolutionMode.HAS_SELECT)
           .build();
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index 3e40ddc3fdb73e..233f6c8023cb44 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -485,7 +485,7 @@ public static ComputedToolchainContexts computeUnloadedToolchainContexts(
         ExecGroupCollection.builder(defaultExecGroup, rule.getRuleClassObject().getExecGroups());
 
     // Short circuit and end now if this target doesn't require toolchain resolution.
-    if (!rule.getRuleClassObject().useToolchainResolution()) {
+    if (!rule.useToolchainResolution()) {
       ComputedToolchainContexts result = new ComputedToolchainContexts();
       result.execGroupCollectionBuilder = execGroupCollectionBuilder;
       return result;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
index f0317b0c923030..7f756df8cbc336 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
@@ -29,7 +29,7 @@
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Package;
-import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.server.FailureDetails.Toolchain.Code;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
@@ -161,16 +161,18 @@ private static PlatformInfo findPlatformInfo(
   }
 
   static boolean hasPlatformInfo(Target target) {
+    Rule rule = target.getAssociatedRule();
     // If the rule uses toolchain resolution, it can't be used as a target or exec platform.
-    if (target.getAssociatedRule() == null) {
+    if (rule == null) {
       return false;
     }
-    RuleClass ruleClass = target.getAssociatedRule().getRuleClassObject();
-    if (ruleClass == null || ruleClass.useToolchainResolution()) {
+    if (rule.useToolchainResolution()) {
       return false;
     }
 
-    return ruleClass.getAdvertisedProviders().advertises(PlatformInfo.PROVIDER.id());
+    return rule.getRuleClassObject()
+        .getAdvertisedProviders()
+        .advertises(PlatformInfo.PROVIDER.id());
   }
 
   /** Exception used when a platform label is not a valid platform. */
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
index 79fb42bbc258af..264c3e166449cf 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
@@ -1290,6 +1290,49 @@ public void nonToolchainResolvingTargetsCantSelectDirectlyOnConstraints() throws
     assertContainsEvent("//conditions:apple is not a valid select() condition for //check:hello");
   }
 
+  @Test
+  public void selectOnlyToolchainResolvingTargetsCanSelectDirectlyOnConstraints() throws Exception {
+    // Tests select()ing directly on a constraint_value when the rule uses toolchain resolution
+    // *only if it has a select()*. As of this test, alias() is the only rule that supports that
+    // (see Alias#useToolchainResolution(ToolchainResolutionMode.HAS_SELECT).
+    scratch.file(
+        "conditions/BUILD",
+        "constraint_setting(name = 'fruit')",
+        "constraint_value(name = 'apple', constraint_setting = 'fruit')",
+        "constraint_value(name = 'banana', constraint_setting = 'fruit')",
+        "platform(",
+        "    name = 'apple_platform',",
+        "    constraint_values = [':apple'],",
+        ")");
+    scratch.file(
+        "check/defs.bzl",
+        "def _impl(ctx):",
+        "  pass",
+        "simple_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {}",
+        ")");
+    scratch.file(
+        "check/BUILD",
+        "load('//check:defs.bzl', 'simple_rule')",
+        "filegroup(name = 'bdep', srcs = ['bfile'])",
+        "simple_rule(name = 'hello')",
+        "simple_rule(name = 'tere')",
+        "alias(",
+        "    name = 'selectable_alias',",
+        "    actual = select({",
+        "        '//conditions:apple': ':hello',",
+        "        '//conditions:banana': ':tere',",
+        "    }))");
+    useConfiguration("--platforms=//conditions:apple_platform");
+    assertThat(
+            getConfiguredTarget("//check:selectable_alias")
+                .getActual()
+                .getLabel()
+                .getCanonicalForm())
+        .isEqualTo("//check:hello");
+  }
+
   @Test
   public void multipleMatchErrorWhenAliasResolvesToSameSetting() throws Exception {
     scratch.file(

From e51aedeb79bcb893492179324183278d77f6eb89 Mon Sep 17 00:00:00 2001
From: Greg Estren <gregestren@gmail.com>
Date: Mon, 22 Nov 2021 21:36:20 -0500
Subject: [PATCH 2/3] Update cquery test to account for new toolchain deps.

---
 .../lib/query2/cquery/ProtoOutputFormatterCallbackTest.java   | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallbackTest.java b/src/test/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallbackTest.java
index 0cc0cd6bf5b581..73eaf94502a3e1 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallbackTest.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallbackTest.java
@@ -203,7 +203,9 @@ public void testAlias_withSelect() throws Exception {
     myAliasRuleProto.forEach(
         configuredTarget -> depNames.add(configuredTarget.getTarget().getRule().getName()));
     assertThat(depNames)
-        .containsExactly("//test:my_alias_rule", "//test:config1", "//test:target1");
+        // The alias also includes platform info since aliases with select() trigger toolchain
+        // resolution. We're not interested in those here.
+        .containsAtLeast("//test:my_alias_rule", "//test:config1", "//test:target1");
   }
 
   private MockRule getSimpleRule() {

From e86e22fd308e0ded80b13030d7eeb180cbb5041e Mon Sep 17 00:00:00 2001
From: Greg Estren <gregestren@gmail.com>
Date: Tue, 23 Nov 2021 14:41:12 -0500
Subject: [PATCH 3/3] Update comments for review feedback

---
 .../java/com/google/devtools/build/lib/packages/RuleClass.java | 3 +++
 1 file changed, 3 insertions(+)

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 88270be29baf6a..6460114b9df63e 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
@@ -229,6 +229,9 @@ public enum ToolchainResolutionMode {
      * platform. Such instances need to know what platform the build is targeting, which Bazel won't
      * provide unless toolchain resolution is enabled.
      *
+     * <p>This is set statically in rule definitions on an opt-in basis. Bazel doesn't automatically
+     * infer this for any target with a select().
+     *
      * <p>Ultimately, we should remove this when <a
      * href="https://github.com/bazelbuild/bazel/issues/12899#issuecomment-767759147}#12899</a>is
      * addressed, so platforms are unconditionally provided for all rules.