From 37d39a2554063e50ee124d80c9a24b6866c743e0 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Sat, 5 Mar 2022 18:56:45 +0100
Subject: [PATCH] Do not validate input-only settings in transitions

Starlark transition logic temporarily explicitly sets all input build
settings of a transition to their defaults. Since #13971, these values
are cleared after the transition. However, since then they have also
been subject to type validation, which is not only unnecessary, but
also breaks in the special case of a string build setting with
allow_multiple.

With this commit, input-only build settings at their literal default
values are removed from the post-transition BuildOptions without going
through validation. This is made possible by a refactoring of
`StarlarkTransition#validate` that extracts the validation logic into a
separate function and also improves some variable names.
---
 .../analysis/starlark/StarlarkTransition.java | 179 +++++++++++-------
 .../StarlarkAttrTransitionProviderTest.java   | 132 +++++++++++++
 2 files changed, 239 insertions(+), 72 deletions(-)

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 64a0de883867a6..9f1518591f2896 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
@@ -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,27 @@ 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 blaze-configurability@google.com.",
-            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 +347,75 @@ public static Map<String, BuildOptions> validate(
     return cleanedOptionMap.buildOrThrow();
   }
 
+  /**
+   * 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 blaze-configurability@google.com.",
+        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
    *
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 5a9544686e7975..718fd6fca22115 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
@@ -2549,4 +2549,136 @@ 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");
+  }
 }