Warn about config expansions as we do for other expansions.

If an expanded value overrides an explicit value, users who do not know the contents of the expansion may be surprised. We already warned about this for hard-coded expansions, and this is now applicable for --config expansions as well.

This will only warn when a single-valued option has its value replaced. Options that accumulate multiple values in a list (e.g., --copt) will silently include both explicit and expanded values.

RELNOTES: None.
PiperOrigin-RevId: 179857526

5 files changed, 99 insertions, 51 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
index 584e75b9fc..b0782f83e7 100644
--- a/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java
+++ b/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java
@@ -22,14 +22,14 @@ import javax.annotation.Nullable;
 public class OptionInstanceOrigin {
   private final OptionPriority priority;
   @Nullable private final String source;
-  @Nullable private final OptionDefinition implicitDependent;
-  @Nullable private final OptionDefinition expandedFrom;
+  @Nullable private final ParsedOptionDescription implicitDependent;
+  @Nullable private final ParsedOptionDescription expandedFrom;
 
   public OptionInstanceOrigin(
       OptionPriority priority,
       String source,
-      OptionDefinition implicitDependent,
-      OptionDefinition expandedFrom) {
+      ParsedOptionDescription implicitDependent,
+      ParsedOptionDescription expandedFrom) {
     this.priority = priority;
     this.source = source;
     this.implicitDependent = implicitDependent;
@@ -46,12 +46,12 @@ public class OptionInstanceOrigin {
   }
 
   @Nullable
-  public OptionDefinition getImplicitDependent() {
+  public ParsedOptionDescription getImplicitDependent() {
     return implicitDependent;
   }
 
   @Nullable
-  public OptionDefinition getExpandedFrom() {
+  public ParsedOptionDescription 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 3fde1389d0..f52e8a57a2 100644
--- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -185,11 +185,11 @@ public abstract class OptionValueDescription {
       // 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 =
+        ParsedOptionDescription implicitDependent = parsedOption.getImplicitDependent();
+        ParsedOptionDescription expandedFrom = parsedOption.getExpandedFrom();
+        ParsedOptionDescription optionThatDependsOnEffectiveValue =
             effectiveOptionInstance.getImplicitDependent();
-        OptionDefinition optionThatExpandedToEffectiveValue =
+        ParsedOptionDescription optionThatExpandedToEffectiveValue =
             effectiveOptionInstance.getExpandedFrom();
 
         Object newValue = parsedOption.getConvertedValue();
@@ -225,7 +225,7 @@ 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 with %s",
+                    "%s was expanded and now overrides the explicit option %s with %s",
                     expandedFrom,
                     effectiveOptionInstance.getCommandLineForm(),
                     parsedOption.getCommandLineForm()));
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index fb7161ca46..b7da00466e 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -619,13 +619,26 @@ public class OptionsParser implements OptionsProvider {
     }
   }
 
-  public void parseOptionsFixedAtSpecificPriority(
-      OptionPriority priority, String source, List<String> args) throws OptionsParsingException {
-    Preconditions.checkNotNull(priority, "Priority not specified for arglist " + args);
+  /**
+   * Parses the args at the priority of the provided option. This is useful for after-the-fact
+   * expansion.
+   *
+   * @param optionToExpand the option that is being "expanded" after the fact. The provided args
+   *     will have the same priority as this option.
+   * @param source a description of where the expansion arguments came from.
+   * @param args the arguments to parse as the expansion. Order matters, as the value of a flag may
+   *     be in the following argument.
+   */
+  public void parseArgsFixedAsExpansionOfOption(
+      ParsedOptionDescription optionToExpand, String source, List<String> args)
+      throws OptionsParsingException {
+    Preconditions.checkNotNull(
+        optionToExpand, "Option for expansion not specified for arglist " + args);
     Preconditions.checkArgument(
-        priority.getPriorityCategory() != OptionPriority.PriorityCategory.DEFAULT,
+        optionToExpand.getPriority().getPriorityCategory()
+            != OptionPriority.PriorityCategory.DEFAULT,
         "Priority cannot be default, which was specified for arglist " + args);
-    residue.addAll(impl.parseOptionsFixedAtSpecificPriority(priority, o -> source, args));
+    residue.addAll(impl.parseArgsFixedAsExpansionOfOption(optionToExpand, o -> source, args));
     if (!allowResidue && !residue.isEmpty()) {
       String errorMsg = "Unrecognized arguments: " + Joiner.on(' ').join(residue);
       throw new OptionsParsingException(errorMsg);
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 5ce35daf67..bc66cc38f8 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -204,30 +204,33 @@ class OptionsParserImpl {
    * OptionInstanceOrigin)}
    */
   ImmutableList<ParsedOptionDescription> getExpansionValueDescriptions(
-      OptionDefinition expansionFlag, OptionInstanceOrigin originOfExpansionFlag)
+      OptionDefinition expansionFlagDef, OptionInstanceOrigin originOfExpansionFlag)
       throws OptionsParsingException {
     ImmutableList.Builder<ParsedOptionDescription> builder = ImmutableList.builder();
     OptionInstanceOrigin originOfSubflags;
     ImmutableList<String> options;
-    if (expansionFlag.hasImplicitRequirements()) {
-      options = ImmutableList.copyOf(expansionFlag.getImplicitRequirements());
+    ParsedOptionDescription expansionFlagParsedDummy =
+        ParsedOptionDescription.newDummyInstance(expansionFlagDef, originOfExpansionFlag);
+    if (expansionFlagDef.hasImplicitRequirements()) {
+      options = ImmutableList.copyOf(expansionFlagDef.getImplicitRequirements());
       originOfSubflags =
           new OptionInstanceOrigin(
               originOfExpansionFlag.getPriority(),
               String.format(
                   "implicitly required by %s (source: %s)",
-                  expansionFlag, originOfExpansionFlag.getSource()),
-              expansionFlag,
+                  expansionFlagDef, originOfExpansionFlag.getSource()),
+              expansionFlagParsedDummy,
               null);
-    } else if (expansionFlag.isExpansionOption()) {
-      options = optionsData.getEvaluatedExpansion(expansionFlag);
+    } else if (expansionFlagDef.isExpansionOption()) {
+      options = optionsData.getEvaluatedExpansion(expansionFlagDef);
       originOfSubflags =
           new OptionInstanceOrigin(
               originOfExpansionFlag.getPriority(),
               String.format(
-                  "expanded by %s (source: %s)", expansionFlag, originOfExpansionFlag.getSource()),
+                  "expanded by %s (source: %s)",
+                  expansionFlagDef, originOfExpansionFlag.getSource()),
               null,
-              expansionFlag);
+              expansionFlagParsedDummy);
     } else {
       return ImmutableList.of();
     }
@@ -284,12 +287,19 @@ class OptionsParserImpl {
     }
   }
 
-  /** Parses the args at the fixed priority. */
-  List<String> parseOptionsFixedAtSpecificPriority(
-      OptionPriority priority, Function<OptionDefinition, String> sourceFunction, List<String> args)
+  /** Implements {@link OptionsParser#parseArgsFixedAsExpansionOfOption} */
+  List<String> parseArgsFixedAsExpansionOfOption(
+      ParsedOptionDescription optionToExpand,
+      Function<OptionDefinition, String> sourceFunction,
+      List<String> args)
       throws OptionsParsingException {
     ResidueAndPriority residueAndPriority =
-        parse(OptionPriority.getLockedPriority(priority), sourceFunction, null, null, args);
+        parse(
+            OptionPriority.getLockedPriority(optionToExpand.getPriority()),
+            sourceFunction,
+            null,
+            optionToExpand,
+            args);
     return residueAndPriority.residue;
   }
 
@@ -304,8 +314,8 @@ class OptionsParserImpl {
   private ResidueAndPriority parse(
       OptionPriority priority,
       Function<OptionDefinition, String> sourceFunction,
-      OptionDefinition implicitDependent,
-      OptionDefinition expandedFrom,
+      ParsedOptionDescription implicitDependent,
+      ParsedOptionDescription expandedFrom,
       List<String> args)
       throws OptionsParsingException {
     List<String> unparsedArgs = new ArrayList<>();
@@ -369,7 +379,7 @@ class OptionsParserImpl {
         priorityCategory);
 
     handleNewParsedOption(
-        new ParsedOptionDescription(
+        ParsedOptionDescription.newParsedOptionDescription(
             option,
             String.format("--%s=%s", option.getOptionName(), unconvertedValue),
             unconvertedValue,
@@ -411,8 +421,8 @@ class OptionsParserImpl {
           parse(
               OptionPriority.getLockedPriority(parsedOption.getPriority()),
               o -> expansionBundle.sourceOfExpansionArgs,
-              optionDefinition.hasImplicitRequirements() ? optionDefinition : null,
-              optionDefinition.isExpansionOption() ? optionDefinition : null,
+              optionDefinition.hasImplicitRequirements() ? parsedOption : null,
+              optionDefinition.isExpansionOption() ? parsedOption : null,
               expansionBundle.expansionArgs);
       if (!residueAndPriority.residue.isEmpty()) {
 
@@ -433,8 +443,8 @@ class OptionsParserImpl {
       Iterator<String> nextArgs,
       OptionPriority priority,
       Function<OptionDefinition, String> sourceFunction,
-      OptionDefinition implicitDependent,
-      OptionDefinition expandedFrom)
+      ParsedOptionDescription implicitDependent,
+      ParsedOptionDescription expandedFrom)
       throws OptionsParsingException {
 
     // Store the way this option was parsed on the command line.
@@ -510,7 +520,7 @@ class OptionsParserImpl {
       }
     }
 
-    return new ParsedOptionDescription(
+    return ParsedOptionDescription.newParsedOptionDescription(
         optionDefinition,
         commandLineForm.toString(),
         unconvertedValue,
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 f55f8addc7..5088153d56 100644
--- a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
+++ b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.common.options;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import java.util.function.Function;
 import javax.annotation.Nullable;
@@ -27,25 +28,49 @@ import javax.annotation.Nullable;
 public final class ParsedOptionDescription {
 
   private final OptionDefinition optionDefinition;
-  private final String commandLineForm;
+  @Nullable private final String commandLineForm;
   @Nullable private final String unconvertedValue;
   private final OptionInstanceOrigin origin;
 
-  public ParsedOptionDescription(
+  private ParsedOptionDescription(
       OptionDefinition optionDefinition,
-      String commandLineForm,
+      @Nullable String commandLineForm,
       @Nullable String unconvertedValue,
       OptionInstanceOrigin origin) {
-    this.optionDefinition = optionDefinition;
+    this.optionDefinition = Preconditions.checkNotNull(optionDefinition);
     this.commandLineForm = commandLineForm;
     this.unconvertedValue = unconvertedValue;
-    this.origin = origin;
+    this.origin = Preconditions.checkNotNull(origin);
+  }
+
+  static ParsedOptionDescription newParsedOptionDescription(
+      OptionDefinition optionDefinition,
+      String commandLineForm,
+      @Nullable String unconvertedValue,
+      OptionInstanceOrigin origin) {
+    // An actual ParsedOptionDescription should always have a form in which it was parsed, but some
+    // options, such as expansion options, legitimately have no value.
+    return new ParsedOptionDescription(
+        optionDefinition,
+        Preconditions.checkNotNull(commandLineForm),
+        unconvertedValue,
+        origin);
+  }
+
+  /**
+   * This factory should be used when there is no actual parsed option, since in those cases we do
+   * not have an original value or form that the option took.
+   */
+  static ParsedOptionDescription newDummyInstance(
+      OptionDefinition optionDefinition, OptionInstanceOrigin origin) {
+    return new ParsedOptionDescription(optionDefinition, null, null, origin);
   }
 
   public OptionDefinition getOptionDefinition() {
     return optionDefinition;
   }
 
+  @Nullable
   public String getCommandLineForm() {
     return commandLineForm;
   }
@@ -127,11 +152,11 @@ public final class ParsedOptionDescription {
     return origin.getSource();
   }
 
-  OptionDefinition getImplicitDependent() {
+  ParsedOptionDescription getImplicitDependent() {
     return origin.getImplicitDependent();
   }
 
-  OptionDefinition getExpandedFrom() {
+  ParsedOptionDescription getExpandedFrom() {
     return origin.getExpandedFrom();
   }
 
@@ -152,14 +177,14 @@ public final class ParsedOptionDescription {
 
   @Override
   public String toString() {
-    StringBuilder result = new StringBuilder();
-    result.append(optionDefinition);
-    result.append("set to '").append(unconvertedValue).append("' ");
-    result.append("with priority ").append(origin.getPriority());
-    if (origin.getSource() != null) {
-      result.append(" and source '").append(origin.getSource()).append("'");
+    // Check that a dummy value-less option instance does not output all the default information.
+    if (commandLineForm == null) {
+      return optionDefinition.toString();
     }
-    return result.toString();
+    String source = origin.getSource();
+    return String.format(
+        "option '%s'%s",
+        commandLineForm, source == null ? "" : String.format(" (source %s)", source));
   }
 
 }