Fail on inapplicable flags on non-common commands expanded from configs

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

@@ -43,6 +43,7 @@
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.common.options.Converters;
 import com.google.devtools.common.options.InvocationPolicyEnforcer;
 import com.google.devtools.common.options.OptionAndRawValue;
@@ -87,7 +88,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 ImmutableSet<String> BUILD_COMMAND_ANCESTORS =
       ImmutableSet.of("build", COMMON_PSEUDO_COMMAND, ALWAYS_PSEUDO_COMMAND);

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

@@ -17,6 +17,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.util.OS;
@@ -108,15 +109,16 @@ 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);
         var ignoredArgs =
-            optionsParser.parseArgsAsExpansionOfOption(
+            parseArgsAsExpansionOfOptionPossiblyWithCommon(
+                optionsParser,
                 configInstance,
                 String.format("expanded from --config=%s", configValueToExpand),
                 expansion,
@@ -134,18 +136,27 @@ 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);
+      var ignoredArgs =
+          parseArgsAsExpansionOfOptionPossiblyWithCommon(
+              optionsParser,
+              Iterables.getOnlyElement(
+                  enablePlatformSpecificConfigDescription.getCanonicalInstances()),
+              String.format("enabled by --enable_platform_specific_config"),
+              expansion,
+              fallbackData);
+      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,7 +179,27 @@ static void expandConfigOptions(
     }
   }
 
-  private static List<String> getExpansion(
+  private static ImmutableList<String> parseArgsAsExpansionOfOptionPossiblyWithCommon(
+      OptionsParser parser,
+      ParsedOptionDescription optionToExpand,
+      String source,
+      List<ArgAndCommand> args,
+      @Nullable OpaqueOptionsData fallbackData)
+      throws OptionsParsingException {
+    return parser.parseArgsAsExpansionOfOption(
+        optionToExpand,
+        source,
+        Lists.transform(
+            args,
+            argAndCommand ->
+                new OptionsParser.ArgAndFallbackData(
+                    argAndCommand.arg,
+                    argAndCommand.command.equals(BlazeOptionHandler.COMMON_PSEUDO_COMMAND)
+                        ? fallbackData
+                        : null)));
+  }
+
+  private static List<ArgAndCommand> getExpansion(
       EventHandler eventHandler,
       ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
       List<String> commandsToParse,
@@ -178,7 +209,7 @@ private static List<String> getExpansion(
     LinkedHashSet<String> configAncestorSet = new LinkedHashSet<>();
     configAncestorSet.add(configToExpand);
     List<String> longestChain = new ArrayList<>();
-    List<String> finalExpansion =
+    List<ArgAndCommand> finalExpansion =
         getExpansion(
             commandToRcArgs,
             commandsToParse,
@@ -212,15 +243,15 @@ 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<ArgAndCommand> getExpansion(
       ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
       List<String> commandsToParse,
       LinkedHashSet<String> configAncestorSet,
       String configToExpand,
       List<String> longestChain,
       Consumer<String> rcFileNotesConsumer)
       throws OptionsParsingException {
-    List<String> expansion = new ArrayList<>();
+    List<ArgAndCommand> 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 +267,7 @@ 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 ArgAndCommand(arg, commandToParse));
           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.
@@ -284,4 +315,6 @@ private static List<String> getExpansion(
     }
     return expansion;
   }
+
+  private record ArgAndCommand(String arg, String command) {}
 }

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

@@ -18,5 +18,6 @@
 /** 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<OptionsParserImpl.ArgAndFallbackData> preProcess(
+      List<OptionsParserImpl.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;
@@ -665,7 +666,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);
   }
 
   /**
@@ -681,9 +682,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
@@ -699,7 +697,12 @@ 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,
+            Lists.transform(
+                args,
+                arg -> new OptionsParserImpl.ArgAndFallbackData(arg, (OptionsData) fallbackData)));
     addResidueFromResult(optionsParserImplResult);
     aliases.putAll(optionsParserImplResult.aliases);
     return optionsParserImplResult.ignoredArgs;
@@ -713,19 +716,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);
@@ -736,7 +735,13 @@ public ImmutableList<String> parseArgsAsExpansionOfOption(
         args);
     OptionsParserImplResult optionsParserImplResult =
         impl.parseArgsAsExpansionOfOption(
-            optionToExpand, o -> source, args, (OptionsData) fallbackData);
+            optionToExpand,
+            o -> source,
+            Lists.transform(
+                args,
+                argAndFallbackData ->
+                    new OptionsParserImpl.ArgAndFallbackData(
+                        argAndFallbackData.arg, (OptionsData) argAndFallbackData.fallbackData)));
     addResidueFromResult(optionsParserImplResult);
     return optionsParserImplResult.ignoredArgs;
   }
@@ -929,4 +934,18 @@ 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 public final OpaqueOptionsData fallbackData;
+
+    public ArgAndFallbackData(String arg, @Nullable OpaqueOptionsData fallbackData) {
+      this.arg = arg;
+      this.fallbackData = fallbackData;
+    }
+  }
 }

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

@@ -25,6 +25,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterators;
+import com.google.common.collect.Lists;
 import com.google.devtools.common.options.OptionPriority.PriorityCategory;
 import com.google.devtools.common.options.OptionValueDescription.ExpansionBundle;
 import com.google.devtools.common.options.OptionsParser.OptionDescription;
@@ -460,21 +461,24 @@ 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;
   }
 
+  static final class ArgAndFallbackData {
+    public final String arg;
+    @Nullable public final OptionsData fallbackData;
+
+    public ArgAndFallbackData(String arg, @Nullable OptionsData fallbackData) {
+      this.arg = arg;
+      this.fallbackData = fallbackData;
+    }
+  }
+
   /**
    * 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
@@ -488,16 +492,16 @@ 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();
+    Iterator<ArgAndFallbackData> argsIterator = argsPreProcessor.preProcess(args).iterator();
     while (argsIterator.hasNext()) {
-      String arg = argsIterator.next();
+      ArgAndFallbackData argAndFallbackData = argsIterator.next();
+      String arg = argAndFallbackData.arg;
 
       if (!arg.startsWith("-")) {
         unparsedArgs.add(arg);
@@ -519,7 +523,7 @@ private OptionsParserImplResult parse(
       arg = swapShorthandAlias(arg);
 
       if (arg.equals("--")) { // "--" means all remaining args aren't options
-        Iterators.addAll(unparsedPostDoubleDashArgs, argsIterator);
+        Iterators.addAll(unparsedPostDoubleDashArgs, Iterators.transform(argsIterator, a -> a.arg));
         break;
       }
 
@@ -545,17 +549,17 @@ private OptionsParserImplResult parse(
         ParsedOptionDescriptionOrIgnoredArgs result =
             identifyOptionAndPossibleArgument(
                 arg,
-                argsIterator,
+                Iterators.transform(argsIterator, a -> a.arg),
                 priority,
                 sourceFunction,
                 implicitDependent,
                 expandedFrom,
-                fallbackData);
+                argAndFallbackData.fallbackData);
         result.ignoredArgs.ifPresent(ignoredArgs::add);
         parsedOption = result.parsedOptionDescription;
       }
       if (parsedOption.isPresent()) {
-        handleNewParsedOption(parsedOption.get(), fallbackData);
+        handleNewParsedOption(parsedOption.get(), argAndFallbackData.fallbackData);
       }
       priority = OptionPriority.nextOptionPriority(priority);
     }
@@ -605,16 +609,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);
   }
 
   /**
@@ -655,7 +657,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);
@@ -668,8 +671,8 @@ private void handleNewParsedOption(ParsedOptionDescription parsedOption, Options
               o -> expansionBundle.sourceOfExpansionArgs,
               optionDefinition.hasImplicitRequirements() ? parsedOption : null,
               optionDefinition.isExpansionOption() ? parsedOption : null,
-              expansionBundle.expansionArgs,
-              fallbackData);
+              Lists.transform(
+                  expansionBundle.expansionArgs, arg -> new ArgAndFallbackData(arg, fallbackData)));
       if (!optionsParserImplResult.getResidue().isEmpty()) {
 
         // Throw an assertion here, because this indicates an error in the definition of this