-
Notifications
You must be signed in to change notification settings - Fork 418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[issue 1387] Validate final primitive|String fields w/ Option|Parameters annotations #1711
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
package picocli.examples.validation; | ||
|
||
import static picocli.CommandLine.Command; | ||
import static picocli.CommandLine.Option; | ||
import static picocli.CommandLine.Parameters; | ||
|
||
// https://github.com/remkop/picocli/issues/1387 | ||
|
||
@Command | ||
public class InvalidFinal { | ||
|
||
// Options | ||
|
||
@Option(names = {"--boolean"}) | ||
private final boolean invalidBoolean = false; | ||
|
||
@Option(names = {"--valid-boolean"}) | ||
private final boolean validBoolean; | ||
|
||
@Option(names = {"--byte"}) | ||
private final byte invalidByte = 0; | ||
|
||
@Option(names = {"--valid-byte"}) | ||
private final byte validByte; | ||
|
||
@Option(names = {"--short"}) | ||
private final short invalidShort = 123; | ||
|
||
@Option(names = {"--valid-short"}) | ||
private final short validShort; | ||
|
||
@Option(names = {"--int"}) | ||
private final int invalidInt = -1; | ||
|
||
@Option(names = {"--valid-int"}) | ||
private final int validInt; | ||
|
||
@Option(names = {"--long"}) | ||
private final long invalidLong = -2L; | ||
|
||
@Option(names = {"--valid-long"}) | ||
private final long validLong; | ||
|
||
@Option(names = {"--char"}) | ||
private final char invalidChar = 'c'; | ||
|
||
@Option(names = {"--valid-char"}) | ||
private final char validChar; | ||
|
||
@Option(names = {"--float"}) | ||
private final float invalidFloat = 1.0f; | ||
|
||
@Option(names = {"--valid-float"}) | ||
private final float validFloat; | ||
|
||
@Option(names = {"--double"}) | ||
private final double invalidDouble = 1.0; | ||
|
||
@Option(names = {"--valid-double"}) | ||
private final double validDouble; | ||
|
||
@Option(names = {"--string"}) | ||
private final String invalidString = "foo"; | ||
|
||
@Option(names = {"--valid-string"}) | ||
private final String validString; | ||
|
||
// Parameters | ||
|
||
@Parameters(index = "0") | ||
private final boolean invalidBooleanParam = false; | ||
|
||
@Parameters(index = "1") | ||
private final boolean validBooleanParam; | ||
|
||
@Parameters(index = "2") | ||
private final byte invalidByteParam = 0; | ||
|
||
@Parameters(index = "3") | ||
private final byte validByteParam; | ||
|
||
@Parameters(index = "4") | ||
private final short invalidShortParam = 123; | ||
|
||
@Parameters(index = "5") | ||
private final short validShortParam; | ||
|
||
@Parameters(index = "6") | ||
private final int invalidIntParam = -1; | ||
|
||
@Parameters(index = "7") | ||
private final int validIntParam; | ||
|
||
@Parameters(index = "8") | ||
private final long invalidLongParam = -2L; | ||
|
||
@Parameters(index = "9") | ||
private final long validLongParam; | ||
|
||
@Parameters(index = "10") | ||
private final char invalidCharParam = 'c'; | ||
|
||
@Parameters(index = "11") | ||
private final char validCharParam; | ||
|
||
@Parameters(index = "12") | ||
private final float invalidFloatParam = 1.0f; | ||
|
||
@Parameters(index = "13") | ||
private final float validFloatParam; | ||
|
||
@Parameters(index = "14") | ||
private final double invalidDoubleParam = 1.0; | ||
|
||
@Parameters(index = "15") | ||
private final double validDoubleParam; | ||
|
||
@Parameters(index = "16") | ||
private final String invalidStringParam = "bar"; | ||
|
||
@Parameters(index = "17") | ||
private final String validStringParam; | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,11 +11,14 @@ | |
import javax.lang.model.element.Element; | ||
import javax.lang.model.element.ElementKind; | ||
import javax.lang.model.element.ExecutableElement; | ||
import javax.lang.model.element.Modifier; | ||
import javax.lang.model.element.VariableElement; | ||
import javax.lang.model.type.ExecutableType; | ||
import javax.lang.model.type.TypeKind; | ||
import javax.lang.model.type.TypeMirror; | ||
import javax.lang.model.util.ElementFilter; | ||
import javax.lang.model.util.SimpleElementVisitor6; | ||
import javax.lang.model.util.Types; | ||
import javax.tools.Diagnostic; | ||
import java.lang.annotation.Annotation; | ||
import java.util.Arrays; | ||
|
@@ -41,10 +44,14 @@ class AnnotationValidator { | |
CommandLine.Unmatched.class, | ||
CommandLine.ArgGroup.class | ||
)); | ||
private ProcessingEnvironment processingEnv; | ||
private final ProcessingEnvironment processingEnv; | ||
private final TypeMirror stringType; | ||
private final Types typeUtils; | ||
|
||
public AnnotationValidator(ProcessingEnvironment processingEnv) { | ||
this.processingEnv = Assert.notNull(processingEnv, "processingEnv"); | ||
this.stringType = processingEnv.getElementUtils().getTypeElement("java.lang.String").asType(); | ||
this.typeUtils = processingEnv.getTypeUtils(); | ||
} | ||
|
||
public void validateAnnotations(RoundEnvironment roundEnv) { | ||
|
@@ -105,6 +112,7 @@ private void checkOption(Element e, TypeMirror type, Option option) { | |
if (option.split().length() > 0 && !new CompileTimeTypeInfo(type).isMultiValue()) { | ||
error(e, null, "%s has a split regex but is a single-value type", e.getSimpleName()); | ||
} | ||
maybeValidateFinalFields(e, type, "@Option"); | ||
} | ||
}, element.getAnnotation(Option.class)); | ||
} | ||
|
@@ -149,6 +157,7 @@ private void checkOption(Element e, TypeMirror type, Parameters positional) { | |
if (positional.split().length() > 0 && !new CompileTimeTypeInfo(type).isMultiValue()) { | ||
error(e, null, "%s has a split regex but is a single-value type", e.getSimpleName()); | ||
} | ||
maybeValidateFinalFields(e, type, "@Parameters"); | ||
} | ||
}, element.getAnnotation(Parameters.class)); | ||
} | ||
|
@@ -178,15 +187,18 @@ private void validateNoAnnotationsOnInterfaceField(RoundEnvironment roundEnv) { | |
|
||
private void validateNoAnnotationsOnInterfaceField(Set<? extends Element> all) { | ||
for (Element element : all) { | ||
if (element.getKind() == ElementKind.FIELD && | ||
element.getEnclosingElement().getKind() == ElementKind.INTERFACE) { | ||
if (isInterfaceField(element)) { | ||
AnnotationMirror annotationMirror = getPicocliAnnotationMirror(element); | ||
error(element, annotationMirror, "Invalid picocli annotation on interface field %s.%s", | ||
element.getEnclosingElement().toString(), element.getSimpleName()); | ||
element.getEnclosingElement().toString(), element.getSimpleName()); | ||
} | ||
} | ||
} | ||
|
||
private boolean isInterfaceField(Element element) { | ||
return element.getKind() == ElementKind.FIELD && element.getEnclosingElement().getKind() == ElementKind.INTERFACE; | ||
} | ||
|
||
private void validateInvalidCombinations(RoundEnvironment roundEnv) { | ||
for (int i = 0; i < ALL.size(); i++) { | ||
for (int j = i + 1; j < ALL.size(); j++) { | ||
|
@@ -196,16 +208,33 @@ private void validateInvalidCombinations(RoundEnvironment roundEnv) { | |
} | ||
|
||
private <T1 extends Annotation, T2 extends Annotation> void validateInvalidCombination( | ||
RoundEnvironment roundEnv, Class<T1> c1, Class<T2> c2) { | ||
RoundEnvironment roundEnv, Class<T1> c1, Class<T2> c2) { | ||
for (Element element : roundEnv.getElementsAnnotatedWith(c1)) { | ||
if (element.getAnnotation(c2) != null) { | ||
AnnotationMirror annotationMirror = getPicocliAnnotationMirror(element); | ||
error(element, annotationMirror, "%s cannot have both @%s and @%s annotations", | ||
element, c1.getCanonicalName(), c2.getCanonicalName()); | ||
element, c1.getCanonicalName(), c2.getCanonicalName()); | ||
} | ||
} | ||
} | ||
|
||
private void maybeValidateFinalFields(Element e, TypeMirror type, String annotation) { | ||
ElementKind elementKind = e.getKind(); | ||
boolean isFinal = e.getModifiers().contains(Modifier.FINAL); | ||
// We have an existing validator for fields in an interface. Ignore those elements here. | ||
boolean shouldValidateFinal = elementKind.isField() && !isInterfaceField(e); | ||
if (!isFinal || !shouldValidateFinal) { | ||
return; | ||
} | ||
boolean isConstantValueField = false; | ||
for (VariableElement variableElement : ElementFilter.fieldsIn(Collections.singletonList(e))) { | ||
isConstantValueField = isConstantValueField || variableElement.getConstantValue() != null; | ||
} | ||
Comment on lines
+229
to
+232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In reality, the result of |
||
if ((type.getKind().isPrimitive() || typeUtils.isAssignable(type, stringType)) && isConstantValueField) { | ||
Comment on lines
+229
to
+233
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I'd love to also apply this compile-time validation to the boxed primitive types, I don't know if there's actually a way to determine if |
||
error(e, null, "Constant (final) primitive and String fields like %s cannot be used as %s: compile-time constant inlining may hide new values written to it.", e.getSimpleName(), annotation); | ||
} | ||
} | ||
|
||
private AnnotationMirror getPicocliAnnotationMirror(Element element) { | ||
AnnotationMirror annotationMirror = null; | ||
for (AnnotationMirror mirror : element.getAnnotationMirrors()) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I also wasn't certain about the java version where these tests get executed, but I demonstrated considerable restraint and wrote this test to only use java 6+ language features.