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

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<OptionDefinition> {
 
   // TODO(b/65049598) make ConstructionException checked, which will make this checked as well.
   static class NotAnOptionException extends ConstructionException {
@@ -304,6 +304,11 @@ public class OptionDefinition {
   }
 
   @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.
@@ -74,6 +75,16 @@ public abstract class OptionValueDescription {
   }
 
   /**
+   * Returns the canonical instances of this option - the instances that affect the current value.
+   *
+   * <p>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<ParsedOptionDescription> 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<ParsedOptionDescription> getCanonicalInstances() {
+      return null;
+    }
   }
 
   /**
@@ -227,6 +243,16 @@ public abstract class OptionValueDescription {
       return null;
     }
 
+    @Override
+    ImmutableList<ParsedOptionDescription> 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<ParsedOptionDescription> 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<ParsedOptionDescription> 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<ParsedOptionDescription> 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<OptionValueDescription> asListOfEffectiveOptions() {
+  public List<ParsedOptionDescription> asListOfCanonicalOptions() {
+    return impl.asCanonicalizedListOfParsedOptions();
+  }
+
+  @Override
+  public List<OptionValueDescription> 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<ParsedOptionDescription> 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<OptionDefinition, ParsedOptionDescription> canonicalizeValues =
-      LinkedHashMultimap.create();
-
   private final List<String> warnings = new ArrayList<>();
 
   private boolean allowSingleDashLongOptions = false;
@@ -135,36 +124,28 @@ class OptionsParserImpl {
         .collect(toCollection(ArrayList::new));
   }
 
-  /**
-   * Implements {@link OptionsParser#canonicalize}.
-   */
-  List<String> asCanonicalizedList() {
-    return canonicalizeValues
-        .values()
+  private Stream<ParsedOptionDescription> 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<String> asCanonicalizedList() {
+    return asStreamOfCanonicalParsedOptions()
         .map(ParsedOptionDescription::getDeprecatedCanonicalForm)
-        .collect(toCollection(ArrayList::new));
+        .collect(ImmutableList.toImmutableList());
   }
 
-  /**
-   * Implements {@link OptionsParser#asListOfEffectiveOptions()}.
-   */
+  /** Implements {@link OptionsParser#canonicalize}. */
+  List<ParsedOptionDescription> asCanonicalizedListOfParsedOptions() {
+    return asStreamOfCanonicalParsedOptions().collect(ImmutableList.toImmutableList());
+  }
+
+  /** Implements {@link OptionsParser#asListOfOptionValues()}. */
   List<OptionValueDescription> asListOfEffectiveOptions() {
     List<OptionValueDescription> result = new ArrayList<>();
     for (Map.Entry<String, OptionDefinition> 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<ParsedOptionDescription> 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.
+   *
+   * <p>The list includes undocumented options.
+   */
+  List<ParsedOptionDescription> asListOfCanonicalOptions();
+
+  /**
+   * Returns a list of all options, including undocumented ones, and their effective values. There
+   * is no guaranteed ordering for the result.
    */
-  List<OptionValueDescription> asListOfEffectiveOptions();
+  List<OptionValueDescription> asListOfOptionValues();
 
   /**
    * Canonicalizes the list of options that this OptionsParser has parsed. The