Draft: Compute affectedByStarlarkTransition in BuildConfigurationValue

This includes debug prints and has at least the following deficencies:
* no regard for Skyframe intricacies (e.g. no equals implementation for
  BuildConfigurationValue).
* no normalization of hashed BuildOptions options (akin to what happens
  in FunctionTransitionUtil#applyTransition).

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

@@ -153,6 +153,10 @@ private ActionEnvironment setupTestEnvironment() {
     return ActionEnvironment.split(testEnv);
   }
 
+  /**
+   * Only used for AutoCodec, construct an instance via
+   * {@link BuildConfigurationValue#createBuildConfigurationValue} instead.
+   */
   public BuildConfigurationValue(
       BlazeDirectories directories,
       ImmutableMap<Class<? extends Fragment>, Fragment> fragments,
@@ -161,7 +165,8 @@ public BuildConfigurationValue(
       ImmutableSet<String> reservedActionMnemonics,
       ActionEnvironment actionEnvironment,
       RepositoryName mainRepositoryName,
-      boolean siblingRepositoryLayout)
+      boolean siblingRepositoryLayout,
+      String transitionDirectoryNameFragment)
       throws InvalidMnemonicException {
     this.fragments =
         fragmentsInterner.intern(
@@ -174,8 +179,7 @@ public BuildConfigurationValue(
     if (buildOptions.contains(PlatformOptions.class)) {
       platformOptions = buildOptions.get(PlatformOptions.class);
     }
-    this.transitionDirectoryNameFragment =
-        FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions);
+    this.transitionDirectoryNameFragment = transitionDirectoryNameFragment;
     this.outputDirectories =
         new OutputDirectories(
             directories,
@@ -221,6 +225,37 @@ public BuildConfigurationValue(
     this.commandLineLimits = new CommandLineLimits(options.minParamFileSize);
   }
 
+  /**
+   * Constructs an instance of {@link BuildConfigurationValue} with the
+   * transitionDirectoryNameFragment computed from the buildOptions and topLevelBuildOptions.
+   * <p>
+   * This factory method exists so that AutoCodec does not store the topLevelBuildOptions for each
+   * BuildConfigurationValue.
+   */
+  public static BuildConfigurationValue createBuildConfigurationValue(
+      BlazeDirectories directories,
+      ImmutableMap<Class<? extends Fragment>, Fragment> fragments,
+      FragmentClassSet fragmentClassSet,
+      BuildOptions buildOptions,
+      BuildOptions topLevelBuildOptions,
+      ImmutableSet<String> reservedActionMnemonics,
+      ActionEnvironment actionEnvironment,
+      RepositoryName mainRepositoryName,
+      boolean siblingRepositoryLayout) throws InvalidMnemonicException {
+    return new BuildConfigurationValue(
+        directories,
+        fragments,
+        fragmentClassSet,
+        buildOptions,
+        reservedActionMnemonics,
+        actionEnvironment,
+        mainRepositoryName,
+        siblingRepositoryLayout,
+        FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions,
+            topLevelBuildOptions)
+    );
+  }
+
   private ImmutableMap<String, Class<? extends Fragment>> buildIndexOfStarlarkVisibleFragments() {
     ImmutableMap.Builder<String, Class<? extends Fragment>> builder = ImmutableMap.builder();
 

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

@@ -248,40 +248,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
       metadataTags = {OptionMetadataTag.EXPERIMENTAL})
   public boolean enableAspectHints;
 
-  /** Regardless of input, converts to an empty list. For use with affectedByStarlarkTransition */
-  public static class EmptyListConverter implements Converter<List<String>> {
-    @Override
-    public List<String> convert(String input) throws OptionsParsingException {
-      return ImmutableList.of();
-    }
-
-    @Override
-    public String getTypeDescription() {
-      return "Regardless of input, converts to an empty list. For use with"
-          + " affectedByStarlarkTransition";
-    }
-  }
-
-  /**
-   * This internal option is a *set* of names of options that have been changed by starlark
-   * transitions at any point in the build at the time of accessing. It contains both native and
-   * starlark options in label form. e.g. "//command_line_option:cpu" for native options and
-   * "//myapp:foo" for starlark options. This is used to regenerate {@code
-   * transitionDirectoryNameFragment} after each starlark transition.
-   */
-  @Option(
-      name = "affected by starlark transition",
-      defaultValue = "",
-      converter = EmptyListConverter.class,
-      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-      effectTags = {
-        OptionEffectTag.LOSES_INCREMENTAL_STATE,
-        OptionEffectTag.AFFECTS_OUTPUTS,
-        OptionEffectTag.LOADING_AND_ANALYSIS
-      },
-      metadataTags = {OptionMetadataTag.INTERNAL})
-  public List<String> affectedByStarlarkTransition;
-
   @Option(
       name = "platform_suffix",
       defaultValue = "null",
@@ -823,7 +789,6 @@ public IncludeConfigFragmentsEnumConverter() {
   public FragmentOptions getHost() {
     CoreOptions host = (CoreOptions) getDefault();
 
-    host.affectedByStarlarkTransition = affectedByStarlarkTransition;
     host.compilationMode = hostCompilationMode;
     host.isHost = true;
     host.isExec = false;

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

@@ -20,12 +20,12 @@
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.base.Joiner;
-import com.google.common.base.VerifyException;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
@@ -44,9 +44,9 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.TreeMap;
-import java.util.TreeSet;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
@@ -396,76 +396,62 @@ private static BuildOptions applyTransition(
       toOptions.get(CoreOptions.class).evaluatingForAnalysisTest = true;
     }
 
-    updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedNewValues);
     return toOptions;
   }
 
   /**
    * Compute the output directory name fragment corresponding to the new BuildOptions based on the
-   * names and values of all options (both native and Starlark) previously transitioned anywhere in
-   * the build by Starlark transitions. Options only set on command line are not affecting the
-   * computation.
+   * names and values of all options (both native and Starlark) that differ from the options set on
+   * the command line.
    *
-   * @param toOptions the {@link BuildOptions} to use to calculate which we need to compute {@code
-   *     transitionDirectoryNameFragment}.
+   * @param toOptions       the {@link BuildOptions} to compute {@code transitionDirectoryNameFragment}
+   *                        for.
+   * @param topLevelOptions the {@link BuildOptions} representing the top-level configuration as
+   *                        determined by the command-line.
    */
   // TODO(bazel-team): This hashes different forms of equivalent values differently though they
   // should be the same configuration. Starlark transitions are flexible about the values they
   // take (e.g. bool-typed options can take 0/1, True/False, "0"/"1", or "True"/"False") which
   // makes it so that two configurations that are the same in value may hash differently.
-  public static String computeOutputDirectoryNameFragment(BuildOptions toOptions) {
-    CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
-    if (buildConfigOptions.affectedByStarlarkTransition.isEmpty()) {
+  public static String computeOutputDirectoryNameFragment(BuildOptions toOptions,
+      BuildOptions topLevelOptions) {
+    OptionsDiff difference = BuildOptions.diff(toOptions, topLevelOptions);
+
+    if (difference.getFirst().isEmpty()) {
+      // The current configuration is identical to the top-level configuration determined by the
+      // command line. Either we never transitioned away from it or a chain of transition has
+      // returned to an identical configuration. In both cases, there is no need to append a hash to
+      // the output directory.
+      System.err.println("Identical options\ntransitionDirectoryNameFragment = \"\"\n");
       return "";
     }
-    // TODO(blaze-configurability-team): A mild performance optimization would have this be global.
-    Map<String, OptionInfo> optionInfoMap = buildOptionInfo(toOptions);
-
     TreeMap<String, Object> toHash = new TreeMap<>();
-    for (String optionName : buildConfigOptions.affectedByStarlarkTransition) {
-      if (optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
-        String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());
-        Object value;
-        try {
-          value =
-              optionInfoMap
-                  .get(nativeOptionName)
-                  .getDefinition()
-                  .getField()
-                  .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass()));
-        } catch (IllegalAccessException e) {
-          throw new VerifyException(
-              "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage());
-        }
-        toHash.put(optionName, value);
-      } else {
-        Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
-        toHash.put(optionName, value);
-      }
+    StringBuilder sb = new StringBuilder("Differing options:\n");
+    for (Entry<OptionDefinition, Object> nativeOption : difference.getFirst().entrySet()) {
+      sb.append(String.format("%s: %s -> %s%n",
+          COMMAND_LINE_OPTION_PREFIX + nativeOption.getKey().getOptionName(),
+          difference.getSecond().get(
+              nativeOption.getKey()).toArray()[0], nativeOption.getValue()));
+      // FIXME: This should probably convert the option value similar to applyTransition.
+      toHash.put(COMMAND_LINE_OPTION_PREFIX + nativeOption.getKey().getOptionName(),
+          nativeOption.getValue());
+    }
+    for (Label starlarkOption : difference.getChangedStarlarkOptions()) {
+      sb.append(String.format("%s: %s -> %s%n", starlarkOption.toString(),
+          topLevelOptions.getStarlarkOptions().get(starlarkOption),
+          toOptions.getStarlarkOptions().get(starlarkOption)));
+      toHash.put(starlarkOption.toString(), toOptions.getStarlarkOptions().get(starlarkOption));
     }
 
     ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size());
     for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) {
       String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
       hashStrs.add(toAdd);
     }
-    return transitionDirectoryNameFragment(hashStrs.build());
-  }
-
-  /**
-   * Extend the global build config affectedByStarlarkTransition, by adding any new option names
-   * from changedOptions
-   */
-  private static void updateAffectedByStarlarkTransition(
-      CoreOptions buildConfigOptions, Set<String> changedOptions) {
-    if (changedOptions.isEmpty()) {
-      return;
-    }
-    Set<String> mutableCopyToUpdate =
-        new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
-    mutableCopyToUpdate.addAll(changedOptions);
-    buildConfigOptions.affectedByStarlarkTransition =
-        ImmutableList.sortedCopyOf(mutableCopyToUpdate);
+    String stHash = transitionDirectoryNameFragment(hashStrs.build());
+    sb.append(String.format("transitionDirectoryFragment = \"%s\"%n%n", stHash));
+    System.err.println(sb);
+    return stHash;
   }
 
   public static String transitionDirectoryNameFragment(Iterable<String> opts) {

src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java

@@ -32,6 +32,7 @@
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
+import com.google.devtools.build.lib.skyframe.SkyframeExecutor.BuildViewProvider;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -47,13 +48,17 @@ public final class BuildConfigurationFunction implements SkyFunction {
   /** Cache with weak values can't have null values. */
   private static final Fragment NULL_MARKER = new Fragment() {};
 
+  private final BuildViewProvider buildViewProvider;
   private final BlazeDirectories directories;
   private final ConfiguredRuleClassProvider ruleClassProvider;
   private final LoadingCache<FragmentKey, Fragment> fragmentCache =
       Caffeine.newBuilder().weakValues().build(BuildConfigurationFunction::makeFragment);
 
   public BuildConfigurationFunction(
-      BlazeDirectories directories, RuleClassProvider ruleClassProvider) {
+      BuildViewProvider buildViewProvider,
+      BlazeDirectories directories,
+      RuleClassProvider ruleClassProvider) {
+    this.buildViewProvider = buildViewProvider;
     this.directories = directories;
     this.ruleClassProvider = (ConfiguredRuleClassProvider) ruleClassProvider;
   }
@@ -89,12 +94,26 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     ActionEnvironment actionEnvironment =
         ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(key.getOptions());
 
+    BuildConfigurationValue hostConfiguration = buildViewProvider.getSkyframeBuildView()
+        .getHostConfiguration();
+    BuildOptions topLevelBuildOptions;
+    if (hostConfiguration != null) {
+      topLevelBuildOptions = hostConfiguration.getOptions();
+    } else {
+      // hostConfiguration can be null early in the build (see the comment on
+      // SkyframeBuildView#getHostConfiguration. At that point it is probably safe to assume that no
+      // transition has taken place, so the current BuildOptions are still those set on the command
+      // line.
+      System.err.println("hostConfiguration was null");
+      topLevelBuildOptions = key.getOptions();
+    }
     try {
-      return new BuildConfigurationValue(
+      return BuildConfigurationValue.createBuildConfigurationValue(
           directories,
           fragments,
           fragmentClasses,
           key.getOptions(),
+          topLevelBuildOptions,
           ruleClassProvider.getReservedActionMnemonics(),
           actionEnvironment,
           RepositoryName.createFromValidStrippedName(workspaceNameValue.getName()),

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -583,7 +583,7 @@ private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions() {
         new TopLevelActionLookupConflictFindingFunction());
     map.put(
         SkyFunctions.BUILD_CONFIGURATION,
-        new BuildConfigurationFunction(directories, ruleClassProvider));
+        new BuildConfigurationFunction(new BuildViewProvider(), directories, ruleClassProvider));
     map.put(SkyFunctions.WORKSPACE_NAME, new WorkspaceNameFunction());
     map.put(
         WorkspaceFileValue.WORKSPACE_FILE,

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

@@ -1277,20 +1277,11 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception
 
     ConfiguredTarget test = getConfiguredTarget("//test");
 
-    List<String> affectedOptions = getCoreOptions(test).affectedByStarlarkTransition;
-
-    assertThat(affectedOptions).containsExactly("//command_line_option:foo");
-
     @SuppressWarnings("unchecked")
     ConfiguredTarget dep =
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
 
-    affectedOptions = getCoreOptions(dep).affectedByStarlarkTransition;
-
-    assertThat(affectedOptions)
-        .containsExactly("//command_line_option:foo", "//command_line_option:bar");
-
     assertThat(getTransitionDirectoryNameFragment(test))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
@@ -1459,7 +1450,7 @@ public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefaultCom
     assertThat(getTransitionDirectoryNameFragment(dep)).isNotEmpty();
   }
 
-  /** See comment above {@link FunctionTransitionUtil#updateOutputDirectoryNameFragment} */
+  /** See comment above {@link FunctionTransitionUtil#computeOutputDirectoryNameFragment} */
   // TODO(bazel-team): This can be optimized. Make these the same configuration.
   @Test
   public void testOutputDirHash_starlarkOption_differentBoolRepresentationsNotEquals()
@@ -1619,10 +1610,6 @@ public void testOutputDirHash_multipleStarlarkTransitions() throws Exception {
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
 
-    List<String> affectedOptions =
-        getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
-
-    assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo");
     assertThat(getTransitionDirectoryNameFragment(test))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
@@ -1706,10 +1693,6 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
     // test:top (foo_transition)
     ConfiguredTarget top = getConfiguredTarget("//test:top");
 
-    List<String> affectedOptionsTop =
-        getConfiguration(top).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
-
-    assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo");
     assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty();
     assertThat(getTransitionDirectoryNameFragment(top))
         .isEqualTo(
@@ -1721,12 +1704,6 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
     ConfiguredTarget middle =
         Iterables.getOnlyElement((List<ConfiguredTarget>) getMyInfoFromTarget(top).getValue("dep"));
 
-    List<String> affectedOptionsMiddle =
-        getConfiguration(middle).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
-
-    assertThat(affectedOptionsMiddle)
-        .containsExactly("//command_line_option:foo", "//command_line_option:bar", "//test:zee");
-
     assertThat(getConfiguration(middle).getOptions().getStarlarkOptions().entrySet())
         .containsExactly(
             Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"));
@@ -1745,13 +1722,6 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(middle).getValue("dep"));
 
-    List<String> affectedOptionsBottom =
-        getConfiguration(bottom).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
-
-    assertThat(affectedOptionsBottom)
-        .containsExactly(
-            "//command_line_option:foo", "//command_line_option:bar", "//test:xan", "//test:zee");
-
     assertThat(getConfiguration(bottom).getOptions().getStarlarkOptions().entrySet())
         .containsExactly(
             Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"),