From 57fc3d2b3542ef59a032022dad5ab42619c42e73 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Fri, 6 Jan 2023 16:42:09 +0100
Subject: [PATCH 1/2] Do not clear `--platforms` on no-op change to `--cpu`

Resolves a TODO in the code to not unnecessarily fork the configuration
when a transition has `--cpu` as a declared output but doesn't change
it.
---
 .../starlark/FunctionTransitionUtil.java      | 30 ++++++------
 .../StarlarkAttrTransitionProviderTest.java   | 48 +++++++++++++++++++
 2 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
index c6903b01cc68d0..2c8bf969e9bdbe 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
@@ -99,7 +99,8 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
       }
 
       for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {
-        Map<String, Object> newValues = handleImplicitPlatformChange(entry.getValue());
+        Map<String, Object> newValues = handleImplicitPlatformChange(buildOptions,
+            entry.getValue());
         BuildOptions transitionedOptions =
             applyTransition(buildOptions, newValues, optionInfoMap, starlarkTransition);
         splitBuildOptions.put(entry.getKey(), transitionedOptions);
@@ -127,21 +128,22 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
    * <p>Transitions can also explicitly set --platforms to be clear what platform they set.
    *
    * <p>Platform mappings: https://bazel.build/concepts/platforms-intro#platform-mappings.
-   *
-   * <p>This doesn't check that the changed value is actually different than the source (i.e.
-   * setting {@code --cpu=foo} when {@code --cpu} is already {@code foo}). That could unnecessarily
-   * fork configurations that are really the same. That's a possible optimization TODO.
    */
   private static Map<String, Object> handleImplicitPlatformChange(
-      Map<String, Object> originalOutput) {
-    boolean changesCpu = originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "cpu");
-    boolean changesPlatforms = originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms");
-    return changesCpu && !changesPlatforms
-        ? ImmutableMap.<String, Object>builder()
-            .putAll(originalOutput)
-            .put(COMMAND_LINE_OPTION_PREFIX + "platforms", ImmutableList.<Label>of())
-            .build()
-        : originalOutput;
+      BuildOptions options, Map<String, Object> originalOutput) {
+    Object newCpu = originalOutput.get(COMMAND_LINE_OPTION_PREFIX + "cpu");
+    if (newCpu == null || newCpu.equals(options.get(CoreOptions.class).cpu)) {
+      // No effective change to --cpu, so no need to prevent the platform mapping from resetting it.
+      return originalOutput;
+    }
+    if (originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms")) {
+      // Explicitly setting --platforms overrides the implicit clearing.
+      return originalOutput;
+    }
+    return ImmutableMap.<String, Object>builder()
+        .putAll(originalOutput)
+        .put(COMMAND_LINE_OPTION_PREFIX + "platforms", ImmutableList.<Label>of())
+        .build();
   }
 
   private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition transition)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
index fd6da5f65c8b9f..38aa2ab845ec33 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -2501,6 +2501,54 @@ public void testNoPlatformChange() throws Exception {
         .containsExactly(Label.parseAbsoluteUnchecked("//platforms:my_platform"));
   }
 
+  /*
+   * If the transition claims to change --cpu but doesn't, it doesn't constitute a platform change
+   * and also doesn't affect any other options (such as affectedByStarlarkTransition).
+   */
+  @Test
+  public void testCpuNoOpChangeIsFullyNoOp() throws Exception {
+    writeAllowlistFile();
+    scratch.file(
+        "platforms/BUILD",
+        "platform(name = 'my_platform',",
+        "    parents = ['" + TestConstants.LOCAL_CONFIG_PLATFORM_PACKAGE_ROOT + ":host'],",
+        "    constraint_values = [],",
+        ")");
+    scratch.file(
+        "test/starlark/my_rule.bzl",
+        "def transition_func(settings, attr):",
+        "  return settings",
+        "my_transition = transition(implementation = transition_func,",
+        "  inputs = [",
+        "    '//command_line_option:cpu',",
+        "  ],",
+        "  outputs = [",
+        "    '//command_line_option:cpu',",
+        "  ]",
+        ")",
+        "def impl(ctx): ",
+        "  return []",
+        "my_rule = rule(",
+        "  implementation = impl,",
+        "  attrs = {",
+        "    'dep':  attr.label(cfg = my_transition),",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  })");
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:my_rule.bzl', 'my_rule')",
+        "my_rule(name = 'test', dep = ':main1')",
+        "cc_binary(name = 'main1', srcs = ['main1.c'])");
+
+    ConfiguredTarget topLevel = getConfiguredTarget("//test/starlark:test");
+    ConfiguredTarget dep =
+        getDirectPrerequisite(getConfiguredTarget("//test/starlark:test"), "//test/starlark:main1");
+    assertThat(getConfiguration(dep).getOptions()).isEqualTo(
+        getConfiguration(topLevel).getOptions());
+  }
+
   @Test
   public void testEffectiveNoopTransitionTrimsInputBuildSettings() throws Exception {
     writeAllowlistFile();

From 1cf305b459f5d9f73b20408c984f13a3519d83dd Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Fri, 6 Jan 2023 17:21:59 +0100
Subject: [PATCH 2/2] Address review comments

---
 .../analysis/starlark/FunctionTransitionUtil.java    | 12 ++++++------
 .../analysis/StarlarkAttrTransitionProviderTest.java |  2 ++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
index 2c8bf969e9bdbe..af82bab606ae52 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
@@ -130,18 +130,18 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
    * <p>Platform mappings: https://bazel.build/concepts/platforms-intro#platform-mappings.
    */
   private static Map<String, Object> handleImplicitPlatformChange(
-      BuildOptions options, Map<String, Object> originalOutput) {
-    Object newCpu = originalOutput.get(COMMAND_LINE_OPTION_PREFIX + "cpu");
+      BuildOptions options, Map<String, Object> rawTransitionOutput) {
+    Object newCpu = rawTransitionOutput.get(COMMAND_LINE_OPTION_PREFIX + "cpu");
     if (newCpu == null || newCpu.equals(options.get(CoreOptions.class).cpu)) {
       // No effective change to --cpu, so no need to prevent the platform mapping from resetting it.
-      return originalOutput;
+      return rawTransitionOutput;
     }
-    if (originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms")) {
+    if (rawTransitionOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms")) {
       // Explicitly setting --platforms overrides the implicit clearing.
-      return originalOutput;
+      return rawTransitionOutput;
     }
     return ImmutableMap.<String, Object>builder()
-        .putAll(originalOutput)
+        .putAll(rawTransitionOutput)
         .put(COMMAND_LINE_OPTION_PREFIX + "platforms", ImmutableList.<Label>of())
         .build();
   }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
index 38aa2ab845ec33..451f1047e8da38 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -2517,6 +2517,8 @@ public void testCpuNoOpChangeIsFullyNoOp() throws Exception {
     scratch.file(
         "test/starlark/my_rule.bzl",
         "def transition_func(settings, attr):",
+        // Leave --cpu unchanged, but still trigger the full transition logic that would be
+        // bypassed by returning {}.
         "  return settings",
         "my_transition = transition(implementation = transition_func,",
         "  inputs = [",