From 45693cca2541bc7838b3e2a471f640897dcf3b5c Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Mon, 24 Jul 2023 14:59:50 +0200 Subject: [PATCH] suggestions --- .../bugpatterns/MemberOrdering.java | 89 ++++++++----------- 1 file changed, 36 insertions(+), 53 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MemberOrdering.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MemberOrdering.java index 19b3d726477..4a46620f79c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MemberOrdering.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MemberOrdering.java @@ -12,21 +12,20 @@ import com.google.common.collect.ImmutableList; 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.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.google.errorprone.util.ErrorProneTokens; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; import com.sun.tools.javac.parser.Tokens; -import java.util.ArrayList; +import com.sun.tools.javac.parser.Tokens.TokenKind; import java.util.Comparator; import java.util.List; import java.util.Set; @@ -36,7 +35,7 @@ /** A {@link BugChecker} that flags classes with non-standard member ordering. */ @AutoService(BugChecker.class) @BugPattern( - summary = "Members should be ordered in a standard way.", + summary = "Members should be ordered in a standard way", explanation = "Members should be ordered in a standard way, which is: " + "static member variables, non-static member variables, constructors and methods.", @@ -50,14 +49,14 @@ public final class MemberOrdering extends BugChecker implements BugChecker.Class /** A comparator that sorts variable and method (incl. constructors) in a standard order. */ private static final Comparator MEMBER_SORTING = comparing( - (Tree memberTree) -> { - switch (memberTree.getKind()) { + (Tree tree) -> { + switch (tree.getKind()) { case VARIABLE: - return isStatic((VariableTree) memberTree) ? 1 : 2; + return isStatic((VariableTree) tree) ? 1 : 2; case METHOD: - return isConstructor((MethodTree) memberTree) ? 3 : 4; + return isConstructor((MethodTree) tree) ? 3 : 4; default: - throw new IllegalStateException("Unexpected kind: " + memberTree.getKind()); + throw new IllegalStateException("Unexpected kind: " + tree.getKind()); } }); @@ -66,12 +65,12 @@ public MemberOrdering() {} @Override public Description matchClass(ClassTree tree, VisitorState state) { - ImmutableList membersWithComments = + ImmutableList membersWithComments = getMembersWithComments(tree, state).stream() - .filter(memberWithComments -> shouldBeSorted(memberWithComments.member())) + .filter(classMemberWithComments -> shouldBeSorted(classMemberWithComments.member())) .collect(toImmutableList()); - ImmutableList sortedMembersWithComments = + ImmutableList sortedMembersWithComments = ImmutableList.sortedCopyOf( (a, b) -> MEMBER_SORTING.compare(a.member(), b.member()), membersWithComments); @@ -103,13 +102,13 @@ private static boolean shouldBeSorted(Tree tree) { } private static SuggestedFix swapMembersWithComments( - ImmutableList memberWithComments, - ImmutableList sortedMembersWithComments, + ImmutableList memberWithComments, + ImmutableList sortedMembersWithComments, VisitorState state) { SuggestedFix.Builder fix = SuggestedFix.builder(); for (int i = 0; i < memberWithComments.size(); i++) { Tree originalMember = memberWithComments.get(i).member(); - MemberWithComments correct = sortedMembersWithComments.get(i); + ClassMemberWithComments correct = sortedMembersWithComments.get(i); /* Technically this check is not necessary, but it avoids redundant replacements. */ if (!originalMember.equals(correct.member())) { String replacement = @@ -125,62 +124,46 @@ private static SuggestedFix swapMembersWithComments( return fix.build(); } - // XXX: Work around that `ErrorProneTokens.getTokens(memberSrc, ctx)` returns tokens not - // containing the member's comments. /** Returns the class' members with their comments. */ - private static ImmutableList getMembersWithComments( + private static ImmutableList getMembersWithComments( ClassTree classTree, VisitorState state) { List tokens = - new ArrayList<>( - ErrorProneTokens.getTokens(state.getSourceForNode(classTree), state.context)); + state.getOffsetTokens( + ASTHelpers.getStartPosition(classTree), state.getEndPosition(classTree)); - ImmutableList.Builder membersWithComments = ImmutableList.builder(); + ImmutableList.Builder membersWithComments = ImmutableList.builder(); for (Tree member : classTree.getMembers()) { - ImmutableList memberTokens = - ErrorProneTokens.getTokens(state.getSourceForNode(member), state.context); - if (memberTokens.isEmpty() || memberTokens.get(0).kind() == Tokens.TokenKind.EOF) { + if (!shouldBeSorted(member)) { continue; } - @Var - ImmutableList maybeCommentedMemberTokens = - ImmutableList.copyOf(tokens.subList(0, memberTokens.size())); - while (!areTokenListsMatching(memberTokens, maybeCommentedMemberTokens)) { - tokens.remove(0); - maybeCommentedMemberTokens = ImmutableList.copyOf(tokens.subList(0, memberTokens.size())); + /* We start at the previous token's end position to cover any possible comments. */ + int memberStartPos = ASTHelpers.getStartPosition(member); + int startPos = + tokens.stream() + .map(ErrorProneToken::endPos) + .filter(i -> i < memberStartPos) + .reduce((first, second) -> second) + .orElse(memberStartPos); + + ImmutableList memberTokens = + ImmutableList.copyOf(state.getOffsetTokens(startPos, state.getEndPosition(member))); + if (memberTokens.isEmpty() || memberTokens.get(0).kind() == TokenKind.EOF) { + continue; } membersWithComments.add( - new MemberWithComments( - member, ImmutableList.copyOf(maybeCommentedMemberTokens.get(0).comments()))); + new ClassMemberWithComments( + member, ImmutableList.copyOf(memberTokens.get(0).comments()))); } return membersWithComments.build(); } - /** - * Checks whether two lists of error-prone tokens are 'equal' without considering their comments. - */ - private static boolean areTokenListsMatching( - ImmutableList tokens, ImmutableList memberTokens) { - if (tokens.size() != memberTokens.size()) { - return false; - } - for (int i = 0; i < tokens.size() - 1 /* EOF */; i++) { - if (tokens.get(i).kind() != memberTokens.get(i).kind() - || tokens.get(i).hasName() != memberTokens.get(i).hasName() - || (tokens.get(i).hasName() - && !tokens.get(i).name().equals(memberTokens.get(i).name()))) { - return false; - } - } - return true; - } - - private static final class MemberWithComments { + private static final class ClassMemberWithComments { private final Tree member; private final ImmutableList comments; - MemberWithComments(Tree member, ImmutableList comments) { + ClassMemberWithComments(Tree member, ImmutableList comments) { this.member = member; this.comments = comments; }