Properly account for StarlarkOptions at their default (=null) when ca…

…lculating ST-hash

Previously, the hash calculation was skipping including StarlarkOptions that happened to be at their default values.

This is wrong since those values may still be in "affected by Starlark transition" (because either the commandline set them and the Starlark transition reset them to their Starlark defaults thus still requiring a hash change OR the commandline did not set them but a series of Starlark transitions did an default->B->default anyways causing the Starlark option to still be 'stuck' in "affected by Starlark transition").

Resolves bazelbuild#14239

PiperOrigin-RevId: 408701552

src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java

@@ -440,16 +440,19 @@ public static String computeOutputDirectoryNameFragment(BuildOptions toOptions)
         toHash.put(optionName, value);
       } else {
         Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
-        if (value != null) {
-          toHash.put(optionName, value);
-        }
+        toHash.put(optionName, value);
       }
     }
 
     ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size());
     for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) {
-      String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
-      hashStrs.add(toAdd);
+      Object value = singleOptionAndValue.getValue();
+      if (value != null) {
+        hashStrs.add(singleOptionAndValue.getKey() + "=" + value);
+      } else {
+        // Avoid using =null to different from value being the non-null String "null"
+        hashStrs.add(singleOptionAndValue.getKey() + "@null");
+      }
     }
     return transitionDirectoryNameFragment(hashStrs.build());
   }

src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java

@@ -1181,6 +1181,81 @@ public void testTransitionOnBuildSetting_fromCommandLine() throws Exception {
         .isEqualTo(StarlarkInt.of(42));
   }
 
+  @Test
+  public void testTransitionBackToStarlarkDefaultOK() throws Exception {
+    writeAllowlistFile();
+    writeBuildSettingsBzl();
+    scratch.file(
+        "test/starlark/rules.bzl",
+        "load('//myinfo:myinfo.bzl', 'MyInfo')",
+        "def _transition_impl(settings, attr):",
+        "  return {",
+        "    '//test/starlark:the-answer': attr.answer_for_dep,",
+        "    '//test/starlark:did-transition': 1,",
+        "}",
+        "my_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = [],",
+        "  outputs = ['//test/starlark:the-answer', '//test/starlark:did-transition']",
+        ")",
+        "def _rule_impl(ctx):",
+        "  return MyInfo(dep = ctx.attr.dep)",
+        "my_rule = rule(",
+        "  implementation = _rule_impl,",
+        "  attrs = {",
+        "    'dep': attr.label(cfg = my_transition),",
+        "    'answer_for_dep': attr.int(),",
+        "    '_allowlist_function_transition': attr.label(",
+        "      default = '//tools/allowlists/function_transition_allowlist'),",
+        "  }",
+        ")");
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:rules.bzl', 'my_rule')",
+        "load('//test/starlark:build_settings.bzl', 'int_flag')",
+        "my_rule(name = 'test', dep = ':dep1', answer_for_dep=0)",
+        "my_rule(name = 'dep1', dep = ':dep2', answer_for_dep=42)",
+        "my_rule(name = 'dep2', dep = ':dep3', answer_for_dep=0)",
+        "my_rule(name = 'dep3')",
+        "int_flag(name = 'the-answer', build_setting_default=0)",
+        "int_flag(name = 'did-transition', build_setting_default=0)");
+    useConfiguration(ImmutableMap.of(), "--cpu=FOO");
+
+    ConfiguredTarget test = getConfiguredTarget("//test/starlark:test");
+
+    // '//test/starlark:did-transition ensures ST-hash is 'turned on' since :test has no ST-hash
+    //   and thus will trivially have a unique getTransitionDirectoryNameFragment
+
+    @SuppressWarnings("unchecked")
+    ConfiguredTarget dep1 =
+        Iterables.getOnlyElement(
+            (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
+
+    @SuppressWarnings("unchecked")
+    ConfiguredTarget dep2 =
+        Iterables.getOnlyElement(
+            (List<ConfiguredTarget>) getMyInfoFromTarget(dep1).getValue("dep"));
+
+    @SuppressWarnings("unchecked")
+    ConfiguredTarget dep3 =
+        Iterables.getOnlyElement(
+            (List<ConfiguredTarget>) getMyInfoFromTarget(dep2).getValue("dep"));
+
+    // These must be true
+    assertThat(getTransitionDirectoryNameFragment(dep1))
+        .isNotEqualTo(getTransitionDirectoryNameFragment(dep2));
+
+    assertThat(getTransitionDirectoryNameFragment(dep2))
+        .isNotEqualTo(getTransitionDirectoryNameFragment(dep3));
+
+    // TODO(blaze-configurability-team): When "affected by starlark transition" is gone,
+    //    will be equal and thus getTransitionDirectoryNameFragment can be equal.
+    if (!getConfiguration(dep1).equals(getConfiguration(dep3))) {
+      assertThat(getTransitionDirectoryNameFragment(dep1))
+          .isNotEqualTo(getTransitionDirectoryNameFragment(dep3));
+    }
+  }
+
   @Test
   public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throws Exception {
     writeAllowlistFile();