From edee4f2b02528f240cb342e9fe8c2ad14be0ada5 Mon Sep 17 00:00:00 2001
From: ccalvarin <ccalvarin@google.com>
Date: Wed, 23 Aug 2017 21:40:49 +0200
Subject: Check at compile time that all options are declared public, and are
 non-final and non-static.

Remove the now redundant check in the testing framework, as the cases being tested no longer compile. Keep tests that check the contents of the OptionsBase as a whole, however, as this is still not being tested at compile time.

PiperOrigin-RevId: 166239209
---
 .../devtools/common/options/OptionProcessor.java   | 25 ++++++++++++++++++++++
 .../common/options/testing/OptionsTester.java      | 23 --------------------
 2 files changed, 25 insertions(+), 23 deletions(-)

(limited to 'src/main/java')

diff --git a/src/main/java/com/google/devtools/common/options/OptionProcessor.java b/src/main/java/com/google/devtools/common/options/OptionProcessor.java
index c2ef64409b..ae0448518f 100644
--- a/src/main/java/com/google/devtools/common/options/OptionProcessor.java
+++ b/src/main/java/com/google/devtools/common/options/OptionProcessor.java
@@ -23,6 +23,7 @@ import javax.annotation.processing.SupportedSourceVersion;
 import javax.lang.model.SourceVersion;
 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.type.TypeMirror;
 import javax.lang.model.util.Elements;
@@ -76,6 +77,29 @@ public final class OptionProcessor extends AbstractProcessor {
     }
   }
 
+  /**
+   * Checks that the Option variables is public and neither final nor static.
+   *
+   * <p>Private or protected fields would prevent the options parser from having full access to the
+   * fields it's expected to read, and {@link OptionsBase} equality would not work as intended.
+   *
+   * <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)) {
+      throw new OptionProcessorException(
+          annotatedElement, "@Option annotated fields should be public.");
+    }
+    if (annotatedElement.getModifiers().contains(Modifier.STATIC)) {
+      throw new OptionProcessorException(
+          annotatedElement, "@Option annotated fields should not be static.");
+    }
+    if (annotatedElement.getModifiers().contains(Modifier.FINAL)) {
+      throw new OptionProcessorException(
+          annotatedElement, "@Option annotated fields should not be final.");
+    }
+  }
+
   @Override
   public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {
     try {
@@ -83,6 +107,7 @@ public final class OptionProcessor extends AbstractProcessor {
         // Only fields are annotated with Option, this should already be checked by the
         // @Target(ElementType.FIELD) annotation.
 
+        checkModifiers(annotatedElement);
         checkInOptionBase(annotatedElement);
       }
     } catch (OptionProcessorException e) {
diff --git a/src/main/java/com/google/devtools/common/options/testing/OptionsTester.java b/src/main/java/com/google/devtools/common/options/testing/OptionsTester.java
index 9ecfdc5e0a..53a80dcb15 100644
--- a/src/main/java/com/google/devtools/common/options/testing/OptionsTester.java
+++ b/src/main/java/com/google/devtools/common/options/testing/OptionsTester.java
@@ -69,29 +69,6 @@ public final class OptionsTester {
     return this;
   }
 
-  /** Tests that there are no non-public fields which would interfere with option parsing. */
-  public OptionsTester testAllOptionFieldsPublic() {
-    for (Field field : getAllFields(optionsClass)) {
-      if (field.isAnnotationPresent(Option.class)) {
-        assertWithMessage(
-                field
-                    + " is Option-annotated, but is not public; it will not be considered as part"
-                    + " of the options. Change the visibility to public.")
-            .that(Modifier.isPublic(field.getModifiers()))
-            .isTrue();
-      }
-      if (Modifier.isStatic(field.getModifiers()) || Modifier.isFinal(field.getModifiers())) {
-        assertWithMessage(
-                field
-                    + " is Option-annotated, but is either static or final; it cannot be properly"
-                    + " set by the option parser. Remove either the annotation or the modifier(s).")
-            .that(field.getAnnotation(Option.class))
-            .isNull();
-      }
-    }
-    return this;
-  }
-
   /**
    * Tests that the default values of this class were part of the test data for the appropriate
    * ConverterTester, ensuring that the defaults at least obey proper equality semantics.
-- 
cgit v1.2.3