From 404f9194eb718cf193122dc4b01f4e60d33bdc50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Halil=20=C4=B0brahim=20=C5=9Eener?= Date: Mon, 1 Jun 2020 10:51:40 +0200 Subject: [PATCH 1/3] PSM-442 Add `AmbiguousJsonCreatorCheck` --- .../AmbiguousJsonCreatorCheck.java | 73 +++++++++++++ .../AmbiguousJsonCreatorCheckTest.java | 101 ++++++++++++++++++ 2 files changed, 174 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheckTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java new file mode 100644 index 0000000000..f27954eeb5 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java @@ -0,0 +1,73 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.LinkType; +import com.google.errorprone.BugPattern.ProvidesFix; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.StandardTags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.AnnotationType; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol; +import java.util.Map; +import java.util.Optional; +import javax.lang.model.element.AnnotationValue; + +/** A {@link BugChecker} which flags ambiguous {@code @JsonCreator}s in enums. */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AmbiguousJsonCreator", + summary = "JsonCreator.Mode should be set for single-argument creators", + linkType = LinkType.NONE, + severity = SeverityLevel.SUGGESTION, + tags = StandardTags.LIKELY_ERROR, + providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION) +public final class AmbiguousJsonCreatorCheck extends BugChecker implements AnnotationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher JSON_CREATOR_ANNOTATION = + new AnnotationType("com.fasterxml.jackson.annotation.JsonCreator"); + + @Override + public Description matchAnnotation(AnnotationTree tree, VisitorState state) { + if (!JSON_CREATOR_ANNOTATION.matches(tree, state)) { + return Description.NO_MATCH; + } + + ClassTree clazz = state.findEnclosing(ClassTree.class); + if (clazz == null || clazz.getKind() != Tree.Kind.ENUM) { + return Description.NO_MATCH; + } + + MethodTree method = state.findEnclosing(MethodTree.class); + if (method == null || method.getParameters().size() != 1) { + return Description.NO_MATCH; + } + + Optional mode = + ASTHelpers.getAnnotationMirror(tree).getElementValues().entrySet().stream() + .filter(entry -> entry.getKey().getSimpleName().contentEquals("mode")) + .map(Map.Entry::getValue) + .map(AnnotationValue::getValue) + .filter(Symbol.VarSymbol.class::isInstance) + .map(o -> (Symbol.VarSymbol) o) + .filter(varSymbol -> !varSymbol.getSimpleName().contentEquals("DEFAULT")) + .findFirst(); + + if (mode.isPresent()) { + return Description.NO_MATCH; + } + + return describeMatch( + tree, SuggestedFix.replace(tree, "@JsonCreator(mode = JsonCreator.Mode.DELEGATING)")); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheckTest.java new file mode 100644 index 0000000000..fb7b23bf99 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheckTest.java @@ -0,0 +1,101 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.common.base.Predicates; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class AmbiguousJsonCreatorCheckTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(AmbiguousJsonCreatorCheck.class, getClass()) + .expectErrorMessage( + "X", + Predicates.containsPattern( + "JsonCreator.Mode should be set for single-argument creators")); + + @Test + void testIdentification() { + compilationTestHelper + .addSourceLines( + "Container.java", + "import com.fasterxml.jackson.annotation.JsonCreator;", + "import com.fasterxml.jackson.annotation.JsonValue;", + "", + "interface Container {", + " enum A {", + " FOO(1);", + " private final int i;", + " A(int i) {", + " this.i = i;", + " }", + " // BUG: Diagnostic matches: X", + " @JsonCreator", + " public static A of(int i) {", + " return FOO;", + " }", + " }", + "", + " enum B {", + " FOO(1);", + " private final int i;", + " B(int i) {", + " this.i = i;", + " }", + " @JsonCreator(mode = JsonCreator.Mode.DELEGATING)", + " public static B of(int i) {", + " return FOO;", + " }", + " }", + "", + " enum C {", + " FOO(1, \"s\");", + " @JsonValue private final int i;", + " private final String s;", + " C(int i, String s) {", + " this.i = i;", + " this.s = s;", + " }", + " // BUG: Diagnostic matches: X", + " @JsonCreator", + " public static C of(int i) {", + " return FOO;", + " }", + " }", + "", + " enum D {", + " FOO(1, \"s\");", + " private final int i;", + " private final String s;", + " D(int i, String s) {", + " this.i = i;", + " this.s = s;", + " }", + " @JsonCreator", + " public static D of(int i, String s) {", + " return FOO;", + " }", + " }", + "", + " enum E {", + " FOO;", + " // BUG: Diagnostic matches: X", + " @JsonCreator", + " public static E of(String s) {", + " return FOO;", + " }", + " }", + "", + " class F {", + " private final String s;", + " F(String s) {", + " this.s = s;", + " }", + " @JsonCreator", + " public static F of(String s) {", + " return new F(s);", + " }", + " }", + "", + "}") + .doTest(); + } +} From dd769eae6188ade11b1ada0223f07c76f6167553 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 6 Dec 2020 10:50:45 +0100 Subject: [PATCH 2/3] PSM-442 Tweaks --- .../bugpatterns/AmbiguousJsonCreatorCheck.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java index f27954eeb5..670698edda 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java @@ -1,5 +1,7 @@ package tech.picnic.errorprone.bugpatterns; +import static com.google.errorprone.matchers.Matchers.isType; + import com.google.auto.service.AutoService; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.LinkType; @@ -10,7 +12,6 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; -import com.google.errorprone.matchers.AnnotationType; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; @@ -29,13 +30,13 @@ name = "AmbiguousJsonCreator", summary = "JsonCreator.Mode should be set for single-argument creators", linkType = LinkType.NONE, - severity = SeverityLevel.SUGGESTION, + severity = SeverityLevel.WARNING, tags = StandardTags.LIKELY_ERROR, providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION) public final class AmbiguousJsonCreatorCheck extends BugChecker implements AnnotationTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher JSON_CREATOR_ANNOTATION = - new AnnotationType("com.fasterxml.jackson.annotation.JsonCreator"); + isType("com.fasterxml.jackson.annotation.JsonCreator"); @Override public Description matchAnnotation(AnnotationTree tree, VisitorState state) { @@ -59,7 +60,7 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { .map(Map.Entry::getValue) .map(AnnotationValue::getValue) .filter(Symbol.VarSymbol.class::isInstance) - .map(o -> (Symbol.VarSymbol) o) + .map(Symbol.VarSymbol.class::cast) .filter(varSymbol -> !varSymbol.getSimpleName().contentEquals("DEFAULT")) .findFirst(); From ae1fd8671c0e1cf82ffc76fb4b90a9b707ed94a9 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 14 Feb 2021 16:45:29 +0100 Subject: [PATCH 3/3] PSM-442 Suggestions --- .../AmbiguousJsonCreatorCheck.java | 22 ++++---- .../AmbiguousJsonCreatorCheckTest.java | 50 ++++++++++++++++++- 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java index 670698edda..f599e655cc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheck.java @@ -21,26 +21,25 @@ import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; import java.util.Map; -import java.util.Optional; import javax.lang.model.element.AnnotationValue; /** A {@link BugChecker} which flags ambiguous {@code @JsonCreator}s in enums. */ @AutoService(BugChecker.class) @BugPattern( name = "AmbiguousJsonCreator", - summary = "JsonCreator.Mode should be set for single-argument creators", + summary = "`JsonCreator.Mode` should be set for single-argument creators", linkType = LinkType.NONE, severity = SeverityLevel.WARNING, tags = StandardTags.LIKELY_ERROR, providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION) public final class AmbiguousJsonCreatorCheck extends BugChecker implements AnnotationTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher JSON_CREATOR_ANNOTATION = + private static final Matcher IS_JSON_CREATOR_ANNOTATION = isType("com.fasterxml.jackson.annotation.JsonCreator"); @Override public Description matchAnnotation(AnnotationTree tree, VisitorState state) { - if (!JSON_CREATOR_ANNOTATION.matches(tree, state)) { + if (!IS_JSON_CREATOR_ANNOTATION.matches(tree, state)) { return Description.NO_MATCH; } @@ -54,21 +53,18 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { return Description.NO_MATCH; } - Optional mode = + boolean customMode = ASTHelpers.getAnnotationMirror(tree).getElementValues().entrySet().stream() .filter(entry -> entry.getKey().getSimpleName().contentEquals("mode")) .map(Map.Entry::getValue) .map(AnnotationValue::getValue) .filter(Symbol.VarSymbol.class::isInstance) .map(Symbol.VarSymbol.class::cast) - .filter(varSymbol -> !varSymbol.getSimpleName().contentEquals("DEFAULT")) - .findFirst(); + .anyMatch(varSymbol -> !varSymbol.getSimpleName().contentEquals("DEFAULT")); - if (mode.isPresent()) { - return Description.NO_MATCH; - } - - return describeMatch( - tree, SuggestedFix.replace(tree, "@JsonCreator(mode = JsonCreator.Mode.DELEGATING)")); + return customMode + ? Description.NO_MATCH + : describeMatch( + tree, SuggestedFix.replace(tree, "@JsonCreator(mode = JsonCreator.Mode.DELEGATING)")); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheckTest.java index fb7b23bf99..05a7c40669 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AmbiguousJsonCreatorCheckTest.java @@ -1,6 +1,7 @@ package tech.picnic.errorprone.bugpatterns; import com.google.common.base.Predicates; +import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; @@ -10,7 +11,9 @@ final class AmbiguousJsonCreatorCheckTest { .expectErrorMessage( "X", Predicates.containsPattern( - "JsonCreator.Mode should be set for single-argument creators")); + "`JsonCreator.Mode` should be set for single-argument creators")); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(new AmbiguousJsonCreatorCheck(), getClass()); @Test void testIdentification() { @@ -23,10 +26,13 @@ void testIdentification() { "interface Container {", " enum A {", " FOO(1);", + "", " private final int i;", + "", " A(int i) {", " this.i = i;", " }", + "", " // BUG: Diagnostic matches: X", " @JsonCreator", " public static A of(int i) {", @@ -36,10 +42,13 @@ void testIdentification() { "", " enum B {", " FOO(1);", + "", " private final int i;", + "", " B(int i) {", " this.i = i;", " }", + "", " @JsonCreator(mode = JsonCreator.Mode.DELEGATING)", " public static B of(int i) {", " return FOO;", @@ -48,12 +57,15 @@ void testIdentification() { "", " enum C {", " FOO(1, \"s\");", + "", " @JsonValue private final int i;", " private final String s;", + "", " C(int i, String s) {", " this.i = i;", " this.s = s;", " }", + "", " // BUG: Diagnostic matches: X", " @JsonCreator", " public static C of(int i) {", @@ -63,12 +75,15 @@ void testIdentification() { "", " enum D {", " FOO(1, \"s\");", + "", " private final int i;", " private final String s;", + "", " D(int i, String s) {", " this.i = i;", " this.s = s;", " }", + "", " @JsonCreator", " public static D of(int i, String s) {", " return FOO;", @@ -77,6 +92,7 @@ void testIdentification() { "", " enum E {", " FOO;", + "", " // BUG: Diagnostic matches: X", " @JsonCreator", " public static E of(String s) {", @@ -86,9 +102,11 @@ void testIdentification() { "", " class F {", " private final String s;", + "", " F(String s) {", " this.s = s;", " }", + "", " @JsonCreator", " public static F of(String s) {", " return new F(s);", @@ -98,4 +116,34 @@ void testIdentification() { "}") .doTest(); } + + @Test + public void testReplacement() { + refactoringTestHelper + .addInputLines( + "in/A.java", + "import com.fasterxml.jackson.annotation.JsonCreator;", + "", + "enum A {", + " FOO;", + "", + " @JsonCreator", + " public static A of(String s) {", + " return FOO;", + " }", + "}") + .addOutputLines( + "out/A.java", + "import com.fasterxml.jackson.annotation.JsonCreator;", + "", + "enum A {", + " FOO;", + "", + " @JsonCreator(mode = JsonCreator.Mode.DELEGATING)", + " public static A of(String s) {", + " return FOO;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } }