Skip to content

Commit

Permalink
Consolidate the "unparsed" option value tracking.
Browse files Browse the repository at this point in the history 
In preparation for linking the parsed and unparsed values of options, consolidate and standardize our representation of the flag values as we received them (what is meant by "unparsed" values in this case). This was being done separately in ParseOptionResult, which, with extra context added, is being folded into UnparsedOptionValueDescription. We now track how an option was provided and where it came from for all option parsing.

RELNOTES: None.
PiperOrigin-RevId: 168682082
  • Loading branch information
@cvcal @philwo
cvcal authored and philwo committed Sep 14, 2017
1 parent 28d3d2a commit 5fe8e66
Show file tree
Hide file tree
Showing 10 changed files with 161 additions and 123 deletions.
14 changes: 8 additions & 6 deletions src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,19 @@ public static String asShellEscapedString(Iterable<UnparsedOptionValueDescriptio
if (result.length() != 0) {
result.append(' ');
}
String value = option.getUnparsedValue();
String value = option.getUnconvertedValue();
if (option.isBooleanOption()) {
boolean isEnabled = false;
try {
isEnabled = new Converters.BooleanConverter().convert(value);
} catch (OptionsParsingException e) {
throw new RuntimeException("Unexpected parsing exception", e);
}
result.append(isEnabled ? "--" : "--no").append(option.getName());
result
.append(isEnabled ? "--" : "--no")
.append(option.getOptionDefinition().getOptionName());
} else {
result.append("--").append(option.getName());
result.append("--").append(option.getOptionDefinition().getOptionName());
if (value != null) { // Can be null for Void options.
result.append("=").append(ShellEscaper.escapeString(value));
}
Expand All @@ -80,17 +82,17 @@ public static List<String> asArgumentList(Iterable<UnparsedOptionValueDescriptio
if (option.isHidden()) {
continue;
}
String value = option.getUnparsedValue();
String value = option.getUnconvertedValue();
if (option.isBooleanOption()) {
boolean isEnabled = false;
try {
isEnabled = new Converters.BooleanConverter().convert(value);
} catch (OptionsParsingException e) {
throw new RuntimeException("Unexpected parsing exception", e);
}
builder.add((isEnabled ? "--" : "--no") + option.getName());
builder.add((isEnabled ? "--" : "--no") + option.getOptionDefinition().getOptionName());
} else {
String optionString = "--" + option.getName();
String optionString = "--" + option.getOptionDefinition().getOptionName();
if (value != null) { // Can be null for Void options.
optionString += "=" + value;
}
Expand Down
38 changes: 27 additions & 11 deletions src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ public final class InvocationPolicyEnforcer {

private static final Logger logger = Logger.getLogger(InvocationPolicyEnforcer.class.getName());

private static final Function<OptionDefinition, String> INVOCATION_POLICY_SOURCE =
o -> "Invocation policy";

private static final String INVOCATION_POLICY_SOURCE = "Invocation policy";
private static final Function<OptionDefinition, String> INVOCATION_POLICY_SOURCE_FUNCTION =
o -> INVOCATION_POLICY_SOURCE;
@Nullable private final InvocationPolicy invocationPolicy;

/**
Expand Down Expand Up @@ -114,8 +114,10 @@ public void enforce(OptionsParser parser, @Nullable String command)
continue;
}

OptionDescription optionDescription = parser.getOptionDescription(flagName);
// extractOptionDefinition() will return null if the option does not exist, however
OptionDescription optionDescription =
parser.getOptionDescription(
flagName, OptionPriority.INVOCATION_POLICY, INVOCATION_POLICY_SOURCE);
// getOptionDescription() will return null if the option does not exist, however
// getOptionValueDescription() above would have thrown an IllegalArgumentException if that
// were the case.
Verify.verifyNotNull(optionDescription);
Expand Down Expand Up @@ -255,19 +257,28 @@ private static ImmutableList<OptionValueDescription> getExpansionsFromFlagPolicy
for (String value : setValue.getFlagValueList()) {
resultsBuilder.addAll(
parser.getExpansionOptionValueDescriptions(
optionDescription.getOptionDefinition(), value));
optionDescription.getOptionDefinition(),
value,
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE));
}
} else {
resultsBuilder.addAll(
parser.getExpansionOptionValueDescriptions(
optionDescription.getOptionDefinition(), null));
optionDescription.getOptionDefinition(),
null,
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE));
}
}
break;
case USE_DEFAULT:
resultsBuilder.addAll(
parser.getExpansionOptionValueDescriptions(
optionDescription.getOptionDefinition(), null));
optionDescription.getOptionDefinition(),
null,
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE));
break;
case ALLOW_VALUES:
// All expansions originally given to the parser have been expanded by now, so these two
Expand Down Expand Up @@ -307,7 +318,10 @@ private static List<FlagPolicy> expandPolicy(
List<FlagPolicy> expandedPolicies = new ArrayList<>();

OptionDescription originalOptionDescription =
parser.getOptionDescription(originalPolicy.getFlagName());
parser.getOptionDescription(
originalPolicy.getFlagName(),
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE);
if (originalOptionDescription == null) {
// InvocationPolicy ignores policy on non-existing flags by design, for version compatibility.
return expandedPolicies;
Expand Down Expand Up @@ -574,7 +588,9 @@ private static void applyUseDefaultOperation(
String originalValue = clearedValueDescription.getValue().toString();
String source = clearedValueDescription.getSource();

OptionDescription desc = parser.getOptionDescription(clearedFlagName);
OptionDescription desc =
parser.getOptionDescription(
clearedFlagName, OptionPriority.INVOCATION_POLICY, INVOCATION_POLICY_SOURCE);
Object clearedFlagDefaultValue = null;
if (desc != null) {
clearedFlagDefaultValue = desc.getOptionDefinition().getDefaultValue();
Expand Down Expand Up @@ -794,7 +810,7 @@ private static void setFlagValue(OptionsParser parser, OptionDefinition flag, St

parser.parseWithSourceFunction(
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE,
INVOCATION_POLICY_SOURCE_FUNCTION,
ImmutableList.of(String.format("--%s=%s", flag.getOptionName(), flagValue)));
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes
optionName,
"option name collision with another option's old name");
checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option");
if (optionDefinition.isBooleanField()) {
if (optionDefinition.usesBooleanValueSyntax()) {
checkAndUpdateBooleanAliases(
nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, optionName);
}
Expand All @@ -277,7 +277,7 @@ static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes
"old option name collision with another old option name");
checkForBooleanAliasCollisions(booleanAliasMap, oldName, "old option name");
// If boolean, repeat the alias dance for the old name.
if (optionDefinition.isBooleanField()) {
if (optionDefinition.usesBooleanValueSyntax()) {
checkAndUpdateBooleanAliases(
nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, oldName);
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/common/options/OptionDefinition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ Type getFieldSingularType() {
*
* <p>Memoizes the converter-finding logic to avoid repeating the computation.
*/
Converter<?> getConverter() {
public Converter<?> getConverter() {
if (converter != null) {
return converter;
}
Expand Down Expand Up @@ -240,7 +240,7 @@ Converter<?> getConverter() {
*
* <p>Can be used for usage help and controlling whether the "no" prefix is allowed.
*/
boolean isBooleanField() {
public boolean usesBooleanValueSyntax() {
return getType().equals(boolean.class)
|| getType().equals(TriState.class)
|| getConverter() instanceof BoolOrEnumConverter;
Expand Down
14 changes: 8 additions & 6 deletions src/main/java/com/google/devtools/common/options/OptionsParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,20 +405,22 @@ public void visitOptions(
* @return The {@link OptionDescription} for the option, or null if there is no option by the
* given name.
*/
OptionDescription getOptionDescription(String name) throws OptionsParsingException {
return impl.getOptionDescription(name);
OptionDescription getOptionDescription(String name, OptionPriority priority, String source)
throws OptionsParsingException {
return impl.getOptionDescription(name, priority, source);
}

/**
* Returns a description of the options values that get expanded from this option with the given
* value.
*
* @return The {@link com.google.devtools.common.options.OptionValueDescriptionlueDescription>}
* for the option, or null if there is no option by the given name.
* @return The {@link com.google.devtools.common.options.OptionValueDescription>} for the option,
* or null if there is no option by the given name.
*/
ImmutableList<OptionValueDescription> getExpansionOptionValueDescriptions(
OptionDefinition option, @Nullable String optionValue) throws OptionsParsingException {
return impl.getExpansionOptionValueDescriptions(option, optionValue);
OptionDefinition option, @Nullable String optionValue, OptionPriority priority, String source)
throws OptionsParsingException {
return impl.getExpansionOptionValueDescriptions(option, optionValue, priority, source);
}

/**
Expand Down
Loading

0 comments on commit 5fe8e66

Please sign in to comment.