Skip to content

Commit

Permalink
Suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
Stephan202 authored and rickie committed Jun 2, 2022
1 parent 8f86625 commit 17bede8
Show file tree
Hide file tree
Showing 4 changed files with 187 additions and 129 deletions.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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.
*
* <p>The tree's edges are string-labeled, while its leaves store values of type {@code T}.
*/
@AutoValue
abstract class Node<T> {
static <T> Node<T> create(Map<String, Node<T>> children, ImmutableSet<T> candidateRules) {
return new AutoValue_Node<>(ImmutableMap.copyOf(children), candidateRules);
static <T> Node<T> create(Map<String, Node<T>> children, ImmutableList<T> values) {
return new AutoValue_Node<>(ImmutableSortedMap.copyOf(children), values);
}

public abstract ImmutableMap<String, Node<T>> children();

public abstract ImmutableSet<T> candidateRules();

static <C> Node<C> createRefasterTemplateTree(
List<C> refasterRules, Function<C, ImmutableSet<ImmutableSortedSet<String>>> edgeExtractor) {
// XXX: Improve this method...
List<ImmutableSet<ImmutableSortedSet<String>>> beforeTemplateIdentifiers =
refasterRules.stream().map(edgeExtractor).collect(toImmutableList());

BuildNode<C> tree = BuildNode.create();
for (int i = 0; i < refasterRules.size(); i++) {
tree.register(beforeTemplateIdentifiers.get(i), refasterRules.get(i));
}
static <T> Node<T> create(
List<T> values, Function<T, ImmutableSet<ImmutableSortedSet<String>>> pathExtractor) {
BuildNode<T> tree = BuildNode.create();
tree.register(values, pathExtractor);
return tree.immutable();
}

void collectCandidateTemplates(ImmutableList<String> sourceIdentifiers, Consumer<T> sink) {
candidateRules().forEach(sink);
abstract ImmutableMap<String, Node<T>> children();

if (sourceIdentifiers.isEmpty() || children().isEmpty()) {
abstract ImmutableList<T> values();

void collectCandidateTemplates(ImmutableList<String> candidateEdges, Consumer<T> sink) {
values().forEach(sink);

if (candidateEdges.isEmpty() || children().isEmpty()) {
return;
}

if (children().size() < sourceIdentifiers.size()) {
if (children().size() < candidateEdges.size()) {
for (Map.Entry<String, Node<T>> 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<String> remainingSourceCandidateEdges =
sourceIdentifiers.subList(1, sourceIdentifiers.size());
Node<T> child = children().get(sourceIdentifiers.get(0));
ImmutableList<String> remainingCandidateEdges =
candidateEdges.subList(1, candidateEdges.size());
Node<T> 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<T> {
static <T> BuildNode<T> create() {
return new AutoValue_Node_BuildNode<>(new HashMap<>(), new ArrayList<>());
}

abstract Map<String, BuildNode<T>> children();

abstract List<T> values();

private void register(
List<T> values, Function<T, ImmutableSet<ImmutableSortedSet<String>>> pathsExtractor) {
for (T value : values) {
for (ImmutableSet<String> path : pathsExtractor.apply(value)) {
registerPath(value, path.asList());
}
}
collectCandidateTemplates(remainingSourceCandidateEdges, sink);
}

private void registerPath(T value, ImmutableList<String> path) {
path.stream()
.findFirst()
.ifPresentOrElse(
edge ->
children()
.computeIfAbsent(edge, k -> BuildNode.create())
.registerPath(value, path.subList(1, path.size())),
() -> values().add(value));
}

private Node<T> immutable() {
return Node.create(
Maps.transformValues(children(), BuildNode::immutable), ImmutableList.copyOf(values()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -92,7 +98,7 @@ public final class RefasterCheck extends BugChecker implements CompilationUnitTr
static final Supplier<ImmutableListMultimap<String, CodeTransformer>> ALL_CODE_TRANSFORMERS =
Suppliers.memoize(RefasterCheck::loadAllCodeTransformers);

private final Node<RefasterRule<?, ?>> refasterTemplatesTree;
private final Node<RefasterRule<?, ?>> refasterRules;

/** Instantiates the default {@link RefasterCheck}. */
public RefasterCheck() {
Expand All @@ -105,30 +111,26 @@ public RefasterCheck() {
* @param flags Any provided command line flags.
*/
public RefasterCheck(ErrorProneFlags flags) {
List<RefasterRule<?, ?>> 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<String> sourceIdentifiers = extractSourceIdentifiers(tree);

HashSet<RefasterRule<?, ?>> candidateRules = new HashSet<>();
refasterTemplatesTree.collectCandidateTemplates(
sourceIdentifiers.asList(), candidateRules::add);
// XXX: Inline this variable.
Set<RefasterRule<?, ?>> candidateRules = getCandidateRefasterRules(tree);

// 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. */
SubContext context = new SubContext(state.context);
List<Description> 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.
Expand All @@ -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<RefasterRule<?, ?>> getCandidateRefasterRules(CompilationUnitTree tree) {
Set<RefasterRule<?, ?>> candidateRules = newSetFromMap(new IdentityHashMap<>());
refasterRules.collectCandidateTemplates(
extractSourceIdentifiers(tree).asList(), candidateRules::add);

return candidateRules;
}

private static List<RefasterRule<?, ?>> getRefasterRules(ErrorProneFlags flags) {
CodeTransformer compositeCodeTransformer = createCompositeCodeTransformer(flags);

Expand All @@ -166,6 +177,7 @@ private static void collectRefasterRules(
}
}

// XXX: Decompose `RefasterRule`s such that each has exactly one `@BeforeTemplate`.
private static ImmutableSet<ImmutableSortedSet<String>> extractTemplateIdentifiers(
RefasterRule<?, ?> refasterRule) {
ImmutableSet.Builder<ImmutableSortedSet<String>> results = ImmutableSet.builder();
Expand Down Expand Up @@ -223,22 +235,6 @@ private String getSimpleName(String fcqn) {
return index < 0 ? fcqn : fcqn.substring(index + 1);
}

@Override
public Void visitOther(Tree node, List<Set<String>> identifierCombinations) {
if (node instanceof UAnyOf) {
List<Set<String>> base = copy(identifierCombinations);
identifierCombinations.clear();

for (UExpression expr : ((UAnyOf) node).expressions()) {
List<Set<String>> branch = copy(base);
scan(expr, branch);
identifierCombinations.addAll(branch);
}
}

return null;
}

@Override
public Void visitMemberReference(
MemberReferenceTree node, List<Set<String>> identifierCombinations) {
Expand All @@ -258,17 +254,48 @@ public Void visitMemberSelect(
}

@Override
public Void visitBinary(BinaryTree node, List<Set<String>> 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<Set<String>> identifierCombinations) {
registerOperator(node, identifierCombinations);
return super.visitAssignment(node, identifierCombinations);
}

@Override
public Void visitCompoundAssignment(
CompoundAssignmentTree node, List<Set<String>> identifierCombinations) {
registerOperator(node, identifierCombinations);
return super.visitCompoundAssignment(node, identifierCombinations);
}

@Override
public Void visitUnary(UnaryTree node, List<Set<String>> identifierCombinations) {
super.visitUnary(node, identifierCombinations);
registerOperator(node, identifierCombinations);
return super.visitUnary(node, identifierCombinations);
}

@Override
public Void visitBinary(BinaryTree node, List<Set<String>> identifierCombinations) {
registerOperator(node, identifierCombinations);
return super.visitBinary(node, identifierCombinations);
}

// XXX: Rename!
private void registerOperator(ExpressionTree node, List<Set<String>> identifierCombinations) {
identifierCombinations.forEach(ids -> ids.add(Util.treeKindToString(node.getKind())));
}

@Override
public Void visitOther(Tree node, List<Set<String>> identifierCombinations) {
if (node instanceof UAnyOf) {
List<Set<String>> base = copy(identifierCombinations);
identifierCombinations.clear();

for (UExpression expr : ((UAnyOf) node).expressions()) {
List<Set<String>> branch = copy(base);
scan(expr, branch);
identifierCombinations.addAll(branch);
}
}

return null;
}

Expand Down Expand Up @@ -311,17 +338,32 @@ public Void visitMemberSelect(MemberSelectTree node, Set<String> identifiers) {
}

@Override
public Void visitBinary(BinaryTree node, Set<String> identifiers) {
super.visitBinary(node, identifiers);
identifiers.add(Util.treeKindToString(node.getKind()));
return null;
public Void visitAssignment(AssignmentTree node, Set<String> identifiers) {
registerOperator(node, identifiers);
return super.visitAssignment(node, identifiers);
}

@Override
public Void visitCompoundAssignment(CompoundAssignmentTree node, Set<String> identifiers) {
registerOperator(node, identifiers);
return super.visitCompoundAssignment(node, identifiers);
}

@Override
public Void visitUnary(UnaryTree node, Set<String> identifiers) {
super.visitUnary(node, identifiers);
registerOperator(node, identifiers);
return super.visitUnary(node, identifiers);
}

@Override
public Void visitBinary(BinaryTree node, Set<String> identifiers) {
registerOperator(node, identifiers);
return super.visitBinary(node, identifiers);
}

// XXX: Rename!
private void registerOperator(ExpressionTree node, Set<String> identifiers) {
identifiers.add(Util.treeKindToString(node.getKind()));
return null;
}
}.scan(tree, identifiers);

Expand Down
Loading

0 comments on commit 17bede8

Please sign in to comment.