From c3b5ead02bc300a86d855ff90d4c08ffb55de42c Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 12 Dec 2018 00:00:06 +0100 Subject: [PATCH] WIP: Model `modernizer-maven-plugin` --- .mvn/jvm.config | 1 + error-prone-contrib/pom.xml | 10 + .../errorprone/bugpatterns/Modernizer.java | 371 ++++++++++++++++++ .../bugpatterns/ModernizerTest.java | 221 +++++++++++ pom.xml | 10 +- 5 files changed, 612 insertions(+), 1 deletion(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Modernizer.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ModernizerTest.java diff --git a/.mvn/jvm.config b/.mvn/jvm.config index b79f9a23..e203deb3 100644 --- a/.mvn/jvm.config +++ b/.mvn/jvm.config @@ -4,6 +4,7 @@ --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED +--add-exports=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index 9912be1a..2a409a2a 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -72,6 +72,10 @@ jackson-annotations provided + + com.fasterxml.jackson.dataformat + jackson-dataformat-xml + com.github.ben-manes.caffeine caffeine @@ -162,6 +166,12 @@ assertj-core provided + + org.gaul + modernizer-maven-plugin + runtime + + org.immutables value-annotations diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Modernizer.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Modernizer.java new file mode 100644 index 00000000..9fc48cfc --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Modernizer.java @@ -0,0 +1,371 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableTable.toImmutableTable; +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.REFACTORING; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.predicates.TypePredicates.isDescendantOf; +import static com.google.errorprone.predicates.TypePredicates.isExactType; +import static java.util.Objects.requireNonNull; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.dataformat.xml.XmlMapper; +import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper; +import com.google.auto.service.AutoService; +import com.google.auto.value.AutoValue; +import com.google.auto.value.AutoValue.CopyAnnotations; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableTable; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.annotations.Immutable; +import com.google.errorprone.annotations.Var; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.IdentifierTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MemberSelectTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.predicates.TypePredicate; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.util.JavacTask; +import com.sun.tools.javac.api.BasicJavacTask; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.jvm.Target; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.util.List; +import java.util.Optional; +import java.util.function.Consumer; +import java.util.function.Predicate; +import java.util.regex.Pattern; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.Name; + +/** + * A {@link BugChecker} that flags the same legacy APIs as the Modernizer Maven Plugin. + * + *

This checker is primarily useful for people who run Error Prone anyway; it obviates the need + * for an additional source code analysis pass using another Maven plugin. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Avoid constants and methods superseded by more recent equivalents", + link = BUG_PATTERNS_BASE_URL + "Modernizer", + linkType = CUSTOM, + severity = SUGGESTION, + tags = REFACTORING) +public final class Modernizer extends BugChecker + implements AnnotationTreeMatcher, + IdentifierTreeMatcher, + MemberReferenceTreeMatcher, + MemberSelectTreeMatcher, + NewClassTreeMatcher { + private static final long serialVersionUID = 1L; + + // XXX: Load lazily? + private final ImmutableTable, String> violations = + loadViolations(); + + /** Instantiates a new {@link MockitoStubbing} instance. */ + public Modernizer() {} + + @Override + public Description matchAnnotation(AnnotationTree tree, VisitorState state) { + // XXX: Use or drop + // XXX: See + // https://github.com/google/guava-beta-checker/commit/9b26aa980be7f70631921fd6695013547728eb1e; + // we may be on the right track without this. + return Description.NO_MATCH; + } + + @Override + public Description matchIdentifier(IdentifierTree tree, VisitorState state) { + return match(tree.getName(), tree, state); + } + + @Override + public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { + return match(tree.getName(), tree, state); + } + + @Override + public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) { + return match(tree.getIdentifier(), tree, state); + } + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + Symbol createdType = + requireNonNull(ASTHelpers.getSymbol(tree).getEnclosingElement(), "No enclosing class"); + return match(createdType.getQualifiedName(), tree, state); + } + + private Description match(Name identifier, ExpressionTree tree, VisitorState state) { + return violations.row(identifier.toString()).entrySet().stream() + .filter(e -> e.getKey().matches(tree, state)) + .findFirst() + .map(e -> buildDescription(tree).setMessage(e.getValue()).build()) + .orElse(Description.NO_MATCH); + } + + private ImmutableTable, String> loadViolations() { + InputStream resource = getClass().getResourceAsStream("/modernizer.xml"); + // XXX: Or silently skip? + checkState(resource != null, "Modernizer configuration not found on classpath"); + + XmlMapper mapper = new XmlMapper(); + try (resource) { + return mapper.readValue(resource, Violations.class).getViolation().stream() + .filter(v -> v.getChecker().isPresent()) + .collect( + toImmutableTable( + Violation::getIdentifier, + v -> v.getChecker().orElseThrow(), + Violation::getComment)); + } catch (IOException e) { + throw new UncheckedIOException("Failed to parse Modernizer configuration", e); + } + } + + // XXX: Further simplify with Auto Value? + @Immutable + static final class Violations { + @JacksonXmlElementWrapper(useWrapping = false) + private final ImmutableList violation; + + @JsonCreator + private Violations(@JsonProperty("violation") List violation) { + this.violation = ImmutableList.copyOf(violation); + } + + // XXX: Jackson relies on this naming and visibility. Ugh. + public ImmutableList getViolation() { + return violation; + } + } + + @Immutable + @AutoValue + abstract static class Violation { + private static final Pattern NAME_PATTERN = + Pattern.compile( + "(?[^.]+)(?:\\.(?[^:]+):(?:\\((?[^)]*)\\))?(?[^()]+))?"); + + abstract Optional getTarget(); + + abstract String getIdentifier(); + + @CopyAnnotations + @SuppressWarnings("Immutable") + abstract Matcher getMatcher(); + + abstract String getComment(); + + Optional> getChecker() { + return getTarget().map(t -> allOf(getMatcher(), targetMatcher(t))); + } + + // XXX: Overkill? Not if we use auto value. + // XXX: Modernizer also flags annotation declarations, presumably by type. + // XXX: `ExpressionTree` is wrong here. Depends on type. + @JsonCreator + static Violation create( + @JsonProperty("version") String version, + @JsonProperty("name") String signature, + @JsonProperty("comment") String comment) { + Optional target = Optional.ofNullable(Target.lookup(version)); + + java.util.regex.Matcher matcher = NAME_PATTERN.matcher(signature); + checkState(matcher.matches(), "Failed to parse signature '%s'", signature); + + String type = + replaceSlashes(requireNonNull(matcher.group("type"), "Signature must contain type")); + + String member = matcher.group("member"); + if (member == null) { + // XXX: Should not implement this interface. Something like: + // violations.put(type, allOf(isSubtypeOf(type), versionRequirement), this.comment) + return new AutoValue_Modernizer_Violation(target, type, (t, s) -> false, comment); + } + + String params = matcher.group("params"); + if (params == null) { + return new AutoValue_Modernizer_Violation(target, member, isField(type), comment); + } + + ImmutableList> parameters = parseParams(params); + + if ("\"\"".equals(member)) { + return new AutoValue_Modernizer_Violation( + target, type, isConstructor(type, parameters), comment); + } + + // XXX: Should we disallow _extension_ of this method? + return new AutoValue_Modernizer_Violation( + target, member, isMethod(type, parameters), comment); + } + + private static Matcher targetMatcher(Target target) { + return (tree, state) -> target.compareTo(getTargetVersion(state)) <= 0; + } + + private static Target getTargetVersion(VisitorState state) { + return Target.instance( + Optional.ofNullable(state.context.get(JavacTask.class)) + .filter(BasicJavacTask.class::isInstance) + .map(BasicJavacTask.class::cast) + .map(BasicJavacTask::getContext) + .orElse(state.context)); + } + + private static Matcher isField(String onDescendantOf) { + return isMember(ElementKind::isField, isDescendantOf(onDescendantOf), ImmutableList.of()); + } + + private static Matcher isConstructor( + String ofClass, ImmutableList> withParameters) { + return isMember(k -> k == ElementKind.CONSTRUCTOR, isExactType(ofClass), withParameters); + } + + private static Matcher isMethod( + String onDescendantOf, ImmutableList> withParameters) { + return isMember(k -> k == ElementKind.METHOD, isDescendantOf(onDescendantOf), withParameters); + } + + private static Matcher isMember( + Predicate ofKind, + TypePredicate ownedBy, + ImmutableList> withParameters) { + return (tree, state) -> + Optional.ofNullable(ASTHelpers.getSymbol(tree)) + .filter(s -> ofKind.test(s.getKind())) + .filter(s -> isOwnedBy(s, ownedBy, state)) + .filter(s -> hasSameParameters(s, withParameters, state)) + .isPresent(); + } + + private static boolean isOwnedBy(Symbol symbol, TypePredicate expected, VisitorState state) { + Symbol owner = symbol.getEnclosingElement(); + return owner != null && expected.apply(owner.asType(), state); + } + + private static boolean hasSameParameters( + Symbol method, ImmutableList> expected, VisitorState state) { + List actual = method.asType().getParameterTypes(); + if (actual.size() != expected.size()) { + return false; + } + + for (int i = 0; i < actual.size(); ++i) { + if (!ASTHelpers.isSameType(actual.get(i), expected.get(i).get(state), state)) { + return false; + } + } + + return true; + } + + private static ImmutableList> parseParams(String params) { + ImmutableList.Builder> types = ImmutableList.builder(); + + @Var int index = 0; + while (index < params.length()) { + index = parseType(params, index, types::add); + } + + return types.build(); + } + + private static int parseType(String params, int index, Consumer> sink) { + return switch (params.charAt(index)) { + case '[' -> parseArrayType(params, index, sink); + case 'L' -> parseTypeReference(params, index, sink); + default -> parsePrimitiveType(params, index, sink); + }; + } + + private static int parseArrayType(String params, int index, Consumer> sink) { + int typeIndex = index + 1; + checkArgument( + params.length() > typeIndex && params.charAt(index) == '[', + "Cannot parse array type in parameter string '%s' at index %s", + params, + index); + + return parseType( + params, + typeIndex, + type -> + sink.accept(s -> s.getType(type.get(s), /* isArray= */ true, ImmutableList.of()))); + } + + private static int parsePrimitiveType(String params, int index, Consumer> sink) { + String primitive = + Optional.of(params) + .filter(p -> p.length() > index) + .flatMap(p -> fromPrimitiveAlias(p.charAt(index))) + .orElseThrow( + () -> + new IllegalArgumentException( + String.format( + "Cannot parse primitive type in parameter string '%s' at index %s", + params, index))); + sink.accept(s -> s.getTypeFromString(primitive)); + return index + 1; + } + + private static Optional fromPrimitiveAlias(char alias) { + return switch (alias) { + case 'Z' -> Optional.of("boolean"); + case 'B' -> Optional.of("byte"); + case 'C' -> Optional.of("char"); + case 'S' -> Optional.of("short"); + case 'I' -> Optional.of("int"); + case 'J' -> Optional.of("long"); + case 'F' -> Optional.of("float"); + case 'D' -> Optional.of("double"); + default -> Optional.empty(); + }; + } + + private static int parseTypeReference(String params, int index, Consumer> sink) { + int identifierIndex = index + 1; + if (params.length() > identifierIndex && params.charAt(index) == 'L') { + int delimiter = params.indexOf(';', identifierIndex); + if (delimiter > index) { + sink.accept( + s -> + s.getTypeFromString( + replaceSlashes(params.substring(identifierIndex, delimiter)))); + return delimiter + 1; + } + } + + throw new IllegalArgumentException( + String.format( + "Cannot parse reference type in parameter string '%s' at index %s", params, index)); + } + + private static String replaceSlashes(String typeName) { + return typeName.replace('/', '.'); + } + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ModernizerTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ModernizerTest.java new file mode 100644 index 00000000..ce562ea2 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ModernizerTest.java @@ -0,0 +1,221 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +public final class ModernizerTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(Modernizer.class, getClass()); + + // XXX: Also add calls that should not be flagged. + // XXX: Test extension, field references, instance methods, static methods. + // methods with primitives, primitive arrays, references, reference arrays + // zero, one two args. + // XXX: Also test constructors! + // XXX: Test that the appropriate "prefer" message is emitted. + // XXX: List the test cases in `ModernizerTest`? + + @Test + void fieldIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static com.google.common.base.Charsets.ISO_8859_1;", + "", + "import com.google.common.base.Charsets;", + "import java.nio.charset.StandardCharsets;", + "", + "class A {", + " {", + " // BUG: Diagnostic contains: Prefer java.nio.charset.StandardCharsets", + " Object o1 = ISO_8859_1;", + " // BUG: Diagnostic contains: Prefer java.nio.charset.StandardCharsets", + " Object o2 = Charsets.ISO_8859_1;", + " // BUG: Diagnostic contains: Prefer java.nio.charset.StandardCharsets", + " Object o3 = com.google.common.base.Charsets.ISO_8859_1;", + "", + " Object o4 = StandardCharsets.ISO_8859_1;", + " Object o5 = java.nio.charset.StandardCharsets.ISO_8859_1;", + " }", + "}") + .doTest(); + } + + @Test + void nullaryMethodIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static com.google.common.base.Optional.absent;", + "", + "import com.google.common.base.Optional;", + "import java.util.function.Supplier;", + "", + "class A {", + " {", + " // BUG: Diagnostic contains: Prefer java.util.Optional", + " absent();", + " // BUG: Diagnostic contains: Prefer java.util.Optional", + " Optional.absent();", + " // BUG: Diagnostic contains: Prefer java.util.Optional", + " com.google.common.base.Optional.absent();", + " // BUG: Diagnostic contains: Prefer java.util.Optional", + " Supplier s1 = Optional::absent;", + " // BUG: Diagnostic contains: Prefer java.util.Optional", + " Supplier s2 = com.google.common.base.Optional::absent;", + "", + " java.util.Optional.empty();", + " Supplier s3 = java.util.Optional::empty;", + "", + " Dummy.absent();", + " }", + "", + " static final class Dummy {", + " static Optional absent() {", + " return null;", + " }", + " }", + "}") + .doTest(); + } + + @Test + void unaryMethodWithIntegerArgumentIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static com.google.common.collect.Lists.newArrayListWithCapacity;", + "", + "import com.google.common.collect.Lists;", + "import java.util.ArrayList;", + "import java.util.function.IntFunction;", + "", + "class A {", + " {", + " // BUG: Diagnostic contains: Prefer java.util.ArrayList<>(int)", + " newArrayListWithCapacity(0);", + " // BUG: Diagnostic contains: Prefer java.util.ArrayList<>(int)", + " Lists.newArrayListWithCapacity(1);", + " // BUG: Diagnostic contains: Prefer java.util.ArrayList<>(int)", + " com.google.common.collect.Lists.newArrayListWithCapacity(2);", + " // BUG: Diagnostic contains: Prefer java.util.ArrayList<>(int)", + " IntFunction f1 = Lists::newArrayListWithCapacity;", + " // BUG: Diagnostic contains: Prefer java.util.ArrayList<>(int)", + " IntFunction f2 = com.google.common.collect.Lists::newArrayListWithCapacity;", + "", + " new ArrayList<>(3);", + " IntFunction f3 = ArrayList::new;", + " IntFunction f4 = java.util.ArrayList::new;", + "", + " Dummy.newArrayListWithCapacity(4);", + " }", + "", + " static final class Dummy {", + " static ArrayList newArrayListWithCapacity(int initialArraySize) {", + " return null;", + " }", + " }", + "}") + .doTest(); + } + + @Test + void binaryMethodWithObjectArgumentsIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static com.google.common.base.Objects.equal;", + "", + "import com.google.common.base.Objects;", + "import java.util.function.BiPredicate;", + "", + "class A {", + " {", + " // BUG: Diagnostic contains: Prefer java.util.Objects.equals(Object, Object)", + " equal(null, null);", + " // BUG: Diagnostic contains: Prefer java.util.Objects.equals(Object, Object)", + " Objects.equal(null, null);", + " // BUG: Diagnostic contains: Prefer java.util.Objects.equals(Object, Object)", + " com.google.common.base.Objects.equal(null, null);", + " // BUG: Diagnostic contains: Prefer java.util.Objects.equals(Object, Object)", + " BiPredicate p1 = Objects::equal;", + " // BUG: Diagnostic contains: Prefer java.util.Objects.equals(Object, Object)", + " BiPredicate p2 = com.google.common.base.Objects::equal;", + "", + " java.util.Objects.equals(null, null);", + " BiPredicate p3 = java.util.Objects::equals;", + "", + " Dummy.equal(null, null);", + " }", + "", + " static final class Dummy {", + " static boolean equal(Object a, Object b) {", + " return false;", + " }", + " }", + "}") + .doTest(); + } + + @Test + void varargsMethodWithObjectArgumentsIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import com.google.common.base.Objects;", + "import java.util.function.ToIntFunction;", + "", + "class A {", + " {", + " // BUG: Diagnostic contains: Prefer java.util.Objects.hash(Object...)", + " Objects.hashCode((Object) null);", + " // BUG: Diagnostic contains: Prefer java.util.Objects.hash(Object...)", + " com.google.common.base.Objects.hashCode(null, null);", + " // BUG: Diagnostic contains: Prefer java.util.Objects.hash(Object...)", + " ToIntFunction f1 = Objects::hashCode;", + " // BUG: Diagnostic contains: Prefer java.util.Objects.hash(Object...)", + " ToIntFunction f2 = com.google.common.base.Objects::hashCode;", + "", + " java.util.Objects.hash(null, null, null);", + " ToIntFunction f3 = java.util.Objects::hash;", + "", + " Dummy.hashCode(null, null, null, null);", + " }", + "", + " static final class Dummy {", + " static int hashCode(Object... objects) {", + " return 0;", + " }", + " }", + "}") + .doTest(); + } + + @Test + void binaryConstructorWithByteArrayAndObjectArgumentsIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import java.io.UnsupportedEncodingException;", + "import java.nio.charset.StandardCharsets;", + "", + "class A {", + " void m() throws UnsupportedEncodingException {", + " // BUG: Diagnostic contains: Prefer java.lang.String.(byte[], java.nio.charset.Charset)", + " new String(new byte[0], \"\");", + " // BUG: Diagnostic contains: Prefer java.lang.String.(byte[], java.nio.charset.Charset)", + " new java.lang.String(new byte[] {}, toString());", + "", + " new String(new byte[0], StandardCharsets.UTF_8);", + " new java.lang.String(new byte[0], StandardCharsets.UTF_8);", + "", + " new Dummy(new byte[0], \"\");", + " }", + "", + " static final class Dummy {", + " Dummy(byte bytes[], String charsetName) {}", + " }", + "}") + .doTest(); + } +} diff --git a/pom.xml b/pom.xml index e5c2119c..0cf91798 100644 --- a/pom.xml +++ b/pom.xml @@ -93,6 +93,7 @@ --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED + --add-exports=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED @@ -113,7 +114,7 @@ -Xmx${argLine.xmx} - -javaagent:${org.mockito:mockito-core:jar} + BSD-2-Clause + | The BSD 2-Clause License | The BSD License