Expansion flags need some restraints.

How expanding flags interact with other possible flag qualities is not defined. Should repeated values have effects multiple times and accumulate? This doesn't really make sense, expansion flags don't have values that would accumulate. For this reason, don't allow expansion options to have allowMultiple set to true.

Likewise for other behaviors.

PiperOrigin-RevId: 167580641

3 files changed, 63 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/common/options/OptionsData.java b/src/main/java/com/google/devtools/common/options/OptionsData.java
index eb20a9cd89..d29af3df39 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsData.java
@@ -130,15 +130,12 @@ final class OptionsData extends IsolatedOptionsData {
         ImmutableMap.<OptionDefinition, ExpansionData>builder();
     for (Map.Entry<String, OptionDefinition> entry : isolatedData.getAllNamedFields()) {
       OptionDefinition optionDefinition = entry.getValue();
-      // Determine either the hard-coded expansion, or the ExpansionFunction class.
+      // Determine either the hard-coded expansion, or the ExpansionFunction class. The
+      // OptionProcessor checks at compile time that these aren't used together.
       String[] constExpansion = optionDefinition.getOptionExpansion();
       Class<? extends ExpansionFunction> expansionFunctionClass =
           optionDefinition.getExpansionFunction();
-      if (constExpansion.length > 0 && optionDefinition.usesExpansionFunction()) {
-        throw new AssertionError(
-            "Cannot set both expansion and expansionFunction for option --"
-                + optionDefinition.getOptionName());
-      } else if (constExpansion.length > 0) {
+      if (constExpansion.length > 0) {
         expansionDataBuilder.put(
             optionDefinition, new ExpansionData(ImmutableList.copyOf(constExpansion)));
       } else if (optionDefinition.usesExpansionFunction()) {
diff --git a/src/main/java/com/google/devtools/common/options/processor/OptionProcessor.java b/src/main/java/com/google/devtools/common/options/processor/OptionProcessor.java
index 7474408a9d..5ccb868cee 100644
--- a/src/main/java/com/google/devtools/common/options/processor/OptionProcessor.java
+++ b/src/main/java/com/google/devtools/common/options/processor/OptionProcessor.java
@@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMap.Builder;
 import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.Converters;
+import com.google.devtools.common.options.ExpansionFunction;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
@@ -66,6 +67,8 @@ import javax.tools.Diagnostic;
  *       into the option type. For options that do not specify a converter, check that there is a
  *       valid match in the {@link Converters#DEFAULT_CONVERTERS} list.
  * <li>Options must list valid combinations of tags and documentation categories.
+ * <li>Expansion options and options with implicit requirements cannot expand in more than one way,
+ *       how multiple expansions would interact is not defined and should not be necessary.
  * </ul>
  *
  * <p>These properties can be relied upon at runtime without additional checks.
@@ -439,6 +442,52 @@ public final class OptionProcessor extends AbstractProcessor {
     }
   }
 
+  /**
+   * Some flags expand to other flags, either in place, or with "implicit requirements" that get
+   * added on top of the flag's value. Don't let these flags do too many crazy things, dealing with
+   * this is enough.
+   */
+  private void checkExpansionOptions(VariableElement optionField) throws OptionProcessorException {
+    Option annotation = optionField.getAnnotation(Option.class);
+    boolean isStaticExpansion = annotation.expansion().length > 0;
+    boolean hasImplicitRequirements = annotation.implicitRequirements().length > 0;
+
+    AnnotationMirror annotationMirror =
+        ProcessorUtils.getAnnotation(elementUtils, typeUtils, optionField, Option.class);
+    TypeElement expansionFunction =
+        ProcessorUtils.getClassTypeFromAnnotationField(
+            elementUtils, annotationMirror, "expansionFunction");
+    TypeElement defaultExpansionFunction =
+        elementUtils.getTypeElement(ExpansionFunction.class.getCanonicalName());
+    boolean isFunctionalExpansion =
+        !typeUtils.isSameType(expansionFunction.asType(), defaultExpansionFunction.asType());
+
+    if (isStaticExpansion && isFunctionalExpansion) {
+      throw new OptionProcessorException(
+          optionField,
+          "Options cannot expand using both a static expansion list and an expansion function.");
+    }
+    boolean isExpansion = isStaticExpansion || isFunctionalExpansion;
+
+    if (isExpansion && hasImplicitRequirements) {
+      throw new OptionProcessorException(
+          optionField,
+          "Can't set an option to be both an expansion option and have implicit requirements.");
+    }
+
+    if (isExpansion || hasImplicitRequirements) {
+      if (annotation.wrapperOption()) {
+        throw new OptionProcessorException(
+            optionField, "Wrapper options cannot have expansions or implicit requirements.");
+      }
+      if (annotation.allowMultiple()) {
+        throw new OptionProcessorException(
+            optionField,
+            "Can't set an option to accumulate multiple values and let it expand to other flags.");
+      }
+    }
+  }
+
   @Override
   public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {
     for (Element annotatedElement : roundEnv.getElementsAnnotatedWith(Option.class)) {
@@ -451,6 +500,7 @@ public final class OptionProcessor extends AbstractProcessor {
         checkInOptionBase(optionField);
         checkOptionName(optionField);
         checkOldCategoriesAreNotUsed(optionField);
+        checkExpansionOptions(optionField);
         checkConverter(optionField);
         checkEffectTagRationality(optionField);
         checkMetadataTagAndCategoryRationality(optionField);
diff --git a/src/main/java/com/google/devtools/common/options/processor/ProcessorUtils.java b/src/main/java/com/google/devtools/common/options/processor/ProcessorUtils.java
index 8f94fa554f..cce8f18458 100644
--- a/src/main/java/com/google/devtools/common/options/processor/ProcessorUtils.java
+++ b/src/main/java/com/google/devtools/common/options/processor/ProcessorUtils.java
@@ -75,9 +75,17 @@ public class ProcessorUtils {
       throws OptionProcessorException {
     for (Map.Entry<? extends ExecutableElement, ? extends AnnotationValue> entry :
         elementUtils.getElementValuesWithDefaults(annotation).entrySet()) {
-      if (fieldName.contentEquals(entry.getKey().getSimpleName())) {
+      if (entry.getKey().getSimpleName().contentEquals(fieldName)) {
+        Object annotationField = entry.getValue().getValue();
+        if (!(annotationField instanceof DeclaredType)) {
+          throw new IllegalStateException(
+              String.format(
+                  "The fieldName provided should only apply to Class<> type annotation fields, "
+                      + "but the field's value (%s) couldn't get cast to a DeclaredType",
+                  entry));
+        }
         String qualifiedName =
-            ((TypeElement) ((DeclaredType) entry.getValue().getValue()).asElement())
+            ((TypeElement) ((DeclaredType) annotationField).asElement())
                 .getQualifiedName()
                 .toString();
         return elementUtils.getTypeElement(qualifiedName);