[7.4.0] Fail on inapplicable flags on non-common commands expanded fr…

…om configs (#23340)

This regressed in
44d3953.

Fixes #22980

Closes #23105.

PiperOrigin-RevId: 657276908
Change-Id: If2e88455a344082bfbec405c30c148c0d044adb6

Commit
9fbf427

Co-authored-by: Fabian Meumertzheim <[email protected]>

src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java

@@ -85,7 +85,7 @@ public final class BlazeOptionHandler {
 
   // All options set on this pseudo command are inherited by all commands, with unrecognized options
   // being ignored as long as they are recognized by at least one (other) command.
-  private static final String COMMON_PSEUDO_COMMAND = "common";
+  static final String COMMON_PSEUDO_COMMAND = "common";
 
   private static final String BUILD_COMMAND = "build";
   private static final ImmutableSet<String> BUILD_COMMAND_ANCESTORS =

src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java

@@ -23,6 +23,7 @@
 import com.google.devtools.common.options.OpaqueOptionsData;
 import com.google.devtools.common.options.OptionValueDescription;
 import com.google.devtools.common.options.OptionsParser;
+import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.ParsedOptionDescription;
 import java.util.ArrayList;
@@ -108,19 +109,19 @@ static void expandConfigOptions(
       // them, to preserve order.
       for (ParsedOptionDescription configInstance : configInstances) {
         String configValueToExpand = (String) configInstance.getConvertedValue();
-        List<String> expansion =
+        var expansion =
             getExpansion(
                 eventHandler,
                 commandToRcArgs,
                 commandsToParse,
                 configValueToExpand,
-                rcFileNotesConsumer);
+                rcFileNotesConsumer,
+                fallbackData);
         var ignoredArgs =
             optionsParser.parseArgsAsExpansionOfOption(
                 configInstance,
                 String.format("expanded from --config=%s", configValueToExpand),
-                expansion,
-                fallbackData);
+                expansion);
         if (!ignoredArgs.isEmpty()) {
           rcFileNotesConsumer.accept(
               String.format(
@@ -134,18 +135,25 @@ static void expandConfigOptions(
         optionsParser.getOptionValueDescription("enable_platform_specific_config");
     if (shouldEnablePlatformSpecificConfig(
         enablePlatformSpecificConfigDescription, commandToRcArgs, commandsToParse)) {
-      List<String> expansion =
+      var expansion =
           getExpansion(
               eventHandler,
               commandToRcArgs,
               commandsToParse,
               getPlatformName(),
-              rcFileNotesConsumer);
-      optionsParser.parseArgsAsExpansionOfOption(
-          Iterables.getOnlyElement(enablePlatformSpecificConfigDescription.getCanonicalInstances()),
-          String.format("enabled by --enable_platform_specific_config"),
-          expansion,
-          fallbackData);
+              rcFileNotesConsumer,
+              fallbackData);
+      ParsedOptionDescription optionToExpand =
+          Iterables.getOnlyElement(enablePlatformSpecificConfigDescription.getCanonicalInstances());
+      var ignoredArgs =
+          optionsParser.parseArgsAsExpansionOfOption(
+              optionToExpand, "enabled by --enable_platform_specific_config", expansion);
+      if (!ignoredArgs.isEmpty()) {
+        rcFileNotesConsumer.accept(
+            String.format(
+                "Ignored as unsupported by '%s': %s",
+                currentCommand, Joiner.on(' ').join(ignoredArgs)));
+      }
     }
 
     // At this point, we've expanded everything, identify duplicates, if any, to warn about
@@ -168,24 +176,26 @@ static void expandConfigOptions(
     }
   }
 
-  private static List<String> getExpansion(
+  private static List<ArgAndFallbackData> getExpansion(
       EventHandler eventHandler,
       ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
       List<String> commandsToParse,
       String configToExpand,
-      Consumer<String> rcFileNotesConsumer)
+      Consumer<String> rcFileNotesConsumer,
+      @Nullable OpaqueOptionsData fallbackData)
       throws OptionsParsingException {
     LinkedHashSet<String> configAncestorSet = new LinkedHashSet<>();
     configAncestorSet.add(configToExpand);
     List<String> longestChain = new ArrayList<>();
-    List<String> finalExpansion =
+    List<ArgAndFallbackData> finalExpansion =
         getExpansion(
             commandToRcArgs,
             commandsToParse,
             configAncestorSet,
             configToExpand,
             longestChain,
-            rcFileNotesConsumer);
+            rcFileNotesConsumer,
+            fallbackData);
 
     // In order to prevent warning about a long chain of 13 configs at the 10, 11, 12, and 13
     // point, we identify the longest chain for this 'high-level' --config found and only warn
@@ -212,15 +222,16 @@ private static List<String> getExpansion(
    *     should not be in the parents list of the second bar.
    * @param longestChain will be populated with the longest inheritance chain of configs.
    */
-  private static List<String> getExpansion(
+  private static List<ArgAndFallbackData> getExpansion(
       ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
       List<String> commandsToParse,
       LinkedHashSet<String> configAncestorSet,
       String configToExpand,
       List<String> longestChain,
-      Consumer<String> rcFileNotesConsumer)
+      Consumer<String> rcFileNotesConsumer,
+      @Nullable OpaqueOptionsData fallbackData)
       throws OptionsParsingException {
-    List<String> expansion = new ArrayList<>();
+    List<ArgAndFallbackData> 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.
@@ -236,7 +247,12 @@ private static List<String> getExpansion(
         // 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.getArgs()) {
-          expansion.add(arg);
+          expansion.add(
+              new ArgAndFallbackData(
+                  arg,
+                  commandToParse.equals(BlazeOptionHandler.COMMON_PSEUDO_COMMAND)
+                      ? fallbackData
+                      : null));
           if (arg.length() >= 8 && arg.substring(0, 8).equals("--config")) {
             // We have a config. Because we don't want to worry about formatting,
             // we will only accept --config=value, and will not accept value on a following line.
@@ -272,7 +288,8 @@ private static List<String> getExpansion(
                     extendedConfigAncestorSet,
                     newConfigValue,
                     longestChain,
-                    rcFileNotesConsumer));
+                    rcFileNotesConsumer,
+                    fallbackData));
           }
         }
       }

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

@@ -13,10 +13,11 @@
 // limitations under the License.
 package com.google.devtools.common.options;
 
+import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData;
 import java.util.List;
 
 /** Defines a preprocessing service for the "args" string list that is executed before parsing. */
 @FunctionalInterface
 interface ArgsPreProcessor {
-  List<String> preProcess(List<String> args) throws OptionsParsingException;
+  List<ArgAndFallbackData> preProcess(List<ArgAndFallbackData> args) throws OptionsParsingException;
 }

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

@@ -28,6 +28,7 @@
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Lists;
 import com.google.common.collect.MoreCollectors;
 import com.google.common.escape.Escaper;
 import com.google.devtools.build.lib.util.Pair;
@@ -709,7 +710,7 @@ public void parse(List<String> args) throws OptionsParsingException {
    */
   public void parse(OptionPriority.PriorityCategory priority, String source, List<String> args)
       throws OptionsParsingException {
-    parseWithSourceFunction(priority, o -> source, args, null);
+    parseWithSourceFunction(priority, o -> source, args, /* fallbackData= */ null);
   }
 
   /**
@@ -725,9 +726,6 @@ public void parse(OptionPriority.PriorityCategory priority, String source, List<
    *     each option will be given an index to track its position. If parse() has already been
    *     called at this priority, the indexing will continue where it left off, to keep ordering.
    * @param sourceFunction a function that maps option names to the source of the option.
-   * @param fallbackData if provided, the full collection of options that should be parsed and
-   *     ignored without raising an error if they are not recognized by the options classes
-   *     registered with this parser.
    * @param args the arg list to parse. Each element might be an option, a value linked to an
    *     option, or residue.
    * @return a list of options and values that were parsed but ignored due to only resolving against
@@ -743,7 +741,8 @@ public ImmutableList<String> parseWithSourceFunction(
     Preconditions.checkNotNull(priority);
     Preconditions.checkArgument(priority != OptionPriority.PriorityCategory.DEFAULT);
     OptionsParserImplResult optionsParserImplResult =
-        impl.parse(priority, sourceFunction, args, (OptionsData) fallbackData);
+        impl.parse(
+            priority, sourceFunction, ArgAndFallbackData.wrapWithFallbackData(args, fallbackData));
     addResidueFromResult(optionsParserImplResult);
     aliases.putAll(optionsParserImplResult.aliases);
     return optionsParserImplResult.ignoredArgs;
@@ -757,19 +756,15 @@ public ImmutableList<String> parseWithSourceFunction(
    *     will have the same priority as this option.
    * @param source a description of where the expansion arguments came from.
    * @param args the arguments to parse as the expansion. Order matters, as the value of a flag may
-   *     be in the following argument.
-   * @param fallbackData if provided, the full collection of options that should be parsed and
-   *     ignored without raising an error if they are not recognized by the options classes
-   *     registered with this parser.
+   *     be in the following argument. Each arg is optionally annotated with the full collection of
+   *     options that should be parsed and ignored without raising an error if they are not
+   *     recognized by the options classes registered with this parser.
    * @return a list of options and values that were parsed but ignored due to only resolving against
    *     the fallback data
    */
   @CanIgnoreReturnValue
   public ImmutableList<String> parseArgsAsExpansionOfOption(
-      ParsedOptionDescription optionToExpand,
-      String source,
-      List<String> args,
-      @Nullable OpaqueOptionsData fallbackData)
+      ParsedOptionDescription optionToExpand, String source, List<ArgAndFallbackData> args)
       throws OptionsParsingException {
     Preconditions.checkNotNull(
         optionToExpand, "Option for expansion not specified for arglist %s", args);
@@ -779,8 +774,7 @@ public ImmutableList<String> parseArgsAsExpansionOfOption(
         "Priority cannot be default, which was specified for arglist %s",
         args);
     OptionsParserImplResult optionsParserImplResult =
-        impl.parseArgsAsExpansionOfOption(
-            optionToExpand, o -> source, args, (OptionsData) fallbackData);
+        impl.parseArgsAsExpansionOfOption(optionToExpand, o -> source, args);
     addResidueFromResult(optionsParserImplResult);
     return optionsParserImplResult.ignoredArgs;
   }
@@ -935,4 +929,23 @@ public static boolean getUsesOnlyCoreTypes(Class<? extends OptionsBase> optionsC
     OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
     return data.getUsesOnlyCoreTypes(optionsClass);
   }
+
+  /**
+   * A container for an arg and associated options that should be silently ignored when parsed but
+   * not recognized by the current command.
+   */
+  public static final class ArgAndFallbackData {
+    public final String arg;
+    @Nullable final OptionsData fallbackData;
+
+    public ArgAndFallbackData(String arg, @Nullable OpaqueOptionsData fallbackData) {
+      this.arg = Preconditions.checkNotNull(arg);
+      this.fallbackData = (OptionsData) fallbackData;
+    }
+
+    public static List<ArgAndFallbackData> wrapWithFallbackData(
+        List<String> args, @Nullable OpaqueOptionsData fallbackData) {
+      return Lists.transform(args, arg -> new ArgAndFallbackData(arg, fallbackData));
+    }
+  }
 }

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

@@ -26,6 +26,7 @@
 import com.google.common.collect.Iterators;
 import com.google.devtools.common.options.OptionPriority.PriorityCategory;
 import com.google.devtools.common.options.OptionValueDescription.ExpansionBundle;
+import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData;
 import com.google.devtools.common.options.OptionsParser.OptionDescription;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.annotations.Keep;
@@ -452,17 +453,10 @@ List<String> getSkippedArgs() {
   OptionsParserImplResult parse(
       PriorityCategory priorityCat,
       Function<OptionDefinition, String> sourceFunction,
-      List<String> args,
-      OptionsData fallbackData)
+      List<ArgAndFallbackData> args)
       throws OptionsParsingException {
     OptionsParserImplResult optionsParserImplResult =
-        parse(
-            nextPriorityPerPriorityCategory.get(priorityCat),
-            sourceFunction,
-            null,
-            null,
-            args,
-            fallbackData);
+        parse(nextPriorityPerPriorityCategory.get(priorityCat), sourceFunction, null, null, args);
     nextPriorityPerPriorityCategory.put(priorityCat, optionsParserImplResult.nextPriority);
     return optionsParserImplResult;
   }
@@ -480,16 +474,19 @@ private OptionsParserImplResult parse(
       Function<OptionDefinition, String> sourceFunction,
       ParsedOptionDescription implicitDependent,
       ParsedOptionDescription expandedFrom,
-      List<String> args,
-      OptionsData fallbackData)
+      List<ArgAndFallbackData> args)
       throws OptionsParsingException {
     List<String> unparsedArgs = new ArrayList<>();
     List<String> unparsedPostDoubleDashArgs = new ArrayList<>();
     List<String> ignoredArgs = new ArrayList<>();
 
-    Iterator<String> argsIterator = argsPreProcessor.preProcess(args).iterator();
-    while (argsIterator.hasNext()) {
-      String arg = argsIterator.next();
+    Iterator<ArgAndFallbackData> argsAndFallbackDataIterator =
+        argsPreProcessor.preProcess(args).iterator();
+    Iterator<String> argsIterator = Iterators.transform(argsAndFallbackDataIterator, a -> a.arg);
+    while (argsAndFallbackDataIterator.hasNext()) {
+      ArgAndFallbackData argAndFallbackData = argsAndFallbackDataIterator.next();
+      String arg = argAndFallbackData.arg;
+      @Nullable OptionsData fallbackData = argAndFallbackData.fallbackData;
 
       if (!arg.startsWith("-")) {
         unparsedArgs.add(arg);
@@ -597,16 +594,14 @@ public List<String> getResidue() {
   OptionsParserImplResult parseArgsAsExpansionOfOption(
       ParsedOptionDescription optionToExpand,
       Function<OptionDefinition, String> sourceFunction,
-      List<String> args,
-      OptionsData fallbackData)
+      List<ArgAndFallbackData> args)
       throws OptionsParsingException {
     return parse(
         OptionPriority.getChildPriority(optionToExpand.getPriority()),
         sourceFunction,
         null,
         optionToExpand,
-        args,
-        fallbackData);
+        args);
   }
 
   /**
@@ -647,7 +642,8 @@ void setOptionValueAtSpecificPriorityWithoutExpansion(
   }
 
   /** Takes care of tracking the parsed option's value in relation to other options. */
-  private void handleNewParsedOption(ParsedOptionDescription parsedOption, OptionsData fallbackData)
+  private void handleNewParsedOption(
+      ParsedOptionDescription parsedOption, @Nullable OptionsData fallbackData)
       throws OptionsParsingException {
     OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
     ExpansionBundle expansionBundle = setOptionValue(parsedOption);
@@ -660,8 +656,7 @@ private void handleNewParsedOption(ParsedOptionDescription parsedOption, Options
               o -> expansionBundle.sourceOfExpansionArgs,
               optionDefinition.hasImplicitRequirements() ? parsedOption : null,
               optionDefinition.isExpansionOption() ? parsedOption : null,
-              expansionBundle.expansionArgs,
-              fallbackData);
+              ArgAndFallbackData.wrapWithFallbackData(expansionBundle.expansionArgs, fallbackData));
       if (!optionsParserImplResult.getResidue().isEmpty()) {
 
         // Throw an assertion here, because this indicates an error in the definition of this

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

@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.common.options;
 
+import com.google.common.collect.Lists;
+import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData;
 import java.io.IOException;
 import java.nio.file.FileSystem;
 import java.nio.file.Path;
@@ -49,18 +51,22 @@ public abstract class ParamsFilePreProcessor implements ArgsPreProcessor {
    * @throws OptionsParsingException if the path does not exist.
    */
   @Override
-  public List<String> preProcess(List<String> args) throws OptionsParsingException {
-    if (!args.isEmpty() && args.get(0).startsWith("@")) {
+  public List<ArgAndFallbackData> preProcess(List<ArgAndFallbackData> args)
+      throws OptionsParsingException {
+    if (!args.isEmpty() && args.get(0).arg.startsWith("@")) {
       if (args.size() > 1) {
         throw new OptionsParsingException(
-            String.format(TOO_MANY_ARGS_ERROR_MESSAGE_FORMAT, args), args.get(0));
+            String.format(
+                TOO_MANY_ARGS_ERROR_MESSAGE_FORMAT,
+                Lists.transform(args, argAndFallbackData -> argAndFallbackData.arg)),
+            args.get(0).arg);
       }
-      Path path = fs.getPath(args.get(0).substring(1));
+      Path path = fs.getPath(args.get(0).arg.substring(1));
       try {
-        return parse(path);
+        return ArgAndFallbackData.wrapWithFallbackData(parse(path), args.get(0).fallbackData);
       } catch (RuntimeException | IOException e) {
         throw new OptionsParsingException(
-            String.format(ERROR_MESSAGE_FORMAT, path, e.getMessage()), args.get(0), e);
+            String.format(ERROR_MESSAGE_FORMAT, path, e.getMessage()), args.get(0).arg, e);
       }
     }
     return args;