Switch on tracking of specific option priorities.

Make sure that multiple calls to parse() follow each other sequentially. This is necessary for blazerc expansion, which occurs first in command order, then blazerc order.

RELNOTES: None.
PiperOrigin-RevId: 174343241

3 files changed, 83 insertions, 37 deletions

diff --git a/src/main/java/com/google/devtools/common/options/OptionPriority.java b/src/main/java/com/google/devtools/common/options/OptionPriority.java
index 96c471ed74..ec5d0d8f32 100644
--- a/src/main/java/com/google/devtools/common/options/OptionPriority.java
+++ b/src/main/java/com/google/devtools/common/options/OptionPriority.java
@@ -26,18 +26,42 @@ import java.util.Objects;
 public class OptionPriority implements Comparable<OptionPriority> {
   private final PriorityCategory priorityCategory;
   private final int index;
+  private final boolean locked;
 
-  private OptionPriority(PriorityCategory priorityCategory, int index) {
+  private OptionPriority(PriorityCategory priorityCategory, int index, boolean locked) {
     this.priorityCategory = priorityCategory;
     this.index = index;
+    this.locked = locked;
   }
 
-  public static OptionPriority lowestOptionPriorityAtCategory(PriorityCategory category) {
-    return new OptionPriority(category, 0);
+  /** Get the first OptionPriority for that category. */
+  static OptionPriority lowestOptionPriorityAtCategory(PriorityCategory category) {
+    return new OptionPriority(category, 0, false);
   }
 
-  public static OptionPriority nextOptionPriority(OptionPriority priority) {
-    return new OptionPriority(priority.priorityCategory, priority.index + 1);
+  /**
+   * Get the priority for the option following this one. In normal, incremental option parsing, the
+   * returned priority would compareTo as after the current one. Does not increment locked
+   * priorities.
+   */
+  static OptionPriority nextOptionPriority(OptionPriority priority) {
+    if (priority.locked) {
+      return priority;
+    }
+    return new OptionPriority(priority.priorityCategory, priority.index + 1, false);
+  }
+
+  /**
+   * Return a priority for this option that will avoid priority increases by calls to
+   * nextOptionPriority.
+   *
+   * <p>Some options are expanded in-place, and need to be all parsed at the priority of the
+   * original option. In this case, parsing one of these after another should not cause the option
+   * to be considered as higher priority than the ones before it (this would cause overlap between
+   * the expansion of --expansion_flag and a option following it in the same list of options).
+   */
+  public static OptionPriority getLockedPriority(OptionPriority priority) {
+    return new OptionPriority(priority.priorityCategory, priority.index, true);
   }
 
   public PriorityCategory getPriorityCategory() {
@@ -66,6 +90,11 @@ public class OptionPriority implements Comparable<OptionPriority> {
     return Objects.hash(priorityCategory, index);
   }
 
+  @Override
+  public String toString() {
+    return String.format("OptionPriority(%s,%s)", priorityCategory, index);
+  }
+
   /**
    * The priority of option values, in order of increasing priority.
    *
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 30c4377b00..9864a46945 100644
--- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.common.options;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
@@ -114,7 +113,7 @@ public abstract class OptionValueDescription {
     return new DefaultOptionValueDescription(option);
   }
 
-  static class DefaultOptionValueDescription extends OptionValueDescription {
+  private static class DefaultOptionValueDescription extends OptionValueDescription {
 
     private DefaultOptionValueDescription(OptionDefinition optionDefinition) {
       super(optionDefinition);
@@ -147,7 +146,7 @@ public abstract class OptionValueDescription {
    * The form of a value for a default type of flag, one that does not accumulate multiple values
    * and has no expansion.
    */
-  static class SingleOptionValueDescription extends OptionValueDescription {
+  private static class SingleOptionValueDescription extends OptionValueDescription {
     private ParsedOptionDescription effectiveOptionInstance;
     private Object effectiveValue;
 
@@ -199,6 +198,11 @@ public abstract class OptionValueDescription {
         // Output warnings if there is conflicting options set different values in a way that might
         // not have been obvious to the user, such as through expansions and implicit requirements.
         if (!effectiveValue.equals(newValue)) {
+          boolean samePriorityCategory =
+              parsedOption
+                  .getPriority()
+                  .getPriorityCategory()
+                  .equals(effectiveOptionInstance.getPriority().getPriorityCategory());
           if ((implicitDependent != null) && (optionThatDependsOnEffectiveValue != null)) {
             if (!implicitDependent.equals(optionThatDependsOnEffectiveValue)) {
               warnings.add(
@@ -206,8 +210,7 @@ public abstract class OptionValueDescription {
                       "%s is implicitly defined by both %s and %s",
                       optionDefinition, optionThatDependsOnEffectiveValue, implicitDependent));
             }
-          } else if ((implicitDependent != null)
-              && parsedOption.getPriority().equals(effectiveOptionInstance.getPriority())) {
+          } else if ((implicitDependent != null) && samePriorityCategory) {
             warnings.add(
                 String.format(
                     "%s is implicitly defined by %s; the implicitly set value "
@@ -219,7 +222,7 @@ public abstract class OptionValueDescription {
                     "A new value for %s overrides a previous implicit setting of that "
                         + "option by %s",
                     optionDefinition, optionThatDependsOnEffectiveValue));
-          } else if ((parsedOption.getPriority().equals(effectiveOptionInstance.getPriority()))
+          } else if (samePriorityCategory
               && ((optionThatExpandedToEffectiveValue == null) && (expandedFrom != null))) {
             // Create a warning if an expansion option overrides an explicit option:
             warnings.add(
@@ -252,15 +255,10 @@ public abstract class OptionValueDescription {
       }
       return ImmutableList.of();
     }
-
-    @VisibleForTesting
-    ParsedOptionDescription getEffectiveOptionInstance() {
-      return effectiveOptionInstance;
-    }
   }
 
   /** The form of a value for an option that accumulates multiple values on the command line. */
-  static class RepeatableOptionValueDescription extends OptionValueDescription {
+  private static class RepeatableOptionValueDescription extends OptionValueDescription {
     ListMultimap<OptionPriority, ParsedOptionDescription> parsedOptions;
     ListMultimap<OptionPriority, Object> optionValues;
 
@@ -334,7 +332,7 @@ public abstract class OptionValueDescription {
    * in place to other options. This should be used for both flags with a static expansion defined
    * in {@link Option#expansion()} and flags with an {@link Option#expansionFunction()}.
    */
-  static class ExpansionOptionValueDescription extends OptionValueDescription {
+  private static class ExpansionOptionValueDescription extends OptionValueDescription {
     private final List<String> expansion;
 
     private ExpansionOptionValueDescription(
@@ -385,7 +383,8 @@ public abstract class OptionValueDescription {
   }
 
   /** The form of a value for a flag with implicit requirements. */
-  static class OptionWithImplicitRequirementsValueDescription extends SingleOptionValueDescription {
+  private static class OptionWithImplicitRequirementsValueDescription
+      extends SingleOptionValueDescription {
 
     private OptionWithImplicitRequirementsValueDescription(OptionDefinition optionDefinition) {
       super(optionDefinition);
@@ -429,7 +428,7 @@ public abstract class OptionValueDescription {
   }
 
   /** Form for options that contain other options in the value text to which they expand. */
-  static final class WrapperOptionValueDescription extends OptionValueDescription {
+  private static final class WrapperOptionValueDescription extends OptionValueDescription {
 
     WrapperOptionValueDescription(OptionDefinition optionDefinition) {
       super(optionDefinition);
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 e8086c0680..96f70f3a2e 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -32,6 +32,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
 
@@ -70,15 +71,19 @@ class OptionsParserImpl {
 
   private final List<String> warnings = new ArrayList<>();
 
+  /**
+   * Since parse() expects multiple calls to it with the same {@link PriorityCategory} to be treated
+   * as though the args in the later call have higher priority over the earlier calls, we need to
+   * track the high water mark of option priority at each category. Each call to parse will start at
+   * this level.
+   */
+  private final Map<PriorityCategory, OptionPriority> nextPriorityPerPriorityCategory =
+      Stream.of(PriorityCategory.values())
+          .collect(Collectors.toMap(p -> p, OptionPriority::lowestOptionPriorityAtCategory));
+
   private boolean allowSingleDashLongOptions = false;
 
-  private ArgsPreProcessor argsPreProcessor =
-      new ArgsPreProcessor() {
-        @Override
-        public List<String> preProcess(List<String> args) throws OptionsParsingException {
-          return args;
-        }
-      };
+  private ArgsPreProcessor argsPreProcessor = args -> args;
 
   /** Create a new parser object. Do not accept a null OptionsData object. */
   OptionsParserImpl(OptionsData optionsData) {
@@ -261,12 +266,24 @@ class OptionsParserImpl {
    * order.
    */
   List<String> parse(
-      OptionPriority.PriorityCategory priority,
+      PriorityCategory priorityCat,
       Function<OptionDefinition, String> sourceFunction,
       List<String> args)
       throws OptionsParsingException {
-    return parse(
-        OptionPriority.lowestOptionPriorityAtCategory(priority), sourceFunction, null, null, args);
+    ResidueAndPriority residueAndPriority =
+        parse(nextPriorityPerPriorityCategory.get(priorityCat), sourceFunction, null, null, args);
+    nextPriorityPerPriorityCategory.put(priorityCat, residueAndPriority.nextPriority);
+    return residueAndPriority.residue;
+  }
+
+  private static final class ResidueAndPriority {
+    List<String> residue;
+    OptionPriority nextPriority;
+
+    public ResidueAndPriority(List<String> residue, OptionPriority nextPriority) {
+      this.residue = residue;
+      this.nextPriority = nextPriority;
+    }
   }
 
   /**
@@ -277,7 +294,7 @@ class OptionsParserImpl {
    * <p>The method treats options that have neither an implicitDependent nor an expandedFrom value
    * as explicitly set.
    */
-  private List<String> parse(
+  private ResidueAndPriority parse(
       OptionPriority priority,
       Function<OptionDefinition, String> sourceFunction,
       OptionDefinition implicitDependent,
@@ -304,6 +321,7 @@ class OptionsParserImpl {
           identifyOptionAndPossibleArgument(
               arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom);
       handleNewParsedOption(parsedOption);
+      priority = OptionPriority.nextOptionPriority(priority);
     }
 
     // Go through the final values and make sure they are valid values for their option. Unlike any
@@ -313,7 +331,7 @@ class OptionsParserImpl {
       valueDescription.getValue();
     }
 
-    return unparsedArgs;
+    return new ResidueAndPriority(unparsedArgs, priority);
   }
 
   /**
@@ -383,20 +401,20 @@ class OptionsParserImpl {
     }
 
     if (expansionBundle != null) {
-      List<String> unparsed =
+      ResidueAndPriority residueAndPriority =
           parse(
-              parsedOption.getPriority(),
+              OptionPriority.getLockedPriority(parsedOption.getPriority()),
               o -> expansionBundle.sourceOfExpansionArgs,
               optionDefinition.hasImplicitRequirements() ? optionDefinition : null,
               optionDefinition.isExpansionOption() ? optionDefinition : null,
               expansionBundle.expansionArgs);
-      if (!unparsed.isEmpty()) {
+      if (!residueAndPriority.residue.isEmpty()) {
         if (optionDefinition.isWrapperOption()) {
           throw new OptionsParsingException(
               "Unparsed options remain after unwrapping "
                   + unconvertedValue
                   + ": "
-                  + Joiner.on(' ').join(unparsed));
+                  + Joiner.on(' ').join(residueAndPriority.residue));
         } else {
           // Throw an assertion here, because this indicates an error in the definition of this
           // option's expansion or requirements, not with the input as provided by the user.
@@ -404,7 +422,7 @@ class OptionsParserImpl {
               "Unparsed options remain after processing "
                   + unconvertedValue
                   + ": "
-                  + Joiner.on(' ').join(unparsed));
+                  + Joiner.on(' ').join(residueAndPriority.residue));
         }
       }
     }