Track the origin of an option in the option instance, not its final value.

A single instance of an option has a single origin, but the final value only has a single origin if it has a single value. For multi-valued options, it is wrong to expect that the final value of an option will have a single parent. Track the option parents (which option expanded to the current instance, if any) in the right place, with the ParsedOptionDescription.

Also fix some inconsistent spelling of 'dependent,' in favor of the American English standard.

RELNOTES: None.
PiperOrigin-RevId: 169487515

4 files changed, 135 insertions, 85 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
new file mode 100644
index 0000000000..584e75b9fc
--- /dev/null
+++ b/src/main/java/com/google/devtools/common/options/OptionInstanceOrigin.java
@@ -0,0 +1,57 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.common.options;
+
+import javax.annotation.Nullable;
+
+/**
+ * Contains metadata describing the origin of an option. This includes its priority, a message about
+ * where it came from, and whether it was set explicitly or expanded/implied by other flags.
+ */
+public class OptionInstanceOrigin {
+  private final OptionPriority priority;
+  @Nullable private final String source;
+  @Nullable private final OptionDefinition implicitDependent;
+  @Nullable private final OptionDefinition expandedFrom;
+
+  public OptionInstanceOrigin(
+      OptionPriority priority,
+      String source,
+      OptionDefinition implicitDependent,
+      OptionDefinition expandedFrom) {
+    this.priority = priority;
+    this.source = source;
+    this.implicitDependent = implicitDependent;
+    this.expandedFrom = expandedFrom;
+  }
+
+  public OptionPriority getPriority() {
+    return priority;
+  }
+
+  @Nullable
+  public String getSource() {
+    return source;
+  }
+
+  @Nullable
+  public OptionDefinition getImplicitDependent() {
+    return implicitDependent;
+  }
+
+  @Nullable
+  public OptionDefinition 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 8222aee12b..0d81d49a84 100644
--- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -26,9 +26,7 @@ import java.util.stream.Collectors;
 /**
  * The value of an option.
  *
- * <p>This takes care of tracking the final value as multiple instances of an option are parsed. It
- * also tracks additional metadata describing its priority, source, whether it was set via an
- * implicit dependency, and if so, by which other option.
+ * <p>This takes care of tracking the final value as multiple instances of an option are parsed.
  */
 public abstract class OptionValueDescription {
 
@@ -48,12 +46,8 @@ public abstract class OptionValueDescription {
   /** Returns the source(s) of this option, if there were multiple, duplicates are removed. */
   public abstract String getSourceString();
 
-  // TODO(b/65540004) implicitDependant and expandedFrom are artifacts of an option instance, and
-  // should be in ParsedOptionDescription.
   abstract void addOptionInstance(
       ParsedOptionDescription parsedOption,
-      OptionDefinition implicitDependant,
-      OptionDefinition expandedFrom,
       List<String> warnings)
       throws OptionsParsingException;
 
@@ -105,8 +99,6 @@ public abstract class OptionValueDescription {
     @Override
     void addOptionInstance(
         ParsedOptionDescription parsedOption,
-        OptionDefinition implicitDependant,
-        OptionDefinition expandedFrom,
         List<String> warnings) {
       throw new IllegalStateException(
           "Cannot add values to the default option value. Create a modifiable "
@@ -121,8 +113,6 @@ public abstract class OptionValueDescription {
   static class SingleOptionValueDescription extends OptionValueDescription {
     private ParsedOptionDescription effectiveOptionInstance;
     private Object effectiveValue;
-    private OptionDefinition optionThatDependsOnEffectiveValue;
-    private OptionDefinition optionThatExpandedToEffectiveValue;
 
     private SingleOptionValueDescription(OptionDefinition optionDefinition) {
       super(optionDefinition);
@@ -134,8 +124,6 @@ public abstract class OptionValueDescription {
       }
       effectiveOptionInstance = null;
       effectiveValue = null;
-      optionThatDependsOnEffectiveValue = null;
-      optionThatExpandedToEffectiveValue = null;
     }
 
     @Override
@@ -152,15 +140,11 @@ public abstract class OptionValueDescription {
     @Override
     void addOptionInstance(
         ParsedOptionDescription parsedOption,
-        OptionDefinition implicitDependant,
-        OptionDefinition expandedFrom,
         List<String> warnings)
         throws OptionsParsingException {
       // This might be the first value, in that case, just store it!
       if (effectiveOptionInstance == null) {
         effectiveOptionInstance = parsedOption;
-        optionThatDependsOnEffectiveValue = implicitDependant;
-        optionThatExpandedToEffectiveValue = expandedFrom;
         effectiveValue = effectiveOptionInstance.getConvertedValue();
         return;
       }
@@ -168,23 +152,31 @@ public abstract class OptionValueDescription {
       // If there was another value, check whether the new one will override it, and if so,
       // 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 =
+            effectiveOptionInstance.getImplicitDependent();
+        OptionDefinition optionThatExpandedToEffectiveValue =
+            effectiveOptionInstance.getExpandedFrom();
+
         // Output warnings:
-        if ((implicitDependant != null) && (optionThatDependsOnEffectiveValue != null)) {
-          if (!implicitDependant.equals(optionThatDependsOnEffectiveValue)) {
+        if ((implicitDependent != null) && (optionThatDependsOnEffectiveValue != null)) {
+          if (!implicitDependent.equals(optionThatDependsOnEffectiveValue)) {
             warnings.add(
                 String.format(
                     "Option '%s' is implicitly defined by both option '%s' and option '%s'",
                     optionDefinition.getOptionName(),
                     optionThatDependsOnEffectiveValue.getOptionName(),
-                    implicitDependant.getOptionName()));
+                    implicitDependent.getOptionName()));
           }
-        } else if ((implicitDependant != null)
+        } else if ((implicitDependent != null)
             && parsedOption.getPriority().equals(effectiveOptionInstance.getPriority())) {
           warnings.add(
               String.format(
                   "Option '%s' is implicitly defined by option '%s'; the implicitly set value "
                       + "overrides the previous one",
-                  optionDefinition.getOptionName(), implicitDependant.getOptionName()));
+                  optionDefinition.getOptionName(), implicitDependent.getOptionName()));
         } else if (optionThatDependsOnEffectiveValue != null) {
           warnings.add(
               String.format(
@@ -211,8 +203,6 @@ public abstract class OptionValueDescription {
 
         // Record the new value:
         effectiveOptionInstance = parsedOption;
-        optionThatDependsOnEffectiveValue = implicitDependant;
-        optionThatExpandedToEffectiveValue = expandedFrom;
         effectiveValue = parsedOption.getConvertedValue();
       } else {
         // The new value does not override the old value, as it has lower priority.
@@ -275,17 +265,17 @@ public abstract class OptionValueDescription {
     @Override
     void addOptionInstance(
         ParsedOptionDescription parsedOption,
-        OptionDefinition implicitDependant,
-        OptionDefinition expandedFrom,
         List<String> warnings)
         throws OptionsParsingException {
       // For repeatable options, we allow flags that take both single values and multiple values,
       // potentially collapsing them down.
       Object convertedValue = parsedOption.getConvertedValue();
+      OptionPriority priority = parsedOption.getPriority();
+      parsedOptions.put(priority, parsedOption);
       if (convertedValue instanceof List<?>) {
-        optionValues.putAll(parsedOption.getPriority(), (List<?>) convertedValue);
+        optionValues.putAll(priority, (List<?>) convertedValue);
       } else {
-        optionValues.put(parsedOption.getPriority(), convertedValue);
+        optionValues.put(priority, convertedValue);
       }
     }
   }
@@ -318,8 +308,6 @@ public abstract class OptionValueDescription {
     @Override
     void addOptionInstance(
         ParsedOptionDescription parsedOption,
-        OptionDefinition implicitDependant,
-        OptionDefinition expandedFrom,
         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.
@@ -341,13 +329,11 @@ public abstract class OptionValueDescription {
     @Override
     void addOptionInstance(
         ParsedOptionDescription parsedOption,
-        OptionDefinition implicitDependant,
-        OptionDefinition expandedFrom,
         List<String> warnings)
         throws OptionsParsingException {
       // This is a valued flag, its value is handled the same way as a normal
       // SingleOptionValueDescription.
-      super.addOptionInstance(parsedOption, implicitDependant, expandedFrom, warnings);
+      super.addOptionInstance(parsedOption, warnings);
 
       // Now deal with the implicit requirements.
       // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
@@ -375,8 +361,6 @@ public abstract class OptionValueDescription {
     @Override
     void addOptionInstance(
         ParsedOptionDescription parsedOption,
-        OptionDefinition implicitDependant,
-        OptionDefinition expandedFrom,
         List<String> warnings)
         throws OptionsParsingException {
       // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
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 a7745f6fe6..176d51e66a 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -223,17 +223,17 @@ class OptionsParserImpl {
     return new OptionDescription(
         optionDefinition,
         optionsData.getExpansionDataForField(optionDefinition),
-        getImplicitDependantDescriptions(
+        getImplicitDependentDescriptions(
             ImmutableList.copyOf(optionDefinition.getImplicitRequirements()),
             optionDefinition,
             priority,
             source));
   }
 
-  /** @return A list of the descriptions corresponding to the implicit dependant flags passed in. */
-  private ImmutableList<ParsedOptionDescription> getImplicitDependantDescriptions(
+  /** @return A list of the descriptions corresponding to the implicit dependent flags passed in. */
+  private ImmutableList<ParsedOptionDescription> getImplicitDependentDescriptions(
       ImmutableList<String> options,
-      OptionDefinition implicitDependant,
+      OptionDefinition implicitDependent,
       OptionPriority priority,
       String source)
       throws OptionsParsingException {
@@ -244,12 +244,17 @@ class OptionsParserImpl {
         o ->
             String.format(
                 "implicitely required for option %s (source: %s)",
-                implicitDependant.getOptionName(), source);
+                implicitDependent.getOptionName(), source);
     while (optionsIterator.hasNext()) {
       String unparsedFlagExpression = optionsIterator.next();
       ParsedOptionDescription parsedOption =
           identifyOptionAndPossibleArgument(
-              unparsedFlagExpression, optionsIterator, priority, sourceFunction, false);
+              unparsedFlagExpression,
+              optionsIterator,
+              priority,
+              sourceFunction,
+              implicitDependent,
+              null);
       builder.add(parsedOption);
     }
     return builder.build();
@@ -257,9 +262,7 @@ class OptionsParserImpl {
 
   /**
    * @return A list of the descriptions corresponding to options expanded from the flag for the
-   *     given value. These descriptions are are divorced from the command line - there is no
-   *     correct priority or source for these, as they are not actually set values. The value itself
-   *     is also a string, no conversion has taken place.
+   *     given value. The value itself is a string, no conversion has taken place.
    */
   ImmutableList<ParsedOptionDescription> getExpansionOptionValueDescriptions(
       OptionDefinition expansionFlag,
@@ -277,7 +280,12 @@ class OptionsParserImpl {
       String unparsedFlagExpression = optionsIterator.next();
       ParsedOptionDescription parsedOption =
           identifyOptionAndPossibleArgument(
-              unparsedFlagExpression, optionsIterator, priority, sourceFunction, false);
+              unparsedFlagExpression,
+              optionsIterator,
+              priority,
+              sourceFunction,
+              null,
+              expansionFlag);
       builder.add(parsedOption);
     }
     return builder.build();
@@ -317,7 +325,6 @@ class OptionsParserImpl {
       OptionDefinition expandedFrom,
       List<String> args)
       throws OptionsParsingException {
-    boolean isExplicit = expandedFrom == null && implicitDependent == null;
     List<String> unparsedArgs = new ArrayList<>();
     LinkedHashMap<OptionDefinition, List<String>> implicitRequirements = new LinkedHashMap<>();
 
@@ -337,7 +344,7 @@ class OptionsParserImpl {
 
       ParsedOptionDescription parsedOption =
           identifyOptionAndPossibleArgument(
-              arg, argsIterator, priority, sourceFunction, isExplicit);
+              arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom);
       OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
       // All options can be deprecated; check and warn before doing any option-type specific work.
       maybeAddDeprecationWarning(optionDefinition);
@@ -347,7 +354,7 @@ class OptionsParserImpl {
       OptionValueDescription entry =
           optionValues.computeIfAbsent(
               optionDefinition, OptionValueDescription::createOptionValueDescription);
-      entry.addOptionInstance(parsedOption, implicitDependent, expandedFrom, warnings);
+      entry.addOptionInstance(parsedOption, warnings);
 
       @Nullable String unconvertedValue = parsedOption.getUnconvertedValue();
       if (optionDefinition.isWrapperOption()) {
@@ -404,11 +411,13 @@ class OptionsParserImpl {
         ImmutableList<String> expansion =
             optionsData.getEvaluatedExpansion(optionDefinition, unconvertedValue);
 
+        String sourceFunctionApplication = sourceFunction.apply(optionDefinition);
         String sourceMessage =
-            "expanded from option --"
-                + optionDefinition.getOptionName()
-                + " from "
-                + sourceFunction.apply(optionDefinition);
+            (sourceFunctionApplication == null)
+                ? String.format("expanded from option --%s", optionDefinition.getOptionName())
+                : String.format(
+                    "expanded from option --%s from %s",
+                    optionDefinition.getOptionName(), sourceFunctionApplication);
         Function<OptionDefinition, String> expansionSourceFunction = o -> sourceMessage;
         List<String> unparsed =
             parse(priority, expansionSourceFunction, null, optionDefinition, expansion);
@@ -434,11 +443,15 @@ class OptionsParserImpl {
     // TODO(bazel-team): this should happen when the option is encountered.
     if (!implicitRequirements.isEmpty()) {
       for (Map.Entry<OptionDefinition, List<String>> entry : implicitRequirements.entrySet()) {
+        OptionDefinition optionDefinition = entry.getKey();
+        String sourceFunctionApplication = sourceFunction.apply(optionDefinition);
         String sourceMessage =
-            "implicit requirement of option --"
-                + entry.getKey()
-                + " from "
-                + sourceFunction.apply(entry.getKey());
+            (sourceFunctionApplication == null)
+                ? String.format(
+                    "implicit requirement of option --%s", optionDefinition.getOptionName())
+                : String.format(
+                    "implicit requirement of option --%s from %s",
+                    optionDefinition.getOptionName(), sourceFunctionApplication);
         Function<OptionDefinition, String> requirementSourceFunction = o -> sourceMessage;
 
         List<String> unparsed = parse(priority, requirementSourceFunction, entry.getKey(), null,
@@ -467,7 +480,8 @@ class OptionsParserImpl {
       Iterator<String> nextArgs,
       OptionPriority priority,
       Function<OptionDefinition, String> sourceFunction,
-      boolean explicit)
+      OptionDefinition implicitDependent,
+      OptionDefinition expandedFrom)
       throws OptionsParsingException {
 
     // Store the way this option was parsed on the command line.
@@ -548,9 +562,8 @@ class OptionsParserImpl {
         optionDefinition,
         commandLineForm.toString(),
         unconvertedValue,
-        priority,
-        sourceFunction.apply(optionDefinition),
-        explicit);
+        new OptionInstanceOrigin(
+            priority, sourceFunction.apply(optionDefinition), implicitDependent, expandedFrom));
   }
 
   /**
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 036ac4188b..0910579541 100644
--- a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
+++ b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
@@ -18,39 +18,27 @@ import com.google.common.collect.ImmutableList;
 import javax.annotation.Nullable;
 
 /**
- * The value of an option with additional metadata describing its origin.
+ * The representation of a parsed option instance.
  *
- * <p>This class represents an option as the parser received it, which is distinct from the final
- * value of an option, as these values may be overridden or combined in some way.
- *
- * <p>The origin includes the form it had when parsed, its priority, a message about where it came
- * from, and whether it was set explicitly or expanded/implied by other flags.
+ * <p>An option instance is distinct from the final value of an option, as multiple instances
+ * provide values may be overridden or combined in some way.
  */
 public final class ParsedOptionDescription {
+
   private final OptionDefinition optionDefinition;
   private final String commandLineForm;
   @Nullable private final String unconvertedValue;
-  private final OptionPriority priority;
-  @Nullable private final String source;
-
-  // Whether this flag was explicitly given, as opposed to having been added by an expansion flag
-  // or an implicit dependency. Notice that this does NOT mean it was explicitly given by the
-  // user, for that to be true, it needs the right combination of explicit & priority.
-  private final boolean explicit;
+  private final OptionInstanceOrigin origin;
 
   public ParsedOptionDescription(
       OptionDefinition optionDefinition,
       String commandLineForm,
       @Nullable String unconvertedValue,
-      OptionPriority priority,
-      @Nullable String source,
-      boolean explicit) {
+      OptionInstanceOrigin origin) {
     this.optionDefinition = optionDefinition;
     this.commandLineForm = commandLineForm;
     this.unconvertedValue = unconvertedValue;
-    this.priority = priority;
-    this.source = source;
-    this.explicit = explicit;
+    this.origin = origin;
   }
 
   public OptionDefinition getOptionDefinition() {
@@ -87,15 +75,23 @@ public final class ParsedOptionDescription {
   }
 
   OptionPriority getPriority() {
-    return priority;
+    return origin.getPriority();
   }
 
   public String getSource() {
-    return source;
+    return origin.getSource();
+  }
+
+  OptionDefinition getImplicitDependent() {
+    return origin.getImplicitDependent();
+  }
+
+  OptionDefinition getExpandedFrom() {
+    return origin.getExpandedFrom();
   }
 
   public boolean isExplicit() {
-    return explicit;
+    return origin.getExpandedFrom() == null && origin.getImplicitDependent() == null;
   }
 
   public Object getConvertedValue() throws OptionsParsingException {
@@ -114,9 +110,9 @@ public final class ParsedOptionDescription {
     StringBuilder result = new StringBuilder();
     result.append("option '").append(optionDefinition.getOptionName()).append("' ");
     result.append("set to '").append(unconvertedValue).append("' ");
-    result.append("with priority ").append(priority);
-    if (source != null) {
-      result.append(" and source '").append(source).append("'");
+    result.append("with priority ").append(origin.getPriority());
+    if (origin.getSource() != null) {
+      result.append(" and source '").append(origin.getSource()).append("'");
     }
     return result.toString();
   }