Allow some native options to be marked as IMMUTABLE.

This means they cannot be changed during a transition.

Currently only handles starlark transitions. The exec transition is specifically exempted, because it is a core part of configuration handling.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 596917609
Change-Id: Icae25fd907dd1a16e2c83cefe1a14a162438dc99

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

@@ -102,7 +102,20 @@ public static Optional<StarlarkAttributeTransitionProvider> loadStarlarkExecTran
           flagName, userRef, parsedRef.starlarkSymbolName() + " is not a Starlark transition");
     }
     return Optional.of(
-        new StarlarkAttributeTransitionProvider((StarlarkDefinedConfigTransition) transition));
+        new StarlarkExecTransitionProvider((StarlarkDefinedConfigTransition) transition));
+  }
+
+  /** A marker class to distinguish the exec transition from other starlark transitions. */
+  static class StarlarkExecTransitionProvider extends StarlarkAttributeTransitionProvider {
+    StarlarkExecTransitionProvider(StarlarkDefinedConfigTransition execTransition) {
+      super(execTransition);
+    }
+
+    @Override
+    public boolean allowImmutableFlagChanges() {
+      // The exec transition must be allowed to change otherwise immutable flags.
+      return true;
+    }
   }
 
   /**

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

@@ -45,6 +45,7 @@
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.common.options.OptionDefinition;
+import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.lang.reflect.Field;
 import java.util.HashSet;
@@ -76,7 +77,15 @@ public final class FunctionTransitionUtil {
    * incoming {@link BuildOptions}. For native options, this involves a preprocess step of
    * converting options to their "command line form".
    *
-   * <p>Also validate that transitions output the declared results.
+   * <p>Also perform validation on the inputs and outputs:
+   *
+   * <ol>
+   *   <li>Ensure that all native input options exist
+   *   <li>Ensure that all native output options exist
+   *   <li>Ensure that there are no attempts to update the {@code --define} option.
+   *   <li>Ensure that no {@link OptionMetadataTag#IMMUTABLE immutable} native options are updated.
+   *   <li>Ensure that transitions output all of the declared options.
+   * </ol>
    *
    * @param fromOptions the pre-transition build options
    * @param starlarkTransition the transition to apply
@@ -87,15 +96,19 @@ public final class FunctionTransitionUtil {
   static ImmutableMap<String, BuildOptions> applyAndValidate(
       BuildOptions fromOptions,
       StarlarkDefinedConfigTransition starlarkTransition,
+      boolean allowImmutableFlagChanges,
       StructImpl attrObject,
       EventHandler handler)
       throws InterruptedException {
     try {
-      checkForDenylistedOptions(starlarkTransition);
-
       // TODO(waltl): Consider building this once and using it across different split transitions,
       // or reusing BuildOptionDetails.
       ImmutableMap<String, OptionInfo> optionInfoMap = OptionInfo.buildMapFrom(fromOptions);
+
+      validateInputOptions(starlarkTransition.getInputs(), optionInfoMap);
+      validateOutputOptions(
+          starlarkTransition.getOutputs(), allowImmutableFlagChanges, optionInfoMap);
+
       ImmutableMap<String, Object> settings =
           buildSettings(fromOptions, optionInfoMap, starlarkTransition);
 
@@ -236,13 +249,83 @@ private static Map<String, Object> handleImplicitPlatformChange(
         .buildOrThrow();
   }
 
-  private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition transition)
+  private static boolean isNativeOptionValid(
+      ImmutableMap<String, OptionInfo> optionInfoMap, String flag) {
+    String optionName = flag.substring(COMMAND_LINE_OPTION_PREFIX.length());
+
+    // Make sure the option exists.
+    return optionInfoMap.containsKey(optionName);
+  }
+
+  /**
+   * Check if a native option is immutable.
+   *
+   * @return whether or not the option is immutable
+   * @throws VerifyException if the option does not exist
+   */
+  private static boolean isNativeOptionImmutable(
+      ImmutableMap<String, OptionInfo> optionInfoMap, String flag) {
+    String optionName = flag.substring(COMMAND_LINE_OPTION_PREFIX.length());
+    OptionInfo optionInfo = optionInfoMap.get(optionName);
+    if (optionInfo == null) {
+      throw new VerifyException(
+          "Cannot check if option " + flag + " is immutable: it does not exist");
+    }
+    return optionInfo.hasOptionMetadataTag(OptionMetadataTag.IMMUTABLE);
+  }
+
+  private static void validateInputOptions(
+      ImmutableList<String> options, ImmutableMap<String, OptionInfo> optionInfoMap)
+      throws ValidationException {
+    ImmutableList<String> invalidNativeOptions =
+        options.stream()
+            .filter(IS_NATIVE_OPTION)
+            .filter(optionName -> !isNativeOptionValid(optionInfoMap, optionName))
+            .collect(toImmutableList());
+    if (!invalidNativeOptions.isEmpty()) {
+      throw ValidationException.format(
+          "transition inputs [%s] do not correspond to valid settings",
+          Joiner.on(", ").join(invalidNativeOptions));
+    }
+  }
+
+  private static void validateOutputOptions(
+      ImmutableList<String> options,
+      boolean allowImmutableFlagChanges,
+      ImmutableMap<String, OptionInfo> optionInfoMap)
       throws ValidationException {
-    if (transition.getOutputs().contains("//command_line_option:define")) {
+    if (options.contains("//command_line_option:define")) {
       throw new ValidationException(
           "Starlark transition on --define not supported - try using build settings"
               + " (https://bazel.build/rules/config#user-defined-build-settings).");
     }
+
+    // TODO: blaze-configurability - Move the checks for incompatible and experimental flags to here
+    // (currently in ConfigGlobalLibrary.validateBuildSettingKeys).
+
+    ImmutableList<String> invalidNativeOptions =
+        options.stream()
+            .filter(IS_NATIVE_OPTION)
+            .filter(optionName -> !isNativeOptionValid(optionInfoMap, optionName))
+            .collect(toImmutableList());
+    if (!invalidNativeOptions.isEmpty()) {
+      throw ValidationException.format(
+          "transition outputs [%s] do not correspond to valid settings",
+          Joiner.on(", ").join(invalidNativeOptions));
+    }
+
+    if (!allowImmutableFlagChanges) {
+      ImmutableList<String> immutableNativeOptions =
+          options.stream()
+              .filter(IS_NATIVE_OPTION)
+              .filter(optionName -> isNativeOptionImmutable(optionInfoMap, optionName))
+              .collect(toImmutableList());
+      if (!immutableNativeOptions.isEmpty()) {
+        throw ValidationException.format(
+            "transition outputs [%s] cannot be changed: they are immutable",
+            Joiner.on(", ").join(immutableNativeOptions));
+      }
+    }
   }
 
   /**
@@ -389,6 +472,7 @@ private static BuildOptions applyTransition(
       } else {
         // The transition changes a native option.
         String optionName = optionKey.substring(COMMAND_LINE_OPTION_PREFIX.length());
+        OptionInfo optionInfo = optionInfoMap.get(optionName);
 
         // Convert NoneType to null.
         if (optionValue instanceof NoneType) {
@@ -409,12 +493,6 @@ private static BuildOptions applyTransition(
           optionValue = ImmutableMap.copyOf(((Map<?, ?>) optionValue));
         }
         try {
-          if (!optionInfoMap.containsKey(optionName)) {
-            throw ValidationException.format(
-                "transition output '%s' does not correspond to a valid setting", entry.getKey());
-          }
-
-          OptionInfo optionInfo = optionInfoMap.get(optionName);
           OptionDefinition def = optionInfo.getDefinition();
           Field field = def.getField();
           // TODO(b/153867317): check for crashing options types in this logic.

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

@@ -80,6 +80,10 @@ public SplitTransition create(AttributeTransitionData data) {
         starlarkDefinedConfigTransition, (ConfiguredAttributeMapper) attributeMap);
   }
 
+  public boolean allowImmutableFlagChanges() {
+    return false;
+  }
+
   @Override
   public TransitionType transitionType() {
     return TransitionType.ATTRIBUTE;
@@ -137,7 +141,12 @@ public final ImmutableMap<String, BuildOptions> split(
       // we just use the original BuildOptions and trust the transition's enforcement logic.
       BuildOptions buildOptions = buildOptionsView.underlying();
       ImmutableMap<String, BuildOptions> res =
-          applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler);
+          applyAndValidate(
+              buildOptions,
+              starlarkDefinedConfigTransition,
+              allowImmutableFlagChanges(),
+              attrObject,
+              eventHandler);
       if (res == null) {
         return ImmutableMap.of("error", buildOptions.clone());
       }

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

@@ -93,6 +93,10 @@ public TransitionType transitionType() {
     return TransitionType.RULE;
   }
 
+  public boolean allowImmutableFlagChanges() {
+    return false;
+  }
+
   private FunctionPatchTransition createTransition(
       Rule rule, ImmutableMap<Label, ConfigMatchingProvider> configConditions, String configHash) {
     LinkedHashMap<String, Object> attributes = new LinkedHashMap<>();
@@ -198,7 +202,12 @@ public BuildOptions patch(BuildOptionsView buildOptionsView, EventHandler eventH
       // we just use the original BuildOptions and trust the transition's enforcement logic.
       BuildOptions buildOptions = buildOptionsView.underlying();
       Map<String, BuildOptions> result =
-          applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler);
+          applyAndValidate(
+              buildOptions,
+              starlarkDefinedConfigTransition,
+              allowImmutableFlagChanges(),
+              attrObject,
+              eventHandler);
       if (result == null) {
         return buildOptions.clone();
       }

src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java

@@ -187,7 +187,8 @@ public static ImmutableMap<OptionMetadataTag, String> getOptionMetadataTagDescri
             "This option should not be used by a user, and should not be logged.")
         .put(
             OptionMetadataTag.INTERNAL, // Here for completeness, these options are UNDOCUMENTED.
-            "This option isn't even a option, and should not be logged.");
+            "This option isn't even a option, and should not be logged.")
+        .put(OptionMetadataTag.IMMUTABLE, "This option cannot be changed in a transition.");
     return effectTagDescriptionBuilder.build();
   }
 }

src/main/java/com/google/devtools/common/options/OptionMetadataTag.java

@@ -55,9 +55,13 @@ public enum OptionMetadataTag {
    *
    * <p>These should be in category {@link OptionDocumentationCategory.UNDOCUMENTED}.
    */
-  INTERNAL(4);
+  INTERNAL(4),
 
   // reserved TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES(5)
+  // reserved EXPLICIT_IN_OUTPUT_PATH(6)
+
+  /** Options which are IMMUTABLE cannot be changed in (non-exec) Starlark transitions. */
+  IMMUTABLE(7);
 
   private final int value;
 

src/main/protobuf/option_filters.proto

@@ -55,5 +55,7 @@ enum OptionMetadataTag {
   INTERNAL = 4;
   reserved "TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES";
   reserved 5;
+  reserved "EXPLICIT_IN_OUTPUT_PATH";
   reserved 6;
+  IMMUTABLE = 7;
 }

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

@@ -810,8 +810,8 @@ public void testInvalidNativeOptionOutput() throws Exception {
     reporter.removeHandler(failFastHandler);
     getConfiguredTarget("//test/starlark:test");
     assertContainsEvent(
-        "transition output '//command_line_option:foobarbaz' "
-            + "does not correspond to a valid setting");
+        "transition outputs [//command_line_option:foobarbaz] "
+            + "do not correspond to valid settings");
   }
 
   @Test
@@ -955,8 +955,8 @@ public void testInvalidNativeOptionOutput_analysisTest() throws Exception {
     reporter.removeHandler(failFastHandler);
     getConfiguredTarget("//test/starlark:test");
     assertContainsEvent(
-        "transition output '//command_line_option:foobarbaz' "
-            + "does not correspond to a valid setting");
+        "transition outputs [//command_line_option:foobarbaz] "
+            + "do not correspond to valid settings");
   }
 
   @Test

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

@@ -15,9 +15,18 @@
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.Fragment;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.build.lib.analysis.config.RequiresOptions;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.testutil.Scratch;
+import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionDocumentationCategory;
+import com.google.devtools.common.options.OptionEffectTag;
+import com.google.devtools.common.options.OptionMetadataTag;
 import java.util.Map;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -30,6 +39,43 @@
  */
 @RunWith(JUnit4.class)
 public class StarlarkTransitionTest extends BuildViewTestCase {
+
+  /** Extra options for this test. */
+  public static class DummyTestOptions extends FragmentOptions {
+    public DummyTestOptions() {}
+
+    @Option(
+        name = "immutable_option",
+        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+        effectTags = {OptionEffectTag.NO_OP},
+        defaultValue = "super secret",
+        metadataTags = {OptionMetadataTag.IMMUTABLE})
+    public String immutableOption;
+  }
+
+  /** Test fragment. */
+  @RequiresOptions(options = {DummyTestOptions.class})
+  public static final class DummyTestOptionsFragment extends Fragment {
+    private final BuildOptions buildOptions;
+
+    public DummyTestOptionsFragment(BuildOptions buildOptions) {
+      this.buildOptions = buildOptions;
+    }
+
+    // Getter required to satisfy AutoCodec.
+    public BuildOptions getBuildOptions() {
+      return buildOptions;
+    }
+  }
+
+  @Override
+  protected ConfiguredRuleClassProvider createRuleClassProvider() {
+    ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
+    TestRuleClassProvider.addStandardRules(builder);
+    builder.addConfigurationFragment(DummyTestOptionsFragment.class);
+    return builder.build();
+  }
+
   static void writeAllowlistFile(Scratch scratch) throws Exception {
     scratch.overwriteFile(
         "tools/allowlists/function_transition_allowlist/BUILD",
@@ -240,4 +286,30 @@ public void testAliasChangeRerunsTransitionTest() throws Exception {
             getConfiguration(getConfiguredTarget("//test:foo")).getOptions().getStarlarkOptions())
         .containsExactly(Label.parseCanonicalUnchecked("//options:usually_orange"), "orange-eaten");
   }
+
+  @Test
+  public void testChangingImmutableOptionFails() throws Exception {
+    scratch.file(
+        "test/defs.bzl",
+        "def _transition_impl(settings, attr):",
+        "  return {'//command_line_option:immutable_option': 'something_else'}",
+        "_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = [],",
+        "  outputs = ['//command_line_option:immutable_option'],",
+        ")",
+        "def _impl(ctx):",
+        "  return []",
+        "state = rule(",
+        "  implementation = _impl,",
+        "  cfg = _transition,",
+        ")");
+    scratch.file("test/BUILD", "load('//test:defs.bzl', 'state')", "state(name = 'arizona')");
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test:arizona");
+    assertContainsEvent(
+        "transition outputs [//command_line_option:immutable_option] cannot be changed: they are"
+            + " immutable");
+  }
 }