From d95ce7f66a7a1d94e124e299f1613e4da7e4af7d Mon Sep 17 00:00:00 2001 From: ccalvarin Date: Mon, 30 Oct 2017 07:04:07 -0400 Subject: Compute canonical list of options using OptionValueDescription's tracking of instances. Stop storing the canonical list of arguments separately. For the canonicalize-flags command, we want to avoid showing options that either have no values in their own right (such as expansion options and wrapper options) and instances of options that did not make it to the final value. This is work we already do in OptionValueDescription, so we can generate the canonical form from the values tracked there, instead of tracking it separately. This means the canonical list is more correct where implicit requirements are concerned: implicit requirements are not listed in the canonical form, but now the values they overwrote will be correctly absent as well. Use this improved list for the effective command line published to the BEP. RELNOTES: Published command lines should have improved lists of effective options. PiperOrigin-RevId: 173873154 GitOrigin-RevId: c50cd13c75a2a1685f5ac9bd70561ac1e50722e7 Change-Id: I9c6802dc3ab1e263048862f931f0c5f4933576ae --- .../devtools/common/options/OptionDefinition.java | 7 ++- .../common/options/OptionValueDescription.java | 52 +++++++++++++++++ .../devtools/common/options/OptionsParser.java | 7 ++- .../devtools/common/options/OptionsParserImpl.java | 66 +++++++--------------- .../devtools/common/options/OptionsProvider.java | 16 +++++- 5 files changed, 97 insertions(+), 51 deletions(-) diff --git a/java/com/google/devtools/common/options/OptionDefinition.java b/java/com/google/devtools/common/options/OptionDefinition.java index 1c01932..84a9d2d 100644 --- a/java/com/google/devtools/common/options/OptionDefinition.java +++ b/java/com/google/devtools/common/options/OptionDefinition.java @@ -29,7 +29,7 @@ import java.util.Comparator; * the {@link Field} that is annotated, and should contain all logic about default settings and * behavior. */ -public class OptionDefinition { +public class OptionDefinition implements Comparable { // TODO(b/65049598) make ConstructionException checked, which will make this checked as well. static class NotAnOptionException extends ConstructionException { @@ -303,6 +303,11 @@ public class OptionDefinition { return field.hashCode(); } + @Override + public int compareTo(OptionDefinition o) { + return getOptionName().compareTo(o.getOptionName()); + } + @Override public String toString() { return String.format("option '--%s'", getOptionName()); diff --git a/java/com/google/devtools/common/options/OptionValueDescription.java b/java/com/google/devtools/common/options/OptionValueDescription.java index 0d31137..30c4377 100644 --- a/java/com/google/devtools/common/options/OptionValueDescription.java +++ b/java/com/google/devtools/common/options/OptionValueDescription.java @@ -25,6 +25,7 @@ import java.util.Comparator; import java.util.List; import java.util.Map.Entry; import java.util.stream.Collectors; +import javax.annotation.Nullable; /** * The value of an option. @@ -73,6 +74,16 @@ public abstract class OptionValueDescription { } } + /** + * Returns the canonical instances of this option - the instances that affect the current value. + * + *

For options that do not have values in their own right, this should be the empty list. In + * contrast, the DefaultOptionValue does not have a canonical form at all, since it was never set, + * and is null. + */ + @Nullable + abstract List getCanonicalInstances(); + /** * For the given option, returns the correct type of OptionValueDescription, to which unparsed * values can be added. @@ -125,6 +136,11 @@ public abstract class OptionValueDescription { "Cannot add values to the default option value. Create a modifiable " + "OptionValueDescription using createOptionValueDescription() instead."); } + + @Override + ImmutableList getCanonicalInstances() { + return null; + } } /** @@ -227,6 +243,16 @@ public abstract class OptionValueDescription { return null; } + @Override + ImmutableList getCanonicalInstances() { + // If the current option is an implicit requirement, we don't need to list this value since + // the parent implies it. In this case, it is sufficient to not list this value at all. + if (effectiveOptionInstance.getImplicitDependent() == null) { + return ImmutableList.of(effectiveOptionInstance); + } + return ImmutableList.of(); + } + @VisibleForTesting ParsedOptionDescription getEffectiveOptionInstance() { return effectiveOptionInstance; @@ -289,6 +315,18 @@ public abstract class OptionValueDescription { } return null; } + + @Override + ImmutableList getCanonicalInstances() { + return parsedOptions + .asMap() + .entrySet() + .stream() + .sorted(Comparator.comparing(Entry::getKey)) + .map(Entry::getValue) + .flatMap(Collection::stream) + .collect(ImmutableList.toImmutableList()); + } } /** @@ -337,6 +375,13 @@ public abstract class OptionValueDescription { : String.format( "expanded from %s (source %s)", optionDefinition, parsedOption.getSource())); } + + @Override + ImmutableList getCanonicalInstances() { + // The options this expands to are incorporated in their own right - this option does + // not have a canonical form. + return ImmutableList.of(); + } } /** The form of a value for a flag with implicit requirements. */ @@ -418,6 +463,13 @@ public abstract class OptionValueDescription { : String.format( "unwrapped from %s (source %s)", optionDefinition, parsedOption.getSource())); } + + @Override + ImmutableList getCanonicalInstances() { + // No wrapper options get listed in the canonical form - the options they are wrapping will + // be in the right place. + return ImmutableList.of(); + } } } diff --git a/java/com/google/devtools/common/options/OptionsParser.java b/java/com/google/devtools/common/options/OptionsParser.java index f84ee47..4164e7c 100644 --- a/java/com/google/devtools/common/options/OptionsParser.java +++ b/java/com/google/devtools/common/options/OptionsParser.java @@ -680,7 +680,12 @@ public class OptionsParser implements OptionsProvider { } @Override - public List asListOfEffectiveOptions() { + public List asListOfCanonicalOptions() { + return impl.asCanonicalizedListOfParsedOptions(); + } + + @Override + public List asListOfOptionValues() { return impl.asListOfEffectiveOptions(); } diff --git a/java/com/google/devtools/common/options/OptionsParserImpl.java b/java/com/google/devtools/common/options/OptionsParserImpl.java index b543328..e8086c0 100644 --- a/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -21,18 +21,18 @@ import com.google.common.base.Joiner; import com.google.common.base.Preconditions; 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.OptionValueDescription.ExpansionBundle; import com.google.devtools.common.options.OptionsParser.OptionDescription; import java.lang.reflect.Constructor; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.function.Function; +import java.util.stream.Stream; import javax.annotation.Nullable; /** @@ -68,17 +68,6 @@ class OptionsParserImpl { */ private final List parsedOptions = new ArrayList<>(); - /** - * The options for use with the canonicalize command are stored separately from parsedOptions so - * that invocation policy can modify the values for canonicalization (e.g. override user-specified - * values with default values) without corrupting the data used to represent the user's original - * invocation for {@link #asListOfExplicitOptions()} and {@link #asCompleteListOfParsedOptions()}. - * A LinkedHashMultimap is used so that canonicalization happens in the correct order and multiple - * values can be stored for flags that allow multiple values. - */ - private final Multimap canonicalizeValues = - LinkedHashMultimap.create(); - private final List warnings = new ArrayList<>(); private boolean allowSingleDashLongOptions = false; @@ -135,36 +124,28 @@ class OptionsParserImpl { .collect(toCollection(ArrayList::new)); } - /** - * Implements {@link OptionsParser#canonicalize}. - */ - List asCanonicalizedList() { - return canonicalizeValues - .values() + private Stream asStreamOfCanonicalParsedOptions() { + return optionValues + .keySet() .stream() - // Sort implicit requirement options to the end, keeping their existing order, and sort - // the other options alphabetically. - .sorted( - (v1, v2) -> { - if (v1.getOptionDefinition().hasImplicitRequirements()) { - return v2.getOptionDefinition().hasImplicitRequirements() ? 0 : 1; - } - if (v2.getOptionDefinition().hasImplicitRequirements()) { - return -1; - } - return v1.getOptionDefinition() - .getOptionName() - .compareTo(v2.getOptionDefinition().getOptionName()); - }) - // Ignore expansion options. - .filter(value -> !value.getOptionDefinition().isExpansionOption()) + .sorted() + .map(optionDefinition -> optionValues.get(optionDefinition).getCanonicalInstances()) + .flatMap(Collection::stream); + } + + /** Implements {@link OptionsParser#canonicalize}. */ + List asCanonicalizedList() { + return asStreamOfCanonicalParsedOptions() .map(ParsedOptionDescription::getDeprecatedCanonicalForm) - .collect(toCollection(ArrayList::new)); + .collect(ImmutableList.toImmutableList()); } - /** - * Implements {@link OptionsParser#asListOfEffectiveOptions()}. - */ + /** Implements {@link OptionsParser#canonicalize}. */ + List asCanonicalizedListOfParsedOptions() { + return asStreamOfCanonicalParsedOptions().collect(ImmutableList.toImmutableList()); + } + + /** Implements {@link OptionsParser#asListOfOptionValues()}. */ List asListOfEffectiveOptions() { List result = new ArrayList<>(); for (Map.Entry mapEntry : optionsData.getAllOptionDefinitions()) { @@ -196,8 +177,6 @@ class OptionsParserImpl { OptionValueDescription clearValue(OptionDefinition optionDefinition) throws OptionsParsingException { - // Actually remove the value from various lists tracking effective options. - canonicalizeValues.removeAll(optionDefinition); return optionValues.remove(optionDefinition); } @@ -401,11 +380,6 @@ class OptionsParserImpl { // 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 (expansionBundle != null) { diff --git a/java/com/google/devtools/common/options/OptionsProvider.java b/java/com/google/devtools/common/options/OptionsProvider.java index 5fd8ac0..d467fe5 100644 --- a/java/com/google/devtools/common/options/OptionsProvider.java +++ b/java/com/google/devtools/common/options/OptionsProvider.java @@ -57,10 +57,20 @@ public interface OptionsProvider extends OptionsClassProvider { List asListOfExplicitOptions(); /** - * Returns a list of all options, including undocumented ones, and their - * effective values. There is no guaranteed ordering for the result. + * Returns a list of the parsed options whose values are in the final value of the option, i.e. + * the options that were added explicitly, expanded if necessary to the valued options they + * affect. This will not include values that were set and then overridden by a later value of the + * same option. + * + *

The list includes undocumented options. + */ + List asListOfCanonicalOptions(); + + /** + * Returns a list of all options, including undocumented ones, and their effective values. There + * is no guaranteed ordering for the result. */ - List asListOfEffectiveOptions(); + List asListOfOptionValues(); /** * Canonicalizes the list of options that this OptionsParser has parsed. The -- cgit v1.2.3