Move option tag checks to compile time.

The rationality checks failed as RuntimeExceptions that weren't getting reported properly because the options parser is created too early in Bazel's lifetime to be reported properly to standard error. This was a minor annoyance for authors of new options, and there was nothing gained by waiting until runtime to check these values.

RELNOTES: None
PiperOrigin-RevId: 166395759

3 files changed, 83 insertions, 71 deletions

diff --git a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
index e087dbbb56..406b6cd94b 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -339,52 +339,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     booleanAliasMap.put("no" + optionName, optionName);
   }
 
-  private static void checkEffectTagRationality(String optionName, OptionEffectTag[] effectTags) {
-    // Check that there is at least one OptionEffectTag listed.
-    if (effectTags.length < 1) {
-      throw new ConstructionException(
-          "Option "
-              + optionName
-              + " does not list at least one OptionEffectTag. If the option has no effect, "
-              + "please add NO_OP, otherwise, add a tag representing its effect.");
-    } else if (effectTags.length > 1) {
-      // If there are more than 1 tag, make sure that NO_OP and UNKNOWN is not one of them.
-      // These don't make sense if other effects are listed.
-      ImmutableList<OptionEffectTag> tags = ImmutableList.copyOf(effectTags);
-      if (tags.contains(OptionEffectTag.UNKNOWN)) {
-        throw new ConstructionException(
-            "Option "
-                + optionName
-                + " includes UNKNOWN with other, known, effects. Please remove UNKNOWN from "
-                + "the list.");
-      }
-      if (tags.contains(OptionEffectTag.NO_OP)) {
-        throw new ConstructionException(
-            "Option "
-                + optionName
-                + " includes NO_OP with other effects. This doesn't make much sense. Please "
-                + "remove NO_OP or the actual effects from the list, whichever is correct.");
-      }
-    }
-  }
-
-  private static void checkMetadataTagAndCategoryRationality(
-      String optionName, OptionMetadataTag[] metadataTags, OptionDocumentationCategory category) {
-    for (OptionMetadataTag tag : metadataTags) {
-      if (tag == OptionMetadataTag.HIDDEN || tag == OptionMetadataTag.INTERNAL) {
-        if (category != OptionDocumentationCategory.UNDOCUMENTED) {
-          throw new ConstructionException(
-              "Option "
-                  + optionName
-                  + " has metadata tag "
-                  + tag
-                  + " but does not have category UNDOCUMENTED. "
-                  + "Please fix.");
-        }
-      }
-    }
-  }
-
   /**
    * Constructs an {@link IsolatedOptionsData} object for a parser that knows about the given
    * {@link OptionsBase} classes. No inter-option analysis is done. Performs basic sanity checking
@@ -437,11 +391,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
                   + "\" is disallowed.");
         }
 
-        checkEffectTagRationality(optionName, optionDefinition.getOptionEffectTags());
-        checkMetadataTagAndCategoryRationality(
-            optionName,
-            optionDefinition.getOptionMetadataTags(),
-            optionDefinition.getDocumentationCategory());
         Type fieldType = getFieldSingularType(optionDefinition);
         // For simple, static expansions, don't accept non-Void types.
         if (optionDefinition.getOptionExpansion().length != 0 && !optionDefinition.isVoidField()) {
diff --git a/src/main/java/com/google/devtools/common/options/processor/BUILD b/src/main/java/com/google/devtools/common/options/processor/BUILD
index 2b05631e3f..4d495e606b 100644
--- a/src/main/java/com/google/devtools/common/options/processor/BUILD
+++ b/src/main/java/com/google/devtools/common/options/processor/BUILD
@@ -21,5 +21,6 @@ java_plugin(
     processor_class = "com.google.devtools.common.options.processor.OptionProcessor",
     deps = [
         "//src/main/java/com/google/devtools/common/options:options_internal",
+        "//third_party:guava",
     ],
 )
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 a961e92546..5afcc39f48 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
@@ -13,7 +13,11 @@
 // limitations under the License.
 package com.google.devtools.common.options.processor;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionDocumentationCategory;
+import com.google.devtools.common.options.OptionEffectTag;
+import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParser;
 import java.util.Set;
@@ -28,6 +32,7 @@ import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.Modifier;
 import javax.lang.model.element.TypeElement;
+import javax.lang.model.element.VariableElement;
 import javax.lang.model.type.TypeMirror;
 import javax.lang.model.util.Elements;
 import javax.lang.model.util.Types;
@@ -59,23 +64,23 @@ public final class OptionProcessor extends AbstractProcessor {
   private static class OptionProcessorException extends Exception {
     private Element elementInError;
 
-    OptionProcessorException(Element element, String message) {
-      super(message);
+    OptionProcessorException(Element element, String message, Object... args) {
+      super(String.format(message, args));
       elementInError = element;
     }
   }
 
   /** Check that the Option variables only occur in OptionBase-inheriting classes. */
-  private void checkInOptionBase(Element annotatedElement) throws OptionProcessorException {
-    if (annotatedElement.getEnclosingElement().getKind() != ElementKind.CLASS) {
-      throw new OptionProcessorException(annotatedElement, "The field should belong to a class.");
+  private void checkInOptionBase(VariableElement optionField) throws OptionProcessorException {
+    if (optionField.getEnclosingElement().getKind() != ElementKind.CLASS) {
+      throw new OptionProcessorException(optionField, "The field should belong to a class.");
     }
-    TypeMirror thisOptionClass = annotatedElement.getEnclosingElement().asType();
+    TypeMirror thisOptionClass = optionField.getEnclosingElement().asType();
     TypeMirror optionsBase =
         elementUtils.getTypeElement("com.google.devtools.common.options.OptionsBase").asType();
     if (!typeUtils.isAssignable(thisOptionClass, optionsBase)) {
       throw new OptionProcessorException(
-          annotatedElement,
+          optionField,
           "@Option annotated fields can only be in classes that inherit from OptionsBase.");
     }
   }
@@ -88,33 +93,90 @@ public final class OptionProcessor extends AbstractProcessor {
    *
    * <p>Static or final fields would cause issue with correct value assigning at the end of parsing.
    */
-  private void checkModifiers(Element annotatedElement) throws OptionProcessorException {
-    if (!annotatedElement.getModifiers().contains(Modifier.PUBLIC)) {
+  private void checkModifiers(VariableElement optionField) throws OptionProcessorException {
+    if (!optionField.getModifiers().contains(Modifier.PUBLIC)) {
+      throw new OptionProcessorException(optionField, "@Option annotated fields should be public.");
+    }
+    if (optionField.getModifiers().contains(Modifier.STATIC)) {
       throw new OptionProcessorException(
-          annotatedElement, "@Option annotated fields should be public.");
+          optionField, "@Option annotated fields should not be static.");
     }
-    if (annotatedElement.getModifiers().contains(Modifier.STATIC)) {
+    if (optionField.getModifiers().contains(Modifier.FINAL)) {
       throw new OptionProcessorException(
-          annotatedElement, "@Option annotated fields should not be static.");
+          optionField, "@Option annotated fields should not be final.");
     }
-    if (annotatedElement.getModifiers().contains(Modifier.FINAL)) {
+  }
+
+  /**
+   * Check that the option lists at least one effect, and that no nonsensical combinations are
+   * listed, such as having a known effect listed with UNKNOWN.
+   */
+  private void checkEffectTagRationality(VariableElement optionField)
+      throws OptionProcessorException {
+    Option annotation = optionField.getAnnotation(Option.class);
+    OptionEffectTag[] effectTags = annotation.effectTags();
+    // Check that there is at least one OptionEffectTag listed.
+    if (effectTags.length < 1) {
       throw new OptionProcessorException(
-          annotatedElement, "@Option annotated fields should not be final.");
+          optionField,
+          "Option does not list at least one OptionEffectTag. If the option has no effect, "
+              + "please be explicit and add NO_OP. Otherwise, add a tag representing its effect.");
+    } else if (effectTags.length > 1) {
+      // If there are more than 1 tag, make sure that NO_OP and UNKNOWN is not one of them.
+      // These don't make sense if other effects are listed.
+      ImmutableList<OptionEffectTag> tags = ImmutableList.copyOf(effectTags);
+      if (tags.contains(OptionEffectTag.UNKNOWN)) {
+        throw new OptionProcessorException(
+            optionField,
+            "Option includes UNKNOWN with other, known, effects. Please remove UNKNOWN from "
+                + "the list.");
+      }
+      if (tags.contains(OptionEffectTag.NO_OP)) {
+        throw new OptionProcessorException(
+            optionField,
+            "Option includes NO_OP with other effects. This doesn't make much sense. Please "
+                + "remove NO_OP or the actual effects from the list, whichever is correct.");
+      }
+    }
+  }
+
+  /**
+   * Check that if the metadata tags listed by an option require the option to be unknown by the
+   * average user, the same option will be omitted from documentation.
+   */
+  private void checkMetadataTagAndCategoryRationality(VariableElement optionField)
+      throws OptionProcessorException {
+    Option annotation = optionField.getAnnotation(Option.class);
+    OptionMetadataTag[] metadataTags = annotation.metadataTags();
+    OptionDocumentationCategory category = annotation.documentationCategory();
+
+    for (OptionMetadataTag tag : metadataTags) {
+      if (tag == OptionMetadataTag.HIDDEN || tag == OptionMetadataTag.INTERNAL) {
+        if (category != OptionDocumentationCategory.UNDOCUMENTED) {
+          throw new OptionProcessorException(
+              optionField,
+              "Option has metadata tag %s but does not have category UNDOCUMENTED. Please fix.",
+              tag);
+        }
+      }
     }
   }
 
   @Override
   public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {
-    try {
-      for (Element annotatedElement : roundEnv.getElementsAnnotatedWith(Option.class)) {
+    for (Element annotatedElement : roundEnv.getElementsAnnotatedWith(Option.class)) {
+      try {
         // Only fields are annotated with Option, this should already be checked by the
         // @Target(ElementType.FIELD) annotation.
+        VariableElement optionField = (VariableElement) annotatedElement;
 
-        checkModifiers(annotatedElement);
-        checkInOptionBase(annotatedElement);
+        checkModifiers(optionField);
+        checkInOptionBase(optionField);
+        checkEffectTagRationality(optionField);
+        checkMetadataTagAndCategoryRationality(optionField);
+      } catch (OptionProcessorException e) {
+        error(e.elementInError, e.getMessage());
       }
-    } catch (OptionProcessorException e) {
-      error(e.elementInError, e.getMessage());
     }
     // Claim all Option annotated fields.
     return true;