Track expansions in OptionValueDescription.

Add warnings for behavior that is likely unexpected. Expansion values do not accept values at all, and implicit requirements are set regardless of whether the option was turned "on" or not, so warn in the cases where this weird behavior might rear its ugly head.

RELNOTES: None
PiperOrigin-RevId: 172883214

2 files changed, 106 insertions, 72 deletions

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 ed03e9776a..0d31137fa4 100644
--- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -15,7 +15,9 @@
 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;
 import com.google.common.collect.ListMultimap;
 import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.util.Collection;
@@ -47,10 +49,29 @@ public abstract class OptionValueDescription {
   /** Returns the source(s) of this option, if there were multiple, duplicates are removed. */
   public abstract String getSourceString();
 
-  abstract void addOptionInstance(
-      ParsedOptionDescription parsedOption,
-      List<String> warnings)
-      throws OptionsParsingException;
+  /**
+   * Add an instance of the option to this value. The various types of options are in charge of
+   * making sure that the value is correctly stored, with proper tracking of its priority and
+   * placement amongst other options.
+   *
+   * @return a bundle containing arguments that need to be parsed further.
+   */
+  abstract ExpansionBundle addOptionInstance(
+      ParsedOptionDescription parsedOption, List<String> warnings) throws OptionsParsingException;
+
+  /**
+   * Grouping of convenience for the options that expand to other options, to attach an
+   * option-appropriate source string along with the options that need to be parsed.
+   */
+  public static class ExpansionBundle {
+    List<String> expansionArgs;
+    String sourceOfExpansionArgs;
+
+    public ExpansionBundle(List<String> args, String source) {
+      expansionArgs = args;
+      sourceOfExpansionArgs = source;
+    }
+  }
 
   /**
    * For the given option, returns the correct type of OptionValueDescription, to which unparsed
@@ -59,11 +80,12 @@ public abstract class OptionValueDescription {
    * <p>The categories of option types are non-overlapping, an invariant checked by the
    * OptionProcessor at compile time.
    */
-  public static OptionValueDescription createOptionValueDescription(OptionDefinition option) {
-    if (option.allowsMultiple()) {
+  public static OptionValueDescription createOptionValueDescription(
+      OptionDefinition option, OptionsData optionsData) {
+    if (option.isExpansionOption()) {
+      return new ExpansionOptionValueDescription(option, optionsData);
+    } else if (option.allowsMultiple()) {
       return new RepeatableOptionValueDescription(option);
-    } else if (option.isExpansionOption()) {
-      return new ExpansionOptionValueDescription(option);
     } else if (option.hasImplicitRequirements()) {
       return new OptionWithImplicitRequirementsValueDescription(option);
     } else if (option.isWrapperOption()) {
@@ -98,9 +120,7 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    void addOptionInstance(
-        ParsedOptionDescription parsedOption,
-        List<String> warnings) {
+    ExpansionBundle addOptionInstance(ParsedOptionDescription parsedOption, List<String> warnings) {
       throw new IllegalStateException(
           "Cannot add values to the default option value. Create a modifiable "
               + "OptionValueDescription using createOptionValueDescription() instead.");
@@ -139,15 +159,13 @@ public abstract class OptionValueDescription {
 
     // Warnings should not end with a '.' because the internal reporter adds one automatically.
     @Override
-    void addOptionInstance(
-        ParsedOptionDescription parsedOption,
-        List<String> warnings)
+    ExpansionBundle addOptionInstance(ParsedOptionDescription parsedOption, List<String> warnings)
         throws OptionsParsingException {
       // This might be the first value, in that case, just store it!
       if (effectiveOptionInstance == null) {
         effectiveOptionInstance = parsedOption;
         effectiveValue = effectiveOptionInstance.getConvertedValue();
-        return;
+        return null;
       }
 
       // If there was another value, check whether the new one will override it, and if so,
@@ -190,8 +208,10 @@ public abstract class OptionValueDescription {
             // 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",
-                    expandedFrom, optionDefinition));
+                    "%s was expanded and now overrides a previous explicitly specified %s with %s",
+                    expandedFrom,
+                    effectiveOptionInstance.getCommandLineForm(),
+                    parsedOption.getCommandLineForm()));
           } else if ((optionThatExpandedToEffectiveValue != null) && (expandedFrom != null)) {
             warnings.add(
                 String.format(
@@ -204,6 +224,7 @@ public abstract class OptionValueDescription {
         effectiveOptionInstance = parsedOption;
         effectiveValue = newValue;
       }
+      return null;
     }
 
     @VisibleForTesting
@@ -254,9 +275,7 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    void addOptionInstance(
-        ParsedOptionDescription parsedOption,
-        List<String> warnings)
+    ExpansionBundle addOptionInstance(ParsedOptionDescription parsedOption, List<String> warnings)
         throws OptionsParsingException {
       // For repeatable options, we allow flags that take both single values and multiple values,
       // potentially collapsing them down.
@@ -268,6 +287,7 @@ public abstract class OptionValueDescription {
       } else {
         optionValues.put(priority, convertedValue);
       }
+      return null;
     }
   }
 
@@ -277,9 +297,12 @@ public abstract class OptionValueDescription {
    * in {@link Option#expansion()} and flags with an {@link Option#expansionFunction()}.
    */
   static class ExpansionOptionValueDescription extends OptionValueDescription {
+    private final List<String> expansion;
 
-    private ExpansionOptionValueDescription(OptionDefinition optionDefinition) {
+    private ExpansionOptionValueDescription(
+        OptionDefinition optionDefinition, OptionsData optionsData) {
       super(optionDefinition);
+      this.expansion = optionsData.getEvaluatedExpansion(optionDefinition);
       if (!optionDefinition.isExpansionOption()) {
         throw new ConstructionException(
             "Options without expansions can't be tracked using ExpansionOptionValueDescription");
@@ -297,11 +320,22 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    void addOptionInstance(
-        ParsedOptionDescription parsedOption,
-        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.
+    ExpansionBundle addOptionInstance(ParsedOptionDescription parsedOption, List<String> warnings) {
+      if (parsedOption.getUnconvertedValue() != null
+          && !parsedOption.getUnconvertedValue().isEmpty()) {
+        warnings.add(
+            String.format(
+                "%s is an expansion option. It does not accept values, and does not change its "
+                    + "expansion based on the value provided. Value '%s' will be ignored.",
+                optionDefinition, parsedOption.getUnconvertedValue()));
+      }
+
+      return new ExpansionBundle(
+          expansion,
+          (parsedOption.getSource() == null)
+              ? String.format("expanded from %s", optionDefinition)
+              : String.format(
+                  "expanded from %s (source %s)", optionDefinition, parsedOption.getSource()));
     }
   }
 
@@ -318,17 +352,34 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    void addOptionInstance(
-        ParsedOptionDescription parsedOption,
-        List<String> warnings)
+    ExpansionBundle addOptionInstance(ParsedOptionDescription parsedOption, List<String> warnings)
         throws OptionsParsingException {
       // This is a valued flag, its value is handled the same way as a normal
-      // SingleOptionValueDescription.
-      super.addOptionInstance(parsedOption, warnings);
+      // SingleOptionValueDescription. (We check at compile time that these flags aren't
+      // "allowMultiple")
+      ExpansionBundle superExpansion = super.addOptionInstance(parsedOption, warnings);
+      Preconditions.checkArgument(
+          superExpansion == null, "SingleOptionValueDescription should not expand to anything.");
+      if (parsedOption.getConvertedValue().equals(optionDefinition.getDefaultValue())) {
+        warnings.add(
+            String.format(
+                "%s sets %s to its default value. Since this option has implicit requirements that "
+                    + "are set whenever the option is explicitly provided, regardless of the "
+                    + "value, this will behave differently than letting a default be a default. "
+                    + "Specifically, this options expands to {%s}.",
+                parsedOption.getCommandLineForm(),
+                optionDefinition,
+                String.join(" ", optionDefinition.getImplicitRequirements())));
+      }
 
       // Now deal with the implicit requirements.
-      // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
-      // and track their link to the options they implicitly expanded to to.
+      return new ExpansionBundle(
+          ImmutableList.copyOf(optionDefinition.getImplicitRequirements()),
+          (parsedOption.getSource() == null)
+              ? String.format("implicit requirement of %s", optionDefinition)
+              : String.format(
+                  "implicit requirement of %s (source %s)",
+                  optionDefinition, parsedOption.getSource()));
     }
   }
 
@@ -350,12 +401,22 @@ public abstract class OptionValueDescription {
     }
 
     @Override
-    void addOptionInstance(
-        ParsedOptionDescription parsedOption,
-        List<String> warnings)
+    ExpansionBundle addOptionInstance(ParsedOptionDescription parsedOption, List<String> warnings)
         throws OptionsParsingException {
-      // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
-      // and track their link to the options they implicitly expanded to to.
+      if (!parsedOption.getUnconvertedValue().startsWith("-")) {
+        throw new OptionsParsingException(
+            String.format(
+                "Invalid value format for %s. You may have meant --%s=--%s",
+                optionDefinition,
+                optionDefinition.getOptionName(),
+                parsedOption.getUnconvertedValue()));
+      }
+      return new ExpansionBundle(
+          ImmutableList.of(parsedOption.getUnconvertedValue()),
+          (parsedOption.getSource() == null)
+              ? String.format("unwrapped from %s", optionDefinition)
+              : String.format(
+                  "unwrapped from %s (source %s)", optionDefinition, parsedOption.getSource()));
     }
   }
 }
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 78bfa07ac5..b543328c5c 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -24,6 +24,7 @@ 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;
@@ -381,8 +382,9 @@ class OptionsParserImpl {
     // the OptionValueDescription.
     OptionValueDescription entry =
         optionValues.computeIfAbsent(
-            optionDefinition, OptionValueDescription::createOptionValueDescription);
-    entry.addOptionInstance(parsedOption, warnings);
+            optionDefinition,
+            def -> OptionValueDescription.createOptionValueDescription(def, optionsData));
+    ExpansionBundle expansionBundle = entry.addOptionInstance(parsedOption, warnings);
     @Nullable String unconvertedValue = parsedOption.getUnconvertedValue();
 
     // There are 3 types of flags that expand to other flag values. Expansion flags are the
@@ -406,43 +408,14 @@ class OptionsParserImpl {
       }
     }
 
-    if (optionDefinition.isExpansionOption()
-        || optionDefinition.hasImplicitRequirements()
-        || optionDefinition.isWrapperOption()) {
-      StringBuilder sourceMessage = new StringBuilder();
-      ImmutableList<String> expansionArgs;
-      if (optionDefinition.isExpansionOption()) {
-        expansionArgs = optionsData.getEvaluatedExpansion(optionDefinition);
-        sourceMessage.append("expanded from option ");
-      } else if (optionDefinition.hasImplicitRequirements()) {
-        expansionArgs = ImmutableList.copyOf(optionDefinition.getImplicitRequirements());
-        sourceMessage.append("implicit requirement of option ");
-      } else {
-        if (!unconvertedValue.startsWith("-")) {
-          // Wrapper options are the only "expansion" flag type that expand to other flags
-          // according to user input, so a malformed option is a user error in this case.
-          throw new OptionsParsingException(
-              String.format(
-                  "Invalid value format for %s. You may have meant --%s=--%s",
-                  optionDefinition, optionDefinition.getOptionName(), unconvertedValue));
-        } else {
-          expansionArgs = ImmutableList.of(unconvertedValue);
-          sourceMessage.append("unwrapped from wrapper option ");
-        }
-      }
-      String source = parsedOption.getSource();
-      sourceMessage.append(
-          (source == null)
-              ? String.format("--%s", optionDefinition.getOptionName())
-              : String.format("--%s from %s", optionDefinition.getOptionName(), source));
-
+    if (expansionBundle != null) {
       List<String> unparsed =
           parse(
               parsedOption.getPriority(),
-              o -> sourceMessage.toString(),
+              o -> expansionBundle.sourceOfExpansionArgs,
               optionDefinition.hasImplicitRequirements() ? optionDefinition : null,
               optionDefinition.isExpansionOption() ? optionDefinition : null,
-              expansionArgs);
+              expansionBundle.expansionArgs);
       if (!unparsed.isEmpty()) {
         if (optionDefinition.isWrapperOption()) {
           throw new OptionsParsingException(