diff options
Diffstat (limited to 'src/main/java/com/google/devtools/common')
4 files changed, 135 insertions, 85 deletions
diff --git a/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java b/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java new file mode 100644 index 0000000000..584e75b9fc --- /dev/null +++ b/src/main/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/src/main/java/com/google/devtools/common/options/OptionValueDescription.java b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java index 8222aee12b..0d81d49a84 100644 --- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java +++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java @@ -26,9 +26,7 @@ import java.util.stream.Collectors; /** * 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 @@ public abstract class OptionValueDescription { /** 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 @@ public abstract class OptionValueDescription { @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 @@ public abstract class OptionValueDescription { 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 @@ public abstract class OptionValueDescription { } effectiveOptionInstance = null; effectiveValue = null; - optionThatDependsOnEffectiveValue = null; - optionThatExpandedToEffectiveValue = null; } @Override @@ -152,15 +140,11 @@ public abstract class OptionValueDescription { @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 @@ public abstract class OptionValueDescription { // 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 @@ public abstract class OptionValueDescription { // 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 @@ public abstract class OptionValueDescription { @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 @@ public abstract class OptionValueDescription { @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 @@ public abstract class OptionValueDescription { @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 @@ public abstract class OptionValueDescription { @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/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java index a7745f6fe6..176d51e66a 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -223,17 +223,17 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { /** * @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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { // 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { optionDefinition, commandLineForm.toString(), unconvertedValue, - priority, - sourceFunction.apply(optionDefinition), - explicit); + new OptionInstanceOrigin( + priority, sourceFunction.apply(optionDefinition), implicitDependent, expandedFrom)); } /** 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 036ac4188b..0910579541 100644 --- a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java +++ b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java @@ -18,39 +18,27 @@ import com.google.common.collect.ImmutableList; 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 @@ public final class ParsedOptionDescription { } 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 @@ public final class ParsedOptionDescription { 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(); } |