Prevent multiple "noconfig" configurations.

This is a bug fix. By definition "noconfig" shouldn't produce different configurations.

PiperOrigin-RevId: 501576977
Change-Id: I7312493d3e1b3efcb934955fc896ce251b513b92

src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransition.java

@@ -64,10 +64,9 @@ public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
     // FragmentOptions and propagate them to this configuration. Those flags should also be
     // ineligible outputs for other transitions because they're not meant for rule logic.  That
     // would guarantee consistency of flags like --check_visibility while still preventing forking.
-    return BuildOptions.builder().addFragmentOptions(options.get(CoreOptions.class)).build();
-
-    // return BuildOptions
-    //  .getDefaultBuildOptionsForFragments(options.underlying().getFragmentClasses());
+    return BuildOptions.builder()
+        .addFragmentOptions(options.get(CoreOptions.class).getDefault())
+        .build();
   }
 
   /** Returns a {@link TransitionFactory} instance that generates the transition. */

src/test/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransitionTest.java

@@ -17,6 +17,7 @@
 
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.analysis.util.MockRule;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
@@ -42,10 +43,74 @@ protected ConfiguredRuleClassProvider createRuleClassProvider() {
   }
 
   @Test
-  public void noConfigTransitionPreventsConfiguredTargetForking() throws Exception {
+  public void preventsConfiguredTargetForking() throws Exception {
     // Write a custom Starlark rule that arbitrarily transitions its configuration. Have two
     // instances of that rule transition to different configurations and each depend on the same
     // no_config_rule. We expect there to be only one instance of the no_config_rule.
+    scratch.overwriteFile(
+        "tools/allowlists/function_transition_allowlist/BUILD",
+        "package_group(",
+        "    name = 'function_transition_allowlist',",
+        "    packages = [",
+        "        '//...',",
+        "    ],",
+        ")");
+    scratch.file(
+        "foo/defs.bzl",
+        // Define the flag to transition on:
+        "FlagInfo = provider(fields = {'value': 'The value.'})",
+        "custom_flag = rule(",
+        "  implementation = lambda ctx: FlagInfo(value = ctx.build_setting_value) ,",
+        "  build_setting = config.string(flag = True),",
+        ")",
+        "",
+        // Define the transitioning rule:
+        "my_transition = transition(",
+        "  implementation = lambda settings, attr: {'//foo:my_flag': attr.flag_value} ,",
+        "  inputs = [],",
+        "  outputs = ['//foo:my_flag'],",
+        ")",
+        "",
+        "transition_rule = rule(",
+        "  implementation = lambda ctx: [],",
+        "  cfg = my_transition,",
+        "  attrs = {",
+        "    'flag_value': attr.string(),",
+        "    'dep': attr.label(),",
+        "    '_allowlist_function_transition':",
+        "      attr.label(default = '//tools/allowlists/function_transition_allowlist'),",
+        "  },",
+        ")");
+    scratch.file(
+        "foo/BUILD",
+        "load(':defs.bzl', 'custom_flag', 'transition_rule')",
+        "custom_flag(name = 'my_flag', build_setting_default = 'default flag value')",
+        "no_config_rule(name = 'config_free_target')",
+        "transition_rule(",
+        "    name = 'parent1',",
+        "    flag_value = 'parent1 setting',",
+        "    dep = ':config_free_target',",
+        ")",
+        "transition_rule(",
+        "    name = 'parent2',",
+        "    flag_value = 'parent2 different setting',",
+        "    dep = ':config_free_target',",
+        ")");
+
+    ConfiguredTarget parent1 = getConfiguredTarget("//foo:parent1");
+    ConfiguredTarget parent2 = getConfiguredTarget("//foo:parent2");
+
+    assertThat(parent1.getConfigurationKey()).isNotEqualTo(parent2.getConfigurationKey());
+    assertThat(getDirectPrerequisite(parent1, "//foo:config_free_target"))
+        .isSameInstanceAs(getDirectPrerequisite(parent2, "//foo:config_free_target"));
+  }
+
+  @Test
+  public void preventsConfiguredTargetForkingOnCoreOptions() throws Exception {
+    // Ideally NoConfigTransition would contain empty BuildOptions. In practice it keeps CoreOptions
+    // because core Blaze logic reads CoreOptions (see NoConfigTransition for details). Crucially,
+    // it contains CoreOptions default values - not the values inherited from the source config.
+    // So we still expect config forking to be impossible.
     scratch.overwriteFile(
         "tools/allowlists/function_transition_allowlist/BUILD",
         "package_group(",
@@ -57,41 +122,51 @@ public void noConfigTransitionPreventsConfiguredTargetForking() throws Exception
     scratch.file(
         "foo/defs.bzl",
         "def _my_transition_impl(settings, attr):",
-        "  return {'//command_line_option:compiler': attr.compiler}",
+        "  return {'//command_line_option:features': [attr.feature]}",
         "my_transition = transition(",
         "  implementation = _my_transition_impl,",
         "  inputs = [],",
-        "  outputs = ['//command_line_option:compiler'],",
+        "  outputs = ['//command_line_option:features'],",
         ")",
         "",
-        "transition_compiler_rule = rule(",
+        "transition_features_rule = rule(",
         "  implementation = lambda ctx: [],",
         "  cfg = my_transition,",
         "  attrs = {",
-        "    'compiler': attr.string(),",
+        "    'feature': attr.string(),",
         "    'dep': attr.label(),",
         "    '_allowlist_function_transition':",
         "      attr.label(default = '//tools/allowlists/function_transition_allowlist'),",
         "  },",
         ")");
     scratch.file(
         "foo/BUILD",
-        "load(':defs.bzl', 'transition_compiler_rule')",
+        "load(':defs.bzl', 'transition_features_rule')",
         "no_config_rule(name = 'config_free_target')",
-        "transition_compiler_rule(",
+        "transition_features_rule(",
         "    name = 'parent1',",
-        "    compiler = 'one',",
+        "    feature = 'one',",
         "    dep = ':config_free_target',",
         ")",
-        "transition_compiler_rule(",
+        "transition_features_rule(",
         "    name = 'parent2',",
-        "    compiler = 'two',",
+        "    feature = 'two',",
         "    dep = ':config_free_target',",
         ")");
 
     ConfiguredTarget parent1 = getConfiguredTarget("//foo:parent1");
     ConfiguredTarget parent2 = getConfiguredTarget("//foo:parent2");
 
+    // Sanity check: ensure the flag we're changing is actually in CoreOptions. If you're moving the
+    // flag out of CoreOptions, replace it with another CoreOptions flag.
+    assertThat(
+            parent1
+                .getConfigurationKey()
+                .getOptions()
+                .get(CoreOptions.class)
+                .getClass()
+                .getField("defaultFeatures"))
+        .isNotNull();
     assertThat(parent1.getConfigurationKey()).isNotEqualTo(parent2.getConfigurationKey());
     assertThat(getDirectPrerequisite(parent1, "//foo:config_free_target"))
         .isSameInstanceAs(getDirectPrerequisite(parent2, "//foo:config_free_target"));