Remove FragmentClassSet from BuildConfigurationKey

Ultimately, this feature was not being used and thus contributing to technical debt. In particular, note how the background on constructing ConfiguredTargetKey can now be simplified as no longer need to consider if a given BuildConfigurationKey is 'canonical'.

Future use sites that want to trim based on fragments and their associated option requirements can just re-implement the trimming logic. This could possibly be escalated to a new, separate SkyFunction; potentially also incorporating running (and thus more centrally caching) transition application.

PiperOrigin-RevId: 410362732

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

@@ -93,7 +93,6 @@ public interface ActionEnvironmentProvider {
   private final OutputDirectories outputDirectories;
 
   private final ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments;
-  private final FragmentClassSet fragmentClassSet;
 
   private final ImmutableMap<String, Class<? extends Fragment>> starlarkVisibleFragments;
   private final RepositoryName mainRepositoryName;
@@ -156,7 +155,6 @@ private ActionEnvironment setupTestEnvironment() {
   public BuildConfigurationValue(
       BlazeDirectories directories,
       ImmutableMap<Class<? extends Fragment>, Fragment> fragments,
-      FragmentClassSet fragmentClassSet,
       BuildOptions buildOptions,
       ImmutableSet<String> reservedActionMnemonics,
       ActionEnvironment actionEnvironment,
@@ -166,7 +164,6 @@ public BuildConfigurationValue(
     this.fragments =
         fragmentsInterner.intern(
             ImmutableSortedMap.copyOf(fragments, FragmentClassSet.LEXICAL_FRAGMENT_SORTER));
-    this.fragmentClassSet = fragmentClassSet;
     this.starlarkVisibleFragments = buildIndexOfStarlarkVisibleFragments();
     this.buildOptions = buildOptions;
     this.options = buildOptions.get(CoreOptions.class);
@@ -241,7 +238,7 @@ private ImmutableMap<String, Class<? extends Fragment>> buildIndexOfStarlarkVisi
    * again.
    */
   public BuildConfigurationKey getKey() {
-    return BuildConfigurationKey.withoutPlatformMapping(fragmentClassSet, buildOptions);
+    return BuildConfigurationKey.withoutPlatformMapping(buildOptions);
   }
 
   /** Retrieves the {@link BuildOptionDetails} containing data on this configuration's options. */
@@ -523,11 +520,6 @@ public BlazeDirectories getDirectories() {
     return outputDirectories.getDirectories();
   }
 
-  /** Which fragments does this configuration contain? */
-  public FragmentClassSet fragmentClasses() {
-    return fragmentClassSet;
-  }
-
   /** Returns true if non-functional build stamps are enabled. */
   public boolean stampBinaries() {
     return options.stampBinaries;

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

@@ -260,8 +260,7 @@ public ImmutableList<Dependency> resolveConfiguration(
       return ImmutableList.of(resolveHostTransition(dependencyBuilder, dependencyKey));
     }
 
-    return resolveGenericTransition(
-        getCurrentConfiguration().fragmentClasses(), dependencyBuilder, dependencyKey);
+    return resolveGenericTransition(dependencyBuilder, dependencyKey);
   }
 
   @Nullable
@@ -293,7 +292,6 @@ private Dependency resolveHostTransition(
 
   @Nullable
   private ImmutableList<Dependency> resolveGenericTransition(
-      FragmentClassSet depFragments,
       Dependency.Builder dependencyBuilder,
       DependencyKey dependencyKey)
       throws ConfiguredValueCreationException, InterruptedException {
@@ -312,8 +310,7 @@ private ImmutableList<Dependency> resolveGenericTransition(
       throw new ConfiguredValueCreationException(ctgValue, e.getMessage());
     }
 
-    if (depFragments.equals(getCurrentConfiguration().fragmentClasses())
-        && SplitTransition.equals(getCurrentConfiguration().getOptions(), toOptions.values())) {
+    if (SplitTransition.equals(getCurrentConfiguration().getOptions(), toOptions.values())) {
       // The dep uses the same exact configuration. Let's re-use the current configuration and
       // skip adding a Skyframe dependency edge on it.
       return ImmutableList.of(
@@ -340,7 +337,7 @@ private ImmutableList<Dependency> resolveGenericTransition(
         String transitionKey = optionsEntry.getKey();
         BuildConfigurationKey buildConfigurationKey =
             BuildConfigurationKey.withPlatformMapping(
-                platformMappingValue, depFragments, optionsEntry.getValue());
+                platformMappingValue, optionsEntry.getValue());
         configurationKeys.put(transitionKey, buildConfigurationKey);
       }
     } catch (OptionsParsingException e) {
@@ -358,6 +355,7 @@ private ImmutableList<Dependency> resolveGenericTransition(
         if (valueOrException.get() == null) {
           continue;
         }
+        // TODO(blaze-configurability-team): Should be able to just use BuildConfigurationKey
         BuildConfigurationValue configuration = (BuildConfigurationValue) valueOrException.get();
         if (configuration != null) {
           Dependency resolvedDep =

src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java

@@ -16,7 +16,6 @@
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
 
 import com.google.common.base.Joiner;
-import com.google.common.base.Preconditions;
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -50,7 +49,6 @@
 import com.google.devtools.build.lib.query2.query.aspectresolvers.AspectResolver;
 import com.google.devtools.build.lib.rules.AliasConfiguredTarget;
 import com.google.devtools.build.lib.server.FailureDetails.ConfigurableQuery;
-import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -175,38 +173,15 @@ private static ImmutableList<QueryFunction> getCqueryFunctions() {
     return ImmutableList.of(new ConfigFunction());
   }
 
-  /**
-   * Returns a supplied {@link BuildConfigurationValue} if both have the same build options,
-   * otherwise throws an exception.
-   *
-   * <p>Noting the background of {@link BuildConfigurationKey#toComparableString}, multiple {@link
-   * BuildConfigurationKey} instances can correspond to the same {@link BuildConfigurationValue},
-   * especially when trimming is involved. We are only interested in configurations whose options
-   * differ - intricacies around differing fragments can be disregarded.
-   */
-  private static BuildConfigurationValue mergeEqualBuildConfiguration(
-      BuildConfigurationValue left, BuildConfigurationValue right) {
-    Preconditions.checkArgument(
-        left.getOptions().equals(right.getOptions()),
-        "Non-matching configurations: (%s, %s)",
-        left,
-        right);
-    return left;
-  }
-
   private static ImmutableMap<String, BuildConfigurationValue> getTransitiveConfigurations(
       Collection<SkyKey> transitiveConfigurationKeys, WalkableGraph graph)
       throws InterruptedException {
-    // mergeEqualBuildConfiguration can only fail if two BuildConfigurationValue have the same
-    // checksum but are not equal. This would be a black swan event.
+    // BuildConfigurationKey and BuildConfigurationValue should be 1:1
+    // so merge function intentionally omitted
     return graph.getSuccessfulValues(transitiveConfigurationKeys).values().stream()
         .map(BuildConfigurationValue.class::cast)
         .sorted(Comparator.comparing(BuildConfigurationValue::checksum))
-        .collect(
-            toImmutableMap(
-                BuildConfigurationValue::checksum,
-                Function.identity(),
-                ConfiguredTargetQueryEnvironment::mergeEqualBuildConfiguration));
+        .collect(toImmutableMap(BuildConfigurationValue::checksum, Function.identity()));
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/runtime/commands/ConfigCommand.java

@@ -388,7 +388,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
         .filter(e -> SkyFunctions.BUILD_CONFIGURATION.equals(e.getKey().functionName()))
         .collect(
             toImmutableSortedMap(
-                comparing(BuildConfigurationKey::toComparableString),
+                comparing(e -> e.getOptions().checksum()),
                 e -> (BuildConfigurationKey) e.getKey(),
                 e -> (BuildConfigurationValue) e.getValue()));
   }
@@ -688,8 +688,7 @@ private static ConfigurationDiffForOutput getConfigurationDiffForOutput(
                               e -> toNullableStringPair(e.getValue())));
               fragmentDiffs.add(new FragmentDiffForOutput(fragmentName, sortedOptionDiffs));
             });
-    return new ConfigurationDiffForOutput(
-        configHash1, configHash2, ImmutableList.copyOf(fragmentDiffs.build()));
+    return new ConfigurationDiffForOutput(configHash1, configHash2, fragmentDiffs.build().asList());
   }
 
   private static Pair<String, String> toNullableStringPair(Pair<Object, Object> pair) {

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

@@ -66,21 +66,16 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     if (workspaceNameValue == null) {
       return null;
     }
+    FragmentClassSet fragmentClasses = ruleClassProvider.getFragmentRegistry().getAllFragments();
 
     BuildConfigurationKey key = (BuildConfigurationKey) skyKey.argument();
     ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments;
     try {
-      fragments = getConfigurationFragments(key);
+      fragments = getConfigurationFragments(key, fragmentClasses);
     } catch (InvalidConfigurationException e) {
       throw new BuildConfigurationFunctionException(e);
     }
 
-    // If nothing was trimmed, reuse the same FragmentClassSet.
-    FragmentClassSet fragmentClasses = key.getFragments();
-    if (fragments.size() != fragmentClasses.size()) {
-      fragmentClasses = FragmentClassSet.of(fragments.keySet());
-    }
-
     StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
     if (starlarkSemantics == null) {
       return null;
@@ -93,7 +88,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       return new BuildConfigurationValue(
           directories,
           fragments,
-          fragmentClasses,
           key.getOptions(),
           ruleClassProvider.getReservedActionMnemonics(),
           actionEnvironment,
@@ -105,8 +99,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
   }
 
   private ImmutableSortedMap<Class<? extends Fragment>, Fragment> getConfigurationFragments(
-      BuildConfigurationKey key) throws InvalidConfigurationException {
-    FragmentClassSet fragmentClasses = key.getFragments();
+      BuildConfigurationKey key, FragmentClassSet fragmentClasses)
+      throws InvalidConfigurationException {
     ImmutableSortedMap.Builder<Class<? extends Fragment>, Fragment> fragments =
         ImmutableSortedMap.orderedBy(FragmentClassSet.LEXICAL_FRAGMENT_SORTER);
     for (Class<? extends Fragment> fragmentClass : fragmentClasses) {

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

@@ -17,13 +17,11 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Interner;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
-import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.common.options.OptionsParsingException;
-import java.util.Objects;
 
 /**
  * {@link SkyKey} for {@link com.google.devtools.build.lib.analysis.config.BuildConfigurationValue}.
@@ -37,14 +35,13 @@ public final class BuildConfigurationKey implements SkyKey {
    *
    * @param platformMappingValue sky value that can transform a configuration key based on a
    *     platform mapping
-   * @param fragments set of options fragments this configuration should cover
    * @param options the desired configuration
    * @throws OptionsParsingException if the platform mapping cannot be parsed
    */
   public static BuildConfigurationKey withPlatformMapping(
-      PlatformMappingValue platformMappingValue, FragmentClassSet fragments, BuildOptions options)
+      PlatformMappingValue platformMappingValue, BuildOptions options)
       throws OptionsParsingException {
-    return platformMappingValue.map(withoutPlatformMapping(fragments, options));
+    return platformMappingValue.map(withoutPlatformMapping(options));
   }
 
   /**
@@ -53,29 +50,19 @@ public static BuildConfigurationKey withPlatformMapping(
    * <p>Callers are responsible for applying the platform mapping or ascertaining that a platform
    * mapping is not required.
    *
-   * @param fragments the fragments the configuration should contain
    * @param options the {@link BuildOptions} object the {@link BuildOptions} should be rebuilt from
    */
   @AutoCodec.Instantiator
-  public static BuildConfigurationKey withoutPlatformMapping(
-      FragmentClassSet fragments, BuildOptions options) {
-    return interner.intern(new BuildConfigurationKey(fragments, options));
+  public static BuildConfigurationKey withoutPlatformMapping(BuildOptions options) {
+    return interner.intern(new BuildConfigurationKey(options));
   }
 
   private static final Interner<BuildConfigurationKey> interner = BlazeInterners.newWeakInterner();
 
-  private final FragmentClassSet fragments;
   private final BuildOptions options;
-  private final int hashCode;
 
-  private BuildConfigurationKey(FragmentClassSet fragments, BuildOptions options) {
-    this.fragments = Preconditions.checkNotNull(fragments);
+  private BuildConfigurationKey(BuildOptions options) {
     this.options = Preconditions.checkNotNull(options);
-    this.hashCode = Objects.hash(fragments, options);
-  }
-
-  public FragmentClassSet getFragments() {
-    return fragments;
   }
 
   public BuildOptions getOptions() {
@@ -96,47 +83,17 @@ public boolean equals(Object o) {
       return false;
     }
     BuildConfigurationKey otherConfig = (BuildConfigurationKey) o;
-    return options.equals(otherConfig.options) && fragments.equals(otherConfig.fragments);
+    return options.equals(otherConfig.options);
   }
 
   @Override
   public int hashCode() {
-    return hashCode;
+    return options.hashCode();
   }
 
   @Override
   public String toString() {
     // This format is depended on by integration tests.
-    // TODO(blaze-configurability-team): This should at least include the length of fragments.
-    // to at least remind devs that this Key has TWO key parts.
     return "BuildConfigurationKey[" + options.checksum() + "]";
   }
-
-  /**
-   * Returns a string representation that can be safely used for comparison purposes.
-   *
-   * <p>Unlike toString, which is short and good for printing in debug contexts, this is long
-   * because it includes sufficient information in options and fragments. toString alone is
-   * insufficient because multiple Keys can have the same options checksum (and thus same toString)
-   * but different fragments.
-   *
-   * <p>This function is meant to address two potential, trimming-related scenarios: 1. If trimming
-   * by only trimming BuildOptions (e.g. --trim_test_configuration), then after the initial
-   * trimming, fragments has extra classes (corresponding to those trimmed). Notably, dependencies
-   * of trimmed targets will create Keys with a properly trimmed set of fragments. Thus, will easily
-   * have two Keys with the same (trimmed) BuildOptions but different fragments yet corresponding to
-   * the same (trimmed) BuildConfigurationValue.
-   *
-   * <p>2. If trimming by only trimming fragments (at time of this comment, unsure whether this is
-   * ever done in active code), then BuildOptions has extra classes. The returned
-   * BuildConfigurationValue is properly trimmed (with the extra classes BuildOptions removed)
-   * although notably with a different checksum compared to the Key checksum. Note that given a
-   * target that is doing trimming like this, the reverse dependency of the target (i.e. without
-   * trimming) could easily involve a Key with the same (untrimmed!) BuildOptions but different
-   * fragments. However, unlike in case 1, they will correspond to different
-   * BuildConfigurationValue.
-   */
-  public String toComparableString() {
-    return "BuildConfigurationKey[" + options.checksum() + ", " + fragments + "]";
-  }
 }

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

@@ -522,8 +522,7 @@ public static ComputedToolchainContexts computeUnloadedToolchainContexts(
             env.getListener());
 
     BuildConfigurationKey toolchainConfig =
-        BuildConfigurationKey.withoutPlatformMapping(
-            configuration.fragmentClasses(), toolchainOptions);
+        BuildConfigurationKey.withoutPlatformMapping(toolchainOptions);
 
     Map<String, ToolchainContextKey> toolchainContextKeys = new HashMap<>();
     String targetUnloadedToolchainContext = "target-unloaded-toolchain-context";

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

@@ -32,9 +32,9 @@
  * In simple form, a ({@link Label}, {@link BuildConfigurationValue}) pair used to trigger immediate
  * dependency resolution and the rule analysis.
  *
- * <p>In practice, a ({@link Label}, canonical and post-transition {@link BuildConfigurationKey})
- * pair plus a possible execution platform override {@link Label} with special constraints. To
- * elaborate, in order of highest to lowest potential for concern:
+ * <p>In practice, a ({@link Label} and post-transition {@link BuildConfigurationKey}) pair plus a
+ * possible execution platform override {@link Label} with special constraints. To elaborate, in
+ * order of highest to lowest potential for concern:
  *
  * <p>1. The {@link BuildConfigurationKey} must be post-transition and thus ready for immediate use
  * in dependency resolution and analysis. In practice, this means that if the rule has an
@@ -43,14 +43,7 @@
  * build graphs with ConfiguredTarget that have seemingly impossible {@link BuildConfigurationValue}
  * (due to the skipped transitions).
  *
- * <p>2. The {@link BuildConfigurationKey} must be canonical. Multiple keys can correspond to the
- * same {@link BuildConfigurationValue}. The canonical key is the one returned by {@link
- * BuildConfigurationValue#getKey}. Failure to use a canonical key can result in build graphs with
- * multiple seemingly-identical ConfiguredTarget that have the same ({@link Label}, {@link
- * BuildConfigurationValue}) pair. This is non-performant in all cases and incorrect if those
- * duplications lead to action conflicts due to unsharable actions.
- *
- * <p>3. A build should not request keys with equal ({@link Label}, {@link BuildConfigurationValue})
+ * <p>2. A build should not request keys with equal ({@link Label}, {@link BuildConfigurationValue})
  * pairs but different execution platform override {@link Label} if the invoked rule will register
  * actions. (This is potentially OK if all outputs of all registered actions incorporate the
  * execution platform in their name unless the build also requests keys without an override that
@@ -59,6 +52,9 @@
  * ConfiguredTarget that have the same ({@link Label}, {@link BuildConfigurationValue}) pair.
  *
  * <p>Note that this key may be used to look up the generating action of an artifact.
+ *
+ * <p>TODO(blaze-configurability-team): Consider just using BuildOptions over a
+ * BuildConfigurationKey.
  */
 @AutoCodec
 public class ConfiguredTargetKey implements ActionLookupKey {

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

@@ -237,7 +237,7 @@ private BuildConfigurationKey computeMapping(BuildConfigurationKey original)
       }
     }
 
-    return BuildConfigurationKey.withoutPlatformMapping(original.getFragments(), modifiedOptions);
+    return BuildConfigurationKey.withoutPlatformMapping(modifiedOptions);
   }
 
   private OptionsParsingResult parseWithCache(ImmutableSet<String> args)