From 18a783e1019993d6e2f6abbe0cfeea54319341dd Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 1 May 2022 16:53:25 +0200 Subject: [PATCH] Improve Refaster matching algorithm --- error-prone-contrib/pom.xml | 5 + .../errorprone/bugpatterns/BuildNode.java | 52 ++++ .../picnic/errorprone/bugpatterns/Node.java | 61 +++++ .../errorprone/bugpatterns/RefasterCheck.java | 243 +++++++++++++++++- .../picnic/errorprone/bugpatterns/Util.java | 63 +++++ .../bugpatterns/RefasterCheckTest.java | 8 +- pom.xml | 10 + 7 files changed, 428 insertions(+), 14 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Node.java diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index a49552a2d0..6819028524 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -66,6 +66,11 @@ auto-service-annotations provided + + com.google.auto.value + auto-value-annotations + provided + com.google.code.findbugs jsr305 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 new file mode 100644 index 0000000000..3a7628f616 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BuildNode.java @@ -0,0 +1,52 @@ +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 new file mode 100644 index 0000000000..53ac7cc8ec --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Node.java @@ -0,0 +1,61 @@ +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.ImmutableSortedSet; +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; +import java.util.function.Function; + +@AutoValue +abstract class Node { + static Node create(Map> children, ImmutableSet candidateRules) { + return new AutoValue_Node<>(ImmutableMap.copyOf(children), candidateRules); + } + + 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)); + } + return tree.immutable(); + } + + void collectCandidateTemplates(ImmutableList sourceIdentifiers, Consumer sink) { + candidateRules().forEach(sink); + + if (sourceIdentifiers.isEmpty() || children().isEmpty()) { + return; + } + + if (children().size() < sourceIdentifiers.size()) { + for (Map.Entry> e : children().entrySet()) { + if (sourceIdentifiers.contains(e.getKey())) { + e.getValue().collectCandidateTemplates(sourceIdentifiers, sink); + } + } + } else { + ImmutableList remainingSourceCandidateEdges = + sourceIdentifiers.subList(1, sourceIdentifiers.size()); + Node child = children().get(sourceIdentifiers.get(0)); + if (child != null) { + child.collectCandidateTemplates(remainingSourceCandidateEdges, sink); + } + collectCandidateTemplates(remainingSourceCandidateEdges, sink); + } + } +} 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 bcb1ffa795..0d49924c21 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 @@ -2,11 +2,13 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableRangeSet.toImmutableRangeSet; +import static com.google.common.collect.ImmutableSet.toImmutableSet; 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.Objects.requireNonNullElseGet; import static java.util.function.Predicate.not; +import static java.util.stream.Collectors.toCollection; import com.google.auto.service.AutoService; import com.google.common.annotations.VisibleForTesting; @@ -16,6 +18,7 @@ import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableRangeSet; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Range; import com.google.common.collect.RangeSet; import com.google.common.collect.TreeRangeSet; @@ -31,7 +34,22 @@ import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; import com.google.errorprone.fixes.Replacement; import com.google.errorprone.matchers.Description; +import com.google.errorprone.refaster.BlockTemplate; +import com.google.errorprone.refaster.ExpressionTemplate; +import com.google.errorprone.refaster.RefasterRule; +import com.google.errorprone.refaster.UAnyOf; +import com.google.errorprone.refaster.UClassIdent; +import com.google.errorprone.refaster.UExpression; +import com.google.errorprone.refaster.UStatement; +import com.google.errorprone.refaster.UStaticIdent; +import com.sun.source.tree.BinaryTree; import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.UnaryTree; +import com.sun.source.util.TreeScanner; import com.sun.tools.javac.tree.EndPosTable; import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; import java.io.IOException; @@ -40,10 +58,13 @@ import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.Comparator; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; import java.util.Optional; +import java.util.Set; +import java.util.function.Consumer; import java.util.function.Supplier; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -72,7 +93,7 @@ public final class RefasterCheck extends BugChecker implements CompilationUnitTr static final Supplier> ALL_CODE_TRANSFORMERS = Suppliers.memoize(RefasterCheck::loadAllCodeTransformers); - private final CodeTransformer codeTransformer; + private final Node> refasterTemplatesTree; /** Instantiates the default {@link RefasterCheck}. */ public RefasterCheck() { @@ -85,22 +106,37 @@ public RefasterCheck() { * @param flags Any provided command line flags. */ public RefasterCheck(ErrorProneFlags flags) { - codeTransformer = createCompositeCodeTransformer(flags); + List> refasterRules = getRefasterRules(flags); + refasterTemplatesTree = + Node.createRefasterTemplateTree(refasterRules, 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: Remove these debug lines + // String removeThis = + // candidateRules.stream().map(Object::toString).collect(joining(",")); + // System.out.printf("\nTemplates for %s: \n%s\n", tree.getSourceFile().getName(), removeThis); + /* First, collect all matches. */ List matches = new ArrayList<>(); - try { - codeTransformer.apply(state.getPath(), new SubContext(state.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. - // XXX: Find a way to identify that we're running Picnic's Error Prone fork and disable this - // fallback if so, as it might hide other bugs. - return Description.NO_MATCH; + for (RefasterRule rule : candidateRules) { + try { + rule.apply(state.getPath(), new SubContext(state.context), matches::add); + } catch (LinkageError le) { + // 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. + // XXX: Find a way to identify that we're running Picnic's Error Prone fork and disable this + // fallback if so, as it might hide other bugs. + return Description.NO_MATCH; + } } /* Then apply them. */ applyMatches(matches, ((JCCompilationUnit) tree).endPositions, state); @@ -109,6 +145,190 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s return Description.NO_MATCH; } + private static List> getRefasterRules(ErrorProneFlags flags) { + CodeTransformer compositeCodeTransformer = createCompositeCodeTransformer(flags); + + List> refasterRules = new ArrayList<>(); + collectRefasterRules(compositeCodeTransformer, refasterRules::add); + return refasterRules; + } + + private static void collectRefasterRules( + CodeTransformer transformer, Consumer> sink) { + if (transformer instanceof RefasterRule) { + sink.accept((RefasterRule) transformer); + } else if (transformer instanceof CompositeCodeTransformer) { + for (CodeTransformer t : ((CompositeCodeTransformer) transformer).transformers()) { + collectRefasterRules(t, sink); + } + } else { + throw new IllegalStateException( + String.format("Can't handle `CodeTransformer` of type '%s'", transformer.getClass())); + } + } + + private static ImmutableSet> extractTemplateIdentifiers( + RefasterRule refasterRule) { + ImmutableSet.Builder> results = ImmutableSet.builder(); + + for (Object template : refasterRule.beforeTemplates()) { + if (template instanceof ExpressionTemplate) { + UExpression expr = ((ExpressionTemplate) template).expression(); + results.addAll(extractTemplateIdentifiers(ImmutableList.of(expr))); + } else if (template instanceof BlockTemplate) { + ImmutableList statements = ((BlockTemplate) template).templateStatements(); + results.addAll(extractTemplateIdentifiers(statements)); + } else { + throw new IllegalStateException( + String.format("Unexpected template type '%s'", template.getClass())); + } + } + + return results.build(); + } + + // XXX: Consider interning the strings (once a benchmark is in place). + private static ImmutableSet> extractTemplateIdentifiers( + ImmutableList trees) { + List> identifierCombinations = new ArrayList<>(); + identifierCombinations.add(new HashSet<>()); + + // XXX: Make the scanner static, then make also its helper methods static. + new TreeScanner>>() { + @Override + public Void visitIdentifier(IdentifierTree node, List> identifierCombinations) { + // XXX: Also include the package name if not `java.lang`; it must be present. + if (node instanceof UClassIdent) { + for (Set ids : identifierCombinations) { + ids.add(getSimpleName(((UClassIdent) node).getTopLevelClass())); + ids.add(getIdentifier(node)); + } + } else if (node instanceof UStaticIdent) { + UClassIdent subNode = ((UStaticIdent) node).classIdent(); + for (Set ids : identifierCombinations) { + ids.add(getSimpleName(subNode.getTopLevelClass())); + ids.add(getIdentifier(subNode)); + ids.add(node.getName().toString()); + } + } + + return null; + } + + private String getIdentifier(IdentifierTree tree) { + return getSimpleName(tree.getName().toString()); + } + + private String getSimpleName(String fcqn) { + int index = fcqn.lastIndexOf('.'); + 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) { + super.visitMemberReference(node, identifierCombinations); + String id = node.getName().toString(); + identifierCombinations.forEach(ids -> ids.add(id)); + return null; + } + + @Override + public Void visitMemberSelect( + MemberSelectTree node, List> identifierCombinations) { + super.visitMemberSelect(node, identifierCombinations); + String id = node.getIdentifier().toString(); + identifierCombinations.forEach(ids -> ids.add(id)); + return null; + } + + @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; + } + + @Override + public Void visitUnary(UnaryTree node, List> identifierCombinations) { + super.visitUnary(node, identifierCombinations); + identifierCombinations.forEach(ids -> ids.add(Util.treeKindToString(node.getKind()))); + return null; + } + + private List> copy(List> identifierCombinations) { + return identifierCombinations.stream() + .map(HashSet::new) + .collect(toCollection(ArrayList::new)); + } + }.scan(trees, identifierCombinations); + + return identifierCombinations.stream() + .map(ImmutableSortedSet::copyOf) + .collect(toImmutableSet()); + } + + // XXX: Consider interning! + private static ImmutableSortedSet extractSourceIdentifiers(Tree tree) { + Set identifiers = new HashSet<>(); + + // XXX: Make the scanner static. + new TreeScanner>() { + @Override + public Void visitIdentifier(IdentifierTree node, Set identifiers) { + identifiers.add(node.getName().toString()); + return null; + } + + @Override + public Void visitMemberReference(MemberReferenceTree node, Set identifiers) { + super.visitMemberReference(node, identifiers); + identifiers.add(node.getName().toString()); + return null; + } + + @Override + public Void visitMemberSelect(MemberSelectTree node, Set identifiers) { + super.visitMemberSelect(node, identifiers); + identifiers.add(node.getIdentifier().toString()); + return null; + } + + @Override + public Void visitBinary(BinaryTree node, Set identifiers) { + super.visitBinary(node, identifiers); + identifiers.add(Util.treeKindToString(node.getKind())); + return null; + } + + @Override + public Void visitUnary(UnaryTree node, Set identifiers) { + super.visitUnary(node, identifiers); + identifiers.add(Util.treeKindToString(node.getKind())); + return null; + } + }.scan(tree, identifiers); + + return ImmutableSortedSet.copyOf(identifiers); + } + /** * Reports a subset of the given matches, such that no two reported matches suggest a replacement * of the same part of the source code. @@ -161,6 +381,7 @@ private static Stream getReplacements( return description.fixes.stream().flatMap(fix -> fix.getReplacements(endPositions).stream()); } + // XXX: Instead create an `ImmutableList` private static CodeTransformer createCompositeCodeTransformer(ErrorProneFlags flags) { ImmutableListMultimap allTransformers = ALL_CODE_TRANSFORMERS.get(); return CompositeCodeTransformer.compose( 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 1782adc18a..a92a7b822c 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 @@ -17,4 +17,67 @@ static String treeToString(Tree tree, VisitorState state) { String src = state.getSourceForNode(tree); return src != null ? src : tree.toString(); } + + /** + * Returns a string representation of the given {@link Tree.Kind}. + * + * @return A string representation of the operator, defaults to an empty string. + */ + // XXX: List needs to be extended, as it currently only supports `BinaryTree`s and `UnaryTree`s. + static String treeKindToString(Tree.Kind kind) { + switch (kind) { + case POSTFIX_INCREMENT: + case PREFIX_INCREMENT: + return "++"; + case POSTFIX_DECREMENT: + case PREFIX_DECREMENT: + return "--"; + case UNARY_PLUS: + case PLUS: + return "+"; + case UNARY_MINUS: + case MINUS: + return "-"; + case BITWISE_COMPLEMENT: + return "~"; + case LOGICAL_COMPLEMENT: + return "!"; + case MULTIPLY: + return "*"; + case DIVIDE: + return "/"; + case REMAINDER: + return "%"; + case LEFT_SHIFT: + return "<<"; + case RIGHT_SHIFT: + return ">>"; + case UNSIGNED_RIGHT_SHIFT: + return ">>>"; + case LESS_THAN: + return "<"; + case GREATER_THAN: + return ">"; + case LESS_THAN_EQUAL: + return "<="; + case GREATER_THAN_EQUAL: + return ">="; + case EQUAL_TO: + return "=="; + case NOT_EQUAL_TO: + return "!="; + case AND: + return "&"; + case XOR: + return "^"; + case OR: + return "|"; + case CONDITIONAL_AND: + return "&&"; + case CONDITIONAL_OR: + return "||"; + default: + throw new IllegalStateException("Cannot convert Tree.Kind to a String: " + kind); + } + } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterCheckTest.java index c19513ad76..ccaa263437 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterCheckTest.java @@ -2,7 +2,6 @@ import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; import static java.util.function.Function.identity; -import static java.util.function.Predicate.not; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.junit.jupiter.params.provider.Arguments.arguments; @@ -89,7 +88,9 @@ public final class RefasterCheckTest { @SuppressWarnings("UnusedMethod" /* Used as a `@MethodSource`. */) private static Stream templateGroupsUnderTest() { // XXX: Drop the filter once we have added tests for AssertJ! - return TEMPLATES_BY_GROUP.keySet().stream().filter(not("AssertJ"::equals)).map(Arguments::of); + // return + // TEMPLATES_BY_GROUP.keySet().stream().filter(not("AssertJ"::equals)).map(Arguments::of); + return TEMPLATES_BY_GROUP.keySet().stream().filter("LongStream"::equals).map(Arguments::of); } /** @@ -99,7 +100,8 @@ private static Stream templateGroupsUnderTest() { private static Stream templatesUnderTest() { // XXX: Drop the filter once we have added tests for AssertJ! return TEMPLATES_BY_GROUP.entries().stream() - .filter(e -> !"AssertJ".equals(e.getKey())) + // .filter(e -> !"AssertJ".equals(e.getKey())) + .filter(e -> "LongStream".equals(e.getKey())) .map(e -> arguments(e.getKey(), e.getValue())); } diff --git a/pom.xml b/pom.xml index d7134064da..23db7d0a44 100644 --- a/pom.xml +++ b/pom.xml @@ -752,6 +752,11 @@ 3.10.1 + + com.google.auto.value + auto-value + ${version.auto-value} + com.google.auto.service auto-service @@ -1484,6 +1489,11 @@ Prone bug pattern checkers, so we enable all and then selectively deactivate some. --> -XepAllDisabledChecksAsWarnings + + -XepDisableWarningsInGeneratedCode -Xep:AndroidJdkLibsChecker:OFF