From 919de4c8f5657517b9c53553e3bbe7eb781c03d1 Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Mon, 22 May 2023 11:41:32 +0200 Subject: [PATCH 1/9] Introduce `MoreMatchers#hasTypeArguments()` matcher --- .../bugpatterns/util/MoreMatchers.java | 34 +++++++++++++ .../bugpatterns/util/MoreMatchersTest.java | 50 ++++++++++++++++++- 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java index b62e761db0..11998b31f3 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java @@ -5,6 +5,11 @@ import com.google.errorprone.predicates.TypePredicate; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.ParameterizedTypeTree; +import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; /** @@ -32,6 +37,35 @@ public static Matcher hasMetaAnnotation(String ann }; } + /** + * Returns a {@link Matcher} that determines whether a given {@link ExpressionTree} has type + * arguments. + * + * @param The type of tree to match against. + * @return A {@link Matcher} that matches trees with type annotations. + */ + public static Matcher hasTypeArguments() { + return (tree, state) -> { + switch (tree.getKind()) { + case METHOD_INVOCATION: + return !((MethodInvocationTree) tree).getTypeArguments().isEmpty(); + case NEW_CLASS: + NewClassTree classTree = (NewClassTree) tree; + if (!classTree.getTypeArguments().isEmpty()) { + return true; + } + + if (classTree.getIdentifier().getKind() != Tree.Kind.PARAMETERIZED_TYPE) { + return false; + } + + return !((ParameterizedTypeTree) classTree.getIdentifier()).getTypeArguments().isEmpty(); + default: + return false; + } + }; + } + // XXX: Consider moving to a `MoreTypePredicates` utility class. private static TypePredicate hasAnnotation(String annotationClassName) { return (type, state) -> ASTHelpers.hasAnnotation(type.tsym, annotationClassName, state); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java index 842fb57b6b..460e4ec15f 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java @@ -7,15 +7,20 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; import org.junit.jupiter.api.Test; final class MoreMatchersTest { @Test void hasMetaAnnotation() { - CompilationTestHelper.newInstance(TestMatcher.class, getClass()) + CompilationTestHelper.newInstance(MetaAnnotationTestMatcher.class, getClass()) .addSourceLines( "A.java", "import org.junit.jupiter.api.AfterAll;", @@ -49,7 +54,8 @@ void hasMetaAnnotation() { /** A {@link BugChecker} that delegates to {@link MoreMatchers#hasMetaAnnotation(String)} . */ @BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR) - public static final class TestMatcher extends BugChecker implements AnnotationTreeMatcher { + public static final class MetaAnnotationTestMatcher extends BugChecker + implements AnnotationTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher DELEGATE = MoreMatchers.hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"); @@ -59,4 +65,44 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; } } + + @Test + void hasTypeArgumentsMatcher() { + CompilationTestHelper.newInstance(HasTypeArgumentsTestMatcher.class, getClass()) + .addSourceLines( + "A.java", + "import com.google.common.collect.ImmutableSet;", + "import java.util.ArrayList;", + "import java.util.List;", + "", + "class A {", + " void foo(E first) {", + " // BUG: Diagnostic contains:", + " ImmutableSet.builder().add(first).build();", + " // BUG: Diagnostic contains:", + " new ImmutableSet.Builder().add(first).build();", + "", + " ImmutableSet foo = new ImmutableSet.Builder().add(1).build();", + " List bar = new ArrayList<>();", + " }", + "}") + .doTest(); + } + /** A {@link BugChecker} that delegates to {@link MoreMatchers#hasTypeArguments()} . */ + @BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR) + public static final class HasTypeArgumentsTestMatcher extends BugChecker + implements MethodInvocationTreeMatcher, NewClassTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher DELEGATE = MoreMatchers.hasTypeArguments(); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; + } + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; + } + } } From b4547e2756eb9059135604f5d5baff28e2851d35 Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Mon, 22 May 2023 11:50:23 +0200 Subject: [PATCH 2/9] annotations -> arguments --- .../tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java index 11998b31f3..dba39904d2 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java @@ -42,7 +42,7 @@ public static Matcher hasMetaAnnotation(String ann * arguments. * * @param The type of tree to match against. - * @return A {@link Matcher} that matches trees with type annotations. + * @return A {@link Matcher} that matches trees with type arguments. */ public static Matcher hasTypeArguments() { return (tree, state) -> { From b84664b28b3399ad080e077d337bc475b32d011d Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Mon, 22 May 2023 12:55:45 +0200 Subject: [PATCH 3/9] Move matcher to `refaster-support` --- .../bugpatterns/util/MoreMatchers.java | 34 ---------- .../bugpatterns/util/MoreMatchersTest.java | 50 +-------------- .../refaster/matchers/HasTypeArguments.java | 38 ++++++++++++ .../matchers/HasTypeArgumentsTest.java | 62 +++++++++++++++++++ 4 files changed, 102 insertions(+), 82 deletions(-) create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/HasTypeArguments.java create mode 100644 refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java index dba39904d2..b62e761db0 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java @@ -5,11 +5,6 @@ import com.google.errorprone.predicates.TypePredicate; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.NewClassTree; -import com.sun.source.tree.ParameterizedTypeTree; -import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; /** @@ -37,35 +32,6 @@ public static Matcher hasMetaAnnotation(String ann }; } - /** - * Returns a {@link Matcher} that determines whether a given {@link ExpressionTree} has type - * arguments. - * - * @param The type of tree to match against. - * @return A {@link Matcher} that matches trees with type arguments. - */ - public static Matcher hasTypeArguments() { - return (tree, state) -> { - switch (tree.getKind()) { - case METHOD_INVOCATION: - return !((MethodInvocationTree) tree).getTypeArguments().isEmpty(); - case NEW_CLASS: - NewClassTree classTree = (NewClassTree) tree; - if (!classTree.getTypeArguments().isEmpty()) { - return true; - } - - if (classTree.getIdentifier().getKind() != Tree.Kind.PARAMETERIZED_TYPE) { - return false; - } - - return !((ParameterizedTypeTree) classTree.getIdentifier()).getTypeArguments().isEmpty(); - default: - return false; - } - }; - } - // XXX: Consider moving to a `MoreTypePredicates` utility class. private static TypePredicate hasAnnotation(String annotationClassName) { return (type, state) -> ASTHelpers.hasAnnotation(type.tsym, annotationClassName, state); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java index 460e4ec15f..842fb57b6b 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java @@ -7,20 +7,15 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; -import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; -import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.AnnotationTree; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.NewClassTree; import org.junit.jupiter.api.Test; final class MoreMatchersTest { @Test void hasMetaAnnotation() { - CompilationTestHelper.newInstance(MetaAnnotationTestMatcher.class, getClass()) + CompilationTestHelper.newInstance(TestMatcher.class, getClass()) .addSourceLines( "A.java", "import org.junit.jupiter.api.AfterAll;", @@ -54,8 +49,7 @@ void hasMetaAnnotation() { /** A {@link BugChecker} that delegates to {@link MoreMatchers#hasMetaAnnotation(String)} . */ @BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR) - public static final class MetaAnnotationTestMatcher extends BugChecker - implements AnnotationTreeMatcher { + public static final class TestMatcher extends BugChecker implements AnnotationTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher DELEGATE = MoreMatchers.hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"); @@ -65,44 +59,4 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; } } - - @Test - void hasTypeArgumentsMatcher() { - CompilationTestHelper.newInstance(HasTypeArgumentsTestMatcher.class, getClass()) - .addSourceLines( - "A.java", - "import com.google.common.collect.ImmutableSet;", - "import java.util.ArrayList;", - "import java.util.List;", - "", - "class A {", - " void foo(E first) {", - " // BUG: Diagnostic contains:", - " ImmutableSet.builder().add(first).build();", - " // BUG: Diagnostic contains:", - " new ImmutableSet.Builder().add(first).build();", - "", - " ImmutableSet foo = new ImmutableSet.Builder().add(1).build();", - " List bar = new ArrayList<>();", - " }", - "}") - .doTest(); - } - /** A {@link BugChecker} that delegates to {@link MoreMatchers#hasTypeArguments()} . */ - @BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR) - public static final class HasTypeArgumentsTestMatcher extends BugChecker - implements MethodInvocationTreeMatcher, NewClassTreeMatcher { - private static final long serialVersionUID = 1L; - private static final Matcher DELEGATE = MoreMatchers.hasTypeArguments(); - - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; - } - - @Override - public Description matchNewClass(NewClassTree tree, VisitorState state) { - return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; - } - } } diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/HasTypeArguments.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/HasTypeArguments.java new file mode 100644 index 0000000000..71838d9829 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/HasTypeArguments.java @@ -0,0 +1,38 @@ +package tech.picnic.errorprone.refaster.matchers; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.ParameterizedTypeTree; +import com.sun.source.tree.Tree; + +/** A matcher of expressions with type arguments. */ +public final class HasTypeArguments implements Matcher { + private static final long serialVersionUID = 1L; + + /** Instantiates a new {@link HasTypeArguments} instance. */ + public HasTypeArguments() {} + + @Override + public boolean matches(ExpressionTree tree, VisitorState state) { + switch (tree.getKind()) { + case METHOD_INVOCATION: + return !((MethodInvocationTree) tree).getTypeArguments().isEmpty(); + case NEW_CLASS: + NewClassTree classTree = (NewClassTree) tree; + if (!classTree.getTypeArguments().isEmpty()) { + return true; + } + + if (classTree.getIdentifier().getKind() != Tree.Kind.PARAMETERIZED_TYPE) { + return false; + } + + return !((ParameterizedTypeTree) classTree.getIdentifier()).getTypeArguments().isEmpty(); + default: + return false; + } + } +} diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java new file mode 100644 index 0000000000..608aed2767 --- /dev/null +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java @@ -0,0 +1,62 @@ +package tech.picnic.errorprone.refaster.matchers; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.bugpatterns.BugChecker; +import org.junit.jupiter.api.Test; + +final class HasTypeArgumentsTest { + @Test + void matches() { + CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "import com.google.common.collect.ImmutableSet;", + "import java.util.ArrayList;", + "import java.util.List;", + "", + "class A {", + " Object negative1() {", + " return alwaysNull();", + " }", + "", + " Object negative2() {", + " return new Object();", + " }", + "", + " List negative3() {", + " return new ArrayList<>();", + " }", + "", + " ImmutableSet positive1() {", + " // BUG: Diagnostic contains:", + " return ImmutableSet.builder().build();", + " }", + "", + " ImmutableSet positive2() {", + " // BUG: Diagnostic contains:", + " return new ImmutableSet.Builder().build();", + " }", + "", + " private static T alwaysNull() {", + " return null;", + " }", + "}") + .doTest(); + } + + /** A {@link BugChecker} that simply delegates to {@link HasTypeArguments}. */ + @BugPattern(summary = "Flags expressions matched by `HasTypeArguments`", severity = ERROR) + public static final class MatcherTestChecker extends AbstractMatcherTestChecker { + private static final long serialVersionUID = 1L; + + // XXX: This is a false positive reported by Checkstyle. See + // https://github.com/checkstyle/checkstyle/issues2/10161#issuecomment-1242732120. + @SuppressWarnings("RedundantModifier") + public MatcherTestChecker() { + super(new HasTypeArguments()); + } + } +} From 1655979737e38fa55c2eb067e885ea646bfb4004 Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Mon, 22 May 2023 13:16:43 +0200 Subject: [PATCH 4/9] Add test for type arguments in constructor --- .../matchers/HasTypeArgumentsTest.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java index 608aed2767..1d7426d2d1 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java @@ -30,6 +30,10 @@ void matches() { " return new ArrayList<>();", " }", "", + " Foo negative4() {", + " return new Foo(\"foo\");", + " }", + "", " ImmutableSet positive1() {", " // BUG: Diagnostic contains:", " return ImmutableSet.builder().build();", @@ -40,9 +44,26 @@ void matches() { " return new ImmutableSet.Builder().build();", " }", "", + " Foo positive3() {", + " // BUG: Diagnostic contains:", + " return new >Foo(List.of());", + " }", + "", " private static T alwaysNull() {", " return null;", " }", + "", + " public static class Foo {", + " private final Object value;", + "", + " public Foo(Object value) {", + " this.value = value;", + " }", + "", + " public > Foo(E value) {", + " this.value = value;", + " }", + " }", "}") .doTest(); } From eb23526e15ba065dc7b1132163658284fd0aa5bb Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 5 Jan 2022 10:03:47 +0100 Subject: [PATCH 5/9] Add type arguments in `@AfterTemplate`s for builders --- .../ImmutableListMultimapRules.java | 4 +- .../refasterrules/ImmutableListRules.java | 4 +- .../refasterrules/ImmutableMapRules.java | 4 +- .../refasterrules/ImmutableMultisetRules.java | 4 +- .../ImmutableSetMultimapRules.java | 4 +- .../ImmutableSortedMapRules.java | 8 +- .../ImmutableSetTemplates.java | 155 ++++++++++++++++++ 7 files changed, 162 insertions(+), 21 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java index b68a7a1601..28b1241b34 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java @@ -35,8 +35,6 @@ private ImmutableListMultimapRules() {} * Prefer {@link ImmutableListMultimap#builder()} over the associated constructor on constructions * that produce a less-specific type. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableListMultimapBuilder { @BeforeTemplate ImmutableMultimap.Builder before() { @@ -48,7 +46,7 @@ ImmutableMultimap.Builder before() { @AfterTemplate ImmutableListMultimap.Builder after() { - return ImmutableListMultimap.builder(); + return ImmutableListMultimap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java index 1848c268b9..d48f697c6b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java @@ -28,8 +28,6 @@ final class ImmutableListRules { private ImmutableListRules() {} /** Prefer {@link ImmutableList#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableListBuilder { @BeforeTemplate ImmutableList.Builder before() { @@ -38,7 +36,7 @@ ImmutableList.Builder before() { @AfterTemplate ImmutableList.Builder after() { - return ImmutableList.builder(); + return ImmutableList.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index 49021f1a5a..b6ec15ae8d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -30,8 +30,6 @@ final class ImmutableMapRules { private ImmutableMapRules() {} /** Prefer {@link ImmutableMap#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableMapBuilder { @BeforeTemplate ImmutableMap.Builder before() { @@ -40,7 +38,7 @@ ImmutableMap.Builder before() { @AfterTemplate ImmutableMap.Builder after() { - return ImmutableMap.builder(); + return ImmutableMap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java index fee76fd89d..c70aa0f8d2 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java @@ -21,8 +21,6 @@ final class ImmutableMultisetRules { private ImmutableMultisetRules() {} /** Prefer {@link ImmutableMultiset#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableMultisetBuilder { @BeforeTemplate ImmutableMultiset.Builder before() { @@ -31,7 +29,7 @@ ImmutableMultiset.Builder before() { @AfterTemplate ImmutableMultiset.Builder after() { - return ImmutableMultiset.builder(); + return ImmutableMultiset.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java index 1c704943fc..6e7eaa7a14 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java @@ -29,8 +29,6 @@ final class ImmutableSetMultimapRules { private ImmutableSetMultimapRules() {} /** Prefer {@link ImmutableSetMultimap#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableSetMultimapBuilder { @BeforeTemplate ImmutableSetMultimap.Builder before() { @@ -39,7 +37,7 @@ ImmutableSetMultimap.Builder before() { @AfterTemplate ImmutableSetMultimap.Builder after() { - return ImmutableSetMultimap.builder(); + return ImmutableSetMultimap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java index 36975c7025..48a4e559fb 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java @@ -37,8 +37,6 @@ ImmutableSortedMap.Builder after(Comparator cmp) { * Prefer {@link ImmutableSortedMap#naturalOrder()} over the alternative that requires explicitly * providing the {@link Comparator}. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableSortedMapNaturalOrderBuilder, V> { @BeforeTemplate ImmutableSortedMap.Builder before() { @@ -47,7 +45,7 @@ ImmutableSortedMap.Builder before() { @AfterTemplate ImmutableSortedMap.Builder after() { - return ImmutableSortedMap.naturalOrder(); + return ImmutableSortedMap.naturalOrder(); } } @@ -55,8 +53,6 @@ ImmutableSortedMap.Builder after() { * Prefer {@link ImmutableSortedMap#reverseOrder()} over the alternative that requires explicitly * providing the {@link Comparator}. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableSortedMapReverseOrderBuilder, V> { @BeforeTemplate ImmutableSortedMap.Builder before() { @@ -65,7 +61,7 @@ ImmutableSortedMap.Builder before() { @AfterTemplate ImmutableSortedMap.Builder after() { - return ImmutableSortedMap.reverseOrder(); + return ImmutableSortedMap.reverseOrder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java new file mode 100644 index 0000000000..30bfae2916 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java @@ -0,0 +1,155 @@ +package tech.picnic.errorprone.refastertemplates; + +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static java.util.stream.Collectors.collectingAndThen; +import static java.util.stream.Collectors.toList; +import static java.util.stream.Collectors.toSet; + +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets.SetView; +import com.google.common.collect.Streams; +import com.google.errorprone.refaster.ImportPolicy; +import com.google.errorprone.refaster.Refaster; +import com.google.errorprone.refaster.annotation.AfterTemplate; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.UseImportPolicy; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Iterator; +import java.util.Set; +import java.util.stream.Stream; + +/** Refaster templates related to expressions dealing with {@link ImmutableSet}s. */ +final class ImmutableSetTemplates { + private ImmutableSetTemplates() {} + + /** Prefer {@link ImmutableSet#builder()} over the associated constructor. */ + // XXX: Picnic's Error Prone fork supports method invocation type argument inlining in the + // `@AfterTemplate`. Without using the fork, the expression in the `@AfterTemplate` can result in + // non-compilable code. See: https://github.com/google/error-prone/pull/2706. + static final class ImmutableSetBuilder { + @BeforeTemplate + ImmutableSet.Builder before() { + return new ImmutableSet.Builder<>(); + } + + @AfterTemplate + ImmutableSet.Builder after() { + return ImmutableSet.builder(); + } + } + + /** Prefer {@link ImmutableSet#of()} over more contrived alternatives. */ + static final class EmptyImmutableSet { + @BeforeTemplate + ImmutableSet before() { + return Refaster.anyOf( + ImmutableSet.builder().build(), Stream.empty().collect(toImmutableSet())); + } + + @AfterTemplate + ImmutableSet after() { + return ImmutableSet.of(); + } + } + + /** + * Prefer {@link ImmutableSet#of(Object)} over alternatives that don't communicate the + * immutability of the resulting set at the type level. + */ + // XXX: Note that this rewrite rule is incorrect for nullable elements. + static final class SingletonImmutableSet { + @BeforeTemplate + Set before(T element) { + return Collections.singleton(element); + } + + @AfterTemplate + ImmutableSet after(T element) { + return ImmutableSet.of(element); + } + } + + /** Prefer {@link ImmutableSet#copyOf(Iterable)} and variants over more contrived alternatives. */ + static final class IterableToImmutableSet { + @BeforeTemplate + ImmutableSet before(T[] iterable) { + return Refaster.anyOf( + ImmutableSet.builder().add(iterable).build(), + Arrays.stream(iterable).collect(toImmutableSet())); + } + + @BeforeTemplate + ImmutableSet before(Iterator iterable) { + return Refaster.anyOf( + ImmutableSet.builder().addAll(iterable).build(), + Streams.stream(iterable).collect(toImmutableSet())); + } + + @BeforeTemplate + ImmutableSet before(Iterable iterable) { + return Refaster.anyOf( + ImmutableSet.builder().addAll(iterable).build(), + Streams.stream(iterable).collect(toImmutableSet())); + } + + @BeforeTemplate + ImmutableSet before(Collection iterable) { + return iterable.stream().collect(toImmutableSet()); + } + + @AfterTemplate + ImmutableSet after(Iterable iterable) { + return ImmutableSet.copyOf(iterable); + } + } + + /** Prefer {@link ImmutableSet#toImmutableSet()} over less idiomatic alternatives. */ + // XXX: Once the code base has been sufficiently cleaned up, we might want to also rewrite + // `Collectors.toSet(`), with the caveat that it allows mutation (though this cannot be relied + // upon) as well as nulls. Another option is to explicitly rewrite those variants to + // `Collectors.toSet(HashSet::new)`. + static final class StreamToImmutableSet { + @BeforeTemplate + ImmutableSet before(Stream stream) { + return Refaster.anyOf( + ImmutableSet.copyOf(stream.iterator()), + stream.distinct().collect(toImmutableSet()), + stream.collect(collectingAndThen(toList(), ImmutableSet::copyOf)), + stream.collect(collectingAndThen(toSet(), ImmutableSet::copyOf))); + } + + @AfterTemplate + @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS) + ImmutableSet after(Stream stream) { + return stream.collect(toImmutableSet()); + } + } + + /** Don't unnecessarily copy an {@link ImmutableSet}. */ + static final class ImmutableSetCopyOfImmutableSet { + @BeforeTemplate + ImmutableSet before(ImmutableSet set) { + return ImmutableSet.copyOf(set); + } + + @AfterTemplate + ImmutableSet after(ImmutableSet set) { + return set; + } + } + + /** Prefer {@link SetView#immutableCopy()} over the more verbose alternative. */ + static final class ImmutableSetCopyOfSetView { + @BeforeTemplate + ImmutableSet before(SetView set) { + return ImmutableSet.copyOf(set); + } + + @AfterTemplate + ImmutableSet after(SetView set) { + return set.immutableCopy(); + } + } +} From a83574e42748952f7bbb805ce73fb00aa8b8a412 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 3 Jan 2023 09:12:30 +0100 Subject: [PATCH 6/9] Post-rebase fix --- .../refasterrules/ImmutableSetRules.java | 4 +- .../ImmutableSetTemplates.java | 155 ------------------ 2 files changed, 1 insertion(+), 158 deletions(-) delete mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java index 628bb1cf6b..ea949fdb80 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java @@ -29,8 +29,6 @@ final class ImmutableSetRules { private ImmutableSetRules() {} /** Prefer {@link ImmutableSet#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableSetBuilder { @BeforeTemplate ImmutableSet.Builder before() { @@ -39,7 +37,7 @@ ImmutableSet.Builder before() { @AfterTemplate ImmutableSet.Builder after() { - return ImmutableSet.builder(); + return ImmutableSet.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java deleted file mode 100644 index 30bfae2916..0000000000 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java +++ /dev/null @@ -1,155 +0,0 @@ -package tech.picnic.errorprone.refastertemplates; - -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static java.util.stream.Collectors.collectingAndThen; -import static java.util.stream.Collectors.toList; -import static java.util.stream.Collectors.toSet; - -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets.SetView; -import com.google.common.collect.Streams; -import com.google.errorprone.refaster.ImportPolicy; -import com.google.errorprone.refaster.Refaster; -import com.google.errorprone.refaster.annotation.AfterTemplate; -import com.google.errorprone.refaster.annotation.BeforeTemplate; -import com.google.errorprone.refaster.annotation.UseImportPolicy; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.Iterator; -import java.util.Set; -import java.util.stream.Stream; - -/** Refaster templates related to expressions dealing with {@link ImmutableSet}s. */ -final class ImmutableSetTemplates { - private ImmutableSetTemplates() {} - - /** Prefer {@link ImmutableSet#builder()} over the associated constructor. */ - // XXX: Picnic's Error Prone fork supports method invocation type argument inlining in the - // `@AfterTemplate`. Without using the fork, the expression in the `@AfterTemplate` can result in - // non-compilable code. See: https://github.com/google/error-prone/pull/2706. - static final class ImmutableSetBuilder { - @BeforeTemplate - ImmutableSet.Builder before() { - return new ImmutableSet.Builder<>(); - } - - @AfterTemplate - ImmutableSet.Builder after() { - return ImmutableSet.builder(); - } - } - - /** Prefer {@link ImmutableSet#of()} over more contrived alternatives. */ - static final class EmptyImmutableSet { - @BeforeTemplate - ImmutableSet before() { - return Refaster.anyOf( - ImmutableSet.builder().build(), Stream.empty().collect(toImmutableSet())); - } - - @AfterTemplate - ImmutableSet after() { - return ImmutableSet.of(); - } - } - - /** - * Prefer {@link ImmutableSet#of(Object)} over alternatives that don't communicate the - * immutability of the resulting set at the type level. - */ - // XXX: Note that this rewrite rule is incorrect for nullable elements. - static final class SingletonImmutableSet { - @BeforeTemplate - Set before(T element) { - return Collections.singleton(element); - } - - @AfterTemplate - ImmutableSet after(T element) { - return ImmutableSet.of(element); - } - } - - /** Prefer {@link ImmutableSet#copyOf(Iterable)} and variants over more contrived alternatives. */ - static final class IterableToImmutableSet { - @BeforeTemplate - ImmutableSet before(T[] iterable) { - return Refaster.anyOf( - ImmutableSet.builder().add(iterable).build(), - Arrays.stream(iterable).collect(toImmutableSet())); - } - - @BeforeTemplate - ImmutableSet before(Iterator iterable) { - return Refaster.anyOf( - ImmutableSet.builder().addAll(iterable).build(), - Streams.stream(iterable).collect(toImmutableSet())); - } - - @BeforeTemplate - ImmutableSet before(Iterable iterable) { - return Refaster.anyOf( - ImmutableSet.builder().addAll(iterable).build(), - Streams.stream(iterable).collect(toImmutableSet())); - } - - @BeforeTemplate - ImmutableSet before(Collection iterable) { - return iterable.stream().collect(toImmutableSet()); - } - - @AfterTemplate - ImmutableSet after(Iterable iterable) { - return ImmutableSet.copyOf(iterable); - } - } - - /** Prefer {@link ImmutableSet#toImmutableSet()} over less idiomatic alternatives. */ - // XXX: Once the code base has been sufficiently cleaned up, we might want to also rewrite - // `Collectors.toSet(`), with the caveat that it allows mutation (though this cannot be relied - // upon) as well as nulls. Another option is to explicitly rewrite those variants to - // `Collectors.toSet(HashSet::new)`. - static final class StreamToImmutableSet { - @BeforeTemplate - ImmutableSet before(Stream stream) { - return Refaster.anyOf( - ImmutableSet.copyOf(stream.iterator()), - stream.distinct().collect(toImmutableSet()), - stream.collect(collectingAndThen(toList(), ImmutableSet::copyOf)), - stream.collect(collectingAndThen(toSet(), ImmutableSet::copyOf))); - } - - @AfterTemplate - @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS) - ImmutableSet after(Stream stream) { - return stream.collect(toImmutableSet()); - } - } - - /** Don't unnecessarily copy an {@link ImmutableSet}. */ - static final class ImmutableSetCopyOfImmutableSet { - @BeforeTemplate - ImmutableSet before(ImmutableSet set) { - return ImmutableSet.copyOf(set); - } - - @AfterTemplate - ImmutableSet after(ImmutableSet set) { - return set; - } - } - - /** Prefer {@link SetView#immutableCopy()} over the more verbose alternative. */ - static final class ImmutableSetCopyOfSetView { - @BeforeTemplate - ImmutableSet before(SetView set) { - return ImmutableSet.copyOf(set); - } - - @AfterTemplate - ImmutableSet after(SetView set) { - return set.immutableCopy(); - } - } -} From 339152a37d1e1bf0820f8f356470373c5c580954 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 24 Jan 2023 15:23:53 +0100 Subject: [PATCH 7/9] Start changing some tests --- .../errorprone/refasterrules/ImmutableListRulesTestInput.java | 4 ++-- .../refasterrules/ImmutableListRulesTestOutput.java | 4 ++-- .../errorprone/refasterrules/ImmutableSetRulesTestInput.java | 4 ++-- .../errorprone/refasterrules/ImmutableSetRulesTestOutput.java | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestInput.java index 84882105be..61fa54e65f 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestInput.java @@ -20,8 +20,8 @@ public ImmutableSet elidedTypesAndStaticImports() { Arrays.class, Collections.class, Comparator.class, Streams.class, naturalOrder()); } - ImmutableList.Builder testImmutableListBuilder() { - return new ImmutableList.Builder<>(); + ImmutableSet> testImmutableListBuilder() { + return ImmutableSet.of(new ImmutableList.Builder<>(), new ImmutableList.Builder()); } ImmutableSet> testIterableToImmutableList() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestOutput.java index b152693d25..ac04fd42cf 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestOutput.java @@ -21,8 +21,8 @@ public ImmutableSet elidedTypesAndStaticImports() { Arrays.class, Collections.class, Comparator.class, Streams.class, naturalOrder()); } - ImmutableList.Builder testImmutableListBuilder() { - return ImmutableList.builder(); + ImmutableSet> testImmutableListBuilder() { + return ImmutableSet.of(ImmutableList.builder(), ImmutableListbuilder()); } ImmutableSet> testIterableToImmutableList() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java index 53e7c41556..18f26ea85f 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java @@ -21,8 +21,8 @@ public ImmutableSet elidedTypesAndStaticImports() { return ImmutableSet.of(Arrays.class, Collections.class, Streams.class, not(null)); } - ImmutableSet.Builder testImmutableSetBuilder() { - return new ImmutableSet.Builder<>(); + ImmutableSet> testImmutableSetBuilder() { + return ImmutableSet.of(new ImmutableSet.Builder<>(), new ImmutableSet.Builder()); } ImmutableSet> testIterableToImmutableSet() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestOutput.java index 74a6c9d67e..2c4330db05 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestOutput.java @@ -21,8 +21,8 @@ public ImmutableSet elidedTypesAndStaticImports() { return ImmutableSet.of(Arrays.class, Collections.class, Streams.class, not(null)); } - ImmutableSet.Builder testImmutableSetBuilder() { - return ImmutableSet.builder(); + ImmutableSet> testImmutableSetBuilder() { + return ImmutableSet.of(ImmutableSet.builder(), ImmutableSet.builder()); } ImmutableSet> testIterableToImmutableSet() { From d5755bca757fd92cd38a8889408d5f797b6f3957 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 26 Dec 2024 17:34:44 +0100 Subject: [PATCH 8/9] Drop `HasTypeArguments` --- .../refaster/matchers/HasTypeArguments.java | 38 --------- .../matchers/HasTypeArgumentsTest.java | 83 ------------------- 2 files changed, 121 deletions(-) delete mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/HasTypeArguments.java delete mode 100644 refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/HasTypeArguments.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/HasTypeArguments.java deleted file mode 100644 index 71838d9829..0000000000 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/HasTypeArguments.java +++ /dev/null @@ -1,38 +0,0 @@ -package tech.picnic.errorprone.refaster.matchers; - -import com.google.errorprone.VisitorState; -import com.google.errorprone.matchers.Matcher; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.NewClassTree; -import com.sun.source.tree.ParameterizedTypeTree; -import com.sun.source.tree.Tree; - -/** A matcher of expressions with type arguments. */ -public final class HasTypeArguments implements Matcher { - private static final long serialVersionUID = 1L; - - /** Instantiates a new {@link HasTypeArguments} instance. */ - public HasTypeArguments() {} - - @Override - public boolean matches(ExpressionTree tree, VisitorState state) { - switch (tree.getKind()) { - case METHOD_INVOCATION: - return !((MethodInvocationTree) tree).getTypeArguments().isEmpty(); - case NEW_CLASS: - NewClassTree classTree = (NewClassTree) tree; - if (!classTree.getTypeArguments().isEmpty()) { - return true; - } - - if (classTree.getIdentifier().getKind() != Tree.Kind.PARAMETERIZED_TYPE) { - return false; - } - - return !((ParameterizedTypeTree) classTree.getIdentifier()).getTypeArguments().isEmpty(); - default: - return false; - } - } -} diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java deleted file mode 100644 index 1d7426d2d1..0000000000 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/HasTypeArgumentsTest.java +++ /dev/null @@ -1,83 +0,0 @@ -package tech.picnic.errorprone.refaster.matchers; - -import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; - -import com.google.errorprone.BugPattern; -import com.google.errorprone.CompilationTestHelper; -import com.google.errorprone.bugpatterns.BugChecker; -import org.junit.jupiter.api.Test; - -final class HasTypeArgumentsTest { - @Test - void matches() { - CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass()) - .addSourceLines( - "A.java", - "import com.google.common.collect.ImmutableSet;", - "import java.util.ArrayList;", - "import java.util.List;", - "", - "class A {", - " Object negative1() {", - " return alwaysNull();", - " }", - "", - " Object negative2() {", - " return new Object();", - " }", - "", - " List negative3() {", - " return new ArrayList<>();", - " }", - "", - " Foo negative4() {", - " return new Foo(\"foo\");", - " }", - "", - " ImmutableSet positive1() {", - " // BUG: Diagnostic contains:", - " return ImmutableSet.builder().build();", - " }", - "", - " ImmutableSet positive2() {", - " // BUG: Diagnostic contains:", - " return new ImmutableSet.Builder().build();", - " }", - "", - " Foo positive3() {", - " // BUG: Diagnostic contains:", - " return new >Foo(List.of());", - " }", - "", - " private static T alwaysNull() {", - " return null;", - " }", - "", - " public static class Foo {", - " private final Object value;", - "", - " public Foo(Object value) {", - " this.value = value;", - " }", - "", - " public > Foo(E value) {", - " this.value = value;", - " }", - " }", - "}") - .doTest(); - } - - /** A {@link BugChecker} that simply delegates to {@link HasTypeArguments}. */ - @BugPattern(summary = "Flags expressions matched by `HasTypeArguments`", severity = ERROR) - public static final class MatcherTestChecker extends AbstractMatcherTestChecker { - private static final long serialVersionUID = 1L; - - // XXX: This is a false positive reported by Checkstyle. See - // https://github.com/checkstyle/checkstyle/issues2/10161#issuecomment-1242732120. - @SuppressWarnings("RedundantModifier") - public MatcherTestChecker() { - super(new HasTypeArguments()); - } - } -} From 933e39573e1084285513837108c3193f26766dda Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 26 Dec 2024 17:36:51 +0100 Subject: [PATCH 9/9] Suggestions --- .../refasterrules/ImmutableListMultimapRules.java | 3 ++- .../picnic/errorprone/refasterrules/ImmutableListRules.java | 3 ++- .../picnic/errorprone/refasterrules/ImmutableMapRules.java | 3 ++- .../errorprone/refasterrules/ImmutableMultisetRules.java | 3 ++- .../errorprone/refasterrules/ImmutableSetMultimapRules.java | 3 ++- .../picnic/errorprone/refasterrules/ImmutableSetRules.java | 3 ++- .../errorprone/refasterrules/ImmutableSortedMapRules.java | 6 ++++-- .../refasterrules/ImmutableListRulesTestInput.java | 4 ++-- .../refasterrules/ImmutableListRulesTestOutput.java | 4 ++-- .../refasterrules/ImmutableSetRulesTestInput.java | 4 ++-- .../refasterrules/ImmutableSetRulesTestOutput.java | 4 ++-- 11 files changed, 24 insertions(+), 16 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java index 096697b127..fc5e8b062b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java @@ -36,6 +36,7 @@ private ImmutableListMultimapRules() {} * Prefer {@link ImmutableListMultimap#builder()} over the associated constructor on constructions * that produce a less-specific type. */ + // XXX: This rule may drop generic type information, leading to non-compilable code. static final class ImmutableListMultimapBuilder { @BeforeTemplate ImmutableMultimap.Builder before() { @@ -47,7 +48,7 @@ ImmutableMultimap.Builder before() { @AfterTemplate ImmutableListMultimap.Builder after() { - return ImmutableListMultimap.builder(); + return ImmutableListMultimap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java index 5b7322d907..80732d38b8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java @@ -28,6 +28,7 @@ final class ImmutableListRules { private ImmutableListRules() {} /** Prefer {@link ImmutableList#builder()} over the associated constructor. */ + // XXX: This rule may drop generic type information, leading to non-compilable code. static final class ImmutableListBuilder { @BeforeTemplate ImmutableList.Builder before() { @@ -36,7 +37,7 @@ ImmutableList.Builder before() { @AfterTemplate ImmutableList.Builder after() { - return ImmutableList.builder(); + return ImmutableList.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index 3e25df6d4d..fa976d33dc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -31,6 +31,7 @@ final class ImmutableMapRules { private ImmutableMapRules() {} /** Prefer {@link ImmutableMap#builder()} over the associated constructor. */ + // XXX: This rule may drop generic type information, leading to non-compilable code. static final class ImmutableMapBuilder { @BeforeTemplate ImmutableMap.Builder before() { @@ -39,7 +40,7 @@ ImmutableMap.Builder before() { @AfterTemplate ImmutableMap.Builder after() { - return ImmutableMap.builder(); + return ImmutableMap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java index c70aa0f8d2..9880ad7cd9 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java @@ -21,6 +21,7 @@ final class ImmutableMultisetRules { private ImmutableMultisetRules() {} /** Prefer {@link ImmutableMultiset#builder()} over the associated constructor. */ + // XXX: This rule may drop generic type information, leading to non-compilable code. static final class ImmutableMultisetBuilder { @BeforeTemplate ImmutableMultiset.Builder before() { @@ -29,7 +30,7 @@ ImmutableMultiset.Builder before() { @AfterTemplate ImmutableMultiset.Builder after() { - return ImmutableMultiset.builder(); + return ImmutableMultiset.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java index 6e7eaa7a14..a07d4a0957 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java @@ -29,6 +29,7 @@ final class ImmutableSetMultimapRules { private ImmutableSetMultimapRules() {} /** Prefer {@link ImmutableSetMultimap#builder()} over the associated constructor. */ + // XXX: This rule may drop generic type information, leading to non-compilable code. static final class ImmutableSetMultimapBuilder { @BeforeTemplate ImmutableSetMultimap.Builder before() { @@ -37,7 +38,7 @@ ImmutableSetMultimap.Builder before() { @AfterTemplate ImmutableSetMultimap.Builder after() { - return ImmutableSetMultimap.builder(); + return ImmutableSetMultimap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java index ea949fdb80..351f93d511 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java @@ -29,6 +29,7 @@ final class ImmutableSetRules { private ImmutableSetRules() {} /** Prefer {@link ImmutableSet#builder()} over the associated constructor. */ + // XXX: This rule may drop generic type information, leading to non-compilable code. static final class ImmutableSetBuilder { @BeforeTemplate ImmutableSet.Builder before() { @@ -37,7 +38,7 @@ ImmutableSet.Builder before() { @AfterTemplate ImmutableSet.Builder after() { - return ImmutableSet.builder(); + return ImmutableSet.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java index 48a4e559fb..e2cf20ef0f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java @@ -37,6 +37,7 @@ ImmutableSortedMap.Builder after(Comparator cmp) { * Prefer {@link ImmutableSortedMap#naturalOrder()} over the alternative that requires explicitly * providing the {@link Comparator}. */ + // XXX: This rule may drop generic type information, leading to non-compilable code. static final class ImmutableSortedMapNaturalOrderBuilder, V> { @BeforeTemplate ImmutableSortedMap.Builder before() { @@ -45,7 +46,7 @@ ImmutableSortedMap.Builder before() { @AfterTemplate ImmutableSortedMap.Builder after() { - return ImmutableSortedMap.naturalOrder(); + return ImmutableSortedMap.naturalOrder(); } } @@ -53,6 +54,7 @@ ImmutableSortedMap.Builder after() { * Prefer {@link ImmutableSortedMap#reverseOrder()} over the alternative that requires explicitly * providing the {@link Comparator}. */ + // XXX: This rule may drop generic type information, leading to non-compilable code. static final class ImmutableSortedMapReverseOrderBuilder, V> { @BeforeTemplate ImmutableSortedMap.Builder before() { @@ -61,7 +63,7 @@ ImmutableSortedMap.Builder before() { @AfterTemplate ImmutableSortedMap.Builder after() { - return ImmutableSortedMap.reverseOrder(); + return ImmutableSortedMap.reverseOrder(); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestInput.java index 61fa54e65f..84882105be 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestInput.java @@ -20,8 +20,8 @@ public ImmutableSet elidedTypesAndStaticImports() { Arrays.class, Collections.class, Comparator.class, Streams.class, naturalOrder()); } - ImmutableSet> testImmutableListBuilder() { - return ImmutableSet.of(new ImmutableList.Builder<>(), new ImmutableList.Builder()); + ImmutableList.Builder testImmutableListBuilder() { + return new ImmutableList.Builder<>(); } ImmutableSet> testIterableToImmutableList() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestOutput.java index ac04fd42cf..b152693d25 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListRulesTestOutput.java @@ -21,8 +21,8 @@ public ImmutableSet elidedTypesAndStaticImports() { Arrays.class, Collections.class, Comparator.class, Streams.class, naturalOrder()); } - ImmutableSet> testImmutableListBuilder() { - return ImmutableSet.of(ImmutableList.builder(), ImmutableListbuilder()); + ImmutableList.Builder testImmutableListBuilder() { + return ImmutableList.builder(); } ImmutableSet> testIterableToImmutableList() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java index 18f26ea85f..53e7c41556 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java @@ -21,8 +21,8 @@ public ImmutableSet elidedTypesAndStaticImports() { return ImmutableSet.of(Arrays.class, Collections.class, Streams.class, not(null)); } - ImmutableSet> testImmutableSetBuilder() { - return ImmutableSet.of(new ImmutableSet.Builder<>(), new ImmutableSet.Builder()); + ImmutableSet.Builder testImmutableSetBuilder() { + return new ImmutableSet.Builder<>(); } ImmutableSet> testIterableToImmutableSet() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestOutput.java index 2c4330db05..74a6c9d67e 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestOutput.java @@ -21,8 +21,8 @@ public ImmutableSet elidedTypesAndStaticImports() { return ImmutableSet.of(Arrays.class, Collections.class, Streams.class, not(null)); } - ImmutableSet> testImmutableSetBuilder() { - return ImmutableSet.of(ImmutableSet.builder(), ImmutableSet.builder()); + ImmutableSet.Builder testImmutableSetBuilder() { + return ImmutableSet.builder(); } ImmutableSet> testIterableToImmutableSet() {