Skip to content
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

Merged
merged 4 commits into from
Jun 24, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@
import static com.google.testing.compile.CompilationSubject.assertThat;
import static com.google.testing.compile.Compiler.javac;
import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static picocli.annotation.processing.tests.Resources.slurp;
import static picocli.annotation.processing.tests.Resources.slurpAll;
import static picocli.annotation.processing.tests.YamlAssert.*;
import static picocli.annotation.processing.tests.YamlAssert.compareCommandYamlDump;

public class AbstractCommandSpecProcessorTest {
static Locale old;
Expand Down Expand Up @@ -225,9 +226,49 @@ public void testInvalidSplitOnSinglValueOptionOrPositional() {
validateErrorMessages(compilation, expected, optional);
}

@Test
public void testInvalidFinalOptionsAndParameters() {
CommandSpec2YamlProcessor processor = new CommandSpec2YamlProcessor();
Compilation compilation =
javac()
.withProcessors(processor)
.compile(JavaFileObjects.forResource(
"picocli/examples/validation/InvalidFinal.java"));

assertThat(compilation).failed();

// For every primitive type + String type, the InvalidFinal class defines
// an invalid combination of using a final field with a declared value, for each of those types.
List<String> types = Arrays.asList(
"boolean",
"byte",
"short",
"int",
"long",
"char",
"float",
"double",
"string"
);

String errorFormat = "Constant (final) primitive and String fields like %s cannot be used as %s: compile-time constant inlining may hide new values written to it.";
List<String> expectedValidationErrors = new ArrayList<String>();
for (String type : types) {
String titleized = type.substring(0, 1).toUpperCase() + type.substring(1);
String invalidOptionField = String.format("invalid%s", titleized);
String invalidParamField = String.format("invalid%sParam", titleized);

expectedValidationErrors.add(String.format(errorFormat, invalidOptionField, "@Option"));
expectedValidationErrors.add(String.format(errorFormat, invalidParamField, "@Parameters"));
}
Comment on lines +254 to +263
Copy link
Contributor Author

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.


validateErrorMessages(compilation, expectedValidationErrors);
}

private void validateErrorMessages(Compilation compilation, List<String> expected) {
validateErrorMessages(compilation, expected, Collections.emptyList());
}

private void validateErrorMessages(Compilation compilation, List<String> expected, List<String> optional) {
ImmutableList<Diagnostic<? extends JavaFileObject>> errors = compilation.errors();
for (Diagnostic<? extends JavaFileObject> diag : errors) {
Expand Down
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
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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++) {
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reality, the result of ElementFilter.fieldsIn(...) will either have 0 or 1 item in it, but I just expressed it in a more general sense.

if ((type.getKind().isPrimitive() || typeUtils.isAssignable(type, stringType)) && isConstantValueField) {
Comment on lines +229 to +233
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 final Integer foo has a value specified in the class definition. getConstantValue() only returns a non-null value if the defined value is non-null and the type is a true primitive type or string.

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()) {
Expand Down