From a7a0d48fbeb059ee60e77580e5d05baeefdd5699 Mon Sep 17 00:00:00 2001
From: juliexxia <juliexxia@google.com>
Date: Mon, 17 Aug 2020 15:50:01 -0700
Subject: [PATCH] Make no-op starlark transition not affect the output
 directory.

Not having this logic in was leading to c++ action conflicts. Before this CL we naively hashed the map of options->values returned by a transition into the output directory fragment. Now we only update the output directory fragment if values actually change. In order to do this, we also need to add the default values of all the output options to the starlark options map (if they don't already have a non-default value in the map) to take care of the set-to-default case also being a no-op. Add a bunch of tests and delete tests that showed the undesirable behavior before.

Also fixes https://github.com/bazelbuild/bazel/issues/11196

TESTED: patch unknown commit && devblaze build //googlex/koi/...
PiperOrigin-RevId: 327116054
---
 .../config/ConfigurationResolver.java         |   3 +-
 .../starlark/FunctionTransitionUtil.java      |  38 ++-
 .../analysis/starlark/StarlarkTransition.java |   9 +-
 .../StarlarkAttrTransitionProviderTest.java   | 237 ++++++++++--------
 .../cpp/CcLibraryConfiguredTargetTest.java    |   4 +-
 5 files changed, 169 insertions(+), 122 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
index 3e90ffa937e7b1..c275fa438a8562 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -419,8 +419,7 @@ public static Map<String, BuildOptions> applyTransition(
     if (doesStarlarkTransition) {
       fromOptions =
           addDefaultStarlarkOptions(
-              fromOptions,
-              StarlarkTransition.getDefaultInputValues(buildSettingPackages, transition));
+              fromOptions, StarlarkTransition.getDefaultValues(buildSettingPackages, transition));
     }
 
     // TODO(bazel-team): Add safety-check that this never mutates fromOptions.
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 3e795c405761ae..4611892f06d41d 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
@@ -238,18 +238,28 @@ private static BuildOptions applyTransition(
       StarlarkDefinedConfigTransition starlarkTransition)
       throws EvalException {
     BuildOptions buildOptions = buildOptionsToTransition.clone();
+    // The names and values of options that are different after this transition.
     HashMap<String, Object> convertedNewValues = new HashMap<>();
     for (Map.Entry<String, Object> entry : newValues.entrySet()) {
       String optionName = entry.getKey();
       Object optionValue = entry.getValue();
 
       if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
-        buildOptions =
-            BuildOptions.builder()
-                .merge(buildOptions)
-                .addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue)
-                .build();
-        convertedNewValues.put(optionName, optionValue);
+        Object oldValue =
+            buildOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
+        if ((oldValue == null && optionValue != null)
+            || (oldValue != null && optionValue == null)
+            || (oldValue != null && !oldValue.equals(optionValue))) {
+          // TODO(bazel-team): Figure out if we need to create a whole new build options every
+          // time. Can we just keep track of the running changes and actually build a new build
+          // options after this loop?
+          buildOptions =
+              BuildOptions.builder()
+                  .merge(buildOptions)
+                  .addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue)
+                  .build();
+          convertedNewValues.put(optionName, optionValue);
+        }
       } else {
         optionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());
 
@@ -298,8 +308,15 @@ private static BuildOptions applyTransition(
           } else {
             throw Starlark.errorf("Invalid value type for option '%s'", optionName);
           }
-          field.set(options, convertedValue);
-          convertedNewValues.put(entry.getKey(), convertedValue);
+
+          Object oldValue = field.get(options);
+          if ((oldValue == null && convertedValue != null)
+              || (oldValue != null && convertedValue == null)
+              || (oldValue != null && !oldValue.equals(convertedValue))) {
+            field.set(options, convertedValue);
+            convertedNewValues.put(entry.getKey(), convertedValue);
+          }
+
         } catch (IllegalArgumentException e) {
           throw Starlark.errorf(
               "IllegalArgumentError for option '%s': %s", optionName, e.getMessage());
@@ -342,6 +359,11 @@ private static BuildOptions applyTransition(
   // makes it so that two configurations that are the same in value may hash differently.
   private static void updateOutputDirectoryNameFragment(
       Set<String> changedOptions, Map<String, OptionInfo> optionInfoMap, BuildOptions toOptions) {
+    // Return without doing anything if this transition hasn't changed any option values.
+    if (changedOptions.isEmpty()) {
+      return;
+    }
+
     CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
     Set<String> updatedAffectedByStarlarkTransition =
         new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
index f3e62413fa1ed3..316be455f1c753 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
@@ -391,8 +391,8 @@ private static BuildOptions unalias(
   }
 
   /**
-   * For a given transition, find all Starlark build settings that are read while applying it, then
-   * return a map of their label to their default values.
+   * For a given transition, find all Starlark build settings that are input/output while applying
+   * it, then return a map of their label to their default values.
    *
    * <p>If the build setting is referenced by an {@link com.google.devtools.build.lib.rules.Alias},
    * the returned map entry is still keyed by the alias.
@@ -402,7 +402,7 @@ private static BuildOptions unalias(
    *     build settings *written* by relevant transitions) so do not iterate over for input
    *     packages.
    */
-  public static ImmutableMap<Label, Object> getDefaultInputValues(
+  public static ImmutableMap<Label, Object> getDefaultValues(
       Map<PackageValue.Key, PackageValue> buildSettingPackages, ConfigurationTransition root)
       throws TransitionException {
     ImmutableMap.Builder<Label, Object> defaultValues = new ImmutableMap.Builder<>();
@@ -410,7 +410,8 @@ public static ImmutableMap<Label, Object> getDefaultInputValues(
         (StarlarkTransitionVisitor)
             transition -> {
               ImmutableSet<Label> settings =
-                  getRelevantStarlarkSettingsFromTransition(transition, Settings.INPUTS);
+                  getRelevantStarlarkSettingsFromTransition(
+                      transition, Settings.INPUTS_AND_OUTPUTS);
               for (Label setting : settings) {
                 defaultValues.put(
                     setting,
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 9e35fdea7d4edb..6c9307b56a8277 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
@@ -1161,60 +1161,6 @@ public void testOutputDirHash_noop_emptyReturns() throws Exception {
         .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
   }
 
-  // Test that a no-op starlark transition to the top level configuration results in a
-  // different configuration.
-  // TODO(bazel-team): This can be optimized. Make these the same configuration.
-  @Test
-  public void testOutputDirHash_noop_changeToDifferentStateAsTopLevel() throws Exception {
-    writeAllowlistFile();
-    scratch.file(
-        "test/transitions.bzl",
-        "def _bar_impl(settings, attr):",
-        "  return {'//test:bar': 'barsball'}",
-        "bar_transition = transition(implementation = _bar_impl, inputs = [],",
-        "  outputs = ['//test:bar'])");
-    scratch.file(
-        "test/rules.bzl",
-        "load('//myinfo:myinfo.bzl', 'MyInfo')",
-        "load('//test:transitions.bzl', 'bar_transition')",
-        "def _impl(ctx):",
-        "  return MyInfo(dep = ctx.attr.dep)",
-        "my_rule = rule(",
-        "  implementation = _impl,",
-        "  attrs = {",
-        "    'dep': attr.label(cfg = bar_transition), ",
-        "    '_allowlist_function_transition': attr.label(",
-        "        default = '//tools/allowlists/function_transition_allowlist',",
-        "    ),",
-        "  })",
-        "def _basic_impl(ctx):",
-        "  return []",
-        "string_flag = rule(",
-        "  implementation = _basic_impl,",
-        "  build_setting = config.string(flag = True),",
-        ")",
-        "simple = rule(_basic_impl)");
-    scratch.file(
-        "test/BUILD",
-        "load('//test:rules.bzl', 'my_rule', 'string_flag', 'simple')",
-        "my_rule(name = 'test', dep = ':dep')",
-        "simple(name = 'dep')",
-        "string_flag(name = 'bar', build_setting_default = '')");
-
-    // Use configuration that is same as what the transition will change.
-    useConfiguration(ImmutableMap.of("//test:bar", "barsball"));
-
-    ConfiguredTarget test = getConfiguredTarget("//test");
-
-    @SuppressWarnings("unchecked")
-    ConfiguredTarget dep =
-        Iterables.getOnlyElement(
-            (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
-
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment).isNull();
-    assertThat(getCoreOptions(dep).transitionDirectoryNameFragment).isNotNull();
-  }
-
   // Test that setting all starlark options back to default != null hash of top level.
   // We could set some starlark options on the command line but we don't count this as a starlark
   // transition to the command line configuration will always have a null values for
@@ -1276,58 +1222,6 @@ public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefaultCom
     assertThat(getCoreOptions(dep).transitionDirectoryNameFragment).isNotNull();
   }
 
-  // Test that setting a starlark option to default (if it was already at default) doesn't
-  // produce the same hash. This is because we do hashing  before scrubbing default values
-  // out of {@code BuildOptions}.
-  // TODO(bazel-team): This can be optimized. Make these the same configuration.
-  @Test
-  public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefault() throws Exception {
-    writeAllowlistFile();
-    scratch.file(
-        "test/transitions.bzl",
-        "def _foo_two_impl(settings, attr):",
-        "  return {'//test:foo': 'foosballerina'}",
-        "foo_two_transition = transition(implementation = _foo_two_impl, inputs = [],",
-        "  outputs = ['//test:foo'])");
-    scratch.file(
-        "test/rules.bzl",
-        "load('//myinfo:myinfo.bzl', 'MyInfo')",
-        "load('//test:transitions.bzl', 'foo_two_transition')",
-        "def _impl(ctx):",
-        "  return MyInfo(dep = ctx.attr.dep)",
-        "my_rule = rule(",
-        "  implementation = _impl,",
-        "  attrs = {",
-        "    'dep': attr.label(cfg = foo_two_transition), ",
-        "    '_allowlist_function_transition': attr.label(",
-        "        default = '//tools/allowlists/function_transition_allowlist',",
-        "    ),",
-        "  })",
-        "def _basic_impl(ctx):",
-        "  return []",
-        "string_flag = rule(",
-        "  implementation = _basic_impl,",
-        "  build_setting = config.string(flag = True),",
-        ")",
-        "simple = rule(_basic_impl)");
-    scratch.file(
-        "test/BUILD",
-        "load('//test:rules.bzl', 'my_rule', 'string_flag', 'simple')",
-        "my_rule(name = 'test', dep = ':dep')",
-        "simple(name = 'dep')",
-        "string_flag(name = 'foo', build_setting_default = 'foosballerina')");
-
-    ConfiguredTarget test = getConfiguredTarget("//test");
-
-    @SuppressWarnings("unchecked")
-    ConfiguredTarget dep =
-        Iterables.getOnlyElement(
-            (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
-
-    assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
-        .isNotEqualTo(getCoreOptions(test).transitionDirectoryNameFragment);
-  }
-
   /** See comment above {@link FunctionTransitionUtil#updateOutputDirectoryNameFragment} */
   // TODO(bazel-team): This can be optimized. Make these the same configuration.
   @Test
@@ -1841,6 +1735,137 @@ public void starlarkSplitTransitionRequiredFragments() throws Exception {
         .containsExactly("CppOptions");
   }
 
+  /**
+   * @param directRead if set to true, reads the output value directly from the input dict, else
+   *     just passes in the same value as a string
+   */
+  private void testNoOpTransitionLeavesSameConfig_native(boolean directRead) throws Exception {
+    writeAllowlistFile();
+    setStarlarkSemanticsOptions("--experimental_starlark_config_transitions=true");
+
+    String outputValue = directRead ? "settings['//command_line_option:test_arg']" : "['frisbee']";
+    String inputs = directRead ? "['//command_line_option:test_arg']" : "[]";
+
+    scratch.file(
+        "test/defs.bzl",
+        "load('//myinfo:myinfo.bzl', 'MyInfo')",
+        "def _transition_impl(settings, attr):",
+        "  return {'//command_line_option:test_arg': " + outputValue + "}",
+        "my_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = " + inputs + ",",
+        "  outputs = ['//command_line_option:test_arg'],",
+        ")",
+        "def _impl(ctx):",
+        "  return MyInfo(dep = ctx.attr.dep)",
+        "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/BUILD",
+        "load('//test:defs.bzl', 'my_rule')",
+        "my_rule(name = 'test', dep = ':dep')",
+        "my_rule(name = 'dep')");
+
+    useConfiguration("--test_arg=frisbee");
+    ConfiguredTarget test = getConfiguredTarget("//test");
+
+    @SuppressWarnings("unchecked")
+    ConfiguredTarget dep =
+        Iterables.getOnlyElement(
+            (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
+    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
+        .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
+  }
+
+  @Test
+  public void testNoOpTransitionLeavesSameConfig_native_directRead() throws Exception {
+    testNoOpTransitionLeavesSameConfig_native(true);
+  }
+
+  @Test
+  public void testNoOpTransitionLeavesSameConfig_native_setToSame() throws Exception {
+    testNoOpTransitionLeavesSameConfig_native(false);
+  }
+
+  /**
+   * @param directRead if set to true, reads the output value directly from the input dict, else
+   *     just passes in the same value as a string
+   * @param setToDefault if set to true, value getting passed through the transition is the default
+   *     value of the build settings. Internally we don't keep default values in the build settings
+   *     map inside {@link BuildOptions} so it's nice to test this separately.
+   */
+  private void testNoOpTransitionLeavesSameConfig_starlark(boolean directRead, boolean setToDefault)
+      throws Exception {
+    writeAllowlistFile();
+    setStarlarkSemanticsOptions("--experimental_starlark_config_transitions=true");
+
+    String outputValue = directRead ? "settings['//test:flag']" : "'frisbee'";
+    String inputs = directRead ? "['//test:flag']" : "[]";
+    String buildSettingsDefault = setToDefault ? "frisbee" : "waterpolo";
+
+    scratch.file(
+        "test/defs.bzl",
+        "load('//myinfo:myinfo.bzl', 'MyInfo')",
+        "def _flag_impl(ctx):",
+        "  return []",
+        "my_flag = rule(",
+        "  implementation = _flag_impl,",
+        "  build_setting = config.string(flag = True)",
+        ")",
+        "def _transition_impl(settings, attr):",
+        "  return {'//test:flag': " + outputValue + "}",
+        "my_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = " + inputs + ",",
+        "  outputs = ['//test:flag'],",
+        ")",
+        "def _impl(ctx):",
+        "  return MyInfo(dep = ctx.attr.dep)",
+        "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/BUILD",
+        "load('//test:defs.bzl', 'my_rule', 'my_flag')",
+        "my_rule(name = 'test', dep = ':dep')",
+        "my_rule(name = 'dep')",
+        "my_flag(name = 'flag', build_setting_default = '" + buildSettingsDefault + "')");
+
+    useConfiguration(ImmutableMap.of("//test:flag", "frisbee"));
+    ConfiguredTarget test = getConfiguredTarget("//test");
+
+    @SuppressWarnings("unchecked")
+    ConfiguredTarget dep =
+        Iterables.getOnlyElement(
+            (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
+    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
+        .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
+  }
+
+  @Test
+  public void testNoOpTransitionLeavesSameConfig_starlark_directRead() throws Exception {
+    testNoOpTransitionLeavesSameConfig_starlark(true, false);
+  }
+
+  @Test
+  public void testNoOpTransitionLeavesSameConfig_starlark_setToSame() throws Exception {
+    testNoOpTransitionLeavesSameConfig_starlark(false, false);
+  }
+
+  @Test
+  public void testNoOpTransitionLeavesSameConfig_starlark_setToDefault() throws Exception {
+    testNoOpTransitionLeavesSameConfig_starlark(false, true);
+  }
+
   @Test
   public void testOptionConversionDynamicMode() throws Exception {
     // TODO(waltl): check that dynamic_mode is parsed properly.
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index 498ec4b7fddd81..796c61d3bed53c 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -1598,12 +1598,12 @@ public void testPrecompiledFilesFromDifferentConfigs() throws Exception {
         "def _impl(settings, attr):",
         "    _ignore = (settings, attr)",
         "    return [",
-        "        {'//command_line_option:cpu': 'k8'},",
+        "        {'//command_line_option:test_arg': ['foo']},",
         "    ]",
         "cpu_transition = transition(",
         "    implementation = _impl,",
         "    inputs = [],",
-        "    outputs = ['//command_line_option:cpu'],",
+        "    outputs = ['//command_line_option:test_arg'],",
         ")",
         "def _transitioned_file_impl(ctx):",
         "    return DefaultInfo(files = depset([ctx.file.src]))",