Do not validate input-only settings in transitions

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

@@ -267,31 +267,39 @@ public static Map<String, BuildOptions> validate(
       Map<PackageValue.Key, PackageValue> buildSettingPackages,
       Map<String, BuildOptions> toOptions)
       throws TransitionException {
-    // Collect settings changed during this transition and their types. This includes settings that
-    // were only used as inputs as to the transition and thus had their default values added to the
-    // fromOptions, which in case of a no-op transition directly end up in toOptions.
-    Map<Label, Rule> changedSettingToRule = Maps.newHashMap();
+    // Collect settings that are inputs or outputs of the transition together with their types.
+    // Output setting values will be validated and removed if set to their default.
+    Map<Label, Rule> inputAndOutputSettingsToRule = Maps.newHashMap();
+    // Collect settings that were only used as inputs to the transition and thus possibly had their
+    // default values added to the fromOptions. They will be removed if set to ther default, but
+    // should not be validated.
+    Set<Label> inputOnlySettings = Sets.newHashSet();
     root.visit(
         (StarlarkTransitionVisitor)
             transition -> {
-              ImmutableSet<Label> changedSettings =
+              ImmutableSet<Label> inputAndOutputSettings =
                   getRelevantStarlarkSettingsFromTransition(
                       transition, Settings.INPUTS_AND_OUTPUTS);
-              for (Label setting : changedSettings) {
-                changedSettingToRule.put(
+              ImmutableSet<Label> outputSettings =
+                  getRelevantStarlarkSettingsFromTransition(transition, Settings.OUTPUTS);
+              for (Label setting : inputAndOutputSettings) {
+                inputAndOutputSettingsToRule.put(
                     setting, getActual(buildSettingPackages, setting).getAssociatedRule());
+                if (!outputSettings.contains(setting)) {
+                  inputOnlySettings.add(setting);
+                }
               }
             });
 
-    // Return early if no starlark settings were affected.
-    if (changedSettingToRule.isEmpty()) {
+    // Return early if the transition has neither inputs nor outputs (rare).
+    if (inputAndOutputSettingsToRule.isEmpty()) {
       return toOptions;
     }
 
     ImmutableMap.Builder<Label, Label> aliasToActualBuilder = new ImmutableMap.Builder<>();
-    for (Map.Entry<Label, Rule> changedSettingWithRule : changedSettingToRule.entrySet()) {
-      Label setting = changedSettingWithRule.getKey();
-      Rule rule = changedSettingWithRule.getValue();
+    for (Map.Entry<Label, Rule> settingWithRule : inputAndOutputSettingsToRule.entrySet()) {
+      Label setting = settingWithRule.getKey();
+      Rule rule = settingWithRule.getValue();
       if (!rule.getLabel().equals(setting)) {
         aliasToActualBuilder.put(setting, rule.getLabel());
       }
@@ -307,69 +315,28 @@ public static Map<String, BuildOptions> validate(
       BuildOptions.Builder cleanedOptions = null;
       // Clean up aliased values.
       BuildOptions options = unalias(entry.getValue(), aliasToActual);
-      for (Map.Entry<Label, Rule> changedSettingWithRule : changedSettingToRule.entrySet()) {
+      for (Map.Entry<Label, Rule> settingWithRule : inputAndOutputSettingsToRule.entrySet()) {
         // If the build setting was referenced in the transition via an alias, this is that alias
-        Label maybeAliasSetting = changedSettingWithRule.getKey();
-        Rule rule = changedSettingWithRule.getValue();
-        // If the build setting was *not* referenced in the transition by an alias, this is the same
-        // value as {@code maybeAliasSetting} above.
+        Label maybeAliasSetting = settingWithRule.getKey();
+        Rule rule = settingWithRule.getValue();
         Label actualSetting = rule.getLabel();
-        Object newValue = options.getStarlarkOptions().get(actualSetting);
-        // TODO(b/154132845): fix NPE occasionally observed here.
-        Preconditions.checkState(
-            newValue != null,
-            "Error while attempting to validate new values from starlark"
-                + " transition(s) with the outputs %s. Post-transition configuration should include"
-                + " '%s' but only includes starlark options: %s. If you run into this error"
-                + " please ping b/154132845 or email [email protected].",
-            changedSettingToRule.keySet(),
-            actualSetting,
-            options.getStarlarkOptions().keySet());
-        boolean allowsMultiple = rule.getRuleClassObject().getBuildSetting().allowsMultiple();
-        if (allowsMultiple) {
-          // if this setting allows multiple settings
-          if (!(newValue instanceof List)) {
-            throw new TransitionException(
-                String.format(
-                    "'%s' allows multiple values and must be set"
-                        + " in transition using a starlark list instead of single value '%s'",
-                    actualSetting, newValue));
-          }
-          List<?> rawNewValueAsList = (List<?>) newValue;
-          List<Object> convertedValue = new ArrayList<>();
-          Type<?> type = rule.getRuleClassObject().getBuildSetting().getType();
-          for (Object value : rawNewValueAsList) {
-            try {
-              convertedValue.add(type.convert(value, maybeAliasSetting));
-            } catch (ConversionException e) {
-              throw new TransitionException(e);
-            }
-          }
-          if (convertedValue.equals(
-              ImmutableList.of(rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)))) {
-            if (cleanedOptions == null) {
-              cleanedOptions = options.toBuilder();
-            }
-            cleanedOptions.removeStarlarkOption(rule.getLabel());
-          }
-        } else {
-          // if this setting does not allow multiple settings
-          Object convertedValue;
-          try {
-            convertedValue =
-                rule.getRuleClassObject()
-                    .getBuildSetting()
-                    .getType()
-                    .convert(newValue, maybeAliasSetting);
-          } catch (ConversionException e) {
-            throw new TransitionException(e);
-          }
-          if (convertedValue.equals(rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME))) {
-            if (cleanedOptions == null) {
-              cleanedOptions = options.toBuilder();
-            }
-            cleanedOptions.removeStarlarkOption(rule.getLabel());
+        // Input-only settings may have had their literal default value added to the BuildOptions
+        // so that the transition can read them. We have to remove these explicitly set value here
+        // to preserve the invariant that Starlark settings at default values are not explicitly set
+        // in the BuildOptions.
+        final boolean isInputOnlySettingAtDefault =
+            inputOnlySettings.contains(maybeAliasSetting)
+                && rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)
+                    .equals(options.getStarlarkOptions().get(actualSetting));
+        // For output settings, the raw value returned by the transition first has to be validated
+        // and converted to the proper type before it can be compared to the default value.
+        if (isInputOnlySettingAtDefault
+            || validateAndCheckIfAtDefault(
+                rule, options, maybeAliasSetting, inputAndOutputSettingsToRule.keySet())) {
+          if (cleanedOptions == null) {
+            cleanedOptions = options.toBuilder();
           }
+          cleanedOptions.removeStarlarkOption(rule.getLabel());
         }
       }
       // Keep the same instance if we didn't do anything to maintain reference equality later on.
@@ -381,6 +348,81 @@ public static Map<String, BuildOptions> validate(
     return cleanedOptionMap.build();
   }
 
+  /**
+   * Validate the value of a particular build setting after a transition has been applied.
+   *
+   * @param buildSettingRule the build setting to validate.
+   * @param options the {@link BuildOptions} reflecting the post-transition configuration.
+   * @param maybeAliasSetting the label used to refer to the build setting in the transition,
+   *     possibly an alias. This is only used for error messages.
+   * @param inputAndOutputSettings the transition input and output settings. This is only used for
+   *     error messages.
+   * @return {@code true} if and only if the setting is set to its default value after the
+   *     transition.
+   * @throws TransitionException if the value returned by the transition for this setting has an
+   *     invalid type.
+   */
+  private static boolean validateAndCheckIfAtDefault(
+      Rule buildSettingRule,
+      BuildOptions options,
+      Label maybeAliasSetting,
+      Set<Label> inputAndOutputSettings)
+      throws TransitionException {
+    // If the build setting was *not* referenced in the transition by an alias, this is the same
+    // value as {@code maybeAliasSetting}.
+    Label actualSetting = buildSettingRule.getLabel();
+    Object newValue = options.getStarlarkOptions().get(actualSetting);
+    // TODO(b/154132845): fix NPE occasionally observed here.
+    Preconditions.checkState(
+        newValue != null,
+        "Error while attempting to validate new values from starlark"
+            + " transition(s) with the inputs and outputs %s. Post-transition configuration should"
+            + " include '%s' but only includes starlark options: %s. If you run into this error"
+            + " please ping b/154132845 or email [email protected].",
+        inputAndOutputSettings,
+        actualSetting,
+        options.getStarlarkOptions().keySet());
+    boolean allowsMultiple =
+        buildSettingRule.getRuleClassObject().getBuildSetting().allowsMultiple();
+    if (allowsMultiple) {
+      // if this setting allows multiple settings
+      if (!(newValue instanceof List)) {
+        throw new TransitionException(
+            String.format(
+                "'%s' allows multiple values and must be set"
+                    + " in transition using a starlark list instead of single value '%s'",
+                actualSetting, newValue));
+      }
+      List<?> rawNewValueAsList = (List<?>) newValue;
+      List<Object> convertedValue = new ArrayList<>();
+      Type<?> type = buildSettingRule.getRuleClassObject().getBuildSetting().getType();
+      for (Object value : rawNewValueAsList) {
+        try {
+          convertedValue.add(type.convert(value, maybeAliasSetting));
+        } catch (ConversionException e) {
+          throw new TransitionException(e);
+        }
+      }
+      return convertedValue.equals(
+          ImmutableList.of(buildSettingRule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)));
+    } else {
+      // if this setting does not allow multiple settings
+      Object convertedValue;
+      try {
+        convertedValue =
+            buildSettingRule
+                .getRuleClassObject()
+                .getBuildSetting()
+                .getType()
+                .convert(newValue, maybeAliasSetting);
+      } catch (ConversionException e) {
+        throw new TransitionException(e);
+      }
+      return convertedValue.equals(
+          buildSettingRule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME));
+    }
+  }
+
   /*
    * Resolve aliased build setting issues
    *

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

@@ -2489,4 +2489,140 @@ public void testExplicitNoopTransitionTrimsInputBuildSettings() throws Exception
         new EventBus());
     assertNoEvents();
   }
+
+  @Test
+  public void testTransitionPreservesAllowMultipleDefault() throws Exception {
+    writeAllowlistFile();
+    scratch.file(
+        "test/starlark/rules.bzl",
+        "P = provider(fields = ['value'])",
+        "def _s_impl(ctx):",
+        "    return [P(value = ctx.build_setting_value)]",
+        "def _t_impl(settings, attr):",
+        "    if 'foo' in settings['//test/starlark:a']:",
+        "        return {'//test/starlark:b': ['bar']}",
+        "    else:",
+        "        return {'//test/starlark:b': ['baz']}",
+        "def _r_impl(ctx):",
+        "    pass",
+        "s = rule(",
+        "    implementation = _s_impl,",
+        "    build_setting = config.string(allow_multiple = True, flag = True),",
+        ")",
+        "t = transition(",
+        "    implementation = _t_impl,",
+        "    inputs = ['//test/starlark:a'],",
+        "    outputs = ['//test/starlark:b'],",
+        ")",
+        "r = rule(",
+        "    implementation = _r_impl,",
+        "    attrs = {",
+        "        'setting': attr.label(cfg = t),",
+        "        '_allowlist_function_transition': attr.label(",
+        "            default = '//tools/allowlists/function_transition_allowlist',",
+        "        ),",
+        "    },",
+        ")");
+    scratch.file(
+        "test/starlark/BUILD",
+        "load(':rules.bzl', 's', 'r')",
+        "s(",
+        "  name = 'a',",
+        "  build_setting_default = '',",
+        ")",
+        "s(",
+        "  name = 'b',",
+        "  build_setting_default = '',",
+        ")",
+        "r(",
+        "  name = 'c',",
+        "  setting = ':b',",
+        ")");
+    update(
+        ImmutableList.of("//test/starlark:c"),
+        /*keepGoing=*/ false,
+        LOADING_PHASE_THREADS,
+        /*doAnalysis=*/ true,
+        new EventBus());
+    assertNoEvents();
+  }
+
+  @Test
+  public void testTransitionPreservesNonDefaultInputOnlySetting() throws Exception {
+    writeAllowlistFile();
+    scratch.file(
+        "test/starlark/rules.bzl",
+        "def _string_impl(ctx):",
+        "    return []",
+        "string_flag = rule(",
+        "    implementation = _string_impl,",
+        "    build_setting = config.string(flag = True),",
+        ")",
+        "def _transition_impl(settings, attr):",
+        "    return {",
+        "        '//test/starlark:output_only': settings['//test/starlark:input_only'],",
+        "    }",
+        "_transition = transition(",
+        "    implementation = _transition_impl,",
+        "    inputs = [",
+        "        '//test/starlark:input_only',",
+        "    ],",
+        "    outputs = [",
+        "        '//test/starlark:output_only',",
+        "    ],",
+        ")",
+        "def _apply_transition_impl(ctx):",
+        "    ctx.actions.symlink(",
+        "        output = ctx.outputs.out,",
+        "        target_file = ctx.file.target,",
+        "    )",
+        "    return [DefaultInfo(executable = ctx.outputs.out)]",
+        "apply_transition = rule(",
+        "    implementation = _apply_transition_impl,",
+        "    attrs = {",
+        "        'target': attr.label(",
+        "            cfg = _transition,",
+        "            allow_single_file = True,",
+        "        ),",
+        "        'out': attr.output(),",
+        "        '_allowlist_function_transition': attr.label(",
+        "            default = '//tools/allowlists/function_transition_allowlist',",
+        "        ),",
+        "    },",
+        "    executable = False,",
+        ")");
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:rules.bzl', 'apply_transition', 'string_flag')",
+        "",
+        "string_flag(",
+        "    name = 'input_only',",
+        "    build_setting_default = 'input_only_default',",
+        ")",
+        "string_flag(",
+        "    name = 'output_only',",
+        "    build_setting_default = 'output_only_default',",
+        ")",
+        "cc_binary(",
+        "    name = 'main',",
+        "    srcs = ['main.cc'],",
+        ")",
+        "apply_transition(",
+        "    name = 'transitioned_main',",
+        "    target = ':main',",
+        "    out = 'main_out',",
+        ")");
+
+    useConfiguration(ImmutableMap.of("//test/starlark:input_only", "not_the_default"));
+    Label inputOnlySetting = Label.parseAbsoluteUnchecked("//test/starlark:input_only");
+    ConfiguredTarget transitionedDep =
+        getDirectPrerequisite(
+            getConfiguredTarget("//test/starlark:transitioned_main"), "//test/starlark:main");
+    assertThat(
+            getConfiguration(transitionedDep)
+                .getOptions()
+                .getStarlarkOptions()
+                .get(inputOnlySetting))
+        .isEqualTo("not_the_default");
+  }
 }