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 35ef818191765e..00b91b41b89ed9 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 @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.runtime; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.collect.ArrayListMultimap; @@ -33,8 +35,10 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.InvocationPolicyEnforcer; +import com.google.devtools.common.options.OpaqueOptionsData; import com.google.devtools.common.options.OptionDefinition; import com.google.devtools.common.options.OptionPriority.PriorityCategory; +import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.OptionsParsingResult; @@ -66,6 +70,8 @@ public final class BlazeOptionHandler { "client_env", "client_cwd"); + private static final String ALL_SUPPORTED_PSEUDO_COMMAND = "all-supported"; + // Marks an event to indicate a parsing error. static final String BAD_OPTION_TAG = "invalidOption"; // Separates the invalid tag from the full error message for easier parsing. @@ -78,6 +84,7 @@ public final class BlazeOptionHandler { private final Command commandAnnotation; private final InvocationPolicy invocationPolicy; private final List rcfileNotes = new ArrayList<>(); + private final List> allOptionsClasses; BlazeOptionHandler( BlazeRuntime runtime, @@ -92,6 +99,11 @@ public final class BlazeOptionHandler { this.commandAnnotation = commandAnnotation; this.optionsParser = optionsParser; this.invocationPolicy = invocationPolicy; + allOptionsClasses = runtime.getCommandMap().values().stream() + .map(BlazeCommand::getClass) + .flatMap(cmd -> BlazeCommandUtils.getOptions(cmd, runtime.getBlazeModules(), + runtime.getRuleClassProvider()).stream()) + .collect(toImmutableList()); } /** @@ -191,7 +203,14 @@ void parseRcOptions( "%s:\n %s'%s' options: %s", source, inherited, commandToParse, Joiner.on(' ').join(rcArgs.getArgs()))); } - optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs()); + if (commandToParse.equals(ALL_SUPPORTED_PSEUDO_COMMAND)) { + // Pass in options data for all commands supported by the runtime so that options that + // apply to some but not the current command can be ignored. + optionsParser.parseWithSourceFunction(PriorityCategory.RC_FILE, o -> rcArgs.getRcFile(), + rcArgs.getArgs(), OptionsParser.getFallbackOptionsData(allOptionsClasses)); + } else { + optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs()); + } } } } @@ -227,7 +246,8 @@ private void parseArgsAndConfigs(List args, ExtendedEventHandler eventHa optionsParser.parseWithSourceFunction( PriorityCategory.COMMAND_LINE, commandOptionSourceFunction, - defaultOverridesAndRcSources.build()); + defaultOverridesAndRcSources.build(), + null); // Command-specific options from .blazerc passed in via --default_override and --rc_source. ClientOptions rcFileOptions = optionsParser.getOptions(ClientOptions.class); @@ -241,7 +261,7 @@ private void parseArgsAndConfigs(List args, ExtendedEventHandler eventHa // Parses the remaining command-line options. optionsParser.parseWithSourceFunction( - PriorityCategory.COMMAND_LINE, commandOptionSourceFunction, remainingCmdLine.build()); + PriorityCategory.COMMAND_LINE, commandOptionSourceFunction, remainingCmdLine.build(), null); if (commandAnnotation.builds()) { // splits project files from targets in the traditional sense @@ -374,12 +394,14 @@ void expandConfigOptions( commandToRcArgs, getCommandNamesToParse(commandAnnotation), rcfileNotes::add, - optionsParser); + optionsParser, + OptionsParser.getFallbackOptionsData(allOptionsClasses)); } private static List getCommandNamesToParse(Command commandAnnotation) { List result = new ArrayList<>(); result.add("common"); + result.add(ALL_SUPPORTED_PSEUDO_COMMAND); getCommandNamesToParseHelper(commandAnnotation, result); return result; } @@ -470,7 +492,8 @@ static ListMultimap structureRcOptionsAndConfigs( if (index > 0) { command = command.substring(0, index); } - if (!validCommands.contains(command) && !command.equals("common")) { + if (!validCommands.contains(command) && !command.equals("common") && !command.equals( + ALL_SUPPORTED_PSEUDO_COMMAND)) { eventHandler.handle( Event.warn( "while reading option defaults file '" diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index d79a509bf990b7..076b5e10d66790 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java @@ -1113,7 +1113,7 @@ private static OptionsParsingResult parseStartupOptions( // Then parse the command line again, this time with the correct option sources parser = OptionsParser.builder().optionsClasses(optionClasses).allowResidue(false).build(); - parser.parseWithSourceFunction(PriorityCategory.COMMAND_LINE, sourceFunction, args); + parser.parseWithSourceFunction(PriorityCategory.COMMAND_LINE, sourceFunction, args, null); return parser; } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java b/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java index 998908a5102698..508319bad9f6d4 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java @@ -19,6 +19,7 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.util.OS; +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.OptionsParsingException; @@ -88,7 +89,8 @@ static void expandConfigOptions( ListMultimap commandToRcArgs, List commandsToParse, Consumer rcFileNotesConsumer, - OptionsParser optionsParser) + OptionsParser optionsParser, + OpaqueOptionsData fallbackData) throws OptionsParsingException { OptionValueDescription configValueDescription = @@ -113,7 +115,8 @@ static void expandConfigOptions( optionsParser.parseArgsAsExpansionOfOption( configInstance, String.format("expanded from --config=%s", configValueToExpand), - expansion); + expansion, + fallbackData); } } @@ -131,7 +134,8 @@ static void expandConfigOptions( optionsParser.parseArgsAsExpansionOfOption( Iterables.getOnlyElement(enablePlatformSpecificConfigDescription.getCanonicalInstances()), String.format("enabled by --enable_platform_specific_config"), - expansion); + expansion, + fallbackData); } // At this point, we've expanded everything, identify duplicates, if any, to warn about diff --git a/src/main/java/com/google/devtools/common/options/BUILD b/src/main/java/com/google/devtools/common/options/BUILD index 50d66920e93e15..b5120de838dc0c 100644 --- a/src/main/java/com/google/devtools/common/options/BUILD +++ b/src/main/java/com/google/devtools/common/options/BUILD @@ -49,6 +49,7 @@ java_library( ], ), deps = [ + "//src/main/java/com/google/devtools/build/lib/util:pair", "//third_party:auto_value", "//third_party:caffeine", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java index 1c7c8ecd172e8a..467477a9b7c3d3 100644 --- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java +++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java @@ -83,22 +83,22 @@ public static ImmutableList getAllOptionDefinitionsForClass( /** * Mapping from each options class to its no-arg constructor. Entries appear in the same order - * that they were passed to {@link #from(Collection)}. + * that they were passed to {@link #from(Collection, boolean)}. */ private final ImmutableMap, Constructor> optionsClasses; /** * Mapping from option name to {@code OptionDefinition}. Entries appear ordered first by their - * options class (the order in which they were passed to {@link #from(Collection)}, and then in - * alphabetic order within each options class. + * options class (the order in which they were passed to {@link #from(Collection, boolean)}, and + * then in alphabetic order within each options class. */ private final ImmutableMap nameToField; /** - * For options that have an "OldName", this is a mapping from old name to its corresponding {@code - * OptionDefinition}. Entries appear ordered first by their options class (the order in which they - * were passed to {@link #from(Collection)}, and then in alphabetic order within each options - * class. + * For options that have an "OldName", this is a mapping from old name to its corresponding + * {@code OptionDefinition}. Entries appear ordered first by their options class (the order in + * which they were passed to {@link #from(Collection, boolean)}, and then in alphabetic order + * within each options class. */ private final ImmutableMap oldNameToField; @@ -136,7 +136,7 @@ protected IsolatedOptionsData(IsolatedOptionsData other) { /** * Returns all options classes indexed by this options data object, in the order they were passed - * to {@link #from(Collection)}. + * to {@link #from(Collection, boolean)}. */ public Collection> getOptionsClasses() { return optionsClasses.keySet(); @@ -157,8 +157,8 @@ public OptionDefinition getOptionDefinitionFromName(String name) { /** * Returns all {@link OptionDefinition} objects loaded, mapped by their canonical names. Entries - * appear ordered first by their options class (the order in which they were passed to {@link - * #from(Collection)}, and then in alphabetic order within each options class. + * appear ordered first by their options class (the order in which they were passed to + * {@link #from(Collection, boolean)}, and then in alphabetic order within each options class. */ public Iterable> getAllOptionDefinitions() { return nameToField.entrySet(); @@ -177,9 +177,15 @@ public boolean getUsesOnlyCoreTypes(Class optionsClass) { * both single-character abbreviations and full names. */ private static void checkForCollisions( - Map aFieldMap, A optionName, String description) + Map aFieldMap, A optionName, OptionDefinition definition, + String description, boolean allowDuplicatesParsingEquivalently) throws DuplicateOptionDeclarationException { if (aFieldMap.containsKey(optionName)) { + OptionDefinition otherDefinition = aFieldMap.get(optionName); + if (allowDuplicatesParsingEquivalently && OptionsParserImpl.equivalentForParsing( + otherDefinition, definition)) { + return; + } throw new DuplicateOptionDeclarationException( "Duplicate option name, due to " + description + ": --" + optionName); } @@ -212,22 +218,27 @@ private static void checkAndUpdateBooleanAliases( Map nameToFieldMap, Map oldNameToFieldMap, Map booleanAliasMap, - String optionName) + String optionName, + OptionDefinition optionDefinition, + boolean allowDuplicatesParsingEquivalently) throws DuplicateOptionDeclarationException { // Check that the negating alias does not conflict with existing flags. - checkForCollisions(nameToFieldMap, "no" + optionName, "boolean option alias"); - checkForCollisions(oldNameToFieldMap, "no" + optionName, "boolean option alias"); + checkForCollisions(nameToFieldMap, "no" + optionName, optionDefinition, "boolean option alias", + allowDuplicatesParsingEquivalently); + checkForCollisions(oldNameToFieldMap, "no" + optionName, optionDefinition, + "boolean option alias", allowDuplicatesParsingEquivalently); // Record that the boolean option takes up additional namespace for its negating alias. booleanAliasMap.put("no" + optionName, optionName); } /** - * Constructs an {@link IsolatedOptionsData} object for a parser that knows about the given {@link - * OptionsBase} classes. No inter-option analysis is done. Performs basic validity checks on each - * option in isolation. + * Constructs an {@link IsolatedOptionsData} object for a parser that knows about the given + * {@link OptionsBase} classes. No inter-option analysis is done. Performs basic validity checks + * on each option in isolation. */ - static IsolatedOptionsData from(Collection> classes) { + static IsolatedOptionsData from(Collection> classes, + boolean allowDuplicatesParsingEquivalently) { // Mind which fields have to preserve order. Map, Constructor> constructorBuilder = new LinkedHashMap<>(); Map nameToFieldBuilder = new LinkedHashMap<>(); @@ -256,15 +267,18 @@ static IsolatedOptionsData from(Collection> classes for (OptionDefinition optionDefinition : optionDefinitions) { try { String optionName = optionDefinition.getOptionName(); - checkForCollisions(nameToFieldBuilder, optionName, "option name collision"); + checkForCollisions(nameToFieldBuilder, optionName, optionDefinition, + "option name collision", allowDuplicatesParsingEquivalently); checkForCollisions( oldNameToFieldBuilder, optionName, - "option name collision with another option's old name"); + optionDefinition, + "option name collision with another option's old name", + allowDuplicatesParsingEquivalently); checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option"); if (optionDefinition.usesBooleanValueSyntax()) { - checkAndUpdateBooleanAliases( - nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, optionName); + checkAndUpdateBooleanAliases(nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, + optionName, optionDefinition, allowDuplicatesParsingEquivalently); } nameToFieldBuilder.put(optionName, optionDefinition); @@ -273,23 +287,27 @@ static IsolatedOptionsData from(Collection> classes checkForCollisions( nameToFieldBuilder, oldName, - "old option name collision with another option's canonical name"); + optionDefinition, + "old option name collision with another option's canonical name", + allowDuplicatesParsingEquivalently); checkForCollisions( oldNameToFieldBuilder, oldName, - "old option name collision with another old option name"); + optionDefinition, + "old option name collision with another old option name", + allowDuplicatesParsingEquivalently); checkForBooleanAliasCollisions(booleanAliasMap, oldName, "old option name"); // If boolean, repeat the alias dance for the old name. if (optionDefinition.usesBooleanValueSyntax()) { - checkAndUpdateBooleanAliases( - nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, oldName); + checkAndUpdateBooleanAliases(nameToFieldBuilder, oldNameToFieldBuilder, + booleanAliasMap, oldName, optionDefinition, allowDuplicatesParsingEquivalently); } // Now that we've checked for conflicts, confidently store the old name. oldNameToFieldBuilder.put(oldName, optionDefinition); } if (optionDefinition.getAbbreviation() != '\0') { - checkForCollisions( - abbrevToFieldBuilder, optionDefinition.getAbbreviation(), "option abbreviation"); + checkForCollisions(abbrevToFieldBuilder, optionDefinition.getAbbreviation(), + optionDefinition, "option abbreviation", allowDuplicatesParsingEquivalently); abbrevToFieldBuilder.put(optionDefinition.getAbbreviation(), optionDefinition); } } catch (DuplicateOptionDeclarationException e) { diff --git a/src/main/java/com/google/devtools/common/options/OptionsData.java b/src/main/java/com/google/devtools/common/options/OptionsData.java index 2a3e485b7bf5dd..2cbf251b0a20c0 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsData.java +++ b/src/main/java/com/google/devtools/common/options/OptionsData.java @@ -28,14 +28,26 @@ @Immutable final class OptionsData extends IsolatedOptionsData { - /** Mapping from each option to the (unparsed) options it expands to, if any. */ + /** + * Mapping from each option to the (unparsed) options it expands to, if any. + */ private final ImmutableMap> evaluatedExpansions; - /** Construct {@link OptionsData} by extending an {@link IsolatedOptionsData} with new info. */ + /** + * Whether this options data has been created with duplicate options definitions allowed as long + * as those options are parsed (but not necessarily evaluated) equivalently. + */ + private final boolean allowDuplicatesParsingEquivalently; + + /** + * Construct {@link OptionsData} by extending an {@link IsolatedOptionsData} with new info. + */ private OptionsData( - IsolatedOptionsData base, Map> evaluatedExpansions) { + IsolatedOptionsData base, Map> evaluatedExpansions, + boolean allowDuplicatesParsingEquivalently) { super(base); this.evaluatedExpansions = ImmutableMap.copyOf(evaluatedExpansions); + this.allowDuplicatesParsingEquivalently = allowDuplicatesParsingEquivalently; } private static final ImmutableList EMPTY_EXPANSION = ImmutableList.of(); @@ -50,13 +62,23 @@ public ImmutableList getEvaluatedExpansion(OptionDefinition optionDefini } /** - * Constructs an {@link OptionsData} object for a parser that knows about the given {@link - * OptionsBase} classes. In addition to the work done to construct the {@link - * IsolatedOptionsData}, this also computes expansion information. If an option has an expansion, - * try to precalculate its here. + * Returns whether this options data has been created with duplicate options definitions allowed + * as long as those options are parsed (but not necessarily evaluated) equivalently. + */ + public boolean createdWithAllowDuplicatesParsingEquivalently() { + return allowDuplicatesParsingEquivalently; + } + + /** + * Constructs an {@link OptionsData} object for a parser that knows about the given + * {@link OptionsBase} classes. In addition to the work done to construct the + * {@link IsolatedOptionsData}, this also computes expansion information. If an option has an + * expansion, try to precalculate its here. */ - static OptionsData from(Collection> classes) { - IsolatedOptionsData isolatedData = IsolatedOptionsData.from(classes); + static OptionsData from(Collection> classes, + boolean allowDuplicatesParsingEquivalently) { + IsolatedOptionsData isolatedData = IsolatedOptionsData.from(classes, + allowDuplicatesParsingEquivalently); // All that's left is to compute expansions. ImmutableMap.Builder> evaluatedExpansionsBuilder = @@ -68,6 +90,7 @@ static OptionsData from(Collection> classes) { evaluatedExpansionsBuilder.put(optionDefinition, ImmutableList.copyOf(constExpansion)); } } - return new OptionsData(isolatedData, evaluatedExpansionsBuilder.build()); + return new OptionsData(isolatedData, evaluatedExpansionsBuilder.build(), + allowDuplicatesParsingEquivalently); } } 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 fdea7cc5d4bd08..a3db0b2abe380e 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -30,6 +30,7 @@ import com.google.common.collect.ListMultimap; import com.google.common.collect.MoreCollectors; import com.google.common.escape.Escaper; +import com.google.devtools.build.lib.util.Pair; import com.google.devtools.common.options.OptionsParserImpl.OptionsParserImplResult; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.ArrayList; @@ -103,8 +104,8 @@ public ConstructionException(String message, Throwable cause) { * cache is very unlikely to grow to a significant amount of memory, because there's only a fixed * set of options classes on the classpath. */ - private static final Map>, OptionsData> optionsData = - new HashMap<>(); + private static final Map>, Boolean>, OptionsData> + optionsData = new HashMap<>(); /** Skipped prefixes for starlark options. */ public static final ImmutableList STARLARK_SKIPPED_PREFIXES = @@ -120,30 +121,40 @@ public ConstructionException(String message, Throwable cause) { */ public static OpaqueOptionsData getOptionsData( List> optionsClasses) { - return getOptionsDataInternal(optionsClasses); + return getOptionsDataInternal(optionsClasses, false); } - /** Returns the {@link OptionsData} associated with the given list of options classes. */ - static synchronized OptionsData getOptionsDataInternal( + public static OpaqueOptionsData getFallbackOptionsData( List> optionsClasses) { + return getOptionsDataInternal(optionsClasses, true); + } + + /** + * Returns the {@link OptionsData} associated with the given list of options classes. + */ + static synchronized OptionsData getOptionsDataInternal( + List> optionsClasses, + boolean allowDuplicatesParsingEquivalently) { ImmutableList> immutableOptionsClasses = ImmutableList.copyOf(optionsClasses); - OptionsData result = optionsData.get(immutableOptionsClasses); + Pair>, Boolean> cacheKey = Pair.of( + immutableOptionsClasses, allowDuplicatesParsingEquivalently); + OptionsData result = optionsData.get(cacheKey); if (result == null) { try { - result = OptionsData.from(immutableOptionsClasses); + result = OptionsData.from(immutableOptionsClasses, allowDuplicatesParsingEquivalently); } catch (Exception e) { Throwables.throwIfInstanceOf(e, ConstructionException.class); throw new ConstructionException(e.getMessage(), e); } - optionsData.put(immutableOptionsClasses, result); + optionsData.put(cacheKey, result); } return result; } /** Returns the {@link OptionsData} associated with the given options class. */ static OptionsData getOptionsDataInternal(Class optionsClass) { - return getOptionsDataInternal(ImmutableList.of(optionsClass)); + return getOptionsDataInternal(ImmutableList.of(optionsClass), false); } /** A helper class to create new instances of {@link OptionsParser}. */ @@ -158,6 +169,7 @@ private Builder(OptionsParserImpl.Builder implBuilder) { /** Directly sets the {@link OptionsData} used by this parser. */ @CanIgnoreReturnValue public Builder optionsData(OptionsData optionsData) { + Preconditions.checkArgument(!optionsData.createdWithAllowDuplicatesParsingEquivalently()); this.implBuilder.optionsData(optionsData); return this; } @@ -173,7 +185,7 @@ public Builder optionsData(OpaqueOptionsData optionsData) { @SafeVarargs public final Builder optionsClasses(Class... optionsClasses) { return this.optionsData( - (OpaqueOptionsData) getOptionsDataInternal(ImmutableList.copyOf(optionsClasses))); + (OpaqueOptionsData) getOptionsDataInternal(ImmutableList.copyOf(optionsClasses), false)); } /** @@ -181,7 +193,7 @@ public final Builder optionsClasses(Class... optionsClass */ public Builder optionsClasses(Iterable> optionsClasses) { return this.optionsData( - (OpaqueOptionsData) getOptionsDataInternal(ImmutableList.copyOf(optionsClasses))); + (OpaqueOptionsData) getOptionsDataInternal(ImmutableList.copyOf(optionsClasses), false)); } /** @@ -699,7 +711,7 @@ public void parse(List args) throws OptionsParsingException { */ public void parse(OptionPriority.PriorityCategory priority, String source, List args) throws OptionsParsingException { - parseWithSourceFunction(priority, o -> source, args); + parseWithSourceFunction(priority, o -> source, args, null); } /** @@ -715,17 +727,22 @@ 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. */ public void parseWithSourceFunction( OptionPriority.PriorityCategory priority, Function sourceFunction, - List args) + List args, + @Nullable OpaqueOptionsData fallbackData) throws OptionsParsingException { Preconditions.checkNotNull(priority); Preconditions.checkArgument(priority != OptionPriority.PriorityCategory.DEFAULT); - OptionsParserImplResult optionsParserImplResult = impl.parse(priority, sourceFunction, args); + OptionsParserImplResult optionsParserImplResult = impl.parse(priority, sourceFunction, args, + (OptionsData) fallbackData); addResidueFromResult(optionsParserImplResult); aliases.putAll(optionsParserImplResult.aliases); } @@ -739,9 +756,13 @@ public void parseWithSourceFunction( * @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. */ public void parseArgsAsExpansionOfOption( - ParsedOptionDescription optionToExpand, String source, List args) + ParsedOptionDescription optionToExpand, String source, List args, + @Nullable OpaqueOptionsData fallbackData) throws OptionsParsingException { Preconditions.checkNotNull( optionToExpand, "Option for expansion not specified for arglist %s", args); @@ -750,8 +771,8 @@ public void parseArgsAsExpansionOfOption( != OptionPriority.PriorityCategory.DEFAULT, "Priority cannot be default, which was specified for arglist %s", args); - OptionsParserImplResult optionsParserImplResult = - impl.parseArgsAsExpansionOfOption(optionToExpand, o -> source, args); + OptionsParserImplResult optionsParserImplResult = impl.parseArgsAsExpansionOfOption( + optionToExpand, o -> source, args, (OptionsData) fallbackData); addResidueFromResult(optionsParserImplResult); } 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 d6a7bee4693db7..cae3259c6729e3 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -39,7 +39,9 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; +import java.util.function.BiFunction; import java.util.function.Function; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -406,15 +408,15 @@ ImmutableList getExpansionValueDescriptions( Iterator optionsIterator = options.iterator(); while (optionsIterator.hasNext()) { String unparsedFlagExpression = optionsIterator.next(); - ParsedOptionDescription parsedOption = - identifyOptionAndPossibleArgument( - unparsedFlagExpression, - optionsIterator, - nextOptionPriority, - o -> source, - implicitDependent, - expandedFrom); - builder.add(parsedOption); + identifyOptionAndPossibleArgument( + unparsedFlagExpression, + optionsIterator, + nextOptionPriority, + o -> source, + implicitDependent, + expandedFrom, + /* fallbackData */ null) + .ifPresent(builder::add); nextOptionPriority = OptionPriority.nextOptionPriority(nextOptionPriority); } return builder.build(); @@ -447,10 +449,12 @@ List getSkippedArgs() { OptionsParserImplResult parse( PriorityCategory priorityCat, Function sourceFunction, - List args) + List args, + OptionsData fallbackData) throws OptionsParsingException { - OptionsParserImplResult optionsParserImplResult = - parse(nextPriorityPerPriorityCategory.get(priorityCat), sourceFunction, null, null, args); + OptionsParserImplResult optionsParserImplResult = parse( + nextPriorityPerPriorityCategory.get(priorityCat), sourceFunction, null, null, args, + fallbackData); nextPriorityPerPriorityCategory.put(priorityCat, optionsParserImplResult.nextPriority); return optionsParserImplResult; } @@ -468,7 +472,8 @@ private OptionsParserImplResult parse( Function sourceFunction, ParsedOptionDescription implicitDependent, ParsedOptionDescription expandedFrom, - List args) + List args, + OptionsData fallbackData) throws OptionsParsingException { List unparsedArgs = new ArrayList<>(); List unparsedPostDoubleDashArgs = new ArrayList<>(); @@ -501,14 +506,14 @@ private OptionsParserImplResult parse( break; } - ParsedOptionDescription parsedOption; + Optional parsedOption; if (containsSkippedPrefix(arg)) { // Parse the skipped arg into a synthetic allowMultiple option to preserve its order // relative to skipped args coming from expansions. Simply adding it to the residue would // end up placing expanded skipped args after all explicitly given skipped args, which isn't // correct. parsedOption = - ParsedOptionDescription.newParsedOptionDescription( + Optional.of(ParsedOptionDescription.newParsedOptionDescription( skippedArgsDefinition, arg, arg, @@ -517,13 +522,14 @@ private OptionsParserImplResult parse( sourceFunction.apply(skippedArgsDefinition), implicitDependent, expandedFrom), - conversionContext); + conversionContext)); } else { - parsedOption = - identifyOptionAndPossibleArgument( - arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom); + parsedOption = identifyOptionAndPossibleArgument(arg, argsIterator, priority, + sourceFunction, implicitDependent, expandedFrom, fallbackData); + } + if (parsedOption.isPresent()) { + handleNewParsedOption(parsedOption.get()); } - handleNewParsedOption(parsedOption); priority = OptionPriority.nextOptionPriority(priority); } @@ -569,14 +575,16 @@ public List getResidue() { OptionsParserImplResult parseArgsAsExpansionOfOption( ParsedOptionDescription optionToExpand, Function sourceFunction, - List args) + List args, + OptionsData fallbackData) throws OptionsParsingException { return parse( OptionPriority.getChildPriority(optionToExpand.getPriority()), sourceFunction, null, optionToExpand, - args); + args, + fallbackData); } /** @@ -630,7 +638,8 @@ private void handleNewParsedOption(ParsedOptionDescription parsedOption) o -> expansionBundle.sourceOfExpansionArgs, optionDefinition.hasImplicitRequirements() ? parsedOption : null, optionDefinition.isExpansionOption() ? parsedOption : null, - expansionBundle.expansionArgs); + expansionBundle.expansionArgs, + null); if (!optionsParserImplResult.getResidue().isEmpty()) { // Throw an assertion here, because this indicates an error in the definition of this @@ -693,28 +702,34 @@ private ExpansionBundle setOptionValue(ParsedOptionDescription parsedOption) return expansionBundle; } - private ParsedOptionDescription identifyOptionAndPossibleArgument( + /** + * Keep the properties of OptionsData used below in sync with {@link #equivalentForParsing}. + */ + private Optional identifyOptionAndPossibleArgument( String arg, Iterator nextArgs, OptionPriority priority, Function sourceFunction, ParsedOptionDescription implicitDependent, - ParsedOptionDescription expandedFrom) + ParsedOptionDescription expandedFrom, + @Nullable OptionsData fallbackData) throws OptionsParsingException { // Store the way this option was parsed on the command line. StringBuilder commandLineForm = new StringBuilder(); commandLineForm.append(arg); String unconvertedValue = null; - OptionDefinition optionDefinition; + OptionLookupResult lookupResult; boolean booleanValue = true; if (arg.length() == 2) { // -l (may be nullary or unary) - optionDefinition = optionsData.getFieldForAbbrev(arg.charAt(1)); + lookupResult = getWithFallback(OptionsData::getFieldForAbbrev, arg.charAt(1), + fallbackData); booleanValue = true; } else if (arg.length() == 3 && arg.charAt(2) == '-') { // -l- (boolean) - optionDefinition = optionsData.getFieldForAbbrev(arg.charAt(1)); + lookupResult = getWithFallback(OptionsData::getFieldForAbbrev, arg.charAt(1), + fallbackData); booleanValue = false; } else if (arg.startsWith("--")) { // --long_option @@ -727,16 +742,17 @@ private ParsedOptionDescription identifyOptionAndPossibleArgument( throw new OptionsParsingException("Invalid options syntax: " + arg, arg); } unconvertedValue = equalsAt == -1 ? null : arg.substring(equalsAt + 1); - optionDefinition = optionsData.getOptionDefinitionFromName(name); + lookupResult = getWithFallback(OptionsData::getOptionDefinitionFromName, name, fallbackData); // Look for a "no"-prefixed option name: "no". - if (optionDefinition == null && name.startsWith("no")) { + if (lookupResult == null && name.startsWith("no")) { name = name.substring(2); - optionDefinition = optionsData.getOptionDefinitionFromName(name); + lookupResult = getWithFallback(OptionsData::getOptionDefinitionFromName, name, + fallbackData); booleanValue = false; - if (optionDefinition != null) { + if (lookupResult != null) { // TODO(bazel-team): Add tests for these cases. - if (!optionDefinition.usesBooleanValueSyntax()) { + if (!lookupResult.definition.usesBooleanValueSyntax()) { throw new OptionsParsingException( "Illegal use of 'no' prefix on non-boolean option: " + arg, arg); } @@ -751,16 +767,16 @@ private ParsedOptionDescription identifyOptionAndPossibleArgument( throw new OptionsParsingException("Invalid options syntax: " + arg, arg); } - if (optionDefinition == null || shouldIgnoreOption(optionDefinition)) { + if (lookupResult == null || shouldIgnoreOption(lookupResult.definition)) { // Do not recognize internal options, which are treated as if they did not exist. throw new OptionsParsingException("Unrecognized option: " + arg, arg); } if (unconvertedValue == null) { // Special-case boolean to supply value based on presence of "no" prefix. - if (optionDefinition.usesBooleanValueSyntax()) { + if (lookupResult.definition.usesBooleanValueSyntax()) { unconvertedValue = booleanValue ? "1" : "0"; - } else if (optionDefinition.getType().equals(Void.class)) { + } else if (lookupResult.definition.getType().equals(Void.class)) { // This is expected, Void type options have no args. } else if (nextArgs.hasNext()) { // "--flag value" form @@ -771,22 +787,69 @@ private ParsedOptionDescription identifyOptionAndPossibleArgument( } } - return ParsedOptionDescription.newParsedOptionDescription( - optionDefinition, + if (lookupResult.fromFallback) { + // The option was not found on the current command, but is a valid option for some other + // command. Ignore it. + return Optional.empty(); + } + + return Optional.of(ParsedOptionDescription.newParsedOptionDescription( + lookupResult.definition, commandLineForm.toString(), unconvertedValue, - new OptionInstanceOrigin( - priority, sourceFunction.apply(optionDefinition), implicitDependent, expandedFrom), - conversionContext); + new OptionInstanceOrigin(priority, sourceFunction.apply(lookupResult.definition), + implicitDependent, expandedFrom), + conversionContext)); + } + + /** + * Keep in sync with the properties of OptionsData used in + * {@link #identifyOptionAndPossibleArgument} + */ + public static boolean equivalentForParsing(OptionDefinition definition, + OptionDefinition otherDefinition) { + if (definition.equals(otherDefinition)) { + return true; + } + return (definition.usesBooleanValueSyntax() == otherDefinition.usesBooleanValueSyntax()) + && (definition.getType().equals(Void.class) == otherDefinition.getType().equals(Void.class)) + && (ImmutableList.copyOf(definition.getOptionMetadataTags()) + .contains(OptionMetadataTag.INTERNAL) == ImmutableList.copyOf( + otherDefinition.getOptionMetadataTags()).contains(OptionMetadataTag.INTERNAL)); + } + + private static final class OptionLookupResult { + + final OptionDefinition definition; + final boolean fromFallback; + + private OptionLookupResult(OptionDefinition definition, boolean fromFallback) { + this.definition = definition; + this.fromFallback = fromFallback; + } + } + + private OptionLookupResult getWithFallback( + BiFunction getter, T param, OptionsData fallbackData) { + OptionDefinition optionDefinition; + if ((optionDefinition = getter.apply(optionsData, param)) != null) { + return new OptionLookupResult(optionDefinition, false); + } + if (fallbackData != null && (optionDefinition = getter.apply(fallbackData, param)) != null) { + return new OptionLookupResult(optionDefinition, true); + } + return null; } private boolean shouldIgnoreOption(OptionDefinition optionDefinition) { return ignoreInternalOptions && ImmutableList.copyOf(optionDefinition.getOptionMetadataTags()) - .contains(OptionMetadataTag.INTERNAL); + .contains(OptionMetadataTag.INTERNAL); } - /** Gets the result of parsing the options. */ + /** + * Gets the result of parsing the options. + */ O getParsedOptions(Class optionsClass) { // Create the instance: O optionsInstance; diff --git a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java index ed922c0e6e87c1..776263e7a61019 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java @@ -33,15 +33,14 @@ public class OptionsDataTest { private static IsolatedOptionsData construct(Class optionsClass) throws OptionsParser.ConstructionException { - return IsolatedOptionsData.from(ImmutableList.>of(optionsClass)); + return IsolatedOptionsData.from(ImmutableList.of(optionsClass), false); } private static IsolatedOptionsData construct( Class optionsClass1, Class optionsClass2) throws OptionsParser.ConstructionException { - return IsolatedOptionsData.from( - ImmutableList.>of(optionsClass1, optionsClass2)); + return IsolatedOptionsData.from(ImmutableList.of(optionsClass1, optionsClass2), false); } private static IsolatedOptionsData construct( @@ -49,9 +48,8 @@ private static IsolatedOptionsData construct( Class optionsClass2, Class optionsClass3) throws OptionsParser.ConstructionException { - return IsolatedOptionsData.from( - ImmutableList.>of( - optionsClass1, optionsClass2, optionsClass3)); + return IsolatedOptionsData.from(ImmutableList.of(optionsClass1, optionsClass2, optionsClass3), + false); } /** Dummy options class. */ diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java index b6b63d9a6ece15..5927b1faf5be67 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -275,7 +275,8 @@ public void parseWithSourceFunctionThrowsExceptionIfResidueIsNotAllowed() { parser.parseWithSourceFunction( PriorityCategory.COMMAND_LINE, sourceFunction, - ImmutableList.of("residue", "not", "allowed", "in", "parseWithSource"))); + ImmutableList.of("residue", "not", "allowed", "in", "parseWithSource"), + null)); assertThat(e) .hasMessageThat() .isEqualTo("Unrecognized arguments: residue not allowed in parseWithSource"); @@ -295,7 +296,8 @@ public void parseWithSourceFunctionDoesntThrowExceptionIfResidueIsAllowed() thro parser.parseWithSourceFunction( PriorityCategory.COMMAND_LINE, sourceFunction, - ImmutableList.of("residue", "is", "allowed", "in", "parseWithSource")); + ImmutableList.of("residue", "is", "allowed", "in", "parseWithSource"), + null); assertThat(parser.getResidue()) .containsExactly("residue", "is", "allowed", "in", "parseWithSource"); } @@ -320,7 +322,8 @@ public void parseArgsAsExpansionOfOptionThrowsExceptionIfResidueIsNotAllowed() t parser.parseArgsAsExpansionOfOption( optionToExpand, "source", - ImmutableList.of("--underlying=direct_value", "residue", "in", "expansion"))); + ImmutableList.of("--underlying=direct_value", "residue", "in", "expansion"), + null)); assertThat(parser.getResidue()).isNotEmpty(); assertThat(e).hasMessageThat().isEqualTo("Unrecognized arguments: residue in expansion"); }