Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up Refaster bug checker #261

Draft
wants to merge 55 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
69386f0
Emit website link along with Refaster refactor suggestions
Stephan202 Sep 21, 2022
84061fc
More extensible approach
Stephan202 Sep 22, 2022
827880b
WIP: Some plumbing for annotation support
Stephan202 Sep 22, 2022
0f44844
Another round
Stephan202 Sep 22, 2022
0e46f9c
Tweaks
Stephan202 Sep 23, 2022
914d30a
Better annotation support, simplify setup
Stephan202 Sep 23, 2022
458fb99
Flag why build currently doesn't fail, while it should.
Stephan202 Sep 23, 2022
db8cf3c
Improve logic and test coverage
Stephan202 Sep 24, 2022
abb6cea
Properly document URL placeholder usage
Stephan202 Sep 24, 2022
cf772c4
Expand test coverage
Stephan202 Sep 24, 2022
d26bc18
Flag classpath issue
Stephan202 Sep 25, 2022
01b7e5b
Improve text and minor improvements
rickie Sep 27, 2022
3482641
Use `@AutoValue` for the `AnnotatedCompositeCodeTransformer`
rickie Sep 29, 2022
12d09ad
Support `AllSuggestionsAsWarnings` and add a suggestion
rickie Sep 29, 2022
74d6c9a
Tweak
rickie Sep 29, 2022
c4b6a5f
Suggestions, introduce `ErrorProneFork`
Stephan202 Sep 29, 2022
d899a8a
Also this
Stephan202 Sep 29, 2022
20d194b
Clarify how the default Refaster hit severity comes to be
Stephan202 Sep 30, 2022
ad1c98d
Align documentation of reported description names by the Refaster check
Badbond Sep 30, 2022
50443d1
Suggestion
Badbond Sep 30, 2022
7e49b08
Pass null for urlLink to Description.Builder
Badbond Sep 30, 2022
594f51c
Tweaks
Stephan202 Sep 30, 2022
e6caceb
Move `AnnotatedCompositeCodeTransformer` and `ErrorProneFork` to `ref…
rickie Oct 4, 2022
b2ef631
`s/information/content/`
rickie Oct 4, 2022
8bd88bb
Improve Javadoc `AnnotatedCompositeCodeTransformer`
rickie Oct 4, 2022
302e20b
Revert changes in `OptionalTemplates`
rickie Oct 4, 2022
32300ff
Tweak `AnnotatedCompositeCodeTransformer` Javadoc
Badbond Oct 4, 2022
d21ac59
Apply `StringJoin` suggestion
Stephan202 Oct 7, 2022
0484762
Suggestions
Stephan202 Oct 8, 2022
1624ebf
WIP: Speed up `Refaster` check
Stephan202 May 1, 2022
ee74279
Compatibility with stock Error Prone
Stephan202 Sep 12, 2022
344f4e4
Initial copy over of the improved algorithm
rickie Sep 23, 2022
6739cb4
Improve code and algorithm for Refaster
rickie Sep 23, 2022
72ff8ae
Drop unnecessary `@AutoService` annotation
rickie Sep 23, 2022
39dc9aa
Add XXXs
rickie Sep 23, 2022
2a93011
Merge `RefasterRuleSelector` type hierarchy
Stephan202 Sep 25, 2022
1c5077f
Create selector only once per `Refaster` instantiation
Stephan202 Sep 25, 2022
2c137c0
Move all `RefasterRuleSelector` construction logic into the relevant …
Stephan202 Sep 25, 2022
c3a5106
Push sorting requirements into `Node`, minimize tree, add tests
Stephan202 Sep 25, 2022
9151287
Suggestion
rickie Sep 27, 2022
a27d635
Extract the `TreeScanner`s to their own classes
rickie Sep 27, 2022
2003bd2
Introduce `getClass` method to deduplicate
rickie Sep 27, 2022
7889148
Reorder methods in `RefasterIntrospection`
rickie Sep 27, 2022
afdcf52
Apply some suggestions
Stephan202 Sep 30, 2022
ab03739
Optimize code, introduce benchmark, simplify test
Stephan202 Oct 1, 2022
457a8e2
Comment style, explain both performance-only pieces of code
Stephan202 Oct 2, 2022
63ad14e
Fix typo
rickie Oct 4, 2022
0cf891c
Suggestions
ibabiankou Oct 7, 2022
2cb4bd0
Suggestions
Stephan202 Oct 8, 2022
c34fa4d
Fix typo
rickie Oct 9, 2022
d59b626
Merge remote-tracking branch 'origin/sschroevers/refaster-custom-urls…
Stephan202 Oct 9, 2022
5ba7075
Introduce `AnnotatedCompositeCodeTransformerTest`
Stephan202 Oct 9, 2022
0157692
Merge remote-tracking branch 'origin/sschroevers/refaster-custom-urls…
Stephan202 Oct 9, 2022
72f8881
Suggestions
Stephan202 Oct 9, 2022
77bc107
Merge branch 'master' into sschroevers/refaster-speed-up
Stephan202 Dec 26, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1296,9 +1296,6 @@
<threads>4</threads>
<timeoutFactor>4</timeoutFactor>
<timestampedReports>false</timestampedReports>
<excludedClasses>
<excludedClass>*.AutoValue_*</excludedClass>
</excludedClasses>
</configuration>
<dependencies>
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Maps;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -27,9 +26,9 @@ abstract class Node<T> {
// there be such an overload?
static <T> Node<T> create(
Set<T> values, Function<? super T, ? extends Set<? extends Set<String>>> pathExtractor) {
BuildNode<T> tree = BuildNode.create();
Builder<T> tree = Builder.create();
tree.register(values, pathExtractor);
return tree.immutable();
return tree.build();
}

abstract ImmutableMap<String, Node<T>> children();
Expand Down Expand Up @@ -75,12 +74,12 @@ private void collectReachableValues(ImmutableList<String> candidateEdges, Consum

@AutoValue
@SuppressWarnings("AutoValueImmutableFields" /* Type is used only during `Node` construction. */)
abstract static class BuildNode<T> {
private static <T> BuildNode<T> create() {
return new AutoValue_Node_BuildNode<>(new HashMap<>(), new ArrayList<>());
abstract static class Builder<T> {
private static <T> Builder<T> create() {
return new AutoValue_Node_Builder<>(new HashMap<>(), new ArrayList<>());
}

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

abstract List<T> values();

Expand All @@ -98,7 +97,7 @@ private void register(
* We sort paths by length ascending, so that in case of two paths where one is an initial
* prefix of the other, only the former is encoded (thus saving some space).
*/
Collections.sort(paths, comparingInt(Set::size));
paths.sort(comparingInt(Set::size));
paths.forEach(path -> registerPath(value, ImmutableList.sortedCopyOf(path)));
}
}
Expand All @@ -118,9 +117,9 @@ private void registerPath(T value, ImmutableList<String> path) {
}
}

private Node<T> immutable() {
private Node<T> build() {
return new AutoValue_Node<>(
ImmutableMap.copyOf(Maps.transformValues(children(), BuildNode::immutable)),
ImmutableMap.copyOf(Maps.transformValues(children(), Builder::build)),
ImmutableList.copyOf(values()));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,6 @@ public Refaster(ErrorProneFlags flags) {
public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
Set<CodeTransformer> candidateTransformers = ruleSelector.selectCandidateRules(tree);

// XXX: Remove these debug lines
// String removeThis = candidateRules.stream().map(Object::toString).collect(joining(","));
// System.out.printf("\n---Templates for %s: \n%s\n", tree.getSourceFile().getName(),
// removeThis);

/* First, collect all matches. */
SubContext context = new SubContext(state.context);
List<Description> matches = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ private static void collectRuleIdentifiers(
}
}

// XXX: Decompose `RefasterRule`s such that each rule has exactly one `@BeforeTemplate`.
// XXX: Consider decomposing `RefasterRule`s such that each rule has exactly one
// `@BeforeTemplate`.
private static ImmutableSet<ImmutableSet<String>> extractRuleIdentifiers(
RefasterRule<?, ?> refasterRule) {
ImmutableSet.Builder<ImmutableSet<String>> results = ImmutableSet.builder();
Expand Down Expand Up @@ -285,13 +286,7 @@ private static String treeKindToString(Tree.Kind kind) {

private static final class RefasterIntrospection {
private static final String UCLASS_IDENT_FQCN = "com.google.errorprone.refaster.UClassIdent";
// XXX: Probably there is a better way to fix this... For a few BeforeTemplates like
// `ImmutableMapBuilder` the algorithm wouldn't match so created this fix for now. About 10
// templates would always match.
private static final String AUTO_VALUE_UCLASS_IDENT_FQCN =
"com.google.errorprone.refaster.AutoValue_UClassIdent";
private static final Class<?> UCLASS_IDENT = getClass(UCLASS_IDENT_FQCN);
private static final Class<?> UCLASS_AUTOVALUE_IDENT = getClass(AUTO_VALUE_UCLASS_IDENT_FQCN);
private static final Method METHOD_REFASTER_RULE_BEFORE_TEMPLATES =
getMethod(RefasterRule.class, "beforeTemplates");
private static final Method METHOD_EXPRESSION_TEMPLATE_EXPRESSION =
Expand All @@ -305,7 +300,7 @@ private static final class RefasterIntrospection {
private static final Method METHOD_UANY_OF_EXPRESSIONS = getMethod(UAnyOf.class, "expressions");

static boolean isUClassIdent(IdentifierTree tree) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the full word.

Suggested change
static boolean isUClassIdent(IdentifierTree tree) {
static boolean isUClassIdentifier(IdentifierTree tree) {

return UCLASS_IDENT.equals(tree.getClass()) || UCLASS_AUTOVALUE_IDENT.equals(tree.getClass());
return UCLASS_IDENT.isAssignableFrom(tree.getClass());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @Stephan202, this is way better haha.

}

static ImmutableList<?> getBeforeTemplates(RefasterRule<?, ?> refasterRule) {
Expand All @@ -320,12 +315,11 @@ static ImmutableList<UStatement> getTemplateStatements(BlockTemplate template) {
return invokeMethod(METHOD_BLOCK_TEMPLATE_TEMPLATE_STATEMENTS, template);
}

// Actually UClassIdent.
static IdentifierTree getClassIdent(UStaticIdent tree) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static IdentifierTree getClassIdent(UStaticIdent tree) {
static IdentifierTree getClassIdentifier(UStaticIdent tree) {

return invokeMethod(METHOD_USTATIC_IDENT_CLASS_IDENT, tree);
}

// XXX: Make nicer. Or rename the other params.
// Arguments to this method must actually be of the package-private type `UClassIdent`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/to/of?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One passes arguments to a method. (Corresponding to the parameters of said method. But here we are talking about the provided argument; the comment is here precisely because the parameter type isn't specific enough.)

static String getTopLevelClass(IdentifierTree uClassIdent) {
return invokeMethod(METHOD_UCLASS_IDENT_GET_TOP_LEVEL_CLASS, uClassIdent);
}
Expand Down