diff options
author | ccalvarin <ccalvarin@google.com> | 2017-08-23 21:40:49 +0200 |
---|---|---|
committer | Damien Martin-Guillerez <dmarting@google.com> | 2017-08-24 13:59:07 +0200 |
commit | edee4f2b02528f240cb342e9fe8c2ad14be0ada5 (patch) | |
tree | 12a1920eae7916ca0a0edf801d5a6981bd426507 /src/main | |
parent | 1e2954d4b4094f5f3c1c5a4b5cf70240d0c82d52 (diff) |
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
Diffstat (limited to 'src/main')
-rw-r--r-- | src/main/java/com/google/devtools/common/options/OptionProcessor.java | 25 | ||||
-rw-r--r-- | src/main/java/com/google/devtools/common/options/testing/OptionsTester.java | 23 |
2 files changed, 25 insertions, 23 deletions
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. |