From a2c92b54dd8baeca0f8eeba0be59e5d5edad9192 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 27 May 2024 08:49:00 +0200 Subject: [PATCH] Suggestions --- .../bugpatterns/TypeMemberOrder.java | 77 ++++++++++--------- .../bugpatterns/TypeMemberOrderClassTest.java | 48 +++++++++--- .../bugpatterns/TypeMemberOrderEnumTest.java | 2 +- .../TypeMemberOrderInterfaceTest.java | 12 +-- ...=> TypeMemberOrderUnhandledCasesTest.java} | 2 +- 5 files changed, 88 insertions(+), 53 deletions(-) rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{TypeMemberOrderUnhandledKindsTest.java => TypeMemberOrderUnhandledCasesTest.java} (94%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrder.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrder.java index 3170b17b..65b45d25 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrder.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrder.java @@ -82,28 +82,6 @@ public final class TypeMemberOrder extends BugChecker implements ClassTreeMatche return Description.NO_MATCH; } - /* All members that can be moved or may lay between movable ones. */ - ImmutableList members = - tree.getMembers().stream() - .filter( - member -> !MoreASTHelpers.isGeneratedConstructor(member) && !isEnumerator(member)) - .map(m -> new AutoValue_TypeMemberOrder_TypeMember(m, getPreferredOrdinal(m, state))) - .collect(toImmutableList()); - - /* - List of the sortable members' preferred ordinals, ordered by the member's position in the - original source. - */ - ImmutableList preferredOrdinals = - members.stream() - .filter(m -> m.preferredOrdinal().isPresent()) - .map(m -> m.preferredOrdinal().orElseThrow(/* Unreachable due to preceding check. */ )) - .collect(toImmutableList()); - - if (Comparators.isInOrder(preferredOrdinals, naturalOrder())) { - return Description.NO_MATCH; - } - int bodyStartPos = getBodyStartPos(tree, state); if (bodyStartPos == Position.NOPOS) { /* @@ -114,17 +92,22 @@ public final class TypeMemberOrder extends BugChecker implements ClassTreeMatche return Description.NO_MATCH; } + ImmutableList members = getAllTypeMembers(tree, state); + ImmutableList preferredOrdinals = extractPreferredOrdinals(members); + + if (Comparators.isInOrder(preferredOrdinals, naturalOrder())) { + return Description.NO_MATCH; + } + return describeMatch(tree, sortTypeMembers(bodyStartPos, members, state)); } - /** - * Returns true if {@link Tree} is an enumerator of an enumerated type, false otherwise. - * - * @see com.sun.tools.javac.tree.Pretty#isEnumerator(JCTree) - * @see com.sun.tools.javac.code.Flags#ENUM - */ - private static boolean isEnumerator(Tree tree) { - return tree instanceof JCVariableDecl variableDecl && (variableDecl.mods.flags & ENUM) != 0; + /** Returns all members that can be moved or may lay between movable ones. */ + private ImmutableList getAllTypeMembers(ClassTree tree, VisitorState state) { + return tree.getMembers().stream() + .filter(member -> !MoreASTHelpers.isGeneratedConstructor(member) && !isEnumerator(member)) + .map(m -> new AutoValue_TypeMemberOrder_TypeMember(m, getPreferredOrdinal(m, state))) + .collect(toImmutableList()); } /** @@ -140,7 +123,6 @@ public final class TypeMemberOrder extends BugChecker implements ClassTreeMatche case BLOCK -> Optional.of(isStatic((BlockTree) tree) ? 3 : 4); case METHOD -> Optional.of(isConstructor((MethodTree) tree) ? 5 : 6); case CLASS, INTERFACE, ENUM -> Optional.of(7); - // TODO: Should we log unhandled kinds? default -> Optional.empty(); }; } @@ -158,14 +140,14 @@ public final class TypeMemberOrder extends BugChecker implements ClassTreeMatche return Position.NOPOS; } - /* We return the source code position of the first token that follows the first left brace. */ + /* Returns the source code position of the first token that comes after the first curly left bracket. */ return ErrorProneTokens.getTokens( sourceCode.subSequence(typeStart, typeEnd).toString(), typeStart, state.context) .stream() .dropWhile(token -> token.kind() != TokenKind.LBRACE) /* * To accommodate enums, skip processing their enumerators. - * This is needed as ErrorProne has access to the enumerations individually, but not to the + * This is needed as Error Prone has access to the enumerations individually, but not to the * whole expression that declares them, leaving the semicolon trailing the declarations * unaccounted for. The current logic would move this trailing semicolon with the first * member after the enumerations instead of leaving it to close the enumerations' @@ -177,6 +159,28 @@ public final class TypeMemberOrder extends BugChecker implements ClassTreeMatche .orElse(Position.NOPOS); } + /** + * Returns a list of the sortable members' preferred ordinals, ordered by the member's position in + * the original source. + */ + private static ImmutableList extractPreferredOrdinals( + ImmutableList members) { + return members.stream() + .filter(m -> m.preferredOrdinal().isPresent()) + .map(m -> m.preferredOrdinal().orElseThrow(/* Unreachable due to preceding check. */ )) + .collect(toImmutableList()); + } + + /** + * Returns true if {@link Tree} is an enumerator of an enumerated type, false otherwise. + * + * @see com.sun.tools.javac.tree.Pretty#isEnumerator(JCTree) + * @see com.sun.tools.javac.code.Flags#ENUM + */ + private static boolean isEnumerator(Tree tree) { + return tree instanceof JCVariableDecl variableDecl && (variableDecl.mods.flags & ENUM) != 0; + } + /** * Suggests a different way of ordering the given type members. * @@ -196,8 +200,9 @@ public final class TypeMemberOrder extends BugChecker implements ClassTreeMatche int end = state.getEndPosition(member.tree()); verify( end != Position.NOPOS && start < end, - "Unexpected member end position, member: %s", - member); + "Unexpected member end position, member: %s, end position: %s", + member, + end); if (member.preferredOrdinal().isPresent()) { membersWithSource.add( new AutoValue_TypeMemberOrder_MovableTypeMember( @@ -232,6 +237,7 @@ public final class TypeMemberOrder extends BugChecker implements ClassTreeMatche return ASTHelpers.getSymbol(methodTree).isConstructor(); } + /** XXX: Write this. Every member that is in a ClassTree? */ @AutoValue abstract static class TypeMember { abstract Tree tree(); @@ -239,6 +245,7 @@ public final class TypeMemberOrder extends BugChecker implements ClassTreeMatche abstract Optional preferredOrdinal(); } + /** Type members that have a sourcecode and are not generated, are considered to be movable. */ @AutoValue abstract static class MovableTypeMember { abstract Tree tree(); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderClassTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderClassTest.java index 5c77899d..49f86955 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderClassTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderClassTest.java @@ -78,7 +78,15 @@ final class TypeMemberOrderClassTest { "", " D() {}", "}") - .addSourceLines("E.java", "class E {}") + .addSourceLines( + "E.java", + "@SuppressWarnings(\"TypeMemberOrder\")", + "class E {", + " void unorderedMethod() {}", + "", + " E() {}", + "}") + .addSourceLines("F.java", "class F {}") .doTest(); } @@ -215,7 +223,7 @@ final class TypeMemberOrderClassTest { } @Test - void replacementDefaultConstructor() { + void replacementHandlesGeneratedDefaultConstructor() { BugCheckerRefactoringTestHelper.newInstance(TypeMemberOrder.class, getClass()) .addInputLines( "A.java", @@ -263,9 +271,13 @@ final class TypeMemberOrderClassTest { .addInputLines( "A.java", "class A {", - " /* `foo` method's dangling comment. */", + " /* empty statement's dangling comment */", " ;", - " // `foo` method's comment", + " /**", + " * Multiline JavaDoc", + " *", + " *

`foo` method's comment", + " */", " int foo() {", " return foo;", " }", @@ -279,7 +291,7 @@ final class TypeMemberOrderClassTest { " static {", " System.out.println(\"foo\");", " }", - " /* `bar` field's dangling comment. */ ;", + " /* `bar` field's dangling comment */", "", " private final int bar = 2;", " // `foo` field's comment", @@ -291,7 +303,7 @@ final class TypeMemberOrderClassTest { "class A {", " // `foo` field's comment", " private static final int foo = 1;", - " /* `bar` field's dangling comment. */ ;", + " /* `bar` field's dangling comment */", "", " private final int bar = 2;", "", @@ -304,10 +316,14 @@ final class TypeMemberOrderClassTest { " {", " System.out.println(\"bar\");", " }", - " /* `foo` method's dangling comment. */", + " /* empty statement's dangling comment */", " ;", "", - " // `foo` method's comment", + " /**", + " * Multiline JavaDoc", + " *", + " *

`foo` method's comment", + " */", " int foo() {", " return foo;", " }", @@ -328,7 +344,13 @@ final class TypeMemberOrderClassTest { " }", "", " @AnnotationWithClassReferences({Object.class})", - " class InnerClass {", + " class InnerClassOneValue {", + " String bar;", + " private static final int foo = 1;", + " }", + "", + " @AnnotationWithClassReferences(value = {Integer.class, String.class})", + " class InnerClassTwoValues {", " String bar;", " private static final int foo = 1;", " }", @@ -342,7 +364,13 @@ final class TypeMemberOrderClassTest { " }", "", " @AnnotationWithClassReferences({Object.class})", - " class InnerClass {", + " class InnerClassOneValue {", + " private static final int foo = 1;", + " String bar;", + " }", + "", + " @AnnotationWithClassReferences(value = {Integer.class, String.class})", + " class InnerClassTwoValues {", " private static final int foo = 1;", " String bar;", " }", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderEnumTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderEnumTest.java index a227aedc..e72ae938 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderEnumTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderEnumTest.java @@ -208,7 +208,7 @@ final class TypeMemberOrderEnumTest { } @Test - void replacementDefaultConstructor() { + void replacementHandlesGeneratedDefaultConstructor() { BugCheckerRefactoringTestHelper.newInstance(TypeMemberOrder.class, getClass()) .addInputLines( "A.java", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderInterfaceTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderInterfaceTest.java index 9351d3cd..5186efc1 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderInterfaceTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderInterfaceTest.java @@ -168,22 +168,22 @@ final class TypeMemberOrderInterfaceTest { .addInputLines( "A.java", "interface A {", - " // `bar` method's dangling comment.", + " // empty statement's dangling comment", " ;", " // `bar` method's comment", " void bar();", "", - " // `foo` method's comment", - " static final int foo = 1;", + " // `foo` field's comment", + " int foo = 1;", " // trailing comment", "}") .addOutputLines( "A.java", "interface A {", "", - " // `foo` method's comment", - " static final int foo = 1;", - " // `bar` method's dangling comment.", + " // `foo` field's comment", + " int foo = 1;", + " // empty statement's dangling comment", " ;", "", " // `bar` method's comment", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderUnhandledKindsTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderUnhandledCasesTest.java similarity index 94% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderUnhandledKindsTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderUnhandledCasesTest.java index af11649b..dfd24358 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderUnhandledKindsTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TypeMemberOrderUnhandledCasesTest.java @@ -3,7 +3,7 @@ package tech.picnic.errorprone.bugpatterns; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -final class TypeMemberOrderUnhandledKindsTest { +final class TypeMemberOrderUnhandledCasesTest { @Test void identification() { CompilationTestHelper.newInstance(TypeMemberOrder.class, getClass())