From 3b77e41106d93a301a2b56d8bc779f7b7435e222 Mon Sep 17 00:00:00 2001 From: Ferdinand Swoboda Date: Wed, 18 May 2022 15:11:42 +0200 Subject: [PATCH 01/15] CORE-97 Add MissingImmutableSortedSetDefaultCheck --- error-prone-contrib/pom.xml | 4 + ...MissingImmutableSortedSetDefaultCheck.java | 69 ++++++ ...ingImmutableSortedSetDefaultCheckTest.java | 223 ++++++++++++++++++ pom.xml | 5 + 4 files changed, 301 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index 172e1ce548..51c3f91b3d 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -143,6 +143,10 @@ assertj-core provided + + org.immutables + value + org.junit.jupiter junit-jupiter-api diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java new file mode 100644 index 0000000000..df52fabe39 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -0,0 +1,69 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.NONE; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.matchers.Matchers.hasAnnotation; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import org.immutables.value.Value; + +/** + * A {@link BugChecker} which flags methods with return type {@link + * com.google.common.collect.ImmutableSortedSet} within an {@code @Value.Immutable} or + * {@code @Value.Modifiable} class that lack either a default implementation or + * {@code @Value.NaturalOrder}. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "MissingImmutableSortedSetDefault", + summary = + "Properties of type `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class " + + "should provide a default value or specify the comparator.", + linkType = NONE, + severity = ERROR, + tags = LIKELY_ERROR) +public final class MissingImmutableSortedSetDefaultCheck extends BugChecker + implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final String VALUE_NATURAL_ORDER_ANNOTATION = + "org.immutables.value.Value.NaturalOrder"; + private static final Matcher HAS_NATURAL_ORDER = + hasAnnotation(VALUE_NATURAL_ORDER_ANNOTATION); + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + // is not within immutable or modifiable class -> no match + if (tree.getClass().isAnnotationPresent(org.immutables.value.Value.Immutable.class) + || tree.getClass().isAnnotationPresent(Value.Modifiable.class)) { + return Description.NO_MATCH; + } + + // has implementation -> no match + if (tree.getDefaultValue() != null) { + return Description.NO_MATCH; + } + + // is annotated with @Value.NaturalOrder -> no match + if (HAS_NATURAL_ORDER.matches(tree, state)) { + return Description.NO_MATCH; + } + + // The ImmutableSortedSet has no empty default -> add the `@Value.NaturalOrder` annotation. + return describeMatch( + tree, + SuggestedFix.builder() + .addStaticImport(VALUE_NATURAL_ORDER_ANNOTATION) + .prefixWith(tree, "@Value.NaturalOrder ") + .build()); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java new file mode 100644 index 0000000000..c612088e01 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java @@ -0,0 +1,223 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.common.base.Predicates.containsPattern; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +public final class MissingImmutableSortedSetDefaultCheckTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(MissingImmutableSortedSetDefaultCheck.class, getClass()) + .expectErrorMessage( + "X", + containsPattern( + "Properties of type `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class should provide a default value or specify the comparator.")); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance( + MissingImmutableSortedSetDefaultCheck.class, getClass()); + + @Test + void identification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Immutable", + "interface A {", + " // BUG: Diagnostic matches: X", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Modifiable", + "interface B {", + " // BUG: Diagnostic matches: X", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Immutable", + "abstract class C {", + " // BUG: Diagnostic matches: X", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Modifiable", + "abstract class D {", + " // BUG: Diagnostic matches: X", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}") + .doTest(); + } + + @Test + void replacement() { + refactoringTestHelper + .addInputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Immutable", + "interface A {", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Modifiable", + "interface B {", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Immutable", + "abstract class C {", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Modifiable", + "abstract class D {", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}") + .addOutputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Immutable", + "interface A {", + " @Value.NaturalOrder", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Modifiable", + "interface B {", + " @Value.NaturalOrder", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Immutable", + "abstract class C {", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "@Value.Modifiable", + "abstract class D {", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/pom.xml b/pom.xml index f8d4dd71ec..4e6566a5e9 100644 --- a/pom.xml +++ b/pom.xml @@ -386,6 +386,11 @@ hamcrest-core 2.2 + + org.immutables + value + 2.8.2 + org.junit junit-bom From 9066020aaf0c4e1df5929004e1bc399f7ade1b42 Mon Sep 17 00:00:00 2001 From: Ferdinand Swoboda Date: Tue, 31 May 2022 19:44:18 +0200 Subject: [PATCH 02/15] CORE-97 Refinements Getting there... --- ...MissingImmutableSortedSetDefaultCheck.java | 43 ++--- ...ingImmutableSortedSetDefaultCheckTest.java | 161 ++++++++++++++---- 2 files changed, 156 insertions(+), 48 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index df52fabe39..b4e027219e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -4,17 +4,22 @@ import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; import static com.google.errorprone.matchers.Matchers.hasAnnotation; +import static com.google.errorprone.matchers.Matchers.hasAnyAnnotation; +import static com.google.errorprone.matchers.Matchers.isSameType; +import static com.google.errorprone.matchers.Matchers.isSubtypeOf; +import static com.google.errorprone.matchers.Matchers.isType; +import static com.google.errorprone.matchers.Matchers.methodReturns; import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableSortedSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MethodTree; -import com.sun.source.tree.Tree; import org.immutables.value.Value; /** @@ -27,7 +32,7 @@ @BugPattern( name = "MissingImmutableSortedSetDefault", summary = - "Properties of type `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class " + "Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class " + "should provide a default value or specify the comparator.", linkType = NONE, severity = ERROR, @@ -35,35 +40,35 @@ public final class MissingImmutableSortedSetDefaultCheck extends BugChecker implements MethodTreeMatcher { private static final long serialVersionUID = 1L; - private static final String VALUE_NATURAL_ORDER_ANNOTATION = - "org.immutables.value.Value.NaturalOrder"; - private static final Matcher HAS_NATURAL_ORDER = - hasAnnotation(VALUE_NATURAL_ORDER_ANNOTATION); @Override public Description matchMethod(MethodTree tree, VisitorState state) { - // is not within immutable or modifiable class -> no match - if (tree.getClass().isAnnotationPresent(org.immutables.value.Value.Immutable.class) - || tree.getClass().isAnnotationPresent(Value.Modifiable.class)) { + // has no return type ImmutableSortedSet -> no match + if (!methodReturns(isSameType(ImmutableSortedSet.class)).matches(tree, state)) { return Description.NO_MATCH; } // has implementation -> no match - if (tree.getDefaultValue() != null) { + if (tree.getBody() != null && !tree.getBody().getStatements().isEmpty()) { + return Description.NO_MATCH; + } + + // is not within immutable or modifiable class -> no match + if (!ASTHelpers.hasAnnotation(tree, org.immutables.value.Value.Immutable.class, state) + && !ASTHelpers.hasAnnotation(tree, Value.Modifiable.class, state)) { return Description.NO_MATCH; } // is annotated with @Value.NaturalOrder -> no match - if (HAS_NATURAL_ORDER.matches(tree, state)) { + if (hasAnnotation(Value.NaturalOrder.class).matches(tree, state)) { return Description.NO_MATCH; } - // The ImmutableSortedSet has no empty default -> add the `@Value.NaturalOrder` annotation. - return describeMatch( - tree, - SuggestedFix.builder() - .addStaticImport(VALUE_NATURAL_ORDER_ANNOTATION) - .prefixWith(tree, "@Value.NaturalOrder ") - .build()); + // The ImmutableSortedSet has no empty default -> add the `@Value.NaturalOrder` annotation or + // provide a default implementation. + return buildDescription(tree) + .addFix(SuggestedFix.builder().prefixWith(tree, "@Value.NaturalOrder ").build()) + .addFix(SuggestedFix.postfixWith(tree, "return ImmutableSortedSet.of();")) + .build(); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java index c612088e01..5000a3f591 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java @@ -13,7 +13,7 @@ public final class MissingImmutableSortedSetDefaultCheckTest { .expectErrorMessage( "X", containsPattern( - "Properties of type `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class should provide a default value or specify the comparator.")); + "Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class should provide a default value or specify the comparator.")); private final BugCheckerRefactoringTestHelper refactoringTestHelper = BugCheckerRefactoringTestHelper.newInstance( MissingImmutableSortedSetDefaultCheck.class, getClass()); @@ -85,12 +85,25 @@ void identification() { " }", " @Value.NaturalOrder", " abstract ImmutableSortedSet defaultSortedSet3();", + "}", + "", + "abstract class E {", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(); } @Test - void replacement() { + void replacementInImmutableInterface() { refactoringTestHelper .addInputLines( "A.java", @@ -110,7 +123,38 @@ void replacement() { " }", " @Value.NaturalOrder", " ImmutableSortedSet defaultSortedSet3();", - "}", + "}") + .addOutputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Immutable", + "interface A {", + " @Value.NaturalOrder", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void replacementInModifiableInterface() { + refactoringTestHelper + .addInputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", "interface B {", @@ -124,7 +168,38 @@ void replacement() { " }", " @Value.NaturalOrder", " ImmutableSortedSet defaultSortedSet3();", - "}", + "}") + .addOutputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Modifiable", + "interface B {", + " @Value.NaturalOrder", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void replacementInImmutableAbstractClass() { + refactoringTestHelper + .addInputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", "abstract class C {", @@ -138,10 +213,16 @@ void replacement() { " }", " @Value.NaturalOrder", " abstract ImmutableSortedSet defaultSortedSet3();", - "}", + "}") + .addOutputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", "", - "@Value.Modifiable", - "abstract class D {", + "@Value.Immutable", + "abstract class C {", + " @Value.NaturalOrder", " abstract ImmutableSortedSet sortedSet();", " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", @@ -153,45 +234,64 @@ void replacement() { " @Value.NaturalOrder", " abstract ImmutableSortedSet defaultSortedSet3();", "}") - .addOutputLines( + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void replacementInModifiableAbstractClass() { + refactoringTestHelper + .addInputLines( "A.java", "import org.immutables.value.Value;", "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", - "@Value.Immutable", - "interface A {", - " @Value.NaturalOrder", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", + "@Value.Modifiable", + "abstract class D {", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", " }", " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", + " ImmutableSortedSet defaultSortedSet2() {", " return ImmutableSortedSet.of();", " }", " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", - "}", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}") + .addOutputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", - "interface B {", + "abstract class D {", " @Value.NaturalOrder", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", " }", " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", + " ImmutableSortedSet defaultSortedSet2() {", " return ImmutableSortedSet.of();", " }", " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", - "}", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void noReplacementInNormalClass() { + refactoringTestHelper + .addInputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", "", - "@Value.Immutable", - "abstract class C {", - " @Value.NaturalOrder", + "abstract class E {", " abstract ImmutableSortedSet sortedSet();", " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", @@ -202,11 +302,14 @@ void replacement() { " }", " @Value.NaturalOrder", " abstract ImmutableSortedSet defaultSortedSet3();", - "}", + "}") + .addOutputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", "", - "@Value.Modifiable", - "abstract class D {", - " @Value.NaturalOrder", + "abstract class E {", " abstract ImmutableSortedSet sortedSet();", " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", From 88f26eb50649c741d2106ed760505fae6f2cc67b Mon Sep 17 00:00:00 2001 From: Ferdinand Swoboda Date: Tue, 14 Jun 2022 10:47:05 +0200 Subject: [PATCH 03/15] CORE-97 Match enclosing class --- .../MissingImmutableSortedSetDefaultCheck.java | 14 ++++++++------ .../MissingImmutableSortedSetDefaultCheckTest.java | 3 ++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index b4e027219e..78ab6cd8a2 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -3,12 +3,12 @@ import static com.google.errorprone.BugPattern.LinkType.NONE; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.enclosingClass; import static com.google.errorprone.matchers.Matchers.hasAnnotation; -import static com.google.errorprone.matchers.Matchers.hasAnyAnnotation; import static com.google.errorprone.matchers.Matchers.isSameType; -import static com.google.errorprone.matchers.Matchers.isSubtypeOf; -import static com.google.errorprone.matchers.Matchers.isType; import static com.google.errorprone.matchers.Matchers.methodReturns; +import static com.google.errorprone.matchers.Matchers.not; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableSortedSet; @@ -18,7 +18,6 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MethodTree; import org.immutables.value.Value; @@ -54,8 +53,11 @@ public Description matchMethod(MethodTree tree, VisitorState state) { } // is not within immutable or modifiable class -> no match - if (!ASTHelpers.hasAnnotation(tree, org.immutables.value.Value.Immutable.class, state) - && !ASTHelpers.hasAnnotation(tree, Value.Modifiable.class, state)) { + if (enclosingClass( + allOf( + not(hasAnnotation(org.immutables.value.Value.Immutable.class)), + not(hasAnnotation(org.immutables.value.Value.Modifiable.class)))) + .matches(tree, state)) { return Description.NO_MATCH; } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java index 5000a3f591..01186e22bc 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java @@ -13,7 +13,8 @@ public final class MissingImmutableSortedSetDefaultCheckTest { .expectErrorMessage( "X", containsPattern( - "Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class should provide a default value or specify the comparator.")); + "Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class " + + "should provide a default value or specify the comparator.")); private final BugCheckerRefactoringTestHelper refactoringTestHelper = BugCheckerRefactoringTestHelper.newInstance( MissingImmutableSortedSetDefaultCheck.class, getClass()); From 9c81adb6b881d66dda730efcf7e0dc0147a32a30 Mon Sep 17 00:00:00 2001 From: Ferdinand Swoboda Date: Tue, 14 Jun 2022 11:06:50 +0200 Subject: [PATCH 04/15] CORE-97 Fix default implementation suggestion --- .../MissingImmutableSortedSetDefaultCheck.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index 78ab6cd8a2..e5f3930918 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -43,7 +43,7 @@ public final class MissingImmutableSortedSetDefaultCheck extends BugChecker @Override public Description matchMethod(MethodTree tree, VisitorState state) { // has no return type ImmutableSortedSet -> no match - if (!methodReturns(isSameType(ImmutableSortedSet.class)).matches(tree, state)) { + if (not(methodReturns(isSameType(ImmutableSortedSet.class))).matches(tree, state)) { return Description.NO_MATCH; } @@ -70,7 +70,13 @@ public Description matchMethod(MethodTree tree, VisitorState state) { // provide a default implementation. return buildDescription(tree) .addFix(SuggestedFix.builder().prefixWith(tree, "@Value.NaturalOrder ").build()) - .addFix(SuggestedFix.postfixWith(tree, "return ImmutableSortedSet.of();")) + .addFix( + SuggestedFix.builder() + .replace( + state.getEndPosition(tree) - 1, + state.getEndPosition(tree), + "{ return ImmutableSortedSet.of(); }") + .build()) .build(); } } From 89812fdf0d71193d0482e4cf10786916ea78386c Mon Sep 17 00:00:00 2001 From: Ferdinand Swoboda Date: Tue, 14 Jun 2022 11:23:16 +0200 Subject: [PATCH 05/15] CORE-97 Add test for default implementation suggestion --- ...MissingImmutableSortedSetDefaultCheck.java | 3 + ...ingImmutableSortedSetDefaultCheckTest.java | 111 ++++++++++++++++-- 2 files changed, 102 insertions(+), 12 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index e5f3930918..c14f9ae428 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -18,6 +18,7 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MethodTree; import org.immutables.value.Value; @@ -72,6 +73,8 @@ public Description matchMethod(MethodTree tree, VisitorState state) { .addFix(SuggestedFix.builder().prefixWith(tree, "@Value.NaturalOrder ").build()) .addFix( SuggestedFix.builder() + .replace( + ASTHelpers.getStartPosition(tree), ASTHelpers.getStartPosition(tree) + 8, "") .replace( state.getEndPosition(tree) - 1, state.getEndPosition(tree), diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java index 01186e22bc..826127cd5b 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java @@ -1,7 +1,5 @@ package tech.picnic.errorprone.bugpatterns; -import static com.google.common.base.Predicates.containsPattern; - import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; @@ -9,12 +7,7 @@ public final class MissingImmutableSortedSetDefaultCheckTest { private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(MissingImmutableSortedSetDefaultCheck.class, getClass()) - .expectErrorMessage( - "X", - containsPattern( - "Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class " + - "should provide a default value or specify the comparator.")); + CompilationTestHelper.newInstance(MissingImmutableSortedSetDefaultCheck.class, getClass()); private final BugCheckerRefactoringTestHelper refactoringTestHelper = BugCheckerRefactoringTestHelper.newInstance( MissingImmutableSortedSetDefaultCheck.class, getClass()); @@ -30,7 +23,7 @@ void identification() { "", "@Value.Immutable", "interface A {", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains: ", " ImmutableSortedSet sortedSet();", " default ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", @@ -45,7 +38,7 @@ void identification() { "", "@Value.Modifiable", "interface B {", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains: ", " ImmutableSortedSet sortedSet();", " default ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", @@ -60,7 +53,7 @@ void identification() { "", "@Value.Immutable", "abstract class C {", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains: ", " abstract ImmutableSortedSet sortedSet();", " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", @@ -75,7 +68,7 @@ void identification() { "", "@Value.Modifiable", "abstract class D {", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains: ", " abstract ImmutableSortedSet sortedSet();", " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", @@ -283,6 +276,100 @@ void replacementInModifiableAbstractClass() { .doTest(TestMode.TEXT_MATCH); } + @Test + void secondaryReplacementInModifiableInterface() { + refactoringTestHelper + .setFixChooser(BugCheckerRefactoringTestHelper.FixChoosers.SECOND) + .addInputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Modifiable", + "interface B {", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}") + .addOutputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Modifiable", + "interface B {", + " default ImmutableSortedSet sortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void secondaryReplacementInImmutableAbstractClass() { + refactoringTestHelper + .setFixChooser(BugCheckerRefactoringTestHelper.FixChoosers.SECOND) + .addInputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Immutable", + "abstract class C {", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}") + .addOutputLines( + "A.java", + "import org.immutables.value.Value;", + "import com.google.common.collect.ImmutableSet;", + "import com.google.common.collect.ImmutableSortedSet;", + "", + "@Value.Immutable", + "abstract class C {", + " ImmutableSortedSet sortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", + "}") + .doTest(TestMode.TEXT_MATCH); + } + @Test void noReplacementInNormalClass() { refactoringTestHelper From aabf777dac57c5c8c40448e044254ae06b159c30 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 14 Jun 2022 16:24:06 +0200 Subject: [PATCH 06/15] Suggestions --- ...MissingImmutableSortedSetDefaultCheck.java | 33 +- ...ingImmutableSortedSetDefaultCheckTest.java | 412 +++++++++--------- 2 files changed, 220 insertions(+), 225 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index c14f9ae428..243632226e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -3,12 +3,11 @@ import static com.google.errorprone.BugPattern.LinkType.NONE; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; -import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.enclosingClass; import static com.google.errorprone.matchers.Matchers.hasAnnotation; import static com.google.errorprone.matchers.Matchers.isSameType; import static com.google.errorprone.matchers.Matchers.methodReturns; -import static com.google.errorprone.matchers.Matchers.not; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableSortedSet; @@ -18,9 +17,13 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MethodTree; -import org.immutables.value.Value; +import com.sun.source.tree.Tree; +import org.immutables.value.Value.Immutable; +import org.immutables.value.Value.Modifiable; +import org.immutables.value.Value.NaturalOrder; /** * A {@link BugChecker} which flags methods with return type {@link @@ -40,33 +43,23 @@ public final class MissingImmutableSortedSetDefaultCheck extends BugChecker implements MethodTreeMatcher { private static final long serialVersionUID = 1L; + private static final Matcher ENCLOSING_IS_MODIFIABLE_OR_IMMUTABLES = + enclosingClass(anyOf(hasAnnotation(Immutable.class), hasAnnotation(Modifiable.class))); + private static final Matcher RETURNS_IMMUTABLE_SORTED_SET = + methodReturns(isSameType(ImmutableSortedSet.class)); @Override public Description matchMethod(MethodTree tree, VisitorState state) { - // has no return type ImmutableSortedSet -> no match - if (not(methodReturns(isSameType(ImmutableSortedSet.class))).matches(tree, state)) { + if (!RETURNS_IMMUTABLE_SORTED_SET.matches(tree, state) + || !ENCLOSING_IS_MODIFIABLE_OR_IMMUTABLES.matches(tree, state) + || hasAnnotation(NaturalOrder.class).matches(tree, state)) { return Description.NO_MATCH; } - // has implementation -> no match if (tree.getBody() != null && !tree.getBody().getStatements().isEmpty()) { return Description.NO_MATCH; } - // is not within immutable or modifiable class -> no match - if (enclosingClass( - allOf( - not(hasAnnotation(org.immutables.value.Value.Immutable.class)), - not(hasAnnotation(org.immutables.value.Value.Modifiable.class)))) - .matches(tree, state)) { - return Description.NO_MATCH; - } - - // is annotated with @Value.NaturalOrder -> no match - if (hasAnnotation(Value.NaturalOrder.class).matches(tree, state)) { - return Description.NO_MATCH; - } - // The ImmutableSortedSet has no empty default -> add the `@Value.NaturalOrder` annotation or // provide a default implementation. return buildDescription(tree) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java index 826127cd5b..43121116c5 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java @@ -1,11 +1,13 @@ package tech.picnic.errorprone.bugpatterns; +import static com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers.SECOND; + import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -public final class MissingImmutableSortedSetDefaultCheckTest { +final class MissingImmutableSortedSetDefaultCheckTest { private final CompilationTestHelper compilationTestHelper = CompilationTestHelper.newInstance(MissingImmutableSortedSetDefaultCheck.class, getClass()); private final BugCheckerRefactoringTestHelper refactoringTestHelper = @@ -23,75 +25,75 @@ void identification() { "", "@Value.Immutable", "interface A {", - " // BUG: Diagnostic contains: ", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " // BUG: Diagnostic contains:", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", "}", "", "@Value.Modifiable", "interface B {", - " // BUG: Diagnostic contains: ", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " // BUG: Diagnostic contains:", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", "}", "", "@Value.Immutable", "abstract class C {", - " // BUG: Diagnostic contains: ", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " // BUG: Diagnostic contains: ", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}", "", "@Value.Modifiable", "abstract class D {", - " // BUG: Diagnostic contains: ", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " // BUG: Diagnostic contains:", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}", "", "abstract class E {", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(); } @@ -107,16 +109,16 @@ void replacementInImmutableInterface() { "", "@Value.Immutable", "interface A {", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -126,17 +128,17 @@ void replacementInImmutableInterface() { "", "@Value.Immutable", "interface A {", - " @Value.NaturalOrder", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " @Value.NaturalOrder", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } @@ -152,16 +154,16 @@ void replacementInModifiableInterface() { "", "@Value.Modifiable", "interface B {", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -171,17 +173,17 @@ void replacementInModifiableInterface() { "", "@Value.Modifiable", "interface B {", - " @Value.NaturalOrder", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " @Value.NaturalOrder", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } @@ -197,16 +199,16 @@ void replacementInImmutableAbstractClass() { "", "@Value.Immutable", "abstract class C {", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -216,17 +218,17 @@ void replacementInImmutableAbstractClass() { "", "@Value.Immutable", "abstract class C {", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } @@ -242,16 +244,16 @@ void replacementInModifiableAbstractClass() { "", "@Value.Modifiable", "abstract class D {", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -261,17 +263,17 @@ void replacementInModifiableAbstractClass() { "", "@Value.Modifiable", "abstract class D {", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } @@ -279,7 +281,7 @@ void replacementInModifiableAbstractClass() { @Test void secondaryReplacementInModifiableInterface() { refactoringTestHelper - .setFixChooser(BugCheckerRefactoringTestHelper.FixChoosers.SECOND) + .setFixChooser(SECOND) .addInputLines( "A.java", "import org.immutables.value.Value;", @@ -288,16 +290,16 @@ void secondaryReplacementInModifiableInterface() { "", "@Value.Modifiable", "interface B {", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " ImmutableSortedSet sortedSet();", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -307,18 +309,18 @@ void secondaryReplacementInModifiableInterface() { "", "@Value.Modifiable", "interface B {", - " default ImmutableSortedSet sortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " default ImmutableSortedSet sortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " default ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " default ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } @@ -326,7 +328,7 @@ void secondaryReplacementInModifiableInterface() { @Test void secondaryReplacementInImmutableAbstractClass() { refactoringTestHelper - .setFixChooser(BugCheckerRefactoringTestHelper.FixChoosers.SECOND) + .setFixChooser(SECOND) .addInputLines( "A.java", "import org.immutables.value.Value;", @@ -335,16 +337,16 @@ void secondaryReplacementInImmutableAbstractClass() { "", "@Value.Immutable", "abstract class C {", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -354,18 +356,18 @@ void secondaryReplacementInImmutableAbstractClass() { "", "@Value.Immutable", "abstract class C {", - " ImmutableSortedSet sortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " ImmutableSortedSet sortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } @@ -380,16 +382,16 @@ void noReplacementInNormalClass() { "import com.google.common.collect.ImmutableSortedSet;", "", "abstract class E {", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -398,16 +400,16 @@ void noReplacementInNormalClass() { "import com.google.common.collect.ImmutableSortedSet;", "", "abstract class E {", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " abstract ImmutableSortedSet sortedSet();", + " ImmutableSortedSet defaultSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.Default", + " ImmutableSortedSet defaultSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", + " @Value.NaturalOrder", + " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } From 69d0b195f72e5d9fdf707526e9c4509d4ac313c3 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 14 Jun 2022 16:55:28 +0200 Subject: [PATCH 07/15] Drop `ImmutableSet` imports in the tests --- ...MissingImmutableSortedSetDefaultCheckTest.java | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java index 43121116c5..dec6a045b8 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java @@ -20,7 +20,6 @@ void identification() { .addSourceLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", @@ -104,7 +103,6 @@ void replacementInImmutableInterface() { .addInputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", @@ -123,7 +121,6 @@ void replacementInImmutableInterface() { .addOutputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", @@ -149,7 +146,6 @@ void replacementInModifiableInterface() { .addInputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", @@ -168,7 +164,6 @@ void replacementInModifiableInterface() { .addOutputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", @@ -194,7 +189,6 @@ void replacementInImmutableAbstractClass() { .addInputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", @@ -213,7 +207,6 @@ void replacementInImmutableAbstractClass() { .addOutputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", @@ -239,7 +232,6 @@ void replacementInModifiableAbstractClass() { .addInputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", @@ -258,7 +250,6 @@ void replacementInModifiableAbstractClass() { .addOutputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", @@ -285,7 +276,6 @@ void secondaryReplacementInModifiableInterface() { .addInputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", @@ -304,7 +294,6 @@ void secondaryReplacementInModifiableInterface() { .addOutputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", @@ -332,7 +321,6 @@ void secondaryReplacementInImmutableAbstractClass() { .addInputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", @@ -351,7 +339,6 @@ void secondaryReplacementInImmutableAbstractClass() { .addOutputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", @@ -378,7 +365,6 @@ void noReplacementInNormalClass() { .addInputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "abstract class E {", @@ -396,7 +382,6 @@ void noReplacementInNormalClass() { .addOutputLines( "A.java", "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", "", "abstract class E {", From 7061f5727e9bbd5d5772c70e510d37b47f7295f9 Mon Sep 17 00:00:00 2001 From: Ferdinand Swoboda Date: Wed, 15 Jun 2022 15:19:18 +0200 Subject: [PATCH 08/15] CORE-97 Collapse replacement tests --- ...MissingImmutableSortedSetDefaultCheck.java | 14 +- ...ingImmutableSortedSetDefaultCheckTest.java | 236 ++---------------- 2 files changed, 26 insertions(+), 224 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index 243632226e..cc547f0675 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -18,7 +18,6 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; import org.immutables.value.Value.Immutable; @@ -63,16 +62,11 @@ public Description matchMethod(MethodTree tree, VisitorState state) { // The ImmutableSortedSet has no empty default -> add the `@Value.NaturalOrder` annotation or // provide a default implementation. return buildDescription(tree) + .setMessage( + "Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class " + + "should include additional deserialization information for absent sets. " + + "Alternatively, specify a default implementation.") .addFix(SuggestedFix.builder().prefixWith(tree, "@Value.NaturalOrder ").build()) - .addFix( - SuggestedFix.builder() - .replace( - ASTHelpers.getStartPosition(tree), ASTHelpers.getStartPosition(tree) + 8, "") - .replace( - state.getEndPosition(tree) - 1, - state.getEndPosition(tree), - "{ return ImmutableSortedSet.of(); }") - .build()) .build(); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java index dec6a045b8..0268846c37 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java @@ -1,7 +1,5 @@ package tech.picnic.errorprone.bugpatterns; -import static com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers.SECOND; - import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; @@ -98,7 +96,7 @@ void identification() { } @Test - void replacementInImmutableInterface() { + void replacementInImmutable() { refactoringTestHelper .addInputLines( "A.java", @@ -106,103 +104,13 @@ void replacementInImmutableInterface() { "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", - "interface A {", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", - "}") - .addOutputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", - "", - "@Value.Immutable", - "interface A {", - " @Value.NaturalOrder", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void replacementInModifiableInterface() { - refactoringTestHelper - .addInputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", - "", - "@Value.Modifiable", - "interface B {", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", - "}") - .addOutputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", - "", - "@Value.Modifiable", - "interface B {", - " @Value.NaturalOrder", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void replacementInImmutableAbstractClass() { - refactoringTestHelper - .addInputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", - "", - "@Value.Immutable", - "abstract class C {", + "abstract class A {", " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", + "", + " @Value.Immutable", + " interface B {", + " ImmutableSortedSet sortedSet();", " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -210,24 +118,21 @@ void replacementInImmutableAbstractClass() { "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Immutable", - "abstract class C {", + "abstract class A {", " @Value.NaturalOrder", " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", + "", + " @Value.Immutable", + " interface B {", + " @Value.NaturalOrder", + " ImmutableSortedSet sortedSet();", " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } @Test - void replacementInModifiableAbstractClass() { + void replacementInModifiable() { refactoringTestHelper .addInputLines( "A.java", @@ -235,61 +140,13 @@ void replacementInModifiableAbstractClass() { "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", - "abstract class D {", + "abstract class A {", " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", - "}") - .addOutputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", "", - "@Value.Modifiable", - "abstract class D {", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", + " @Value.Modifiable", + " interface B {", + " ImmutableSortedSet sortedSet();", " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void secondaryReplacementInModifiableInterface() { - refactoringTestHelper - .setFixChooser(SECOND) - .addInputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", - "", - "@Value.Modifiable", - "interface B {", - " ImmutableSortedSet sortedSet();", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", "}") .addOutputLines( "A.java", @@ -297,64 +154,15 @@ void secondaryReplacementInModifiableInterface() { "import com.google.common.collect.ImmutableSortedSet;", "", "@Value.Modifiable", - "interface B {", - " default ImmutableSortedSet sortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", + "abstract class A {", " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void secondaryReplacementInImmutableAbstractClass() { - refactoringTestHelper - .setFixChooser(SECOND) - .addInputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", - "", - "@Value.Immutable", - "abstract class C {", " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", - "}") - .addOutputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", "", - "@Value.Immutable", - "abstract class C {", - " ImmutableSortedSet sortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", + " @Value.Modifiable", + " interface B {", + " @Value.NaturalOrder", + " ImmutableSortedSet sortedSet();", " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(TestMode.TEXT_MATCH); } From f4f5c4d635061b8273fafe06873aa2b442444658 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 26 Jun 2022 12:39:42 +0200 Subject: [PATCH 09/15] Classpath suggestions --- error-prone-contrib/pom.xml | 3 ++- .../MissingImmutableSortedSetDefaultCheck.java | 10 +++++----- pom.xml | 4 ++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index 51c3f91b3d..9f455e7838 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -145,7 +145,8 @@ org.immutables - value + value-annotations + test org.junit.jupiter diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index cc547f0675..1e95f9e00b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -20,9 +20,6 @@ import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; -import org.immutables.value.Value.Immutable; -import org.immutables.value.Value.Modifiable; -import org.immutables.value.Value.NaturalOrder; /** * A {@link BugChecker} which flags methods with return type {@link @@ -43,7 +40,10 @@ public final class MissingImmutableSortedSetDefaultCheck extends BugChecker implements MethodTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher ENCLOSING_IS_MODIFIABLE_OR_IMMUTABLES = - enclosingClass(anyOf(hasAnnotation(Immutable.class), hasAnnotation(Modifiable.class))); + enclosingClass( + anyOf( + hasAnnotation("org.immutables.value.Value.Immutable"), + hasAnnotation("org.immutables.value.Value.Modifiable"))); private static final Matcher RETURNS_IMMUTABLE_SORTED_SET = methodReturns(isSameType(ImmutableSortedSet.class)); @@ -51,7 +51,7 @@ public final class MissingImmutableSortedSetDefaultCheck extends BugChecker public Description matchMethod(MethodTree tree, VisitorState state) { if (!RETURNS_IMMUTABLE_SORTED_SET.matches(tree, state) || !ENCLOSING_IS_MODIFIABLE_OR_IMMUTABLES.matches(tree, state) - || hasAnnotation(NaturalOrder.class).matches(tree, state)) { + || hasAnnotation("org.immutables.value.Value.NaturalOrder").matches(tree, state)) { return Description.NO_MATCH; } diff --git a/pom.xml b/pom.xml index 4e6566a5e9..b2a01c63b5 100644 --- a/pom.xml +++ b/pom.xml @@ -388,8 +388,8 @@ org.immutables - value - 2.8.2 + value-annotations + 2.9.0 org.junit From 7553dbcb4a15991efa7d3097ba7f5c37f20ff3a2 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 30 Jun 2022 15:57:44 +0200 Subject: [PATCH 10/15] Suggestions --- ...MissingImmutableSortedSetDefaultCheck.java | 34 ++++--- ...ingImmutableSortedSetDefaultCheckTest.java | 91 ++++--------------- 2 files changed, 36 insertions(+), 89 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index 1e95f9e00b..42c8a12b51 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -23,34 +23,37 @@ /** * A {@link BugChecker} which flags methods with return type {@link - * com.google.common.collect.ImmutableSortedSet} within an {@code @Value.Immutable} or - * {@code @Value.Modifiable} class that lack either a default implementation or - * {@code @Value.NaturalOrder}. + * com.google.common.collect.ImmutableSortedSet} within a class or interface annotated with + * {@code @Value.Immutable} or {@code @Value.Modifiable} and lacks either a default implementation + * or {@code @Value.NaturalOrder} annotation. + * + *

Such methods without {@code @Value.NaturalOrder} or default implementation would result in + * deserialization problems in case of absent sets. */ @AutoService(BugChecker.class) @BugPattern( name = "MissingImmutableSortedSetDefault", summary = - "Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class " - + "should provide a default value or specify the comparator.", + "Methods returning an `ImmutableSortedSet` within a `@Value.Immutable` or `@Value.Modifiable` class " + + "or interface should provide a default value or specify a comparator", linkType = NONE, severity = ERROR, tags = LIKELY_ERROR) public final class MissingImmutableSortedSetDefaultCheck extends BugChecker implements MethodTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher ENCLOSING_IS_MODIFIABLE_OR_IMMUTABLES = + private static final Matcher RETURNS_IMMUTABLE_SORTED_SET = + methodReturns(isSameType(ImmutableSortedSet.class)); + private static final Matcher ENCLOSING_IS_IMMUTABLE_OR_MODIFIABLE = enclosingClass( anyOf( hasAnnotation("org.immutables.value.Value.Immutable"), hasAnnotation("org.immutables.value.Value.Modifiable"))); - private static final Matcher RETURNS_IMMUTABLE_SORTED_SET = - methodReturns(isSameType(ImmutableSortedSet.class)); @Override public Description matchMethod(MethodTree tree, VisitorState state) { if (!RETURNS_IMMUTABLE_SORTED_SET.matches(tree, state) - || !ENCLOSING_IS_MODIFIABLE_OR_IMMUTABLES.matches(tree, state) + || !ENCLOSING_IS_IMMUTABLE_OR_MODIFIABLE.matches(tree, state) || hasAnnotation("org.immutables.value.Value.NaturalOrder").matches(tree, state)) { return Description.NO_MATCH; } @@ -59,14 +62,15 @@ public Description matchMethod(MethodTree tree, VisitorState state) { return Description.NO_MATCH; } - // The ImmutableSortedSet has no empty default -> add the `@Value.NaturalOrder` annotation or - // provide a default implementation. return buildDescription(tree) .setMessage( - "Methods returning an `ImmutableSortedSet` within an @Value.Immutable or @Value.Modifiable class " - + "should include additional deserialization information for absent sets. " - + "Alternatively, specify a default implementation.") - .addFix(SuggestedFix.builder().prefixWith(tree, "@Value.NaturalOrder ").build()) + "Methods returning an `ImmutableSortedSet` within a `@Value.Immutable` or `@Value.Modifiable` class or " + + "interface should be annotated with `@Value.NaturalOrder` or specify a default implementation") + .addFix( + SuggestedFix.builder() + .addImport("org.immutables.value.Value") + .prefixWith(tree, "@Value.NaturalOrder ") + .build()) .build(); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java index 0268846c37..be7bd9cfd7 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java @@ -17,80 +17,63 @@ void identification() { compilationTestHelper .addSourceLines( "A.java", - "import org.immutables.value.Value;", "import com.google.common.collect.ImmutableSortedSet;", + "import org.immutables.value.Value;", "", "@Value.Immutable", "interface A {", " // BUG: Diagnostic contains:", " ImmutableSortedSet sortedSet();", + "", " default ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", + "", " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " ImmutableSortedSet defaultSortedSet2();", "}", "", "@Value.Modifiable", "interface B {", " // BUG: Diagnostic contains:", " ImmutableSortedSet sortedSet();", + "", " default ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", " }", - " @Value.Default", - " default ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", + "", " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet3();", + " ImmutableSortedSet defaultSortedSet2();", "}", "", "@Value.Immutable", "abstract class C {", " // BUG: Diagnostic contains: ", " abstract ImmutableSortedSet sortedSet();", + "", " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", + "", " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " abstract ImmutableSortedSet defaultSortedSet2();", "}", "", "@Value.Modifiable", "abstract class D {", " // BUG: Diagnostic contains:", " abstract ImmutableSortedSet sortedSet();", + "", " ImmutableSortedSet defaultSortedSet() {", " return ImmutableSortedSet.of();", " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", + "", " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", + " abstract ImmutableSortedSet defaultSortedSet2();", "}", "", "abstract class E {", " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", "}") .doTest(); } @@ -100,8 +83,8 @@ void replacementInImmutable() { refactoringTestHelper .addInputLines( "A.java", - "import org.immutables.value.Value;", "import com.google.common.collect.ImmutableSortedSet;", + "import org.immutables.value.Value;", "", "@Value.Immutable", "abstract class A {", @@ -114,8 +97,8 @@ void replacementInImmutable() { "}") .addOutputLines( "A.java", - "import org.immutables.value.Value;", "import com.google.common.collect.ImmutableSortedSet;", + "import org.immutables.value.Value;", "", "@Value.Immutable", "abstract class A {", @@ -136,8 +119,8 @@ void replacementInModifiable() { refactoringTestHelper .addInputLines( "A.java", - "import org.immutables.value.Value;", "import com.google.common.collect.ImmutableSortedSet;", + "import org.immutables.value.Value;", "", "@Value.Modifiable", "abstract class A {", @@ -150,8 +133,8 @@ void replacementInModifiable() { "}") .addOutputLines( "A.java", - "import org.immutables.value.Value;", "import com.google.common.collect.ImmutableSortedSet;", + "import org.immutables.value.Value;", "", "@Value.Modifiable", "abstract class A {", @@ -166,44 +149,4 @@ void replacementInModifiable() { "}") .doTest(TestMode.TEXT_MATCH); } - - @Test - void noReplacementInNormalClass() { - refactoringTestHelper - .addInputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", - "", - "abstract class E {", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", - "}") - .addOutputLines( - "A.java", - "import org.immutables.value.Value;", - "import com.google.common.collect.ImmutableSortedSet;", - "", - "abstract class E {", - " abstract ImmutableSortedSet sortedSet();", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.Default", - " ImmutableSortedSet defaultSortedSet2() {", - " return ImmutableSortedSet.of();", - " }", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet3();", - "}") - .doTest(TestMode.TEXT_MATCH); - } } From 363410644dd88a02de436a8e4c15ad5b3dfdab77 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 31 Jul 2022 16:25:29 +0200 Subject: [PATCH 11/15] Suggestions --- ...MissingImmutableSortedSetDefaultCheck.java | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index 42c8a12b51..e024bac5b8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -22,20 +22,21 @@ import com.sun.source.tree.Tree; /** - * A {@link BugChecker} which flags methods with return type {@link - * com.google.common.collect.ImmutableSortedSet} within a class or interface annotated with - * {@code @Value.Immutable} or {@code @Value.Modifiable} and lacks either a default implementation - * or {@code @Value.NaturalOrder} annotation. + * A {@link BugChecker} which flags instance methods with return type {@link + * com.google.common.collect.ImmutableSortedSet} defined inside a {@code @Value.Immutable}- or + * {@code @Value.Modifiable}-annotated type that lack both a default implementation and the + * {@code @Value.NaturalOrder} annotation. * - *

Such methods without {@code @Value.NaturalOrder} or default implementation would result in - * deserialization problems in case of absent sets. + *

Deserialization of the enclosing type then requires that the associated JSON property is + * present, would result in deserialization problems in case of absent sets. */ @AutoService(BugChecker.class) @BugPattern( name = "MissingImmutableSortedSetDefault", summary = - "Methods returning an `ImmutableSortedSet` within a `@Value.Immutable` or `@Value.Modifiable` class " - + "or interface should provide a default value or specify a comparator", + "`ImmutableSortedSet` properties of a `@Value.Immutable` or `@Value.Modifiable` type " + + "should be annotated `@Value.NaturalOrder` or `@Value.ReverseOrder`, or provide a " + + "default value", linkType = NONE, severity = ERROR, tags = LIKELY_ERROR) @@ -44,17 +45,21 @@ public final class MissingImmutableSortedSetDefaultCheck extends BugChecker private static final long serialVersionUID = 1L; private static final Matcher RETURNS_IMMUTABLE_SORTED_SET = methodReturns(isSameType(ImmutableSortedSet.class)); - private static final Matcher ENCLOSING_IS_IMMUTABLE_OR_MODIFIABLE = + private static final Matcher IS_IMMUTABLES_TYPE = enclosingClass( anyOf( hasAnnotation("org.immutables.value.Value.Immutable"), hasAnnotation("org.immutables.value.Value.Modifiable"))); + private static final Matcher HAS_ORDER_ANNOTATION = + anyOf( + hasAnnotation("org.immutables.value.Value.NaturalOrder"), + hasAnnotation("org.immutables.value.Value.ReverseOrder")); @Override public Description matchMethod(MethodTree tree, VisitorState state) { if (!RETURNS_IMMUTABLE_SORTED_SET.matches(tree, state) - || !ENCLOSING_IS_IMMUTABLE_OR_MODIFIABLE.matches(tree, state) - || hasAnnotation("org.immutables.value.Value.NaturalOrder").matches(tree, state)) { + || !IS_IMMUTABLES_TYPE.matches(tree, state) + || HAS_ORDER_ANNOTATION.matches(tree, state)) { return Description.NO_MATCH; } @@ -63,9 +68,6 @@ public Description matchMethod(MethodTree tree, VisitorState state) { } return buildDescription(tree) - .setMessage( - "Methods returning an `ImmutableSortedSet` within a `@Value.Immutable` or `@Value.Modifiable` class or " - + "interface should be annotated with `@Value.NaturalOrder` or specify a default implementation") .addFix( SuggestedFix.builder() .addImport("org.immutables.value.Value") From 5501429678147dfaedde8ecef8f61fb71c61f5f0 Mon Sep 17 00:00:00 2001 From: Ferdinand Swoboda Date: Tue, 9 Aug 2022 17:28:37 +0200 Subject: [PATCH 12/15] CORE-97 Update documentation --- .../bugpatterns/MissingImmutableSortedSetDefaultCheck.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java index e024bac5b8..a321aa5495 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java @@ -24,8 +24,8 @@ /** * A {@link BugChecker} which flags instance methods with return type {@link * com.google.common.collect.ImmutableSortedSet} defined inside a {@code @Value.Immutable}- or - * {@code @Value.Modifiable}-annotated type that lack both a default implementation and the - * {@code @Value.NaturalOrder} annotation. + * {@code @Value.Modifiable}-annotated type that lack the {@code @Value.NaturalOrder} or + * {@code @Value.ReverseOrder} annotation. * *

Deserialization of the enclosing type then requires that the associated JSON property is * present, would result in deserialization problems in case of absent sets. @@ -35,8 +35,7 @@ name = "MissingImmutableSortedSetDefault", summary = "`ImmutableSortedSet` properties of a `@Value.Immutable` or `@Value.Modifiable` type " - + "should be annotated `@Value.NaturalOrder` or `@Value.ReverseOrder`, or provide a " - + "default value", + + "should be annotated `@Value.NaturalOrder` or `@Value.ReverseOrder`.", linkType = NONE, severity = ERROR, tags = LIKELY_ERROR) From 402da2b9095e58fa20fb1302ed1f7772579c4aae Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 14 Aug 2022 10:48:15 +0200 Subject: [PATCH 13/15] Suggestions --- .../ImmutablesSortedSetComparator.java | 80 +++++++++++++++++++ ...MissingImmutableSortedSetDefaultCheck.java | 77 ------------------ ...=> ImmutablesSortedSetComparatorTest.java} | 9 +-- 3 files changed, 84 insertions(+), 82 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java delete mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{MissingImmutableSortedSetDefaultCheckTest.java => ImmutablesSortedSetComparatorTest.java} (93%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java new file mode 100644 index 0000000000..c235c9b147 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java @@ -0,0 +1,80 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.NONE; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.enclosingClass; +import static com.google.errorprone.matchers.Matchers.hasAnnotation; +import static com.google.errorprone.matchers.Matchers.hasModifier; +import static com.google.errorprone.matchers.Matchers.isSubtypeOf; +import static com.google.errorprone.matchers.Matchers.methodReturns; +import static com.google.errorprone.matchers.Matchers.not; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.MethodTree; +import java.util.SortedSet; +import javax.lang.model.element.Modifier; + +/** + * A {@link BugChecker} which flags {@link SortedSet} property declarations inside + * {@code @Value.Immutable}- and {@code @Value.Modifiable}-annotated types that lack a + * {@code @Value.NaturalOrder} or {@code @Value.ReverseOrder} annotation. + * + *

Without such an annotation: + * + *

    + *
  • deserialization of the enclosing type requires that the associated JSON property is + * present, contrary to the way in which Immutables handles other collection properties; and + *
  • different instances may use different comparator implementations (e.g. deserialization + * would default to natural order sorting), potentially leading to subtle bugs. + *
+ */ +@AutoService(BugChecker.class) +@BugPattern( + summary = + "`SortedSet` properties of a `@Value.Immutable` or `@Value.Modifiable` type must be " + + "annotated `@Value.NaturalOrder` or `@Value.ReverseOrder`", + linkType = NONE, + severity = ERROR, + tags = LIKELY_ERROR) +public final class ImmutablesSortedSetComparator extends BugChecker implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher METHOD_LACKS_ANNOTATION = + allOf( + methodReturns(isSubtypeOf(SortedSet.class)), + anyOf( + allOf( + hasModifier(Modifier.ABSTRACT), + enclosingClass( + anyOf( + hasAnnotation("org.immutables.value.Value.Immutable"), + hasAnnotation("org.immutables.value.Value.Modifiable")))), + hasAnnotation("org.immutables.value.Value.Default")), + not( + anyOf( + hasAnnotation("org.immutables.value.Value.NaturalOrder"), + hasAnnotation("org.immutables.value.Value.ReverseOrder")))); + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (!METHOD_LACKS_ANNOTATION.matches(tree, state)) { + return Description.NO_MATCH; + } + + return describeMatch( + tree, + SuggestedFix.builder() + .addImport("org.immutables.value.Value") + .prefixWith(tree, "@Value.NaturalOrder ") + .build()); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java deleted file mode 100644 index a321aa5495..0000000000 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheck.java +++ /dev/null @@ -1,77 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import static com.google.errorprone.BugPattern.LinkType.NONE; -import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; -import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; -import static com.google.errorprone.matchers.Matchers.anyOf; -import static com.google.errorprone.matchers.Matchers.enclosingClass; -import static com.google.errorprone.matchers.Matchers.hasAnnotation; -import static com.google.errorprone.matchers.Matchers.isSameType; -import static com.google.errorprone.matchers.Matchers.methodReturns; - -import com.google.auto.service.AutoService; -import com.google.common.collect.ImmutableSortedSet; -import com.google.errorprone.BugPattern; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; -import com.google.errorprone.fixes.SuggestedFix; -import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; -import com.sun.source.tree.MethodTree; -import com.sun.source.tree.Tree; - -/** - * A {@link BugChecker} which flags instance methods with return type {@link - * com.google.common.collect.ImmutableSortedSet} defined inside a {@code @Value.Immutable}- or - * {@code @Value.Modifiable}-annotated type that lack the {@code @Value.NaturalOrder} or - * {@code @Value.ReverseOrder} annotation. - * - *

Deserialization of the enclosing type then requires that the associated JSON property is - * present, would result in deserialization problems in case of absent sets. - */ -@AutoService(BugChecker.class) -@BugPattern( - name = "MissingImmutableSortedSetDefault", - summary = - "`ImmutableSortedSet` properties of a `@Value.Immutable` or `@Value.Modifiable` type " - + "should be annotated `@Value.NaturalOrder` or `@Value.ReverseOrder`.", - linkType = NONE, - severity = ERROR, - tags = LIKELY_ERROR) -public final class MissingImmutableSortedSetDefaultCheck extends BugChecker - implements MethodTreeMatcher { - private static final long serialVersionUID = 1L; - private static final Matcher RETURNS_IMMUTABLE_SORTED_SET = - methodReturns(isSameType(ImmutableSortedSet.class)); - private static final Matcher IS_IMMUTABLES_TYPE = - enclosingClass( - anyOf( - hasAnnotation("org.immutables.value.Value.Immutable"), - hasAnnotation("org.immutables.value.Value.Modifiable"))); - private static final Matcher HAS_ORDER_ANNOTATION = - anyOf( - hasAnnotation("org.immutables.value.Value.NaturalOrder"), - hasAnnotation("org.immutables.value.Value.ReverseOrder")); - - @Override - public Description matchMethod(MethodTree tree, VisitorState state) { - if (!RETURNS_IMMUTABLE_SORTED_SET.matches(tree, state) - || !IS_IMMUTABLES_TYPE.matches(tree, state) - || HAS_ORDER_ANNOTATION.matches(tree, state)) { - return Description.NO_MATCH; - } - - if (tree.getBody() != null && !tree.getBody().getStatements().isEmpty()) { - return Description.NO_MATCH; - } - - return buildDescription(tree) - .addFix( - SuggestedFix.builder() - .addImport("org.immutables.value.Value") - .prefixWith(tree, "@Value.NaturalOrder ") - .build()) - .build(); - } -} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparatorTest.java similarity index 93% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparatorTest.java index be7bd9cfd7..9f68f59e66 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MissingImmutableSortedSetDefaultCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparatorTest.java @@ -5,12 +5,11 @@ import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -final class MissingImmutableSortedSetDefaultCheckTest { +final class ImmutablesSortedSetComparatorTest { private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(MissingImmutableSortedSetDefaultCheck.class, getClass()); + CompilationTestHelper.newInstance(ImmutablesSortedSetComparator.class, getClass()); private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance( - MissingImmutableSortedSetDefaultCheck.class, getClass()); + BugCheckerRefactoringTestHelper.newInstance(ImmutablesSortedSetComparator.class, getClass()); @Test void identification() { @@ -48,7 +47,7 @@ void identification() { "", "@Value.Immutable", "abstract class C {", - " // BUG: Diagnostic contains: ", + " // BUG: Diagnostic contains:", " abstract ImmutableSortedSet sortedSet();", "", " ImmutableSortedSet defaultSortedSet() {", From 42c5bb77cd2d424b973531873e791d59ba3e00d8 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 18 Aug 2022 08:11:35 +0200 Subject: [PATCH 14/15] Suggestions --- .../ImmutablesSortedSetComparator.java | 9 +- .../ImmutablesSortedSetComparatorTest.java | 155 +++++++++++------- 2 files changed, 98 insertions(+), 66 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java index c235c9b147..8fd35dbbdc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java @@ -18,6 +18,7 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.MethodTree; @@ -70,11 +71,11 @@ public Description matchMethod(MethodTree tree, VisitorState state) { return Description.NO_MATCH; } + SuggestedFix.Builder builder = SuggestedFix.builder(); + String valueTypeIdentifier = + SuggestedFixes.qualifyType(state, builder, "org.immutables.value.Value"); return describeMatch( tree, - SuggestedFix.builder() - .addImport("org.immutables.value.Value") - .prefixWith(tree, "@Value.NaturalOrder ") - .build()); + builder.prefixWith(tree, String.format("@%s.NaturalOrder ", valueTypeIdentifier)).build()); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparatorTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparatorTest.java index 9f68f59e66..3942c6de1e 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparatorTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparatorTest.java @@ -16,87 +16,119 @@ void identification() { compilationTestHelper .addSourceLines( "A.java", + "import com.google.common.collect.ContiguousSet;", + "import com.google.common.collect.ImmutableSet;", "import com.google.common.collect.ImmutableSortedSet;", + "import java.util.NavigableSet;", + "import java.util.Set;", + "import java.util.SortedSet;", + "import java.util.TreeSet;", "import org.immutables.value.Value;", "", - "@Value.Immutable", "interface A {", - " // BUG: Diagnostic contains:", - " ImmutableSortedSet sortedSet();", - "", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", - " }", + " @Value.Immutable", + " interface ImmutableInterface {", + " Set set();", "", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet2();", - "}", + " // BUG: Diagnostic contains:", + " SortedSet sortedSet();", "", - "@Value.Modifiable", - "interface B {", - " // BUG: Diagnostic contains:", - " ImmutableSortedSet sortedSet();", + " @Value.NaturalOrder", + " SortedSet sortedSet2();", + " }", "", - " default ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", + " @Value.Modifiable", + " interface ModifiableInterfaceWithDefaults {", + " @Value.Default", + " default Set set() {", + " return new TreeSet<>();", + " }", + "", + " @Value.Default", + " // BUG: Diagnostic contains:", + " default NavigableSet navigableSet() {", + " return new TreeSet<>();", + " }", + "", + " @Value.Default", + " @Value.ReverseOrder", + " default NavigableSet navigableSet2() {", + " return new TreeSet<>();", + " }", + "", + " default NavigableSet nonPropertyNavigableSet() {", + " return new TreeSet<>();", + " }", " }", "", - " @Value.NaturalOrder", - " ImmutableSortedSet defaultSortedSet2();", - "}", + " interface NonImmutablesInterface {", + " SortedSet sortedSet();", + " }", "", - "@Value.Immutable", - "abstract class C {", - " // BUG: Diagnostic contains:", - " abstract ImmutableSortedSet sortedSet();", + " @Value.Immutable", + " abstract class AbstractImmutableWithDefaults {", + " @Value.Default", + " ImmutableSet immutableSet() {", + " return ImmutableSet.of();", + " }", + "", + " @Value.Default", + " // BUG: Diagnostic contains:", + " ImmutableSortedSet immutableSortedSet() {", + " return ImmutableSortedSet.of();", + " }", + "", + " @Value.Default", + " @Value.NaturalOrder", + " ImmutableSortedSet immutableSortedSet2() {", + " return ImmutableSortedSet.of();", + " }", "", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", + " ImmutableSortedSet nonPropertyImmutableSortedSet() {", + " return ImmutableSortedSet.of();", + " }", " }", "", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet2();", - "}", + " @Value.Modifiable", + " abstract class AbstractModifiable {", + " abstract ImmutableSet immutableSet();", "", - "@Value.Modifiable", - "abstract class D {", - " // BUG: Diagnostic contains:", - " abstract ImmutableSortedSet sortedSet();", + " // BUG: Diagnostic contains:", + " abstract ContiguousSet contiguousSet();", "", - " ImmutableSortedSet defaultSortedSet() {", - " return ImmutableSortedSet.of();", + " @Value.ReverseOrder", + " abstract ContiguousSet contiguousSet2();", " }", "", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet defaultSortedSet2();", - "}", - "", - "abstract class E {", - " abstract ImmutableSortedSet sortedSet();", + " abstract class AbstractNonImmutables {", + " abstract SortedSet sortedSet();", + " }", "}") .doTest(); } @Test - void replacementInImmutable() { + void replacement() { refactoringTestHelper .addInputLines( "A.java", "import com.google.common.collect.ImmutableSortedSet;", + "import java.util.SortedSet;", "import org.immutables.value.Value;", "", "@Value.Immutable", "abstract class A {", " abstract ImmutableSortedSet sortedSet();", "", - " @Value.Immutable", + " @Value.Modifiable", " interface B {", - " ImmutableSortedSet sortedSet();", + " SortedSet sortedSet();", " }", "}") .addOutputLines( "A.java", "import com.google.common.collect.ImmutableSortedSet;", + "import java.util.SortedSet;", "import org.immutables.value.Value;", "", "@Value.Immutable", @@ -104,45 +136,44 @@ void replacementInImmutable() { " @Value.NaturalOrder", " abstract ImmutableSortedSet sortedSet();", "", - " @Value.Immutable", + " @Value.Modifiable", " interface B {", " @Value.NaturalOrder", - " ImmutableSortedSet sortedSet();", + " SortedSet sortedSet();", " }", "}") .doTest(TestMode.TEXT_MATCH); } @Test - void replacementInModifiable() { + void replacementWithImportClash() { refactoringTestHelper .addInputLines( - "A.java", + "MySpringService.java", "import com.google.common.collect.ImmutableSortedSet;", - "import org.immutables.value.Value;", + "import org.springframework.beans.factory.annotation.Value;", "", - "@Value.Modifiable", - "abstract class A {", - " abstract ImmutableSortedSet sortedSet();", + "class MySpringService {", + " MySpringService(@Value(\"${someProperty}\") String prop) {}", + " ;", "", - " @Value.Modifiable", - " interface B {", + " @org.immutables.value.Value.Immutable", + " interface A {", " ImmutableSortedSet sortedSet();", " }", "}") .addOutputLines( - "A.java", + "MySpringService.java", "import com.google.common.collect.ImmutableSortedSet;", - "import org.immutables.value.Value;", + "import org.springframework.beans.factory.annotation.Value;", "", - "@Value.Modifiable", - "abstract class A {", - " @Value.NaturalOrder", - " abstract ImmutableSortedSet sortedSet();", + "class MySpringService {", + " MySpringService(@Value(\"${someProperty}\") String prop) {}", + " ;", "", - " @Value.Modifiable", - " interface B {", - " @Value.NaturalOrder", + " @org.immutables.value.Value.Immutable", + " interface A {", + " @org.immutables.value.Value.NaturalOrder", " ImmutableSortedSet sortedSet();", " }", "}") From 42aae4c10716794906818be7fb07ebd92f0838de Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 18 Aug 2022 10:46:58 +0200 Subject: [PATCH 15/15] Tweak message --- .../errorprone/bugpatterns/ImmutablesSortedSetComparator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java index 8fd35dbbdc..5e8a3c2c71 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImmutablesSortedSetComparator.java @@ -43,7 +43,7 @@ @BugPattern( summary = "`SortedSet` properties of a `@Value.Immutable` or `@Value.Modifiable` type must be " - + "annotated `@Value.NaturalOrder` or `@Value.ReverseOrder`", + + "annotated with `@Value.NaturalOrder` or `@Value.ReverseOrder`", linkType = NONE, severity = ERROR, tags = LIKELY_ERROR)