From 17bede8e8deaf23736fdd8e0632444debd8bbff6 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 21 May 2022 17:19:29 +0200 Subject: [PATCH] Suggestions --- .../errorprone/bugpatterns/BuildNode.java | 52 -------- .../picnic/errorprone/bugpatterns/Node.java | 96 +++++++++----- .../errorprone/bugpatterns/RefasterCheck.java | 120 ++++++++++++------ .../picnic/errorprone/bugpatterns/Util.java | 48 +++++-- 4 files changed, 187 insertions(+), 129 deletions(-) delete mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java deleted file mode 100644 index 3a7628f616..0000000000 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java +++ /dev/null @@ -1,52 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import com.google.auto.value.AutoValue; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableSortedSet; -import com.google.common.collect.Maps; -import java.util.HashSet; -import java.util.Set; -import java.util.SortedMap; -import java.util.TreeMap; - -@AutoValue -abstract class BuildNode { - static BuildNode create() { - return new AutoValue_BuildNode<>(new TreeMap<>(), new HashSet<>()); - } - - static BuildNode create(SortedMap> children, Set candidateRules) { - return new AutoValue_BuildNode<>(children, candidateRules); - } - - @SuppressWarnings("AutoValueImmutableFields" /* The BuildNode is used to construct the tree. */) - public abstract SortedMap> children(); - - @SuppressWarnings("AutoValueImmutableFields" /* The BuildNode is used to construct the tree. */) - public abstract Set candidateRules(); - - public void register(ImmutableSet> identifierCombinations, T rule) { - for (ImmutableSet path : identifierCombinations) { - registerPath(path.asList(), rule); - } - } - - void registerPath(ImmutableList path, T rule) { - path.stream() - .findFirst() - .ifPresentOrElse( - edge -> - children() - .computeIfAbsent(edge, k -> BuildNode.create()) - .registerPath(path.subList(1, path.size()), rule), - () -> candidateRules().add(rule)); - } - - Node immutable() { - return Node.create( - Maps.transformValues(ImmutableSortedMap.copyOfSorted(children()), BuildNode::immutable), - ImmutableSet.copyOf(candidateRules())); - } -} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Node.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Node.java index 53ac7cc8ec..affa8c8d80 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Node.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Node.java @@ -1,61 +1,99 @@ package tech.picnic.errorprone.bugpatterns; -import static com.google.common.collect.ImmutableList.toImmutableList; - import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.collect.Maps; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.function.Consumer; import java.util.function.Function; +/** + * A node in an immutable tree. + * + *

The tree's edges are string-labeled, while its leaves store values of type {@code T}. + */ @AutoValue abstract class Node { - static Node create(Map> children, ImmutableSet candidateRules) { - return new AutoValue_Node<>(ImmutableMap.copyOf(children), candidateRules); + static Node create(Map> children, ImmutableList values) { + return new AutoValue_Node<>(ImmutableSortedMap.copyOf(children), values); } - public abstract ImmutableMap> children(); - - public abstract ImmutableSet candidateRules(); - - static Node createRefasterTemplateTree( - List refasterRules, Function>> edgeExtractor) { - // XXX: Improve this method... - List>> beforeTemplateIdentifiers = - refasterRules.stream().map(edgeExtractor).collect(toImmutableList()); - - BuildNode tree = BuildNode.create(); - for (int i = 0; i < refasterRules.size(); i++) { - tree.register(beforeTemplateIdentifiers.get(i), refasterRules.get(i)); - } + static Node create( + List values, Function>> pathExtractor) { + BuildNode tree = BuildNode.create(); + tree.register(values, pathExtractor); return tree.immutable(); } - void collectCandidateTemplates(ImmutableList sourceIdentifiers, Consumer sink) { - candidateRules().forEach(sink); + abstract ImmutableMap> children(); - if (sourceIdentifiers.isEmpty() || children().isEmpty()) { + abstract ImmutableList values(); + + void collectCandidateTemplates(ImmutableList candidateEdges, Consumer sink) { + values().forEach(sink); + + if (candidateEdges.isEmpty() || children().isEmpty()) { return; } - if (children().size() < sourceIdentifiers.size()) { + if (children().size() < candidateEdges.size()) { for (Map.Entry> e : children().entrySet()) { - if (sourceIdentifiers.contains(e.getKey())) { - e.getValue().collectCandidateTemplates(sourceIdentifiers, sink); + if (candidateEdges.contains(e.getKey())) { + e.getValue().collectCandidateTemplates(candidateEdges, sink); } } } else { - ImmutableList remainingSourceCandidateEdges = - sourceIdentifiers.subList(1, sourceIdentifiers.size()); - Node child = children().get(sourceIdentifiers.get(0)); + ImmutableList remainingCandidateEdges = + candidateEdges.subList(1, candidateEdges.size()); + Node child = children().get(candidateEdges.get(0)); if (child != null) { - child.collectCandidateTemplates(remainingSourceCandidateEdges, sink); + child.collectCandidateTemplates(remainingCandidateEdges, sink); + } + collectCandidateTemplates(remainingCandidateEdges, sink); + } + } + + @AutoValue + @SuppressWarnings("AutoValueImmutableFields" /* Type is used only during `Node` construction. */) + abstract static class BuildNode { + static BuildNode create() { + return new AutoValue_Node_BuildNode<>(new HashMap<>(), new ArrayList<>()); + } + + abstract Map> children(); + + abstract List values(); + + private void register( + List values, Function>> pathsExtractor) { + for (T value : values) { + for (ImmutableSet path : pathsExtractor.apply(value)) { + registerPath(value, path.asList()); + } } - collectCandidateTemplates(remainingSourceCandidateEdges, sink); + } + + private void registerPath(T value, ImmutableList path) { + path.stream() + .findFirst() + .ifPresentOrElse( + edge -> + children() + .computeIfAbsent(edge, k -> BuildNode.create()) + .registerPath(value, path.subList(1, path.size())), + () -> values().add(value)); + } + + private Node immutable() { + return Node.create( + Maps.transformValues(children(), BuildNode::immutable), ImmutableList.copyOf(values())); } } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java index 43a7bed680..220ea7bd77 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java @@ -6,6 +6,8 @@ import static com.google.errorprone.BugPattern.LinkType.NONE; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +import static java.util.Collections.newSetFromMap; +import static java.util.Objects.requireNonNullElseGet; import static java.util.function.Predicate.not; import static java.util.stream.Collectors.toCollection; @@ -41,8 +43,11 @@ import com.google.errorprone.refaster.UExpression; import com.google.errorprone.refaster.UStatement; import com.google.errorprone.refaster.UStaticIdent; +import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.BinaryTree; import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.CompoundAssignmentTree; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MemberReferenceTree; import com.sun.source.tree.MemberSelectTree; @@ -58,6 +63,7 @@ import java.util.ArrayList; import java.util.Comparator; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; @@ -92,7 +98,7 @@ public final class RefasterCheck extends BugChecker implements CompilationUnitTr static final Supplier> ALL_CODE_TRANSFORMERS = Suppliers.memoize(RefasterCheck::loadAllCodeTransformers); - private final Node> refasterTemplatesTree; + private final Node> refasterRules; /** Instantiates the default {@link RefasterCheck}. */ public RefasterCheck() { @@ -105,18 +111,13 @@ public RefasterCheck() { * @param flags Any provided command line flags. */ public RefasterCheck(ErrorProneFlags flags) { - List> refasterRules = getRefasterRules(flags); - refasterTemplatesTree = - Node.createRefasterTemplateTree(refasterRules, RefasterCheck::extractTemplateIdentifiers); + refasterRules = Node.create(getRefasterRules(flags), RefasterCheck::extractTemplateIdentifiers); } @Override public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { - ImmutableSortedSet sourceIdentifiers = extractSourceIdentifiers(tree); - - HashSet> candidateRules = new HashSet<>(); - refasterTemplatesTree.collectCandidateTemplates( - sourceIdentifiers.asList(), candidateRules::add); + // XXX: Inline this variable. + Set> candidateRules = getCandidateRefasterRules(tree); // XXX: Remove these debug lines // String removeThis = @@ -124,11 +125,12 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s // System.out.printf("\nTemplates for %s: \n%s\n", tree.getSourceFile().getName(), removeThis); /* First, collect all matches. */ + SubContext context = new SubContext(state.context); List matches = new ArrayList<>(); for (RefasterRule rule : candidateRules) { try { - rule.apply(state.getPath(), new SubContext(state.context), matches::add); - } catch (LinkageError le) { + rule.apply(state.getPath(), context, matches::add); + } catch (LinkageError e) { // XXX: This `try/catch` block handles the issue described and resolved in // https://github.com/google/error-prone/pull/2456. Drop this block once that change is // released. @@ -144,6 +146,15 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s return Description.NO_MATCH; } + // XXX: Here and below: drop redundant `Refaster` from method names? + private Set> getCandidateRefasterRules(CompilationUnitTree tree) { + Set> candidateRules = newSetFromMap(new IdentityHashMap<>()); + refasterRules.collectCandidateTemplates( + extractSourceIdentifiers(tree).asList(), candidateRules::add); + + return candidateRules; + } + private static List> getRefasterRules(ErrorProneFlags flags) { CodeTransformer compositeCodeTransformer = createCompositeCodeTransformer(flags); @@ -166,6 +177,7 @@ private static void collectRefasterRules( } } + // XXX: Decompose `RefasterRule`s such that each has exactly one `@BeforeTemplate`. private static ImmutableSet> extractTemplateIdentifiers( RefasterRule refasterRule) { ImmutableSet.Builder> results = ImmutableSet.builder(); @@ -223,22 +235,6 @@ private String getSimpleName(String fcqn) { return index < 0 ? fcqn : fcqn.substring(index + 1); } - @Override - public Void visitOther(Tree node, List> identifierCombinations) { - if (node instanceof UAnyOf) { - List> base = copy(identifierCombinations); - identifierCombinations.clear(); - - for (UExpression expr : ((UAnyOf) node).expressions()) { - List> branch = copy(base); - scan(expr, branch); - identifierCombinations.addAll(branch); - } - } - - return null; - } - @Override public Void visitMemberReference( MemberReferenceTree node, List> identifierCombinations) { @@ -258,17 +254,48 @@ public Void visitMemberSelect( } @Override - public Void visitBinary(BinaryTree node, List> identifierCombinations) { - super.visitBinary(node, identifierCombinations); - String operator = Util.treeKindToString(node.getKind()); - identifierCombinations.forEach(ids -> ids.add(operator)); - return null; + public Void visitAssignment(AssignmentTree node, List> identifierCombinations) { + registerOperator(node, identifierCombinations); + return super.visitAssignment(node, identifierCombinations); + } + + @Override + public Void visitCompoundAssignment( + CompoundAssignmentTree node, List> identifierCombinations) { + registerOperator(node, identifierCombinations); + return super.visitCompoundAssignment(node, identifierCombinations); } @Override public Void visitUnary(UnaryTree node, List> identifierCombinations) { - super.visitUnary(node, identifierCombinations); + registerOperator(node, identifierCombinations); + return super.visitUnary(node, identifierCombinations); + } + + @Override + public Void visitBinary(BinaryTree node, List> identifierCombinations) { + registerOperator(node, identifierCombinations); + return super.visitBinary(node, identifierCombinations); + } + + // XXX: Rename! + private void registerOperator(ExpressionTree node, List> identifierCombinations) { identifierCombinations.forEach(ids -> ids.add(Util.treeKindToString(node.getKind()))); + } + + @Override + public Void visitOther(Tree node, List> identifierCombinations) { + if (node instanceof UAnyOf) { + List> base = copy(identifierCombinations); + identifierCombinations.clear(); + + for (UExpression expr : ((UAnyOf) node).expressions()) { + List> branch = copy(base); + scan(expr, branch); + identifierCombinations.addAll(branch); + } + } + return null; } @@ -311,17 +338,32 @@ public Void visitMemberSelect(MemberSelectTree node, Set identifiers) { } @Override - public Void visitBinary(BinaryTree node, Set identifiers) { - super.visitBinary(node, identifiers); - identifiers.add(Util.treeKindToString(node.getKind())); - return null; + public Void visitAssignment(AssignmentTree node, Set identifiers) { + registerOperator(node, identifiers); + return super.visitAssignment(node, identifiers); + } + + @Override + public Void visitCompoundAssignment(CompoundAssignmentTree node, Set identifiers) { + registerOperator(node, identifiers); + return super.visitCompoundAssignment(node, identifiers); } @Override public Void visitUnary(UnaryTree node, Set identifiers) { - super.visitUnary(node, identifiers); + registerOperator(node, identifiers); + return super.visitUnary(node, identifiers); + } + + @Override + public Void visitBinary(BinaryTree node, Set identifiers) { + registerOperator(node, identifiers); + return super.visitBinary(node, identifiers); + } + + // XXX: Rename! + private void registerOperator(ExpressionTree node, Set identifiers) { identifiers.add(Util.treeKindToString(node.getKind())); - return null; } }.scan(tree, identifiers); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Util.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Util.java index b088f989e4..33e53bb570 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Util.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Util.java @@ -19,25 +19,29 @@ static String treeToString(Tree tree, VisitorState state) { } /** - * Returns a string representation of the given {@link Tree.Kind}. + * Returns a unique string representation of the given {@link Tree.Kind}. * - * @return A string representation of the operator or else throws an exception. + * @return A string representation of the operator, if known + * @throws IllegalArgumentException If the given input is not supported. */ - // XXX: List needs to be extended, as it currently only supports `BinaryTree`s and `UnaryTree`s. + // XXX: Extend list to cover remaining cases; at least for any `Kind` that may appear in a + // Refaster template. static String treeKindToString(Tree.Kind kind) { switch (kind) { + case ASSIGNMENT: + return "="; case POSTFIX_INCREMENT: + return "x++"; case PREFIX_INCREMENT: - return "++"; + return "++x"; case POSTFIX_DECREMENT: + return "x--"; case PREFIX_DECREMENT: - return "--"; + return "--x"; case UNARY_PLUS: - case PLUS: - return "+"; + return "+x"; case UNARY_MINUS: - case MINUS: - return "-"; + return "-x"; case BITWISE_COMPLEMENT: return "~"; case LOGICAL_COMPLEMENT: @@ -48,6 +52,10 @@ static String treeKindToString(Tree.Kind kind) { return "/"; case REMAINDER: return "%"; + case PLUS: + return "+"; + case MINUS: + return "-"; case LEFT_SHIFT: return "<<"; case RIGHT_SHIFT: @@ -76,6 +84,28 @@ static String treeKindToString(Tree.Kind kind) { return "&&"; case CONDITIONAL_OR: return "||"; + case MULTIPLY_ASSIGNMENT: + return "*="; + case DIVIDE_ASSIGNMENT: + return "/="; + case REMAINDER_ASSIGNMENT: + return "%="; + case PLUS_ASSIGNMENT: + return "+="; + case MINUS_ASSIGNMENT: + return "-="; + case LEFT_SHIFT_ASSIGNMENT: + return "<<="; + case RIGHT_SHIFT_ASSIGNMENT: + return ">>="; + case UNSIGNED_RIGHT_SHIFT_ASSIGNMENT: + return ">>>="; + case AND_ASSIGNMENT: + return "&="; + case XOR_ASSIGNMENT: + return "^="; + case OR_ASSIGNMENT: + return "|="; default: throw new IllegalStateException("Cannot convert Tree.Kind to a String: " + kind); }