jlengrand/error-prone-support
1
0
Fork 0
mirror of https://github.com/jlengrand/error-prone-support.git synced 2026-03-10 08:11:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

Post-rebase fix, suggestions

Browse Source
This commit is contained in:
Stephan Schroevers
2023-10-17 16:37:05 +02:00
parent f59a85aa9a
commit 997099e957
3 changed files with 287 additions and 285 deletions
Download Patch File Download Diff File Expand all files Collapse all files

202
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassMemberOrder.java Normal file
View File

@@ -0,0 +1,202 @@
package tech.picnic.errorprone.bugpatterns;
import static com.google.common.base.Verify.verify;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.STYLE;
import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
import com.google.auto.service.AutoService;
import com.google.auto.value.AutoValue;
import com.google.common.base.VerifyException;
import com.google.common.collect.Comparators;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Streams;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.Var;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.ErrorProneToken;
import com.google.errorprone.util.ErrorProneTokens;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.parser.Tokens.TokenKind;
import com.sun.tools.javac.util.Position;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import javax.lang.model.element.Modifier;
/**
* A {@link BugChecker} that flags classes with a non-canonical member order.
*
* <p>Class members should be ordered as follows:
*
* <ul>
* <li>Static fields
* <li>Instance fields
* <li>Static initializer blocks
* <li>Instance initializer blocks
* <li>Constructors
* <li>Methods
* <li>Nested classes
* </ul>
*
* @see <a
* href="https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/DeclarationOrderCheck.html">Checkstyle's
* {@code DeclarationOrderCheck}</a>
*/
@AutoService(BugChecker.class)
@BugPattern(
summary = "Class members should be ordered in a canonical way",
link = BUG_PATTERNS_BASE_URL + "ClassMemberOrder",
linkType = CUSTOM,
severity = SUGGESTION,
tags = STYLE)
// XXX: class -> type.
public final class ClassMemberOrder extends BugChecker implements ClassTreeMatcher {
private static final long serialVersionUID = 1L;
/** Orders {@link Tree}s to match the standard Java type member declaration order. */
private static final Comparator<Tree> BY_PREFERRED_TYPE_MEMBER_ORDER =
comparing(
tree -> {
switch (tree.getKind()) {
case VARIABLE:
return isStatic((VariableTree) tree) ? 1 : 2;
case BLOCK:
return ((BlockTree) tree).isStatic() ? 3 : 4;
case METHOD:
return isConstructor((MethodTree) tree) ? 5 : 6;
case CLASS:
// XXX: Ideally we also sort static nested classes after non-static nested classes.
// But as this could create quite some unnecessary churn, we should first introduce
// a check that flags nested classes that could be static, but aren't.
return 7;
default:
throw new VerifyException("Unexpected member kind: " + tree.getKind());
}
});
/** Instantiates a new {@link ClassMemberOrder} instance. */
public ClassMemberOrder() {}
@Override
public Description matchClass(ClassTree tree, VisitorState state) {
ImmutableList<Tree> sortableMembers =
tree.getMembers().stream().filter(m -> canMove(m, state)).collect(toImmutableList());
if (Comparators.isInOrder(sortableMembers, BY_PREFERRED_TYPE_MEMBER_ORDER)) {
return Description.NO_MATCH;
}
int classBodyStart = getBodyStartPos(tree, state);
if (classBodyStart == Position.NOPOS) {
/*
* We can't determine the class body's start position in the source code. This generally means
* that (part of) its code was generated. Even if the source code for a subset of its members
* is available, dealing with this edge case is not worth the trouble.
*/
return Description.NO_MATCH;
}
return describeMatch(tree, sortClassMembers(classBodyStart, sortableMembers, state));
}
private boolean canMove(Tree tree, VisitorState state) {
return state.getEndPosition(tree) != Position.NOPOS && !isSuppressed(tree, state);
}
private static int getBodyStartPos(ClassTree clazz, VisitorState state) {
CharSequence sourceCode = state.getSourceCode();
int classStart = ASTHelpers.getStartPosition(clazz);
int classEnd = state.getEndPosition(clazz);
if (sourceCode == null || classStart == Position.NOPOS || classEnd == Position.NOPOS) {
return Position.NOPOS;
}
/*
* We return the source code position that follows the first left brace after the `class`
* keyword.
*/
// XXX: !! Doesn't work for interfaces. How to make exhaustive? For records we'd even need
// special handling. (But maybe we should support only classes and interfaces for now? Enums
// have other considerations!)
return ErrorProneTokens.getTokens(
sourceCode.subSequence(classStart, classEnd).toString(), classStart, state.context)
.stream()
.dropWhile(t -> t.kind() != TokenKind.CLASS)
.dropWhile(t -> t.kind() != TokenKind.LBRACE)
.findFirst()
.map(ErrorProneToken::endPos)
.orElse(Position.NOPOS);
}
/**
* Suggests a different way of ordering the given class members.
*
* @implNote For each member, this method tracks the source code between the end of the definition
* of the member that precedes it (or the start of the class body if there is no such member)
* and the end of the definition of the member itself. This subsequently enables moving
* members around, including any preceding comments and Javadoc. This approach isn't perfect,
* and may at times move too much code or documentation around; users will have to manually
* resolve this.
*/
private static SuggestedFix sortClassMembers(
int classBodyStart, ImmutableList<Tree> members, VisitorState state) {
List<ClassMember> membersWithSource = new ArrayList<>();
@Var int start = classBodyStart;
for (Tree member : members) {
int end = state.getEndPosition(member);
verify(end != Position.NOPOS && start < end, "Unexpected member end position");
membersWithSource.add(new AutoValue_ClassMemberOrder_ClassMember(member, start, end));
start = end;
}
CharSequence sourceCode = requireNonNull(state.getSourceCode(), "Source code");
return Streams.zip(
membersWithSource.stream(),
membersWithSource.stream()
.sorted(comparing(ClassMember::tree, BY_PREFERRED_TYPE_MEMBER_ORDER)),
(original, replacement) -> original.replaceWith(replacement, sourceCode))
.reduce(SuggestedFix.builder(), SuggestedFix.Builder::merge, SuggestedFix.Builder::merge)
.build();
}
private static boolean isStatic(VariableTree variableTree) {
return variableTree.getModifiers().getFlags().contains(Modifier.STATIC);
}
private static boolean isConstructor(MethodTree methodTree) {
return ASTHelpers.getSymbol(methodTree).isConstructor();
}
@AutoValue
abstract static class ClassMember {
abstract Tree tree();
abstract int startPosition();
abstract int endPosition();
SuggestedFix replaceWith(ClassMember other, CharSequence fullSourceCode) {
return equals(other)
? SuggestedFix.emptyFix()
: SuggestedFix.replace(
startPosition(),
endPosition(),
fullSourceCode.subSequence(other.startPosition(), other.endPosition()).toString());
}
}
}

181
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassMemberOrdering.java
View File

@@ -1,181 +0,0 @@
package tech.picnic.errorprone.bugpatterns;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.BugPattern.StandardTags.STYLE;
import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.joining;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
import com.google.auto.service.AutoService;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Streams;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.ErrorProneToken;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.parser.Tokens;
import com.sun.tools.javac.util.Position;
import java.util.Comparator;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;
import javax.lang.model.element.Modifier;
import tech.picnic.errorprone.bugpatterns.util.SourceCode;
/** A {@link BugChecker} that flags classes with non-standard member ordering. */
// XXX: Reference
// https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/DeclarationOrderCheck.html
@AutoService(BugChecker.class)
@BugPattern(
summary = "Class members should be ordered in a standard way",
explanation =
"Class members should be ordered in a standard way, which is: "
+ "static fields, non-static fields, constructors and methods.",
link = BUG_PATTERNS_BASE_URL + "ClassMemberOrdering",
linkType = CUSTOM,
severity = WARNING,
tags = STYLE)
public final class ClassMemberOrdering extends BugChecker implements BugChecker.ClassTreeMatcher {
private static final long serialVersionUID = 1L;
/** Orders {@link Tree}s to match the standard Java type member declaration order. */
private static final Comparator<Tree> BY_PREFERRED_TYPE_MEMBER_ORDER =
comparing(
tree -> {
switch (tree.getKind()) {
case VARIABLE:
return isStatic((VariableTree) tree) ? 1 : 2;
case METHOD:
return isConstructor((MethodTree) tree) ? 3 : 4;
default:
throw new IllegalStateException("Unexpected kind: " + tree.getKind());
}
});
/** Instantiates a new {@link ClassMemberOrdering} instance. */
public ClassMemberOrdering() {}
@Override
public Description matchClass(ClassTree classTree, VisitorState state) {
ImmutableList<ClassMemberWithComments> classMembers =
getClassMembersWithComments(classTree, state).stream()
.filter(classMember -> shouldBeSorted(classMember.tree()))
.collect(toImmutableList());
ImmutableList<ClassMemberWithComments> sortedClassMembers =
ImmutableList.sortedCopyOf(
(a, b) -> BY_PREFERRED_TYPE_MEMBER_ORDER.compare(a.tree(), b.tree()), classMembers);
if (classMembers.equals(sortedClassMembers)) {
return Description.NO_MATCH;
}
return buildDescription(classTree)
.addFix(replaceClassMembers(classMembers, sortedClassMembers, state))
.setMessage(
"Fields, constructors and methods should follow standard ordering. "
+ "The standard ordering is: static fields, non-static fields, "
+ "constructors and methods.")
.build();
}
private static boolean isStatic(VariableTree variableTree) {
Set<Modifier> modifiers = variableTree.getModifiers().getFlags();
return modifiers.contains(Modifier.STATIC);
}
private static boolean isConstructor(MethodTree methodTree) {
return ASTHelpers.getSymbol(methodTree).isConstructor();
}
private static boolean shouldBeSorted(Tree tree) {
return tree instanceof VariableTree
|| (tree instanceof MethodTree && !ASTHelpers.isGeneratedConstructor((MethodTree) tree));
}
private static SuggestedFix replaceClassMembers(
ImmutableList<ClassMemberWithComments> classMembers,
ImmutableList<ClassMemberWithComments> replacementClassMembers,
VisitorState state) {
return Streams.zip(
classMembers.stream(),
replacementClassMembers.stream(),
(original, replacement) -> replaceClassMember(state, original, replacement))
.reduce(SuggestedFix.builder(), SuggestedFix.Builder::merge, SuggestedFix.Builder::merge)
.build();
}
private static SuggestedFix replaceClassMember(
VisitorState state, ClassMemberWithComments original, ClassMemberWithComments replacement) {
/* Technically this check is not necessary, but it avoids redundant replacements. */
if (original.equals(replacement)) {
return SuggestedFix.emptyFix();
}
String replacementSource =
Stream.concat(
replacement.comments().stream(),
Stream.of(SourceCode.treeToString(replacement.tree(), state)))
.collect(joining(System.lineSeparator()));
return SuggestedFixes.replaceIncludingComments(
TreePath.getPath(state.getPath(), original.tree()), replacementSource, state);
}
/** Returns the class' members with their comments. */
private static ImmutableList<ClassMemberWithComments> getClassMembersWithComments(
ClassTree classTree, VisitorState state) {
return classTree.getMembers().stream()
.map(
member ->
new AutoValue_ClassMemberOrdering_ClassMemberWithComments(
member, getClassMemberComments(state, classTree, member)))
.collect(toImmutableList());
}
private static ImmutableList<String> getClassMemberComments(
VisitorState state, ClassTree classTree, Tree classMember) {
int typeStart = ASTHelpers.getStartPosition(classTree);
int typeEnd = state.getEndPosition(classTree);
int memberStart = ASTHelpers.getStartPosition(classMember);
int memberEnd = state.getEndPosition(classMember);
if (typeStart == Position.NOPOS
|| typeEnd == Position.NOPOS
|| memberStart == Position.NOPOS
|| memberEnd == Position.NOPOS) {
/* Source code details appear to be unavailable. */
return ImmutableList.of();
}
Optional<Integer> previousClassTokenEndPos =
state.getOffsetTokens(typeStart, typeEnd).stream()
.map(ErrorProneToken::endPos)
.takeWhile(endPos -> endPos < memberStart)
.reduce((earlierPos, laterPos) -> laterPos);
List<ErrorProneToken> classMemberTokens =
state.getOffsetTokens(previousClassTokenEndPos.orElse(memberStart), memberEnd);
return classMemberTokens.get(0).comments().stream()
.map(Tokens.Comment::getText)
.collect(toImmutableList());
}
@AutoValue
abstract static class ClassMemberWithComments {
abstract Tree tree();
abstract ImmutableList<String> comments();
}
}

189
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassMemberOrderingTest.java → error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassMemberOrderTest.java
View File

@@ -1,75 +1,83 @@
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;
final class ClassMemberOrderingTest {
final class ClassMemberOrderTest {
@Test
void identification() {
CompilationTestHelper.newInstance(ClassMemberOrdering.class, getClass())
.expectErrorMessage(
"ClassMemberOrdering",
containsPattern("Fields, constructors and methods should follow standard ordering."))
CompilationTestHelper.newInstance(ClassMemberOrder.class, getClass())
.addSourceLines(
"A.java",
"// BUG: Diagnostic matches: ClassMemberOrdering",
"class A {",
" char a = 'a';",
" private static String FOO = \"foo\";",
" static int ONE = 1;",
" class Empty {}",
"",
" void m2() {}",
"",
" public A() {}",
"",
" private static String BAR = \"bar\";",
" char b = 'b';",
"",
" void m1() {",
" System.out.println(\"foo\");",
" class SingleField {",
" private int field;",
" }",
"",
" static int TWO = 2;",
" class FieldAndMethod {",
" private int field;",
"",
" class Inner {}",
"",
" static class StaticInner {}",
"}")
.addSourceLines(
"B.java",
"class B {",
" private static String FOO = \"foo\";",
" static int ONE = 1;",
" private static String BAR = \"bar\";",
"",
" static int TWO = 2;",
"",
" char a = 'a';",
"",
" char b = 'b';",
"",
" public B() {}",
"",
" void m1() {",
" System.out.println(\"foo\");",
" void foo() {}",
" }",
"",
" void m2() {}",
" // BUG: Diagnostic contains:",
" class MethodAndField {",
" void foo() {}",
"",
" class Inner {}",
" private int field;",
" }",
"",
" static class StaticInner {}",
" class AllSorted {",
" private static String FOO = \"foo\";",
" static String BAR = \"bar\";",
" public static final int ONE = 1;",
" protected static final int TWO = 2;",
"",
" private char a = 'a';",
" public char b = 'b';",
"",
" static {",
" FOO = \"foo2\";",
" }",
"",
" static {",
" BAR = \"bar2\";",
" }",
"",
" {",
" a = 'c';",
" }",
"",
" {",
" b = 'd';",
" }",
"",
" public AllSorted() {}",
"",
" AllSorted(int param) {}",
"",
" int m1() {",
" return 42;",
" }",
"",
" public void m2() {}",
"",
" class Nested {}",
"",
" static class StaticNested {}",
" }",
"}")
.doTest();
}
// XXX: Also test with an interface!
@Test
void replacementFirstSuggestedFix() {
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrdering.class, getClass())
void replacement() {
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrder.class, getClass())
.addInputLines(
"A.java",
"class A {",
@@ -80,6 +88,9 @@ final class ClassMemberOrderingTest {
"",
" void m2() {}",
"",
" {",
" }",
"",
" public A() {}",
"",
" private static String BAR = \"bar\";",
@@ -101,14 +112,16 @@ final class ClassMemberOrderingTest {
" private static final int X = 1;",
" private static String FOO = \"foo\";",
" static int ONE = 1;",
"",
" private static String BAR = \"bar\";",
"",
" static int TWO = 2;",
"",
" char a = 'a';",
"",
" char b = 'b';",
"",
" {",
" }",
"",
" public A() {}",
"",
" void m2() {}",
@@ -122,11 +135,9 @@ final class ClassMemberOrderingTest {
" static class StaticInner {}",
"}")
.doTest(TestMode.TEXT_MATCH);
}
@Test
void replacementFirstSuggestedFixConsidersDefaultConstructor() {
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrdering.class, getClass())
// XXX: Merge.
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrder.class, getClass())
.addInputLines(
"A.java",
"class A {",
@@ -141,6 +152,7 @@ final class ClassMemberOrderingTest {
.addOutputLines(
"A.java",
"class A {",
"",
" private static final String foo = \"foo\";",
"",
" static int one = 1;",
@@ -150,17 +162,15 @@ final class ClassMemberOrderingTest {
" void m1() {}",
"}")
.doTest(TestMode.TEXT_MATCH);
}
@SuppressWarnings("ErrorProneTestHelperSourceFormat")
@Test
void replacementFirstSuggestedFixConsidersComments() {
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrdering.class, getClass())
// XXX: Merge.
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrder.class, getClass())
.addInputLines(
"A.java",
"class A {",
" // detached comment from method",
" ;void method1() {}",
" ;",
" void method1() {}",
"",
" // first comment prior to method",
" // second comment prior to method",
@@ -176,11 +186,13 @@ final class ClassMemberOrderingTest {
.addOutputLines(
"A.java",
"class A {",
"",
" // foo",
" /** Instantiates a new {@link A} instance. */",
" public A() {}",
"",
" // detached comment from method",
" ;",
"",
" void method1() {}",
"",
" // first comment prior to method",
@@ -191,11 +203,9 @@ final class ClassMemberOrderingTest {
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
}
@Test
void replacementFirstSuggestedFixConsidersAnnotations() {
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrdering.class, getClass())
// XXX: Merge.
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrder.class, getClass())
.addInputLines(
"A.java",
"class A {",
@@ -208,6 +218,7 @@ final class ClassMemberOrderingTest {
.addOutputLines(
"A.java",
"class A {",
"",
" @SuppressWarnings(\"bar\")",
" A() {}",
"",
@@ -215,90 +226,60 @@ final class ClassMemberOrderingTest {
" void m1() {}",
"}")
.doTest(TestMode.TEXT_MATCH);
}
@SuppressWarnings("ErrorProneTestHelperSourceFormat")
@Test
void replacementFirstSuggestedFixDoesNotModifyWhitespace() {
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrdering.class, getClass())
// XXX: Merge.
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrder.class, getClass())
.addInputLines(
"A.java",
"",
"",
"class A {",
"",
"",
" // `m1()` comment.",
" void m1() {",
" // Print line 'foo' to stdout.",
" System.out.println(\"foo\");",
" }",
" public A () { }",
"",
"",
" public A() {}",
"}")
.addOutputLines(
"A.java",
"",
"",
"class A {",
"",
" public A() {}",
"",
"",
" public A () { }",
" // `m1()` comment.",
" void m1() {",
" // Print line 'foo' to stdout.",
" System.out.println(\"foo\");",
" }",
"",
"",
"}")
.doTest();
}
.doTest(TestMode.TEXT_MATCH);
// XXX: This test should fail, if we verify that whitespace is preserved.
@SuppressWarnings("ErrorProneTestHelperSourceFormat")
void xxx() {
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrdering.class, getClass())
// XXX: Merge.
BugCheckerRefactoringTestHelper.newInstance(ClassMemberOrder.class, getClass())
.addInputLines(
"A.java",
"",
"",
"class A {",
"",
"",
" // `m1()` comment.",
" void m1() {",
" // Print line 'foo' to stdout.",
" System.out.println(\"foo\");",
" }",
" public A () { }",
"",
"",
" public A() {}",
"}")
.addOutputLines(
"A.java",
"",
"",
"class A {",
"",
" ",
" ",
" \t \t",
" ",
" ",
" public A() {}",
"",
" public A () { }",
" // `m1()` comment.",
" void m1",
" ()",
" {",
" void m1() {",
" // Print line 'foo' to stdout.",
" System.out.println(\"foo\");",
" }",
"",
"",
"}")
.doTest(TestMode.TEXT_MATCH);
}