Warn about config expansions as we do for other expansions.
If an expanded value overrides an explicit value, users who do not know the contents of the expansion may be surprised. We already warned about this for hard-coded expansions, and this is now applicable for --config expansions as well.
This will only warn when a single-valued option has its value replaced. Options that accumulate multiple values in a list (e.g., --copt) will silently include both explicit and expanded values.
RELNOTES: None.
PiperOrigin-RevId: 179857526
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
index 436a96d..c7c72ec 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
@@ -452,10 +452,8 @@
for (ParsedOptionDescription configInstance : configInstances) {
String configValueToExpand = (String) configInstance.getConvertedValue();
List<String> expansion = getExpansion(eventHandler, commandToRcArgs, configValueToExpand);
- optionsParser.parseOptionsFixedAtSpecificPriority(
- configInstance.getPriority(),
- String.format("expanded from --%s", configValueToExpand),
- expansion);
+ optionsParser.parseArgsFixedAsExpansionOfOption(
+ configInstance, String.format("expanded from --%s", configValueToExpand), expansion);
}
// At this point, we've expanded everything, identify duplicates, if any, to warn about
diff --git a/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java b/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java
index 584e75b..b0782f8 100644
--- a/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java
+++ b/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java
@@ -22,14 +22,14 @@
public class OptionInstanceOrigin {
private final OptionPriority priority;
@Nullable private final String source;
- @Nullable private final OptionDefinition implicitDependent;
- @Nullable private final OptionDefinition expandedFrom;
+ @Nullable private final ParsedOptionDescription implicitDependent;
+ @Nullable private final ParsedOptionDescription expandedFrom;
public OptionInstanceOrigin(
OptionPriority priority,
String source,
- OptionDefinition implicitDependent,
- OptionDefinition expandedFrom) {
+ ParsedOptionDescription implicitDependent,
+ ParsedOptionDescription expandedFrom) {
this.priority = priority;
this.source = source;
this.implicitDependent = implicitDependent;
@@ -46,12 +46,12 @@
}
@Nullable
- public OptionDefinition getImplicitDependent() {
+ public ParsedOptionDescription getImplicitDependent() {
return implicitDependent;
}
@Nullable
- public OptionDefinition getExpandedFrom() {
+ public ParsedOptionDescription getExpandedFrom() {
return expandedFrom;
}
}
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 3fde138..f52e8a5 100644
--- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -185,11 +185,11 @@
// 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 =
+ ParsedOptionDescription implicitDependent = parsedOption.getImplicitDependent();
+ ParsedOptionDescription expandedFrom = parsedOption.getExpandedFrom();
+ ParsedOptionDescription optionThatDependsOnEffectiveValue =
effectiveOptionInstance.getImplicitDependent();
- OptionDefinition optionThatExpandedToEffectiveValue =
+ ParsedOptionDescription optionThatExpandedToEffectiveValue =
effectiveOptionInstance.getExpandedFrom();
Object newValue = parsedOption.getConvertedValue();
@@ -225,7 +225,7 @@
// Create a warning if an expansion option overrides an explicit option:
warnings.add(
String.format(
- "%s was expanded and now overrides a previous explicitly specified %s with %s",
+ "%s was expanded and now overrides the explicit option %s with %s",
expandedFrom,
effectiveOptionInstance.getCommandLineForm(),
parsedOption.getCommandLineForm()));
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 fb7161c..b7da004 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -619,13 +619,26 @@
}
}
- public void parseOptionsFixedAtSpecificPriority(
- OptionPriority priority, String source, List<String> args) throws OptionsParsingException {
- Preconditions.checkNotNull(priority, "Priority not specified for arglist " + args);
+ /**
+ * Parses the args at the priority of the provided option. This is useful for after-the-fact
+ * expansion.
+ *
+ * @param optionToExpand the option that is being "expanded" after the fact. The provided args
+ * will have the same priority as this option.
+ * @param source a description of where the expansion arguments came from.
+ * @param args the arguments to parse as the expansion. Order matters, as the value of a flag may
+ * be in the following argument.
+ */
+ public void parseArgsFixedAsExpansionOfOption(
+ ParsedOptionDescription optionToExpand, String source, List<String> args)
+ throws OptionsParsingException {
+ Preconditions.checkNotNull(
+ optionToExpand, "Option for expansion not specified for arglist " + args);
Preconditions.checkArgument(
- priority.getPriorityCategory() != OptionPriority.PriorityCategory.DEFAULT,
+ optionToExpand.getPriority().getPriorityCategory()
+ != OptionPriority.PriorityCategory.DEFAULT,
"Priority cannot be default, which was specified for arglist " + args);
- residue.addAll(impl.parseOptionsFixedAtSpecificPriority(priority, o -> source, args));
+ residue.addAll(impl.parseArgsFixedAsExpansionOfOption(optionToExpand, o -> source, args));
if (!allowResidue && !residue.isEmpty()) {
String errorMsg = "Unrecognized arguments: " + Joiner.on(' ').join(residue);
throw new OptionsParsingException(errorMsg);
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 5ce35da..bc66cc3 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -204,30 +204,33 @@
* OptionInstanceOrigin)}
*/
ImmutableList<ParsedOptionDescription> getExpansionValueDescriptions(
- OptionDefinition expansionFlag, OptionInstanceOrigin originOfExpansionFlag)
+ OptionDefinition expansionFlagDef, OptionInstanceOrigin originOfExpansionFlag)
throws OptionsParsingException {
ImmutableList.Builder<ParsedOptionDescription> builder = ImmutableList.builder();
OptionInstanceOrigin originOfSubflags;
ImmutableList<String> options;
- if (expansionFlag.hasImplicitRequirements()) {
- options = ImmutableList.copyOf(expansionFlag.getImplicitRequirements());
+ ParsedOptionDescription expansionFlagParsedDummy =
+ ParsedOptionDescription.newDummyInstance(expansionFlagDef, originOfExpansionFlag);
+ if (expansionFlagDef.hasImplicitRequirements()) {
+ options = ImmutableList.copyOf(expansionFlagDef.getImplicitRequirements());
originOfSubflags =
new OptionInstanceOrigin(
originOfExpansionFlag.getPriority(),
String.format(
"implicitly required by %s (source: %s)",
- expansionFlag, originOfExpansionFlag.getSource()),
- expansionFlag,
+ expansionFlagDef, originOfExpansionFlag.getSource()),
+ expansionFlagParsedDummy,
null);
- } else if (expansionFlag.isExpansionOption()) {
- options = optionsData.getEvaluatedExpansion(expansionFlag);
+ } else if (expansionFlagDef.isExpansionOption()) {
+ options = optionsData.getEvaluatedExpansion(expansionFlagDef);
originOfSubflags =
new OptionInstanceOrigin(
originOfExpansionFlag.getPriority(),
String.format(
- "expanded by %s (source: %s)", expansionFlag, originOfExpansionFlag.getSource()),
+ "expanded by %s (source: %s)",
+ expansionFlagDef, originOfExpansionFlag.getSource()),
null,
- expansionFlag);
+ expansionFlagParsedDummy);
} else {
return ImmutableList.of();
}
@@ -284,12 +287,19 @@
}
}
- /** Parses the args at the fixed priority. */
- List<String> parseOptionsFixedAtSpecificPriority(
- OptionPriority priority, Function<OptionDefinition, String> sourceFunction, List<String> args)
+ /** Implements {@link OptionsParser#parseArgsFixedAsExpansionOfOption} */
+ List<String> parseArgsFixedAsExpansionOfOption(
+ ParsedOptionDescription optionToExpand,
+ Function<OptionDefinition, String> sourceFunction,
+ List<String> args)
throws OptionsParsingException {
ResidueAndPriority residueAndPriority =
- parse(OptionPriority.getLockedPriority(priority), sourceFunction, null, null, args);
+ parse(
+ OptionPriority.getLockedPriority(optionToExpand.getPriority()),
+ sourceFunction,
+ null,
+ optionToExpand,
+ args);
return residueAndPriority.residue;
}
@@ -304,8 +314,8 @@
private ResidueAndPriority parse(
OptionPriority priority,
Function<OptionDefinition, String> sourceFunction,
- OptionDefinition implicitDependent,
- OptionDefinition expandedFrom,
+ ParsedOptionDescription implicitDependent,
+ ParsedOptionDescription expandedFrom,
List<String> args)
throws OptionsParsingException {
List<String> unparsedArgs = new ArrayList<>();
@@ -369,7 +379,7 @@
priorityCategory);
handleNewParsedOption(
- new ParsedOptionDescription(
+ ParsedOptionDescription.newParsedOptionDescription(
option,
String.format("--%s=%s", option.getOptionName(), unconvertedValue),
unconvertedValue,
@@ -411,8 +421,8 @@
parse(
OptionPriority.getLockedPriority(parsedOption.getPriority()),
o -> expansionBundle.sourceOfExpansionArgs,
- optionDefinition.hasImplicitRequirements() ? optionDefinition : null,
- optionDefinition.isExpansionOption() ? optionDefinition : null,
+ optionDefinition.hasImplicitRequirements() ? parsedOption : null,
+ optionDefinition.isExpansionOption() ? parsedOption : null,
expansionBundle.expansionArgs);
if (!residueAndPriority.residue.isEmpty()) {
@@ -433,8 +443,8 @@
Iterator<String> nextArgs,
OptionPriority priority,
Function<OptionDefinition, String> sourceFunction,
- OptionDefinition implicitDependent,
- OptionDefinition expandedFrom)
+ ParsedOptionDescription implicitDependent,
+ ParsedOptionDescription expandedFrom)
throws OptionsParsingException {
// Store the way this option was parsed on the command line.
@@ -510,7 +520,7 @@
}
}
- return new ParsedOptionDescription(
+ return ParsedOptionDescription.newParsedOptionDescription(
optionDefinition,
commandLineForm.toString(),
unconvertedValue,
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 f55f8ad..5088153 100644
--- a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
+++ b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
@@ -14,6 +14,7 @@
package com.google.devtools.common.options;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import java.util.function.Function;
import javax.annotation.Nullable;
@@ -27,25 +28,49 @@
public final class ParsedOptionDescription {
private final OptionDefinition optionDefinition;
- private final String commandLineForm;
+ @Nullable private final String commandLineForm;
@Nullable private final String unconvertedValue;
private final OptionInstanceOrigin origin;
- public ParsedOptionDescription(
+ private ParsedOptionDescription(
+ OptionDefinition optionDefinition,
+ @Nullable String commandLineForm,
+ @Nullable String unconvertedValue,
+ OptionInstanceOrigin origin) {
+ this.optionDefinition = Preconditions.checkNotNull(optionDefinition);
+ this.commandLineForm = commandLineForm;
+ this.unconvertedValue = unconvertedValue;
+ this.origin = Preconditions.checkNotNull(origin);
+ }
+
+ static ParsedOptionDescription newParsedOptionDescription(
OptionDefinition optionDefinition,
String commandLineForm,
@Nullable String unconvertedValue,
OptionInstanceOrigin origin) {
- this.optionDefinition = optionDefinition;
- this.commandLineForm = commandLineForm;
- this.unconvertedValue = unconvertedValue;
- this.origin = origin;
+ // An actual ParsedOptionDescription should always have a form in which it was parsed, but some
+ // options, such as expansion options, legitimately have no value.
+ return new ParsedOptionDescription(
+ optionDefinition,
+ Preconditions.checkNotNull(commandLineForm),
+ unconvertedValue,
+ origin);
+ }
+
+ /**
+ * This factory should be used when there is no actual parsed option, since in those cases we do
+ * not have an original value or form that the option took.
+ */
+ static ParsedOptionDescription newDummyInstance(
+ OptionDefinition optionDefinition, OptionInstanceOrigin origin) {
+ return new ParsedOptionDescription(optionDefinition, null, null, origin);
}
public OptionDefinition getOptionDefinition() {
return optionDefinition;
}
+ @Nullable
public String getCommandLineForm() {
return commandLineForm;
}
@@ -127,11 +152,11 @@
return origin.getSource();
}
- OptionDefinition getImplicitDependent() {
+ ParsedOptionDescription getImplicitDependent() {
return origin.getImplicitDependent();
}
- OptionDefinition getExpandedFrom() {
+ ParsedOptionDescription getExpandedFrom() {
return origin.getExpandedFrom();
}
@@ -152,14 +177,14 @@
@Override
public String toString() {
- StringBuilder result = new StringBuilder();
- result.append(optionDefinition);
- result.append("set to '").append(unconvertedValue).append("' ");
- result.append("with priority ").append(origin.getPriority());
- if (origin.getSource() != null) {
- result.append(" and source '").append(origin.getSource()).append("'");
+ // Check that a dummy value-less option instance does not output all the default information.
+ if (commandLineForm == null) {
+ return optionDefinition.toString();
}
- return result.toString();
+ String source = origin.getSource();
+ return String.format(
+ "option '%s'%s",
+ commandLineForm, source == null ? "" : String.format(" (source %s)", source));
}
}
