From 7cd9e883dd31f54cd505844aa1f1e0ed7bd9f380 Mon Sep 17 00:00:00 2001 From: ccalvarin Date: Mon, 16 Oct 2017 22:18:32 +0200 Subject: Track Option placement within a priority category. An option has precedence over previous options at the same enum-valued priority. Track its placement in this ordering explicitly. This will allow after-the-fact expansion of expansion options such that they correctly take precedence or not compared to other mentions of the same flag. This is needed to fix --config's expansion. RELNOTES: None. PiperOrigin-RevId: 172367996 --- .../runtime/AllIncompatibleChangesExpansion.java | 2 +- .../build/lib/runtime/BlazeCommandDispatcher.java | 8 +- .../devtools/build/lib/runtime/BlazeRuntime.java | 6 +- .../build/lib/runtime/CommandLineEvent.java | 3 +- .../lib/runtime/commands/CoverageCommand.java | 8 +- .../lib/runtime/commands/ProjectFileSupport.java | 4 +- .../build/lib/runtime/commands/TestCommand.java | 5 +- .../mobileinstall/MobileInstallCommand.java | 6 +- .../common/options/InvocationPolicyEnforcer.java | 193 +++++++++------ .../devtools/common/options/OptionDefinition.java | 5 + .../devtools/common/options/OptionPriority.java | 118 ++++++--- .../common/options/OptionValueDescription.java | 53 ++-- .../google/devtools/common/options/Options.java | 2 +- .../devtools/common/options/OptionsData.java | 11 +- .../devtools/common/options/OptionsParser.java | 104 +++++--- .../devtools/common/options/OptionsParserImpl.java | 268 ++++++++++++--------- .../common/options/OptionsParsingException.java | 2 +- .../common/options/ParsedOptionDescription.java | 6 +- 18 files changed, 481 insertions(+), 323 deletions(-) (limited to 'src/main/java/com/google/devtools') diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java index cf81b22c6d..b6861545ec 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java @@ -97,7 +97,7 @@ public class AllIncompatibleChangesExpansion implements ExpansionFunction { * as this constitutes an internal error in the declaration of the option. */ private static void validateIncompatibleChange(OptionDefinition optionDefinition) { - String prefix = "Incompatible change option '--" + optionDefinition.getOptionName() + "' "; + String prefix = String.format("Incompatible change %s ", optionDefinition); // To avoid ambiguity, and the suggestion of using .isEmpty(). String defaultString = ""; 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 86ccfdb004..8f46581ab7 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 @@ -51,7 +51,7 @@ import com.google.devtools.build.lib.vfs.Path; 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; +import com.google.devtools.common.options.OptionPriority.PriorityCategory; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.OptionsProvider; @@ -219,8 +219,8 @@ public class BlazeCommandDispatcher { // Explicit command-line options: List cmdLineAfterCommand = args.subList(1, args.size()); - optionsParser.parseWithSourceFunction(OptionPriority.COMMAND_LINE, - commandOptionSourceFunction, cmdLineAfterCommand); + optionsParser.parseWithSourceFunction( + PriorityCategory.COMMAND_LINE, commandOptionSourceFunction, cmdLineAfterCommand); // Command-specific options from .blazerc passed in via --default_override // and --rc_source. A no-op if none are provided. @@ -818,7 +818,7 @@ public class BlazeCommandDispatcher { rcfileNotes.add(source + ":\n" + " " + inherited + "'" + commandToParse + "' options: " + Joiner.on(' ').join(rcfileOptions)); - optionsParser.parse(OptionPriority.RC_FILE, rcfile, rcfileOptions); + optionsParser.parse(PriorityCategory.RC_FILE, rcfile, rcfileOptions); } } 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 2bbdb48e38..29b9e45650 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 @@ -76,7 +76,7 @@ import com.google.devtools.build.lib.windows.WindowsSubprocessFactory; import com.google.devtools.common.options.CommandNameCache; import com.google.devtools.common.options.InvocationPolicyParser; import com.google.devtools.common.options.OptionDefinition; -import com.google.devtools.common.options.OptionPriority; +import com.google.devtools.common.options.OptionPriority.PriorityCategory; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsClassProvider; import com.google.devtools.common.options.OptionsParser; @@ -874,7 +874,7 @@ public final class BlazeRuntime { // First parse the command line so that we get the option_sources argument OptionsParser parser = OptionsParser.newOptionsParser(optionClasses); parser.setAllowResidue(false); - parser.parse(OptionPriority.COMMAND_LINE, null, args); + parser.parse(PriorityCategory.COMMAND_LINE, null, args); Map optionSources = parser.getOptions(BlazeServerStartupOptions.class).optionSources; Function sourceFunction = @@ -888,7 +888,7 @@ public final class BlazeRuntime { // Then parse the command line again, this time with the correct option sources parser = OptionsParser.newOptionsParser(optionClasses); parser.setAllowResidue(false); - parser.parseWithSourceFunction(OptionPriority.COMMAND_LINE, sourceFunction, args); + parser.parseWithSourceFunction(PriorityCategory.COMMAND_LINE, sourceFunction, args); return parser; } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandLineEvent.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandLineEvent.java index 313fb18948..154a6172fe 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandLineEvent.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandLineEvent.java @@ -248,7 +248,8 @@ public abstract class CommandLineEvent implements BuildEventWithOrderConstraint .stream() .filter( parsedOptionDescription -> - parsedOptionDescription.getPriority() == OptionPriority.COMMAND_LINE) + parsedOptionDescription.getPriority().getPriorityCategory() + == OptionPriority.PriorityCategory.COMMAND_LINE) .collect(Collectors.toList()); return CommandLineSection.newBuilder() .setSectionLabel("command options") diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java index b31bb240c6..c5abba2b7e 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CoverageCommand.java @@ -16,7 +16,7 @@ package com.google.devtools.build.lib.runtime.commands; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.packages.TestTimeout; import com.google.devtools.build.lib.runtime.Command; -import com.google.devtools.common.options.OptionPriority; +import com.google.devtools.common.options.OptionPriority.PriorityCategory; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; @@ -122,10 +122,12 @@ public class CoverageCommand extends TestCommand { public void editOptions(OptionsParser optionsParser) { super.editOptions(optionsParser); try { - optionsParser.parse(OptionPriority.SOFTWARE_REQUIREMENT, + optionsParser.parse( + PriorityCategory.SOFTWARE_REQUIREMENT, "Options required by the coverage command", ImmutableList.of("--collect_code_coverage")); - optionsParser.parse(OptionPriority.COMPUTED_DEFAULT, + optionsParser.parse( + PriorityCategory.COMPUTED_DEFAULT, "Options suggested for the coverage command", ImmutableList.of(TestTimeout.COVERAGE_CMD_TIMEOUT)); } catch (OptionsParsingException e) { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/ProjectFileSupport.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/ProjectFileSupport.java index dd43887930..98c76bdaaf 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/ProjectFileSupport.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/ProjectFileSupport.java @@ -22,7 +22,7 @@ import com.google.devtools.build.lib.runtime.CommonCommandOptions; import com.google.devtools.build.lib.runtime.ProjectFile; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.common.options.OptionPriority; +import com.google.devtools.common.options.OptionPriority.PriorityCategory; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.OptionsProvider; @@ -72,7 +72,7 @@ public final class ProjectFileSupport { eventHandler.handle(Event.info("Using " + projectFile.getName())); optionsParser.parse( - OptionPriority.RC_FILE, projectFile.getName(), projectFile.getCommandLineFor(command)); + PriorityCategory.RC_FILE, projectFile.getName(), projectFile.getCommandLineFor(command)); eventHandler.post(new GotProjectFileEvent(projectFile.getName())); } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java index 06cf1fefe4..60b35bf23b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java @@ -38,7 +38,7 @@ import com.google.devtools.build.lib.runtime.TestResultAnalyzer; import com.google.devtools.build.lib.runtime.TestResultNotifier; import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.util.io.AnsiTerminalPrinter; -import com.google.devtools.common.options.OptionPriority; +import com.google.devtools.common.options.OptionPriority.PriorityCategory; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.OptionsProvider; @@ -70,7 +70,8 @@ public class TestCommand implements BlazeCommand { TestOutputFormat testOutput = optionsParser.getOptions(ExecutionOptions.class).testOutput; try { if (testOutput == TestStrategy.TestOutputFormat.STREAMED) { - optionsParser.parse(OptionPriority.SOFTWARE_REQUIREMENT, + optionsParser.parse( + PriorityCategory.SOFTWARE_REQUIREMENT, "streamed output requires locally run tests, without sharding", ImmutableList.of("--test_sharding_strategy=disabled", "--test_strategy=exclusive")); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java index 8a83400c63..0249403160 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java @@ -42,7 +42,7 @@ import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; -import com.google.devtools.common.options.OptionPriority; +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; @@ -301,12 +301,12 @@ public class MobileInstallCommand implements BlazeCommand { ? "mobile_install_incremental" + INTERNAL_SUFFIX : "mobile_install_full" + INTERNAL_SUFFIX; optionsParser.parse( - OptionPriority.COMMAND_LINE, + PriorityCategory.COMMAND_LINE, "Options required by the mobile-install command", ImmutableList.of("--output_groups=" + outputGroup)); } else { optionsParser.parse( - OptionPriority.COMMAND_LINE, + PriorityCategory.COMMAND_LINE, "Options required by the skylark implementation of mobile-install command", ImmutableList.of( "--aspects=" + options.mobileInstallAspect + "%" + options.mode.getAspectName(), diff --git a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java index ee0e4878c7..37af8b5fdb 100644 --- a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java +++ b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.Fl import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.SetValue; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.UseDefault; +import com.google.devtools.common.options.OptionPriority.PriorityCategory; import com.google.devtools.common.options.OptionsParser.OptionDescription; import java.util.ArrayList; import java.util.Collections; @@ -35,7 +36,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.function.Function; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nullable; @@ -51,8 +51,6 @@ public final class InvocationPolicyEnforcer { private static final Logger logger = Logger.getLogger(InvocationPolicyEnforcer.class.getName()); private static final String INVOCATION_POLICY_SOURCE = "Invocation policy"; - private static final Function INVOCATION_POLICY_SOURCE_FUNCTION = - o -> INVOCATION_POLICY_SOURCE; @Nullable private final InvocationPolicy invocationPolicy; private final Level loglevel; @@ -82,10 +80,13 @@ public final class InvocationPolicyEnforcer { private static final class FlagPolicyWithContext { private final FlagPolicy policy; private final OptionDescription description; + private final OptionInstanceOrigin origin; - public FlagPolicyWithContext(FlagPolicy policy, OptionDescription description) { + public FlagPolicyWithContext( + FlagPolicy policy, OptionDescription description, OptionInstanceOrigin origin) { this.policy = policy; this.description = description; + this.origin = origin; } } @@ -160,6 +161,7 @@ public final class InvocationPolicyEnforcer { new FilterValueOperation.AllowValueOperation(loglevel); allowValueOperation.apply( parser, + flagPolicy.origin, allowValues.getAllowedValuesList(), allowValues.hasNewValue() ? allowValues.getNewValue() : null, allowValues.hasUseDefault(), @@ -173,6 +175,7 @@ public final class InvocationPolicyEnforcer { new FilterValueOperation.DisallowValueOperation(loglevel); disallowValueOperation.apply( parser, + flagPolicy.origin, disallowValues.getDisallowedValuesList(), disallowValues.hasNewValue() ? disallowValues.getNewValue() : null, disallowValues.hasUseDefault(), @@ -242,14 +245,22 @@ public final class InvocationPolicyEnforcer { // Expand all policies to transfer policies on expansion flags to policies on the child flags. List expandedPolicies = new ArrayList<>(); + OptionPriority nextPriority = + OptionPriority.lowestOptionPriorityAtCategory(PriorityCategory.INVOCATION_POLICY); for (FlagPolicy policy : invocationPolicy.getFlagPoliciesList()) { + // These policies are high-level, before expansion, and so are not the implicitDependents or + // expansions of any other flag, other than in an obtuse sense from --invocation_policy. + OptionPriority currentPriority = nextPriority; + OptionInstanceOrigin origin = + new OptionInstanceOrigin(currentPriority, INVOCATION_POLICY_SOURCE, null, null); + nextPriority = OptionPriority.nextOptionPriority(currentPriority); if (!policyApplies(policy, commandAndParentCommands)) { // Only keep and expand policies that are applicable to the current command. continue; } + OptionDescription optionDescription = - parser.getOptionDescription( - policy.getFlagName(), OptionPriority.INVOCATION_POLICY, INVOCATION_POLICY_SOURCE); + parser.getOptionDescription(policy.getFlagName(), origin); if (optionDescription == null) { // InvocationPolicy ignores policy on non-existing flags by design, for version // compatibility. @@ -261,8 +272,9 @@ public final class InvocationPolicyEnforcer { continue; } FlagPolicyWithContext policyWithContext = - new FlagPolicyWithContext(policy, optionDescription); - List policies = expandPolicy(policyWithContext, parser, loglevel); + new FlagPolicyWithContext(policy, optionDescription, origin); + List policies = + expandPolicy(policyWithContext, currentPriority, parser, loglevel); expandedPolicies.addAll(policies); } @@ -289,7 +301,8 @@ public final class InvocationPolicyEnforcer { } private static ImmutableList getExpansionsFromFlagPolicy( - FlagPolicyWithContext expansionPolicy, OptionsParser parser) throws OptionsParsingException { + FlagPolicyWithContext expansionPolicy, OptionPriority priority, OptionsParser parser) + throws OptionsParsingException { if (!expansionPolicy.description.isExpansion()) { return ImmutableList.of(); } @@ -311,7 +324,7 @@ public final class InvocationPolicyEnforcer { parser.getExpansionOptionValueDescriptions( expansionPolicy.description.getOptionDefinition(), value, - OptionPriority.INVOCATION_POLICY, + priority, INVOCATION_POLICY_SOURCE)); } } else { @@ -319,7 +332,7 @@ public final class InvocationPolicyEnforcer { parser.getExpansionOptionValueDescriptions( expansionPolicy.description.getOptionDefinition(), null, - OptionPriority.INVOCATION_POLICY, + priority, INVOCATION_POLICY_SOURCE)); } } @@ -329,7 +342,7 @@ public final class InvocationPolicyEnforcer { parser.getExpansionOptionValueDescriptions( expansionPolicy.description.getOptionDefinition(), null, - OptionPriority.INVOCATION_POLICY, + priority, INVOCATION_POLICY_SOURCE)); break; case ALLOW_VALUES: @@ -364,21 +377,50 @@ public final class InvocationPolicyEnforcer { *

None of the flagPolicies returned should be on expansion flags. */ private static List expandPolicy( - FlagPolicyWithContext originalPolicy, OptionsParser parser, Level loglevel) + FlagPolicyWithContext originalPolicy, + OptionPriority priority, + OptionsParser parser, + Level loglevel) throws OptionsParsingException { List expandedPolicies = new ArrayList<>(); - ImmutableList expansions = - getExpansionsFromFlagPolicy(originalPolicy, parser); - ImmutableList.Builder subflagBuilder = ImmutableList.builder(); - ImmutableList subflags = - subflagBuilder - .addAll(originalPolicy.description.getImplicitRequirements()) - .addAll(expansions) - .build(); + OptionInstanceOrigin originOfSubflags; + ImmutableList subflags; + if (originalPolicy.description.getOptionDefinition().hasImplicitRequirements()) { + originOfSubflags = + new OptionInstanceOrigin( + originalPolicy.origin.getPriority(), + String.format( + "implicitly required by %s (source: %s)", + originalPolicy.description.getOptionDefinition(), + originalPolicy.origin.getSource()), + originalPolicy.description.getOptionDefinition(), + null); + subflags = originalPolicy.description.getImplicitRequirements(); + } else if (originalPolicy.description.getOptionDefinition().isExpansionOption()) { + subflags = getExpansionsFromFlagPolicy(originalPolicy, priority, parser); + originOfSubflags = + new OptionInstanceOrigin( + originalPolicy.origin.getPriority(), + String.format( + "expanded by %s (source: %s)", + originalPolicy.description.getOptionDefinition(), + originalPolicy.origin.getSource()), + null, + originalPolicy.description.getOptionDefinition()); + } else { + return ImmutableList.of(originalPolicy); + } boolean isExpansion = originalPolicy.description.isExpansion(); + // We do not get to this point unless there are flags to expand to. + boolean hasFlagsToExpandTo = !subflags.isEmpty(); + Preconditions.checkArgument( + hasFlagsToExpandTo, + "The only policies that need expanding are those with expansions or implicit requirements, " + + "%s has neither yet was not returned as-is.", + originalPolicy.description.getOptionDefinition()); - if (!subflags.isEmpty() && logger.isLoggable(loglevel)) { + if (logger.isLoggable(loglevel)) { // Log the expansion. This is only really useful for understanding the invocation policy // itself. List subflagNames = new ArrayList<>(subflags.size()); @@ -409,9 +451,7 @@ public final class InvocationPolicyEnforcer { for (ParsedOptionDescription currentSubflag : subflags) { OptionDescription subflagOptionDescription = parser.getOptionDescription( - currentSubflag.getOptionDefinition().getOptionName(), - OptionPriority.INVOCATION_POLICY, - INVOCATION_POLICY_SOURCE); + currentSubflag.getOptionDefinition().getOptionName(), originOfSubflags); if (currentSubflag.getOptionDefinition().allowsMultiple() && originalPolicy.policy.getOperationCase().equals(OperationCase.SET_VALUE)) { @@ -421,7 +461,7 @@ public final class InvocationPolicyEnforcer { getSingleValueSubflagAsPolicy( subflagOptionDescription, currentSubflag, originalPolicy, isExpansion); // In case any of the expanded flags are themselves expansions, recurse. - expandedPolicies.addAll(expandPolicy(subflagAsPolicy, parser, loglevel)); + expandedPolicies.addAll(expandPolicy(subflagAsPolicy, priority, parser, loglevel)); } } @@ -434,7 +474,8 @@ public final class InvocationPolicyEnforcer { for (ParsedOptionDescription setValue : repeatableSubflagsInSetValues.get(repeatableFlag)) { newValues.add(setValue.getUnconvertedValue()); } - expandedPolicies.add(getSetValueSubflagAsPolicy(repeatableFlag, newValues, originalPolicy)); + expandedPolicies.add( + getSetValueSubflagAsPolicy(repeatableFlag, newValues, originOfSubflags, originalPolicy)); } // Don't add the original policy if it was an expansion flag, which have no value, but do add @@ -460,6 +501,7 @@ public final class InvocationPolicyEnforcer { private static FlagPolicyWithContext getSetValueSubflagAsPolicy( OptionDescription subflagDesc, List subflagValue, + OptionInstanceOrigin subflagOrigin, FlagPolicyWithContext originalPolicy) { // Some sanity checks. OptionDefinition subflag = subflagDesc.getOptionDefinition(); @@ -487,7 +529,8 @@ public final class InvocationPolicyEnforcer { .setFlagName(subflag.getOptionName()) .setSetValue(setValueExpansion) .build(), - subflagDesc); + subflagDesc, + subflagOrigin); } /** @@ -516,7 +559,9 @@ public final class InvocationPolicyEnforcer { } else { subflagValue = ImmutableList.of(currentSubflag.getUnconvertedValue()); } - subflagAsPolicy = getSetValueSubflagAsPolicy(subflagContext, subflagValue, originalPolicy); + subflagAsPolicy = + getSetValueSubflagAsPolicy( + subflagContext, subflagValue, currentSubflag.getOrigin(), originalPolicy); break; case USE_DEFAULT: @@ -528,7 +573,8 @@ public final class InvocationPolicyEnforcer { .setFlagName(currentSubflag.getOptionDefinition().getOptionName()) .setUseDefault(UseDefault.getDefaultInstance()) .build(), - subflagContext); + subflagContext, + currentSubflag.getOrigin()); break; case ALLOW_VALUES: @@ -582,8 +628,8 @@ public final class InvocationPolicyEnforcer { if (setValue.getFlagValueCount() == 0) { throw new OptionsParsingException( String.format( - "SetValue operation from invocation policy for flag '%s' does not have a value", - optionDefinition.getOptionName())); + "SetValue operation from invocation policy for %s does not have a value", + optionDefinition)); } // Flag must allow multiple values if multiple values are specified by the policy. @@ -591,9 +637,9 @@ public final class InvocationPolicyEnforcer { && !flagPolicy.description.getOptionDefinition().allowsMultiple()) { throw new OptionsParsingException( String.format( - "SetValue operation from invocation policy sets multiple values for flag '%s' which " + "SetValue operation from invocation policy sets multiple values for %s which " + "does not allow multiple values", - optionDefinition.getOptionName())); + optionDefinition)); } if (setValue.getOverridable() && valueDescription != null) { @@ -601,11 +647,11 @@ public final class InvocationPolicyEnforcer { // value. logInApplySetValueOperation( loglevel, - "Keeping value '%s' from source '%s' for flag '%s' " - + "because the invocation policy specifying the value(s) '%s' is overridable", + "Keeping value '%s' from source '%s' for %s because the invocation policy specifying " + + "the value(s) '%s' is overridable", valueDescription.getValue(), valueDescription.getSourceString(), - optionDefinition.getOptionName(), + optionDefinition, setValue.getFlagValueList()); } else { @@ -619,22 +665,23 @@ public final class InvocationPolicyEnforcer { if (valueDescription == null) { logInApplySetValueOperation( loglevel, - "Setting value for flag '%s' from invocation policy to '%s', overriding the " - + "default value '%s'", - optionDefinition.getOptionName(), + "Setting value for %s from invocation policy to '%s', overriding the default value " + + "'%s'", + optionDefinition, flagValue, optionDefinition.getDefaultValue()); } else { logInApplySetValueOperation( loglevel, - "Setting value for flag '%s' from invocation policy to '%s', overriding " - + "value '%s' from '%s'", - optionDefinition.getOptionName(), + "Setting value for %s from invocation policy to '%s', overriding value '%s' from " + + "'%s'", + optionDefinition, flagValue, valueDescription.getValue(), valueDescription.getSourceString()); } - setFlagValue(parser, optionDefinition, flagValue); + + parser.addOptionValueAtSpecificPriority(flagPolicy.origin, optionDefinition, flagValue); } } } @@ -708,6 +755,7 @@ public final class InvocationPolicyEnforcer { void apply( OptionsParser parser, + OptionInstanceOrigin origin, List policyValues, String newValue, boolean useDefault, @@ -746,11 +794,9 @@ public final class InvocationPolicyEnforcer { if (!defaultValueAllowed && useDefault) { throw new OptionsParsingException( String.format( - "%sValues policy disallows the default value '%s' for flag '%s' but also " - + "specifies to use the default value", - policyType, - optionDefinition.getDefaultValue(), - optionDefinition.getOptionName())); + "%sValues policy disallows the default value '%s' for %s but also specifies to " + + "use the default value", + policyType, optionDefinition.getDefaultValue(), optionDefinition)); } } @@ -760,10 +806,12 @@ public final class InvocationPolicyEnforcer { // the flag allowing multiple values, however, flags that allow multiple values cannot have // default values, and their value is always the empty list if they haven't been specified, // which is why new_default_value is not a repeated field. - checkDefaultValue(parser, optionDescription, policyValues, newValue, convertedPolicyValues); + checkDefaultValue( + parser, origin, optionDescription, policyValues, newValue, convertedPolicyValues); } else { checkUserValue( parser, + origin, optionDescription, valueDescription, policyValues, @@ -775,6 +823,7 @@ public final class InvocationPolicyEnforcer { void checkDefaultValue( OptionsParser parser, + OptionInstanceOrigin origin, OptionDescription optionDescription, List policyValues, String newValue, @@ -785,27 +834,27 @@ public final class InvocationPolicyEnforcer { if (!isFlagValueAllowed( convertedPolicyValues, optionDescription.getOptionDefinition().getDefaultValue())) { if (newValue != null) { - // Use the default value from the policy. + // Use the default value from the policy, since the original default is not allowed logger.log( loglevel, String.format( - "Overriding default value '%s' for flag '%s' with value '%s' specified by " - + "invocation policy. %sed values are: %s", + "Overriding default value '%s' for %s with value '%s' specified by invocation " + + "policy. %sed values are: %s", optionDefinition.getDefaultValue(), - optionDefinition.getOptionName(), + optionDefinition, newValue, policyType, policyValues)); parser.clearValue(optionDefinition); - setFlagValue(parser, optionDefinition, newValue); + parser.addOptionValueAtSpecificPriority(origin, optionDefinition, newValue); } else { // The operation disallows the default value, but doesn't supply a new value. throw new OptionsParsingException( String.format( - "Default flag value '%s' for flag '%s' is not allowed by invocation policy, but " + "Default flag value '%s' for %s is not allowed by invocation policy, but " + "the policy does not provide a new value. %sed values are: %s", optionDescription.getOptionDefinition().getDefaultValue(), - optionDefinition.getOptionName(), + optionDefinition, policyType, policyValues)); } @@ -814,6 +863,7 @@ public final class InvocationPolicyEnforcer { void checkUserValue( OptionsParser parser, + OptionInstanceOrigin origin, OptionDescription optionDescription, OptionValueDescription valueDescription, List policyValues, @@ -833,9 +883,9 @@ public final class InvocationPolicyEnforcer { } else { throw new OptionsParsingException( String.format( - "Flag value '%s' for flag '%s' is not allowed by invocation policy. " - + "%sed values are: %s", - value, option.getOptionName(), policyType, policyValues)); + "Flag value '%s' for %s is not allowed by invocation policy. %sed values " + + "are: %s", + value, option, policyType, policyValues)); } } } @@ -847,35 +897,22 @@ public final class InvocationPolicyEnforcer { logger.log( loglevel, String.format( - "Overriding disallowed value '%s' for flag '%s' with value '%s' " + "Overriding disallowed value '%s' for %s with value '%s' " + "specified by invocation policy. %sed values are: %s", - valueDescription.getValue(), - option.getOptionName(), - newValue, - policyType, - policyValues)); + valueDescription.getValue(), option, newValue, policyType, policyValues)); parser.clearValue(option); - setFlagValue(parser, option, newValue); + parser.addOptionValueAtSpecificPriority(origin, option, newValue); } else if (useDefault) { applyUseDefaultOperation(parser, policyType + "Values", option, loglevel); } else { throw new OptionsParsingException( String.format( - "Flag value '%s' for flag '%s' is not allowed by invocation policy and the " + "Flag value '%s' for %s is not allowed by invocation policy and the " + "policy does not specify a new value. %sed values are: %s", - valueDescription.getValue(), option.getOptionName(), policyType, policyValues)); + valueDescription.getValue(), option, policyType, policyValues)); } } } } } - - private static void setFlagValue(OptionsParser parser, OptionDefinition flag, String flagValue) - throws OptionsParsingException { - - parser.parseWithSourceFunction( - OptionPriority.INVOCATION_POLICY, - INVOCATION_POLICY_SOURCE_FUNCTION, - ImmutableList.of(String.format("--%s=%s", flag.getOptionName(), flagValue))); - } } diff --git a/src/main/java/com/google/devtools/common/options/OptionDefinition.java b/src/main/java/com/google/devtools/common/options/OptionDefinition.java index 40da044a48..1c01932ff3 100644 --- a/src/main/java/com/google/devtools/common/options/OptionDefinition.java +++ b/src/main/java/com/google/devtools/common/options/OptionDefinition.java @@ -303,6 +303,11 @@ public class OptionDefinition { return field.hashCode(); } + @Override + public String toString() { + return String.format("option '--%s'", getOptionName()); + } + static final Comparator BY_OPTION_NAME = Comparator.comparing(OptionDefinition::getOptionName); diff --git a/src/main/java/com/google/devtools/common/options/OptionPriority.java b/src/main/java/com/google/devtools/common/options/OptionPriority.java index a28f012822..96c471ed74 100644 --- a/src/main/java/com/google/devtools/common/options/OptionPriority.java +++ b/src/main/java/com/google/devtools/common/options/OptionPriority.java @@ -13,50 +13,98 @@ // limitations under the License. package com.google.devtools.common.options; +import java.util.Objects; + /** - * The priority of option values, in order of increasing priority. - * - *

In general, new values for options can only override values with a lower or - * equal priority. Option values provided in annotations in an options class are - * implicitly at the priority {@code DEFAULT}. + * The position of an option in the interpretation order. Options are interpreted using a + * last-option-wins system for single valued options, and are listed in that order for + * multiple-valued options. * - *

The ordering of the priorities is the source-code order. This is consistent - * with the automatically generated {@code compareTo} method as specified by the - * Java Language Specification. DO NOT change the source-code order of these - * values, or you will break code that relies on the ordering. + *

The position of the option is in category order, and within the priority category in index + * order. */ -public enum OptionPriority { +public class OptionPriority implements Comparable { + private final PriorityCategory priorityCategory; + private final int index; - /** - * The priority of values specified in the {@link Option} annotation. This - * should never be specified in calls to {@link OptionsParser#parse}. - */ - DEFAULT, + private OptionPriority(PriorityCategory priorityCategory, int index) { + this.priorityCategory = priorityCategory; + this.index = index; + } - /** - * Overrides default options at runtime, while still allowing the values to be - * overridden manually. - */ - COMPUTED_DEFAULT, + public static OptionPriority lowestOptionPriorityAtCategory(PriorityCategory category) { + return new OptionPriority(category, 0); + } - /** - * For options coming from a configuration file or rc file. - */ - RC_FILE, + public static OptionPriority nextOptionPriority(OptionPriority priority) { + return new OptionPriority(priority.priorityCategory, priority.index + 1); + } - /** - * For options coming from the command line. - */ - COMMAND_LINE, + public PriorityCategory getPriorityCategory() { + return priorityCategory; + } - /** - * For options coming from invocation policy. - */ - INVOCATION_POLICY, + @Override + public int compareTo(OptionPriority o) { + if (priorityCategory.equals(o.priorityCategory)) { + return index - o.index; + } + return priorityCategory.ordinal() - o.priorityCategory.ordinal(); + } + + @Override + public boolean equals(Object o) { + if (o instanceof OptionPriority) { + OptionPriority other = (OptionPriority) o; + return other.priorityCategory.equals(priorityCategory) && other.index == index; + } + return false; + } + + @Override + public int hashCode() { + return Objects.hash(priorityCategory, index); + } /** - * This priority can be used to unconditionally override any user-provided options. - * This should be used rarely and with caution! + * The priority of option values, in order of increasing priority. + * + *

In general, new values for options can only override values with a lower or equal priority. + * Option values provided in annotations in an options class are implicitly at the priority {@code + * DEFAULT}. + * + *

The ordering of the priorities is the source-code order. This is consistent with the + * automatically generated {@code compareTo} method as specified by the Java Language + * Specification. DO NOT change the source-code order of these values, or you will break code that + * relies on the ordering. */ - SOFTWARE_REQUIREMENT; + public enum PriorityCategory { + + /** + * The priority of values specified in the {@link Option} annotation. This should never be + * specified in calls to {@link OptionsParser#parse}. + */ + DEFAULT, + + /** + * Overrides default options at runtime, while still allowing the values to be overridden + * manually. + */ + COMPUTED_DEFAULT, + + /** For options coming from a configuration file or rc file. */ + RC_FILE, + + /** For options coming from the command line. */ + COMMAND_LINE, + + /** For options coming from invocation policy. */ + INVOCATION_POLICY, + + /** + * This priority can be used to unconditionally override any user-provided options. This should + * be used rarely and with caution! + */ + SOFTWARE_REQUIREMENT + } } 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 886e97e0ba..ed03e9776a 100644 --- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java +++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java @@ -18,9 +18,10 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ListMultimap; import com.google.devtools.common.options.OptionsParser.ConstructionException; -import java.util.ArrayList; import java.util.Collection; +import java.util.Comparator; import java.util.List; +import java.util.Map.Entry; import java.util.stream.Collectors; /** @@ -168,40 +169,34 @@ public abstract class OptionValueDescription { if (!implicitDependent.equals(optionThatDependsOnEffectiveValue)) { warnings.add( String.format( - "Option '%s' is implicitly defined by both option '%s' and option '%s'", - optionDefinition.getOptionName(), - optionThatDependsOnEffectiveValue.getOptionName(), - implicitDependent.getOptionName())); + "%s is implicitly defined by both %s and %s", + optionDefinition, optionThatDependsOnEffectiveValue, implicitDependent)); } } else if ((implicitDependent != null) && parsedOption.getPriority().equals(effectiveOptionInstance.getPriority())) { warnings.add( String.format( - "Option '%s' is implicitly defined by option '%s'; the implicitly set value " + "%s is implicitly defined by %s; the implicitly set value " + "overrides the previous one", - optionDefinition.getOptionName(), implicitDependent.getOptionName())); + optionDefinition, implicitDependent)); } else if (optionThatDependsOnEffectiveValue != null) { warnings.add( String.format( - "A new value for option '%s' overrides a previous implicit setting of that " - + "option by option '%s'", - optionDefinition.getOptionName(), - optionThatDependsOnEffectiveValue.getOptionName())); - } else if ((parsedOption.getPriority() == effectiveOptionInstance.getPriority()) + "A new value for %s overrides a previous implicit setting of that " + + "option by %s", + optionDefinition, optionThatDependsOnEffectiveValue)); + } else if ((parsedOption.getPriority().equals(effectiveOptionInstance.getPriority())) && ((optionThatExpandedToEffectiveValue == null) && (expandedFrom != null))) { // Create a warning if an expansion option overrides an explicit option: warnings.add( String.format( - "The option '%s' was expanded and now overrides a previous explicitly " - + "specified option '%s'", - expandedFrom.getOptionName(), optionDefinition.getOptionName())); + "%s was expanded and now overrides a previous explicitly specified %s", + expandedFrom, optionDefinition)); } else if ((optionThatExpandedToEffectiveValue != null) && (expandedFrom != null)) { warnings.add( String.format( - "The option '%s' was expanded to from both options '%s' and '%s'", - optionDefinition.getOptionName(), - optionThatExpandedToEffectiveValue.getOptionName(), - expandedFrom.getOptionName())); + "%s was expanded to from both %s and %s", + optionDefinition, optionThatExpandedToEffectiveValue, expandedFrom)); } } @@ -247,17 +242,15 @@ public abstract class OptionValueDescription { @Override public List getValue() { // Sort the results by option priority and return them in a new list. The generic type of - // the list is not known at runtime, so we can't use it here. It was already checked in - // the constructor, so this is type-safe. - List result = new ArrayList<>(); - for (OptionPriority priority : OptionPriority.values()) { - // If there is no mapping for this key, this check avoids object creation (because - // ListMultimap has to return a new object on get) and also an unnecessary addAll call. - if (optionValues.containsKey(priority)) { - result.addAll(optionValues.get(priority)); - } - } - return result; + // the list is not known at runtime, so we can't use it here. + return optionValues + .asMap() + .entrySet() + .stream() + .sorted(Comparator.comparing(Entry::getKey)) + .map(Entry::getValue) + .flatMap(Collection::stream) + .collect(Collectors.toList()); } @Override diff --git a/src/main/java/com/google/devtools/common/options/Options.java b/src/main/java/com/google/devtools/common/options/Options.java index b636c09532..0783480d9b 100644 --- a/src/main/java/com/google/devtools/common/options/Options.java +++ b/src/main/java/com/google/devtools/common/options/Options.java @@ -51,7 +51,7 @@ public class Options { public static Options parse(Class optionsClass, String... args) throws OptionsParsingException { OptionsParser parser = OptionsParser.newOptionsParser(optionsClass); - parser.parse(OptionPriority.COMMAND_LINE, null, Arrays.asList(args)); + parser.parse(OptionPriority.PriorityCategory.COMMAND_LINE, null, Arrays.asList(args)); List remainingArgs = parser.getResidue(); return new Options<>(parser.getOptions(optionsClass), remainingArgs.toArray(new String[0])); } 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 5b9a436337..69f360ad71 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsData.java +++ b/src/main/java/com/google/devtools/common/options/OptionsData.java @@ -61,10 +61,9 @@ final class OptionsData extends IsolatedOptionsData { context.getUnparsedValue() != null ? context.getUnparsedValue() : "(null)"; String name = context.getOptionDefinition().getOptionName(); throw new OptionsParsingException( - "Error expanding option '" - + name - + "': no expansions defined for value: " - + valueString, + String.format( + "Error expanding %s: no expansions defined for value: %s", + context.getOptionDefinition(), valueString), name); } return result; @@ -159,9 +158,7 @@ final class OptionsData extends IsolatedOptionsData { staticExpansion = instance.getExpansion(new ExpansionContext(isolatedData, optionDefinition, null)); Preconditions.checkState( - staticExpansion != null, - "Error calling expansion function for option: %s", - optionDefinition.getOptionName()); + staticExpansion != null, "Error calling expansion function for %s", optionDefinition); expansionDataBuilder.put(optionDefinition, new ExpansionData(staticExpansion)); } catch (ExpansionNeedsValueException e) { // This expansion function needs data that isn't available yet. Save the instance and call 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 25733c7179..48dc9f3974 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -56,9 +56,10 @@ import javax.annotation.Nullable; *

FooOptions and BarOptions would be options specification classes, derived from OptionsBase, * that contain fields annotated with @Option(...). * - *

Alternatively, rather than calling {@link #parseAndExitUponError(OptionPriority, String, - * String[])}, client code may call {@link #parse(OptionPriority,String,List)}, and handle parser - * exceptions usage messages themselves. + *

Alternatively, rather than calling {@link + * #parseAndExitUponError(OptionPriority.PriorityCategory, String, String[])}, client code may call + * {@link #parse(OptionPriority.PriorityCategory,String,List)}, and handle parser exceptions usage + * messages themselves. * *

This options parsing implementation has (at least) one design flaw. It allows both '--foo=baz' * and '--foo baz' for all options except void, boolean and tristate options. For these, the 'baz' @@ -216,15 +217,16 @@ public class OptionsParser implements OptionsProvider { } public void parseAndExitUponError(String[] args) { - parseAndExitUponError(OptionPriority.COMMAND_LINE, "unknown", args); + parseAndExitUponError(OptionPriority.PriorityCategory.COMMAND_LINE, "unknown", args); } /** - * A convenience function for use in main methods. Parses the command line - * parameters, and exits upon error. Also, prints out the usage message - * if "--help" appears anywhere within {@code args}. + * A convenience function for use in main methods. Parses the command line parameters, and exits + * upon error. Also, prints out the usage message if "--help" appears anywhere within {@code + * args}. */ - public void parseAndExitUponError(OptionPriority priority, String source, String[] args) { + public void parseAndExitUponError( + OptionPriority.PriorityCategory priority, String source, String[] args) { for (String arg : args) { if (arg.equals("--help")) { System.out.println( @@ -538,9 +540,9 @@ public class OptionsParser implements OptionsProvider { * @return The {@link OptionDescription} for the option, or null if there is no option by the * given name. */ - OptionDescription getOptionDescription(String name, OptionPriority priority, String source) + OptionDescription getOptionDescription(String name, OptionInstanceOrigin origin) throws OptionsParsingException { - return impl.getOptionDescription(name, priority, source); + return impl.getOptionDescription(name, origin); } /** @@ -558,9 +560,9 @@ public class OptionsParser implements OptionsProvider { /** * Returns a description of the option value set by the last previous call to {@link - * #parse(OptionPriority, String, List)} that successfully set the given option. If the option is - * of type {@link List}, the description will correspond to any one of the calls, but not - * necessarily the last. + * #parse(OptionPriority.PriorityCategory, String, List)} that successfully set the given option. + * If the option is of type {@link List}, the description will correspond to any one of the calls, + * but not necessarily the last. * * @return The {@link com.google.devtools.common.options.OptionValueDescription} for the option, * or null if the value has not been set. @@ -571,48 +573,64 @@ public class OptionsParser implements OptionsProvider { } /** - * A convenience method, equivalent to - * {@code parse(OptionPriority.COMMAND_LINE, null, Arrays.asList(args))}. + * A convenience method, equivalent to {@code parse(PriorityCategory.COMMAND_LINE, null, + * Arrays.asList(args))}. */ public void parse(String... args) throws OptionsParsingException { - parse(OptionPriority.COMMAND_LINE, null, Arrays.asList(args)); + parse(OptionPriority.PriorityCategory.COMMAND_LINE, null, Arrays.asList(args)); } /** - * A convenience method, equivalent to - * {@code parse(OptionPriority.COMMAND_LINE, null, args)}. + * A convenience method, equivalent to {@code parse(PriorityCategory.COMMAND_LINE, null, args)}. */ public void parse(List args) throws OptionsParsingException { - parse(OptionPriority.COMMAND_LINE, null, args); + parse(OptionPriority.PriorityCategory.COMMAND_LINE, null, args); } /** - * Parses {@code args}, using the classes registered with this parser. - * {@link #getOptions(Class)} and {@link #getResidue()} return the results. - * May be called multiple times; later options override existing ones if they - * have equal or higher priority. The source of options is a free-form string - * that can be used for debugging. Strings that cannot be parsed as options - * accumulates as residue, if this parser allows it. + * Parses {@code args}, using the classes registered with this parser, at the given priority. * - * @see OptionPriority - */ - public void parse(OptionPriority priority, String source, - List args) throws OptionsParsingException { + *

May be called multiple times; later options override existing ones if they have equal or + * higher priority. Strings that cannot be parsed as options are accumulated as residue, if this + * parser allows it. + * + *

{@link #getOptions(Class)} and {@link #getResidue()} will return the results. + * + * @param priority the priority at which to parse these options. Within this priority category, + * 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 source the source to track for each option parsed. + * @param args the arg list to parse. Each element might be an option, a value linked to an + * option, or residue. + */ + public void parse(OptionPriority.PriorityCategory priority, String source, List args) + throws OptionsParsingException { parseWithSourceFunction(priority, o -> source, args); } /** - * Parses {@code args}, using the classes registered with this parser. {@link #getOptions(Class)} - * and {@link #getResidue()} return the results. May be called multiple times; later options - * override existing ones if they have equal or higher priority. The source of options is given as - * a function that maps option names to the source of the option. Strings that cannot be parsed as - * options accumulates as* residue, if this parser allows it. + * Parses {@code args}, using the classes registered with this parser, at the given priority. + * + *

May be called multiple times; later options override existing ones if they have equal or + * higher priority. Strings that cannot be parsed as options are accumulated as residue, if this + * parser allows it. + * + *

{@link #getOptions(Class)} and {@link #getResidue()} will return the results. + * + * @param priority the priority at which to parse these options. Within this priority category, + * 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 args the arg list to parse. Each element might be an option, a value linked to an + * option, or residue. */ public void parseWithSourceFunction( - OptionPriority priority, Function sourceFunction, List args) + OptionPriority.PriorityCategory priority, + Function sourceFunction, + List args) throws OptionsParsingException { Preconditions.checkNotNull(priority); - Preconditions.checkArgument(priority != OptionPriority.DEFAULT); + Preconditions.checkArgument(priority != OptionPriority.PriorityCategory.DEFAULT); residue.addAll(impl.parse(priority, sourceFunction, args)); if (!allowResidue && !residue.isEmpty()) { String errorMsg = "Unrecognized arguments: " + Joiner.on(' ').join(residue); @@ -620,6 +638,20 @@ public class OptionsParser implements OptionsProvider { } } + /** + * @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 + * the provided value. If this option already has a value at this priority, this value will + * have precedence, but this should be avoided, as it breaks order tracking. + * @param option the option to add the value for. + * @param value the value to add at the given priority. + */ + void addOptionValueAtSpecificPriority( + OptionInstanceOrigin origin, OptionDefinition option, String value) + throws OptionsParsingException { + impl.addOptionValueAtSpecificPriority(origin, option, value); + } + /** * Clears the given option. * 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 f584991d14..96f4c16649 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterators; import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.Multimap; +import com.google.devtools.common.options.OptionPriority.PriorityCategory; import com.google.devtools.common.options.OptionsParser.OptionDescription; import java.lang.reflect.Constructor; import java.util.ArrayList; @@ -52,7 +53,8 @@ class OptionsParserImpl { * OptionDefinition("--port") -> 80 * * - * This map is modified by repeated calls to {@link #parse(OptionPriority,Function,List)}. + * This map is modified by repeated calls to {@link #parse(OptionPriority.PriorityCategory, + * Function,List)}. */ private final Map optionValues = new HashMap<>(); @@ -185,8 +187,9 @@ class OptionsParserImpl { } private void addDeprecationWarning(String optionName, String warning) { - warnings.add("Option '" + optionName + "' is deprecated" - + (warning.isEmpty() ? "" : ": " + warning)); + warnings.add( + String.format( + "Option '%s' is deprecated%s", optionName, (warning.isEmpty() ? "" : ": " + warning))); } @@ -205,7 +208,7 @@ class OptionsParserImpl { return optionValues.get(optionDefinition); } - OptionDescription getOptionDescription(String name, OptionPriority priority, String source) + OptionDescription getOptionDescription(String name, OptionInstanceOrigin origin) throws OptionsParsingException { OptionDefinition optionDefinition = optionsData.getOptionDefinitionFromName(name); if (optionDefinition == null) { @@ -218,16 +221,14 @@ class OptionsParserImpl { getImplicitDependentDescriptions( ImmutableList.copyOf(optionDefinition.getImplicitRequirements()), optionDefinition, - priority, - source)); + origin)); } /** @return A list of the descriptions corresponding to the implicit dependent flags passed in. */ private ImmutableList getImplicitDependentDescriptions( ImmutableList options, OptionDefinition implicitDependent, - OptionPriority priority, - String source) + OptionInstanceOrigin dependentsOrigin) throws OptionsParsingException { ImmutableList.Builder builder = ImmutableList.builder(); Iterator optionsIterator = options.iterator(); @@ -235,15 +236,15 @@ class OptionsParserImpl { Function sourceFunction = o -> String.format( - "implicitely required for option %s (source: %s)", - implicitDependent.getOptionName(), source); + "implicitely required for %s (source: %s)", + implicitDependent, dependentsOrigin.getSource()); while (optionsIterator.hasNext()) { String unparsedFlagExpression = optionsIterator.next(); ParsedOptionDescription parsedOption = identifyOptionAndPossibleArgument( unparsedFlagExpression, optionsIterator, - priority, + dependentsOrigin.getPriority(), sourceFunction, implicitDependent, null); @@ -267,7 +268,7 @@ class OptionsParserImpl { ImmutableList options = optionsData.getEvaluatedExpansion(expansionFlag, flagValue); Iterator optionsIterator = options.iterator(); Function sourceFunction = - o -> String.format("expanded from %s (source: %s)", expansionFlag.getOptionName(), source); + o -> String.format("expanded from %s (source: %s)", expansionFlag, source); while (optionsIterator.hasNext()) { String unparsedFlagExpression = optionsIterator.next(); ParsedOptionDescription parsedOption = @@ -293,13 +294,18 @@ class OptionsParserImpl { /** * Parses the args, and returns what it doesn't parse. May be called multiple times, and may be - * called recursively. In each call, there may be no duplicates, but separate calls may contain - * intersecting sets of options; in that case, the arg seen last takes precedence. + * called recursively. The option's definition dictates how it reacts to multiple settings. By + * default, the arg seen last at the highest priority takes precedence, overriding the early + * values. Options that accumulate multiple values will track them in priority and appearance + * order. */ List parse( - OptionPriority priority, Function sourceFunction, List args) + OptionPriority.PriorityCategory priority, + Function sourceFunction, + List args) throws OptionsParsingException { - return parse(priority, sourceFunction, null, null, args); + return parse( + OptionPriority.lowestOptionPriorityAtCategory(priority), sourceFunction, null, null, args); } /** @@ -307,8 +313,8 @@ class OptionsParserImpl { * called recursively. Calls may contain intersecting sets of options; in that case, the arg seen * last takes precedence. * - *

The method uses the invariant that if an option has neither an implicit dependent nor an - * expanded from value, then it must have been explicitly set. + *

The method treats options that have neither an implicitDependent nor an expandedFrom value + * as explicitly set. */ private List parse( OptionPriority priority, @@ -336,99 +342,7 @@ class OptionsParserImpl { ParsedOptionDescription parsedOption = identifyOptionAndPossibleArgument( arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom); - OptionDefinition optionDefinition = parsedOption.getOptionDefinition(); - // All options can be deprecated; check and warn before doing any option-type specific work. - maybeAddDeprecationWarning(optionDefinition); - - // Track the value, before any remaining option-type specific work that is done outside of - // the OptionValueDescription. - OptionValueDescription entry = - optionValues.computeIfAbsent( - optionDefinition, OptionValueDescription::createOptionValueDescription); - entry.addOptionInstance(parsedOption, warnings); - @Nullable String unconvertedValue = parsedOption.getUnconvertedValue(); - - // There are 3 types of flags that expand to other flag values. Expansion flags are the - // accepted way to do this, but two legacy features remain: implicit requirements and wrapper - // options. We rely on the OptionProcessor compile-time check's guarantee that no option sets - // multiple of these behaviors. (In Bazel, --config is another such flag, but that expansion - // is not controlled within the options parser, so we ignore it here) - - // As much as possible, we want the behaviors of these different types of flags to be - // identical, as this minimizes the number of edge cases, but we do not yet track these values - // in the same way. Wrapper options are replaced by their value and implicit requirements are - // hidden from the reported lists of parsed options. - if (implicitDependent == null && !optionDefinition.isWrapperOption()) { - // Log explicit options and expanded options in the order they are parsed (can be sorted - // later). This information is needed to correctly canonicalize flags. - parsedOptions.add(parsedOption); - if (optionDefinition.allowsMultiple()) { - canonicalizeValues.put(optionDefinition, parsedOption); - } else { - canonicalizeValues.replaceValues(optionDefinition, ImmutableList.of(parsedOption)); - } - } - - if (optionDefinition.isExpansionOption() - || optionDefinition.hasImplicitRequirements() - || optionDefinition.isWrapperOption()) { - StringBuilder sourceMessage = new StringBuilder(); - ImmutableList expansionArgs; - if (optionDefinition.isExpansionOption()) { - expansionArgs = optionsData.getEvaluatedExpansion(optionDefinition, unconvertedValue); - sourceMessage.append("expanded from option "); - } else if (optionDefinition.hasImplicitRequirements()) { - expansionArgs = ImmutableList.copyOf(optionDefinition.getImplicitRequirements()); - sourceMessage.append("implicit requirement of option "); - } else { - if (!unconvertedValue.startsWith("-")) { - // Wrapper options are the only "expansion" flag type that expand to other flags - // according to user input, so a malformed option is a user error in this case. - throw new OptionsParsingException( - "Invalid --" - + optionDefinition.getOptionName() - + " value format. " - + "You may have meant --" - + optionDefinition.getOptionName() - + "=--" - + unconvertedValue); - } else { - expansionArgs = ImmutableList.of(unconvertedValue); - sourceMessage.append("unwrapped from wrapper option "); - } - } - String sourceFunctionApplication = sourceFunction.apply(optionDefinition); - sourceMessage.append( - (sourceFunctionApplication == null) - ? String.format("--%s", optionDefinition.getOptionName()) - : String.format( - "--%s from %s", optionDefinition.getOptionName(), sourceFunctionApplication)); - - List unparsed = - parse( - priority, - o -> sourceMessage.toString(), - optionDefinition.hasImplicitRequirements() ? optionDefinition : null, - optionDefinition.isExpansionOption() ? optionDefinition : null, - expansionArgs); - if (!unparsed.isEmpty()) { - if (optionDefinition.isWrapperOption()) { - throw new OptionsParsingException( - "Unparsed options remain after unwrapping " - + arg - + ": " - + Joiner.on(' ').join(unparsed)); - } else { - // Throw an assertion here, because this indicates an error in the definition of this - // option's expansion or requirements, not with the input as provided by the user. - throw new AssertionError( - "Unparsed options remain after processing " - + arg - + ": " - + Joiner.on(' ').join(unparsed)); - } - } - } + handleNewParsedOption(parsedOption); } // Go through the final values and make sure they are valid values for their option. Unlike any @@ -441,6 +355,133 @@ class OptionsParserImpl { return unparsedArgs; } + /** + * Implementation of {@link OptionsParser#addOptionValueAtSpecificPriority(OptionInstanceOrigin, + * OptionDefinition, String)} + */ + void addOptionValueAtSpecificPriority( + OptionInstanceOrigin origin, OptionDefinition option, String unconvertedValue) + throws OptionsParsingException { + Preconditions.checkNotNull(option); + Preconditions.checkNotNull( + unconvertedValue, + "Cannot set %s to a null value. Pass \"\" if an empty value is required.", + option); + Preconditions.checkNotNull( + origin, + "Cannot assign value \'%s\' to %s without a clear origin for this value.", + unconvertedValue, + option); + PriorityCategory priorityCategory = origin.getPriority().getPriorityCategory(); + boolean isNotDefault = priorityCategory != OptionPriority.PriorityCategory.DEFAULT; + Preconditions.checkArgument( + isNotDefault, + "Attempt to assign value \'%s\' to %s at priority %s failed. Cannot set options at " + + "default priority - by definition, that means the option is unset.", + unconvertedValue, + option, + priorityCategory); + + handleNewParsedOption( + new ParsedOptionDescription( + option, + String.format("--%s=%s", option.getOptionName(), unconvertedValue), + unconvertedValue, + origin)); + } + + /** Takes care of tracking the parsed option's value in relation to other options. */ + private void handleNewParsedOption(ParsedOptionDescription parsedOption) + throws OptionsParsingException { + OptionDefinition optionDefinition = parsedOption.getOptionDefinition(); + // All options can be deprecated; check and warn before doing any option-type specific work. + maybeAddDeprecationWarning(optionDefinition); + // Track the value, before any remaining option-type specific work that is done outside of + // the OptionValueDescription. + OptionValueDescription entry = + optionValues.computeIfAbsent( + optionDefinition, OptionValueDescription::createOptionValueDescription); + entry.addOptionInstance(parsedOption, warnings); + @Nullable String unconvertedValue = parsedOption.getUnconvertedValue(); + + // There are 3 types of flags that expand to other flag values. Expansion flags are the + // accepted way to do this, but two legacy features remain: implicit requirements and wrapper + // options. We rely on the OptionProcessor compile-time check's guarantee that no option sets + // multiple of these behaviors. (In Bazel, --config is another such flag, but that expansion + // is not controlled within the options parser, so we ignore it here) + + // As much as possible, we want the behaviors of these different types of flags to be + // identical, as this minimizes the number of edge cases, but we do not yet track these values + // in the same way. Wrapper options are replaced by their value and implicit requirements are + // hidden from the reported lists of parsed options. + if (parsedOption.getImplicitDependent() == null && !optionDefinition.isWrapperOption()) { + // Log explicit options and expanded options in the order they are parsed (can be sorted + // later). This information is needed to correctly canonicalize flags. + parsedOptions.add(parsedOption); + if (optionDefinition.allowsMultiple()) { + canonicalizeValues.put(optionDefinition, parsedOption); + } else { + canonicalizeValues.replaceValues(optionDefinition, ImmutableList.of(parsedOption)); + } + } + + if (optionDefinition.isExpansionOption() + || optionDefinition.hasImplicitRequirements() + || optionDefinition.isWrapperOption()) { + StringBuilder sourceMessage = new StringBuilder(); + ImmutableList expansionArgs; + if (optionDefinition.isExpansionOption()) { + expansionArgs = optionsData.getEvaluatedExpansion(optionDefinition, unconvertedValue); + sourceMessage.append("expanded from option "); + } else if (optionDefinition.hasImplicitRequirements()) { + expansionArgs = ImmutableList.copyOf(optionDefinition.getImplicitRequirements()); + sourceMessage.append("implicit requirement of option "); + } else { + if (!unconvertedValue.startsWith("-")) { + // Wrapper options are the only "expansion" flag type that expand to other flags + // according to user input, so a malformed option is a user error in this case. + throw new OptionsParsingException( + String.format( + "Invalid value format for %s. You may have meant --%s=--%s", + optionDefinition, optionDefinition.getOptionName(), unconvertedValue)); + } else { + expansionArgs = ImmutableList.of(unconvertedValue); + sourceMessage.append("unwrapped from wrapper option "); + } + } + String source = parsedOption.getSource(); + sourceMessage.append( + (source == null) + ? String.format("--%s", optionDefinition.getOptionName()) + : String.format("--%s from %s", optionDefinition.getOptionName(), source)); + + List unparsed = + parse( + parsedOption.getPriority(), + o -> sourceMessage.toString(), + optionDefinition.hasImplicitRequirements() ? optionDefinition : null, + optionDefinition.isExpansionOption() ? optionDefinition : null, + expansionArgs); + if (!unparsed.isEmpty()) { + if (optionDefinition.isWrapperOption()) { + throw new OptionsParsingException( + "Unparsed options remain after unwrapping " + + unconvertedValue + + ": " + + Joiner.on(' ').join(unparsed)); + } else { + // Throw an assertion here, because this indicates an error in the definition of this + // option's expansion or requirements, not with the input as provided by the user. + throw new AssertionError( + "Unparsed options remain after processing " + + unconvertedValue + + ": " + + Joiner.on(' ').join(unparsed)); + } + } + } + } + private ParsedOptionDescription identifyOptionAndPossibleArgument( String arg, Iterator nextArgs, @@ -562,10 +603,7 @@ class OptionsParserImpl { optionDefinition.getField().set(optionsInstance, value); } catch (IllegalArgumentException e) { throw new IllegalStateException( - String.format( - "Unable to set option '%s' to value '%s'.", - optionDefinition.getOptionName(), value), - e); + String.format("Unable to set %s to value '%s'.", optionDefinition, value), e); } catch (IllegalAccessException e) { throw new IllegalStateException( "Could not set the field due to access issues. This is impossible, as the " diff --git a/src/main/java/com/google/devtools/common/options/OptionsParsingException.java b/src/main/java/com/google/devtools/common/options/OptionsParsingException.java index 73b48a02da..6b82366d65 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParsingException.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParsingException.java @@ -17,7 +17,7 @@ package com.google.devtools.common.options; /** * An exception that's thrown when the {@link OptionsParser} fails. * - * @see OptionsParser#parse(OptionPriority,String,java.util.List) + * @see OptionsParser#parse(OptionPriority.PriorityCategory,String,java.util.List) */ public class OptionsParsingException extends Exception { private final String invalidArgument; diff --git a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java index d5582635e3..f55f8addc7 100644 --- a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java +++ b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java @@ -115,6 +115,10 @@ public final class ParsedOptionDescription { return unconvertedValue; } + public OptionInstanceOrigin getOrigin() { + return origin; + } + public OptionPriority getPriority() { return origin.getPriority(); } @@ -149,7 +153,7 @@ public final class ParsedOptionDescription { @Override public String toString() { StringBuilder result = new StringBuilder(); - result.append("option '").append(optionDefinition.getOptionName()).append("' "); + result.append(optionDefinition); result.append("set to '").append(unconvertedValue).append("' "); result.append("with priority ").append(origin.getPriority()); if (origin.getSource() != null) { -- cgit v1.2.3