From 3ebca6e9d27d043003cc8e627f35ca59018d335e 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 are unconditionally
stripped from the post-transition BuildOptions and do not undergo
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 | 169 ++++++++++--------
 .../StarlarkAttrTransitionProviderTest.java   |  57 ++++++
 2 files changed, 154 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..cc82a0adbb71d1 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,38 @@ 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 input or outputs of the transition together with their types.
+    // Output setting values will be validated and cleaned if set to the default.
+    Map<Label, Rule> inputAndOutputSettingsToRule = Maps.newHashMap();
+    // Collect settings that were only used as inputs to the transition and thus had their default
+    // values added to the fromOptions. They always have to be cleaned.
+    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()) {
+    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 +314,19 @@ 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 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());
+        Label maybeAliasSetting = settingWithRule.getKey();
+        Rule rule = settingWithRule.getValue();
+        // Input-only settings had their default value added to the BuildOptions and thus always
+        // have to be cleaned, whereas output settings have their value validated, converted and
+        // stripped if it is equivalent to the setting's default value.
+        if (inputOnlySettings.contains(maybeAliasSetting) || 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 +338,74 @@ 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..334be041c17d3d 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,61 @@ 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();
+  }
 }