Track the origin of an option in the option instance, not its final value.
A single instance of an option has a single origin, but the final value only has a single origin if it has a single value. For multi-valued options, it is wrong to expect that the final value of an option will have a single parent. Track the option parents (which option expanded to the current instance, if any) in the right place, with the ParsedOptionDescription.
Also fix some inconsistent spelling of 'dependent,' in favor of the American English standard.
RELNOTES: None.
PiperOrigin-RevId: 169487515
GitOrigin-RevId: 3a0df3cb0637d71dfcf0add7057332c09cd508c5
Change-Id: Ic187397f8204aea3638b109b71d590920e968bdb
diff --git a/java/com/google/devtools/common/options/OptionInstanceOrigin.java b/java/com/google/devtools/common/options/OptionInstanceOrigin.java
new file mode 100644
index 0000000..584e75b
--- /dev/null
+++ b/java/com/google/devtools/common/options/OptionInstanceOrigin.java
@@ -0,0 +1,57 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.common.options;
+
+import javax.annotation.Nullable;
+
+/**
+ * Contains metadata describing the origin of an option. This includes its priority, a message about
+ * where it came from, and whether it was set explicitly or expanded/implied by other flags.
+ */
+public class OptionInstanceOrigin {
+ private final OptionPriority priority;
+ @Nullable private final String source;
+ @Nullable private final OptionDefinition implicitDependent;
+ @Nullable private final OptionDefinition expandedFrom;
+
+ public OptionInstanceOrigin(
+ OptionPriority priority,
+ String source,
+ OptionDefinition implicitDependent,
+ OptionDefinition expandedFrom) {
+ this.priority = priority;
+ this.source = source;
+ this.implicitDependent = implicitDependent;
+ this.expandedFrom = expandedFrom;
+ }
+
+ public OptionPriority getPriority() {
+ return priority;
+ }
+
+ @Nullable
+ public String getSource() {
+ return source;
+ }
+
+ @Nullable
+ public OptionDefinition getImplicitDependent() {
+ return implicitDependent;
+ }
+
+ @Nullable
+ public OptionDefinition getExpandedFrom() {
+ return expandedFrom;
+ }
+}
diff --git a/java/com/google/devtools/common/options/OptionValueDescription.java b/java/com/google/devtools/common/options/OptionValueDescription.java
index 8222aee..0d81d49 100644
--- a/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -26,9 +26,7 @@
/**
* The value of an option.
*
- * <p>This takes care of tracking the final value as multiple instances of an option are parsed. It
- * also tracks additional metadata describing its priority, source, whether it was set via an
- * implicit dependency, and if so, by which other option.
+ * <p>This takes care of tracking the final value as multiple instances of an option are parsed.
*/
public abstract class OptionValueDescription {
@@ -48,12 +46,8 @@
/** Returns the source(s) of this option, if there were multiple, duplicates are removed. */
public abstract String getSourceString();
- // TODO(b/65540004) implicitDependant and expandedFrom are artifacts of an option instance, and
- // should be in ParsedOptionDescription.
abstract void addOptionInstance(
ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom,
List<String> warnings)
throws OptionsParsingException;
@@ -105,8 +99,6 @@
@Override
void addOptionInstance(
ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom,
List<String> warnings) {
throw new IllegalStateException(
"Cannot add values to the default option value. Create a modifiable "
@@ -121,8 +113,6 @@
static class SingleOptionValueDescription extends OptionValueDescription {
private ParsedOptionDescription effectiveOptionInstance;
private Object effectiveValue;
- private OptionDefinition optionThatDependsOnEffectiveValue;
- private OptionDefinition optionThatExpandedToEffectiveValue;
private SingleOptionValueDescription(OptionDefinition optionDefinition) {
super(optionDefinition);
@@ -134,8 +124,6 @@
}
effectiveOptionInstance = null;
effectiveValue = null;
- optionThatDependsOnEffectiveValue = null;
- optionThatExpandedToEffectiveValue = null;
}
@Override
@@ -152,15 +140,11 @@
@Override
void addOptionInstance(
ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom,
List<String> warnings)
throws OptionsParsingException {
// This might be the first value, in that case, just store it!
if (effectiveOptionInstance == null) {
effectiveOptionInstance = parsedOption;
- optionThatDependsOnEffectiveValue = implicitDependant;
- optionThatExpandedToEffectiveValue = expandedFrom;
effectiveValue = effectiveOptionInstance.getConvertedValue();
return;
}
@@ -168,23 +152,31 @@
// If there was another value, check whether the new one will override it, and if so,
// log warnings describing the change.
if (parsedOption.getPriority().compareTo(effectiveOptionInstance.getPriority()) >= 0) {
+ // Identify the option that might have led to the current and new value of this option.
+ OptionDefinition implicitDependent = parsedOption.getImplicitDependent();
+ OptionDefinition expandedFrom = parsedOption.getExpandedFrom();
+ OptionDefinition optionThatDependsOnEffectiveValue =
+ effectiveOptionInstance.getImplicitDependent();
+ OptionDefinition optionThatExpandedToEffectiveValue =
+ effectiveOptionInstance.getExpandedFrom();
+
// Output warnings:
- if ((implicitDependant != null) && (optionThatDependsOnEffectiveValue != null)) {
- if (!implicitDependant.equals(optionThatDependsOnEffectiveValue)) {
+ if ((implicitDependent != null) && (optionThatDependsOnEffectiveValue != null)) {
+ if (!implicitDependent.equals(optionThatDependsOnEffectiveValue)) {
warnings.add(
String.format(
"Option '%s' is implicitly defined by both option '%s' and option '%s'",
optionDefinition.getOptionName(),
optionThatDependsOnEffectiveValue.getOptionName(),
- implicitDependant.getOptionName()));
+ implicitDependent.getOptionName()));
}
- } else if ((implicitDependant != null)
+ } 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 "
+ "overrides the previous one",
- optionDefinition.getOptionName(), implicitDependant.getOptionName()));
+ optionDefinition.getOptionName(), implicitDependent.getOptionName()));
} else if (optionThatDependsOnEffectiveValue != null) {
warnings.add(
String.format(
@@ -211,8 +203,6 @@
// Record the new value:
effectiveOptionInstance = parsedOption;
- optionThatDependsOnEffectiveValue = implicitDependant;
- optionThatExpandedToEffectiveValue = expandedFrom;
effectiveValue = parsedOption.getConvertedValue();
} else {
// The new value does not override the old value, as it has lower priority.
@@ -275,17 +265,17 @@
@Override
void addOptionInstance(
ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom,
List<String> warnings)
throws OptionsParsingException {
// For repeatable options, we allow flags that take both single values and multiple values,
// potentially collapsing them down.
Object convertedValue = parsedOption.getConvertedValue();
+ OptionPriority priority = parsedOption.getPriority();
+ parsedOptions.put(priority, parsedOption);
if (convertedValue instanceof List<?>) {
- optionValues.putAll(parsedOption.getPriority(), (List<?>) convertedValue);
+ optionValues.putAll(priority, (List<?>) convertedValue);
} else {
- optionValues.put(parsedOption.getPriority(), convertedValue);
+ optionValues.put(priority, convertedValue);
}
}
}
@@ -318,8 +308,6 @@
@Override
void addOptionInstance(
ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom,
List<String> warnings) {
// TODO(b/65540004) Deal with expansion options here instead of in parse(), and track their
// link to the options they expanded to to.
@@ -341,13 +329,11 @@
@Override
void addOptionInstance(
ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom,
List<String> warnings)
throws OptionsParsingException {
// This is a valued flag, its value is handled the same way as a normal
// SingleOptionValueDescription.
- super.addOptionInstance(parsedOption, implicitDependant, expandedFrom, warnings);
+ super.addOptionInstance(parsedOption, warnings);
// Now deal with the implicit requirements.
// TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
@@ -375,8 +361,6 @@
@Override
void addOptionInstance(
ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom,
List<String> warnings)
throws OptionsParsingException {
// TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
diff --git a/java/com/google/devtools/common/options/OptionsParserImpl.java b/java/com/google/devtools/common/options/OptionsParserImpl.java
index a7745f6..176d51e 100644
--- a/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -223,17 +223,17 @@
return new OptionDescription(
optionDefinition,
optionsData.getExpansionDataForField(optionDefinition),
- getImplicitDependantDescriptions(
+ getImplicitDependentDescriptions(
ImmutableList.copyOf(optionDefinition.getImplicitRequirements()),
optionDefinition,
priority,
source));
}
- /** @return A list of the descriptions corresponding to the implicit dependant flags passed in. */
- private ImmutableList<ParsedOptionDescription> getImplicitDependantDescriptions(
+ /** @return A list of the descriptions corresponding to the implicit dependent flags passed in. */
+ private ImmutableList<ParsedOptionDescription> getImplicitDependentDescriptions(
ImmutableList<String> options,
- OptionDefinition implicitDependant,
+ OptionDefinition implicitDependent,
OptionPriority priority,
String source)
throws OptionsParsingException {
@@ -244,12 +244,17 @@
o ->
String.format(
"implicitely required for option %s (source: %s)",
- implicitDependant.getOptionName(), source);
+ implicitDependent.getOptionName(), source);
while (optionsIterator.hasNext()) {
String unparsedFlagExpression = optionsIterator.next();
ParsedOptionDescription parsedOption =
identifyOptionAndPossibleArgument(
- unparsedFlagExpression, optionsIterator, priority, sourceFunction, false);
+ unparsedFlagExpression,
+ optionsIterator,
+ priority,
+ sourceFunction,
+ implicitDependent,
+ null);
builder.add(parsedOption);
}
return builder.build();
@@ -257,9 +262,7 @@
/**
* @return A list of the descriptions corresponding to options expanded from the flag for the
- * given value. These descriptions are are divorced from the command line - there is no
- * correct priority or source for these, as they are not actually set values. The value itself
- * is also a string, no conversion has taken place.
+ * given value. The value itself is a string, no conversion has taken place.
*/
ImmutableList<ParsedOptionDescription> getExpansionOptionValueDescriptions(
OptionDefinition expansionFlag,
@@ -277,7 +280,12 @@
String unparsedFlagExpression = optionsIterator.next();
ParsedOptionDescription parsedOption =
identifyOptionAndPossibleArgument(
- unparsedFlagExpression, optionsIterator, priority, sourceFunction, false);
+ unparsedFlagExpression,
+ optionsIterator,
+ priority,
+ sourceFunction,
+ null,
+ expansionFlag);
builder.add(parsedOption);
}
return builder.build();
@@ -317,7 +325,6 @@
OptionDefinition expandedFrom,
List<String> args)
throws OptionsParsingException {
- boolean isExplicit = expandedFrom == null && implicitDependent == null;
List<String> unparsedArgs = new ArrayList<>();
LinkedHashMap<OptionDefinition, List<String>> implicitRequirements = new LinkedHashMap<>();
@@ -337,7 +344,7 @@
ParsedOptionDescription parsedOption =
identifyOptionAndPossibleArgument(
- arg, argsIterator, priority, sourceFunction, isExplicit);
+ 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);
@@ -347,7 +354,7 @@
OptionValueDescription entry =
optionValues.computeIfAbsent(
optionDefinition, OptionValueDescription::createOptionValueDescription);
- entry.addOptionInstance(parsedOption, implicitDependent, expandedFrom, warnings);
+ entry.addOptionInstance(parsedOption, warnings);
@Nullable String unconvertedValue = parsedOption.getUnconvertedValue();
if (optionDefinition.isWrapperOption()) {
@@ -404,11 +411,13 @@
ImmutableList<String> expansion =
optionsData.getEvaluatedExpansion(optionDefinition, unconvertedValue);
+ String sourceFunctionApplication = sourceFunction.apply(optionDefinition);
String sourceMessage =
- "expanded from option --"
- + optionDefinition.getOptionName()
- + " from "
- + sourceFunction.apply(optionDefinition);
+ (sourceFunctionApplication == null)
+ ? String.format("expanded from option --%s", optionDefinition.getOptionName())
+ : String.format(
+ "expanded from option --%s from %s",
+ optionDefinition.getOptionName(), sourceFunctionApplication);
Function<OptionDefinition, String> expansionSourceFunction = o -> sourceMessage;
List<String> unparsed =
parse(priority, expansionSourceFunction, null, optionDefinition, expansion);
@@ -434,11 +443,15 @@
// TODO(bazel-team): this should happen when the option is encountered.
if (!implicitRequirements.isEmpty()) {
for (Map.Entry<OptionDefinition, List<String>> entry : implicitRequirements.entrySet()) {
+ OptionDefinition optionDefinition = entry.getKey();
+ String sourceFunctionApplication = sourceFunction.apply(optionDefinition);
String sourceMessage =
- "implicit requirement of option --"
- + entry.getKey()
- + " from "
- + sourceFunction.apply(entry.getKey());
+ (sourceFunctionApplication == null)
+ ? String.format(
+ "implicit requirement of option --%s", optionDefinition.getOptionName())
+ : String.format(
+ "implicit requirement of option --%s from %s",
+ optionDefinition.getOptionName(), sourceFunctionApplication);
Function<OptionDefinition, String> requirementSourceFunction = o -> sourceMessage;
List<String> unparsed = parse(priority, requirementSourceFunction, entry.getKey(), null,
@@ -467,7 +480,8 @@
Iterator<String> nextArgs,
OptionPriority priority,
Function<OptionDefinition, String> sourceFunction,
- boolean explicit)
+ OptionDefinition implicitDependent,
+ OptionDefinition expandedFrom)
throws OptionsParsingException {
// Store the way this option was parsed on the command line.
@@ -548,9 +562,8 @@
optionDefinition,
commandLineForm.toString(),
unconvertedValue,
- priority,
- sourceFunction.apply(optionDefinition),
- explicit);
+ new OptionInstanceOrigin(
+ priority, sourceFunction.apply(optionDefinition), implicitDependent, expandedFrom));
}
/**
diff --git a/java/com/google/devtools/common/options/ParsedOptionDescription.java b/java/com/google/devtools/common/options/ParsedOptionDescription.java
index 036ac41..0910579 100644
--- a/java/com/google/devtools/common/options/ParsedOptionDescription.java
+++ b/java/com/google/devtools/common/options/ParsedOptionDescription.java
@@ -18,39 +18,27 @@
import javax.annotation.Nullable;
/**
- * The value of an option with additional metadata describing its origin.
+ * The representation of a parsed option instance.
*
- * <p>This class represents an option as the parser received it, which is distinct from the final
- * value of an option, as these values may be overridden or combined in some way.
- *
- * <p>The origin includes the form it had when parsed, its priority, a message about where it came
- * from, and whether it was set explicitly or expanded/implied by other flags.
+ * <p>An option instance is distinct from the final value of an option, as multiple instances
+ * provide values may be overridden or combined in some way.
*/
public final class ParsedOptionDescription {
+
private final OptionDefinition optionDefinition;
private final String commandLineForm;
@Nullable private final String unconvertedValue;
- private final OptionPriority priority;
- @Nullable private final String source;
-
- // Whether this flag was explicitly given, as opposed to having been added by an expansion flag
- // or an implicit dependency. Notice that this does NOT mean it was explicitly given by the
- // user, for that to be true, it needs the right combination of explicit & priority.
- private final boolean explicit;
+ private final OptionInstanceOrigin origin;
public ParsedOptionDescription(
OptionDefinition optionDefinition,
String commandLineForm,
@Nullable String unconvertedValue,
- OptionPriority priority,
- @Nullable String source,
- boolean explicit) {
+ OptionInstanceOrigin origin) {
this.optionDefinition = optionDefinition;
this.commandLineForm = commandLineForm;
this.unconvertedValue = unconvertedValue;
- this.priority = priority;
- this.source = source;
- this.explicit = explicit;
+ this.origin = origin;
}
public OptionDefinition getOptionDefinition() {
@@ -87,15 +75,23 @@
}
OptionPriority getPriority() {
- return priority;
+ return origin.getPriority();
}
public String getSource() {
- return source;
+ return origin.getSource();
+ }
+
+ OptionDefinition getImplicitDependent() {
+ return origin.getImplicitDependent();
+ }
+
+ OptionDefinition getExpandedFrom() {
+ return origin.getExpandedFrom();
}
public boolean isExplicit() {
- return explicit;
+ return origin.getExpandedFrom() == null && origin.getImplicitDependent() == null;
}
public Object getConvertedValue() throws OptionsParsingException {
@@ -114,9 +110,9 @@
StringBuilder result = new StringBuilder();
result.append("option '").append(optionDefinition.getOptionName()).append("' ");
result.append("set to '").append(unconvertedValue).append("' ");
- result.append("with priority ").append(priority);
- if (source != null) {
- result.append(" and source '").append(source).append("'");
+ result.append("with priority ").append(origin.getPriority());
+ if (origin.getSource() != null) {
+ result.append(" and source '").append(origin.getSource()).append("'");
}
return result.toString();
}
