Change config expansion application order, gated by startup flag --expand_configs_in_place.

--config options were expanded in a fix-point expansion, where in practice, the flags that --config values expanded to ended up between the normal bazelrc options and the command line's explicit options. Since the options parser has an order-based priority scheme and it accepts multiple mentions of a single-valued option, this conflicts with users' expectations of being able to override these config expansions by using the order in which they are mentioned.

This change makes it possible to expand the config values defined in your bazelrc (or blazerc) files to occur in-place: --stuff --config=something --laterstuff will interpret the options that --config=something expands to as if they had been mentioned explicitly between --stuff and --laterstuff.

In order to not break users relying on complex flag combinations to configure their builds, this behavior will not yet be turned on by default. Instead, use --expand_configs_in_place as a startup flag to test this feature. --announce_rc may be helpful for debugging any differences between the fixed point and in-place expansions. Once you've debugged your problems, add "startup --expand_configs_in_place" to your blazerc to stick to the new behavior.

RELNOTES: Use --expand_configs_in_place as a startup argument to change the order in which --config expansions are interpreted.
PiperOrigin-RevId: 176371289

6 files changed, 191 insertions, 17 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index 2ed29f3613..7653fd892f 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -267,7 +267,11 @@ public class BlazeCommandDispatcher {
             commandAnnotation,
             // Provide the options parser so that we can cache OptionsData here.
             createOptionsParser(command),
-            invocationPolicy);
+            invocationPolicy,
+            runtime
+                .getStartupOptionsProvider()
+                .getOptions(BlazeServerStartupOptions.class)
+                .expandConfigsInPlace);
     ExitCode earlyExitCode = optionHandler.parseOptions(args, storedEventHandler);
     OptionsProvider options = optionHandler.getOptionsResult();
 
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
index 248ba4578b..a2ff946635 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
@@ -31,9 +31,11 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.common.options.InvocationPolicyEnforcer;
 import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionPriority.PriorityCategory;
+import com.google.devtools.common.options.OptionValueDescription;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.OptionsProvider;
+import com.google.devtools.common.options.ParsedOptionDescription;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -90,9 +92,15 @@ public abstract class BlazeOptionHandler {
       BlazeCommand command,
       Command commandAnnotation,
       OptionsParser optionsParser,
-      InvocationPolicy invocationPolicy) {
-    return new FixPointConfigExpansionRcHandler(
-        runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+      InvocationPolicy invocationPolicy,
+      boolean expandConfigsInPlace) {
+    if (expandConfigsInPlace) {
+      return new InPlaceConfigExpansionRcHandler(
+          runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+    } else {
+      return new FixPointConfigExpansionRcHandler(
+          runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+    }
   }
 
   // Return options as OptionsProvider so the options can't be easily modified after we've
@@ -321,7 +329,7 @@ public abstract class BlazeOptionHandler {
    * necessary, but this is not the case. This is why we are phasing out this behavior. We will
    * maintain the old behavior to avoid breaking users during a transition period.
    */
-  public static final class FixPointConfigExpansionRcHandler extends BlazeOptionHandler {
+  static final class FixPointConfigExpansionRcHandler extends BlazeOptionHandler {
 
     private FixPointConfigExpansionRcHandler(
         BlazeRuntime runtime,
@@ -401,6 +409,136 @@ public abstract class BlazeOptionHandler {
     }
   }
 
+  /**
+   * Expands the rc's options in {@code <rc options> <command line>} order, with config options
+   * expanded wherever the --config option was mentioned.
+   *
+   * <p>This requires a bit more care than the in-place expansion. We need to be wary of potential
+   * cycles in the definitions of config values, and should warn when a single --config expansion
+   * contradicts itself.
+   */
+  static final class InPlaceConfigExpansionRcHandler extends BlazeOptionHandler {
+
+    private InPlaceConfigExpansionRcHandler(
+        BlazeRuntime runtime,
+        BlazeWorkspace workspace,
+        BlazeCommand command,
+        Command commandAnnotation,
+        OptionsParser optionsParser,
+        InvocationPolicy invocationPolicy) {
+      super(runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+    }
+
+    @Override
+    void expandConfigOptions(
+        EventHandler eventHandler, ListMultimap<String, RcChunkOfArgs> commandToRcArgs)
+        throws OptionsParsingException {
+
+      OptionValueDescription configValueDescription =
+          optionsParser.getOptionValueDescription("config");
+      if (configValueDescription == null
+          || configValueDescription.getCanonicalInstances() == null) {
+        // No --config values were set, we can avoid this whole thing.
+        return;
+      }
+
+      // Find the base set of configs. This does not include the config options that might be
+      // recursively incuded.
+      ImmutableList<ParsedOptionDescription> configInstances =
+          ImmutableList.copyOf(configValueDescription.getCanonicalInstances());
+
+      // Expand the configs that are mentioned in the input. Flatten these expansions before parsing
+      // them, to preserve order.
+      for (ParsedOptionDescription configInstance : configInstances) {
+        String configValueToExpand = (String) configInstance.getConvertedValue();
+        List<String> expansion = getExpansion(eventHandler, commandToRcArgs, configValueToExpand);
+        optionsParser.parseOptionsFixedAtSpecificPriority(
+            configInstance.getPriority(),
+            String.format("expanded from --%s", configValueToExpand),
+            expansion);
+      }
+    }
+
+    private List<String> getExpansion(
+        EventHandler eventHandler,
+        ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
+        String configToExpand)
+        throws OptionsParsingException {
+      LinkedHashSet<String> configAncestorSet = new LinkedHashSet<>();
+      configAncestorSet.add(configToExpand);
+      return getExpansion(eventHandler, commandToRcArgs, configAncestorSet, configToExpand);
+    }
+
+    /**
+     * @param configAncestorSet is the chain of configs that have led to this one getting expanded.
+     *     This should only contain the configs that expanded, recursively, to this one, and should
+     *     not contain "siblings," as it is used to detect cycles. {@code build:foo --config=bar},
+     *     {@code build:bar --config=foo}, is a cycle, detected because this list will be [foo, bar]
+     *     when we find another 'foo' to expand. However, {@code build:foo --config=bar}, {@code
+     *     build:foo --config=bar} is not a cycle just because bar is expanded twice, and the 1st
+     *     bar should not be in the parents list of the second bar.
+     */
+    private List<String> getExpansion(
+        EventHandler eventHandler,
+        ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
+        LinkedHashSet<String> configAncestorSet,
+        String configToExpand)
+        throws OptionsParsingException {
+      List<String> expansion = new ArrayList<>();
+      boolean foundDefinition = false;
+      // The expansion order of rc files is first by command priority, and then in the order the
+      // rc files were read, respecting import statement placement.
+      for (String commandToParse : getCommandNamesToParse(commandAnnotation)) {
+        String configDef = commandToParse + ":" + configToExpand;
+        for (RcChunkOfArgs rcArgs : commandToRcArgs.get(configDef)) {
+          foundDefinition = true;
+          rcfileNotes.add(
+              String.format(
+                  "Found applicable config definition %s in file %s: %s",
+                  configDef, rcArgs.rcFile, String.join(" ", rcArgs.args)));
+
+          // For each arg in the rcARgs chunk, we first check if it is a config, and if so, expand
+          // it in place. We avoid cycles by tracking the parents of this config.
+          for (String arg : rcArgs.args) {
+            expansion.add(arg);
+            if (arg.length() >= 8 && arg.substring(0, 8).equals("--config")) {
+              // We have a config. For sanity, because we don't want to worry about formatting,
+              // we will only accept --config=value, and will not accept value on a following line.
+              int charOfConfigValue = arg.indexOf('=');
+              if (charOfConfigValue < 0) {
+                throw new OptionsParsingException("Config flag was provided without a value.");
+              }
+              String newConfigValue = arg.substring(charOfConfigValue + 1);
+              LinkedHashSet<String> extendedConfigAncestorSet =
+                  new LinkedHashSet<>(configAncestorSet);
+              if (!extendedConfigAncestorSet.add(newConfigValue)) {
+                throw new OptionsParsingException(
+                    String.format(
+                        "Config expansion has a cycle: config value %s expands to itself, "
+                            + "see inheritance chain %s",
+                        newConfigValue, extendedConfigAncestorSet));
+              }
+              expansion.addAll(
+                  getExpansion(
+                      eventHandler, commandToRcArgs, extendedConfigAncestorSet, newConfigValue));
+            }
+          }
+        }
+      }
+
+      if (!foundDefinition) {
+        String warning = "Config value " + configToExpand + " is not defined in any .rc file";
+        CommonCommandOptions commonOptions = optionsParser.getOptions(CommonCommandOptions.class);
+        if (commonOptions.allowUndefinedConfigs) {
+          eventHandler.handle(Event.warn(warning));
+        } else {
+          throw new OptionsParsingException(warning);
+        }
+      }
+      return expansion;
+    }
+  }
+
   private static List<String> getCommandNamesToParse(Command commandAnnotation) {
     List<String> result = new ArrayList<>();
     result.add("common");
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
index 6d96793d73..ce75d07d5c 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
@@ -408,4 +408,17 @@ public class BlazeServerStartupOptions extends OptionsBase {
     help = "The amount of time the client waits for each attempt to connect to the server"
   )
   public int connectTimeoutSecs;
+
+  @Option(
+    name = "expand_configs_in_place",
+    defaultValue = "false",
+    category = "server startup",
+    documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
+    effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, OptionEffectTag.CHANGES_INPUTS},
+    metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+    help =
+        "Changes the expansion of --config flags to be done in-place, as opposed to in a fixed "
+            + "point expansion between normal rc options and command-line specified options."
+  )
+  public boolean expandConfigsInPlace;
 }
diff --git a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
index 9864a46945..bb6f24266d 100644
--- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -81,7 +81,7 @@ public abstract class OptionValueDescription {
    * and is null.
    */
   @Nullable
-  abstract List<ParsedOptionDescription> getCanonicalInstances();
+  public abstract List<ParsedOptionDescription> getCanonicalInstances();
 
   /**
    * For the given option, returns the correct type of OptionValueDescription, to which unparsed
@@ -137,7 +137,7 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
+    public ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
       return null;
     }
   }
@@ -247,7 +247,7 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
+    public ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
       // If the current option is an implicit requirement, we don't need to list this value since
       // the parent implies it. In this case, it is sufficient to not list this value at all.
       if (effectiveOptionInstance.getImplicitDependent() == null) {
@@ -315,7 +315,7 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
+    public ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
       return parsedOptions
           .asMap()
           .entrySet()
@@ -375,7 +375,7 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
+    public ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
       // The options this expands to are incorporated in their own right - this option does
       // not have a canonical form.
       return ImmutableList.of();
@@ -464,7 +464,7 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
+    public ImmutableList<ParsedOptionDescription> getCanonicalInstances() {
       // No wrapper options get listed in the canonical form - the options they are wrapping will
       // be in the right place.
       return ImmutableList.of();
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index 4164e7c755..fb7161ca46 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -549,7 +549,7 @@ public class OptionsParser implements OptionsProvider {
    *     or null if the value has not been set.
    * @throws IllegalArgumentException if there is no option by the given name.
    */
-  OptionValueDescription getOptionValueDescription(String name) {
+  public OptionValueDescription getOptionValueDescription(String name) {
     return impl.getOptionValueDescription(name);
   }
 
@@ -619,6 +619,19 @@ public class OptionsParser implements OptionsProvider {
     }
   }
 
+  public void parseOptionsFixedAtSpecificPriority(
+      OptionPriority priority, String source, List<String> args) throws OptionsParsingException {
+    Preconditions.checkNotNull(priority, "Priority not specified for arglist " + args);
+    Preconditions.checkArgument(
+        priority.getPriorityCategory() != OptionPriority.PriorityCategory.DEFAULT,
+        "Priority cannot be default, which was specified for arglist " + args);
+    residue.addAll(impl.parseOptionsFixedAtSpecificPriority(priority, o -> source, args));
+    if (!allowResidue && !residue.isEmpty()) {
+      String errorMsg = "Unrecognized arguments: " + Joiner.on(' ').join(residue);
+      throw new OptionsParsingException(errorMsg);
+    }
+  }
+
   /**
    * @param origin the origin of this option instance, it includes the priority of the value. If
    *     other values have already been or will be parsed at a higher priority, they might override
@@ -652,9 +665,7 @@ public class OptionsParser implements OptionsProvider {
     return ImmutableList.copyOf(residue);
   }
 
-  /**
-   * Returns a list of warnings about problems encountered by previous parse calls.
-   */
+  /** Returns a list of warnings about problems encountered by previous parse calls. */
   public List<String> getWarnings() {
     return impl.getWarnings();
   }
@@ -792,8 +803,7 @@ public class OptionsParser implements OptionsProvider {
    * Option} annotation.
    */
   private static void validateFieldsSets(
-      Class<? extends OptionsBase> optionsClass,
-      LinkedHashSet<Field> fieldsFromMap) {
+      Class<? extends OptionsBase> optionsClass, LinkedHashSet<Field> fieldsFromMap) {
     ImmutableList<OptionDefinition> optionDefsFromClasses =
         OptionsData.getAllOptionDefinitionsForClass(optionsClass);
     Set<Field> fieldsFromClass =
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index d89aad302c..496927b7c3 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -284,6 +284,15 @@ class OptionsParserImpl {
     }
   }
 
+  /** Parses the args at the fixed priority. */
+  List<String> parseOptionsFixedAtSpecificPriority(
+      OptionPriority priority, Function<OptionDefinition, String> sourceFunction, List<String> args)
+      throws OptionsParsingException {
+    ResidueAndPriority residueAndPriority =
+        parse(OptionPriority.getLockedPriority(priority), sourceFunction, null, null, args);
+    return residueAndPriority.residue;
+  }
+
   /**
    * Parses the args, and returns what it doesn't parse. May be called multiple times, and may be
    * called recursively. Calls may contain intersecting sets of options; in that case, the arg seen