From 69386f0b3d7410e415056bb1c60f5a17f518794a Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 21 Sep 2022 22:15:36 +0200 Subject: [PATCH 01/53] Emit website link along with Refaster refactor suggestions --- .../bugpatterns/MethodReferenceUsage.java | 3 ++- .../errorprone/refaster/runner/Refaster.java | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java index 437fb6db1f..0bcd71fee7 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java @@ -150,7 +150,8 @@ private static Optional constructMethodRef( } Type lhsType = ASTHelpers.getType(subTree.getExpression()); - if (lhsType == null || !expectedInstance.orElseThrow().equals(lhs)) { + // XXX: Using `.get()` instead of `.orElseThrows()` for demonstration purposes. + if (lhsType == null || !expectedInstance.get().equals(lhs)) { return Optional.empty(); } diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 118475dbea..98450f30bd 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -118,12 +118,28 @@ private static void applyMatches( ImmutableRangeSet ranges = getReplacementRanges(description, endPositions); if (ranges.asRanges().stream().noneMatch(replacedSections::intersects)) { /* This suggested fix does not overlap with any ("larger") replacement seen until now. Apply it. */ - state.reportMatch(description); + state.reportMatch(withCustomLink(description)); replacedSections.addAll(ranges); } } } + // XXX: Move method down. + // XXX: Give method better name. + // XXX: Make URL configurable using custom annotation. + // XXX: Revisit URL format. Right now this produces + // https://error-prone.picnic.tech/refastertemplates/tech.picnic.errorprone.refastertemplates.OptionalTemplates.OptionalOrElseThrow + private static Description withCustomLink(Description description) { + return Description.builder( + description.position, + description.checkName, + "https://error-prone.picnic.tech/refastertemplates/" + description.checkName, + description.severity, + description.getMessage()) + .addAllFixes(description.fixes) + .build(); + } + private static int getReplacedCodeSize(Description description, EndPosTable endPositions) { return getReplacements(description, endPositions).mapToInt(Replacement::length).sum(); } From 84061fcd069b690548f3b1863bb7055fb153f8fa Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 22 Sep 2022 09:55:23 +0200 Subject: [PATCH 02/53] More extensible approach --- .../refaster/runner/CodeTransformers.java | 53 ++++++++++++++++++- .../errorprone/refaster/runner/Refaster.java | 18 +------ 2 files changed, 53 insertions(+), 18 deletions(-) diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java index f3bde9fd0c..f264ebb85b 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java @@ -1,15 +1,21 @@ package tech.picnic.errorprone.refaster.runner; import com.google.common.base.Suppliers; +import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; import com.google.common.reflect.ClassPath; import com.google.common.reflect.ClassPath.ResourceInfo; import com.google.errorprone.CodeTransformer; +import com.google.errorprone.DescriptionListener; +import com.google.errorprone.matchers.Description; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.util.Context; import java.io.IOException; import java.io.InputStream; import java.io.ObjectInputStream; import java.io.UncheckedIOException; +import java.lang.annotation.Annotation; import java.util.NoSuchElementException; import java.util.Optional; import java.util.function.Supplier; @@ -51,7 +57,12 @@ private static ImmutableListMultimap loadAllCodeTransfo .ifPresent( templateName -> loadCodeTransformer(resource) - .ifPresent(transformer -> transformers.put(templateName, transformer))); + .ifPresent( + transformer -> + transformers.put( + templateName, + new CodeTransformerDescriptionAdapter( + templateName, transformer)))); } return transformers.build(); @@ -95,4 +106,44 @@ private static Optional loadCodeTransformer(ResourceInfo resour throw new IllegalStateException("Can't load `CodeTransformer` from " + resource, e); } } + + // XXX: Move to separate file? + // XXX: Can we find a better class name? + private static final class CodeTransformerDescriptionAdapter implements CodeTransformer { + private final String name; + private final CodeTransformer delegate; + + private CodeTransformerDescriptionAdapter(String name, CodeTransformer delegate) { + this.name = name; + this.delegate = delegate; + } + + @Override + public void apply(TreePath path, Context context, DescriptionListener listener) { + delegate.apply( + path, context, description -> listener.onDescribed(augmentDescription(description))); + } + + @Override + public ImmutableClassToInstanceMap annotations() { + return delegate.annotations(); + } + + private Description augmentDescription(Description description) { + // XXX: Make this configurable based on a Refaster annotation. (E.g. by allowing users to + // specify an optional URL pattern.) + // XXX: Replace only the first `$`. + // XXX: Review URL format. Currently produced format: + // https://error-prone.picnic.tech/refastertemplates/OptionalTemplates#OptionalOrElseThrow + // XXX: Test this. + return Description.builder( + description.position, + description.checkName, + "https://error-prone.picnic.tech/refastertemplates/" + name.replace('$', '#'), + description.severity, + "Refactoring opportunity") + .addAllFixes(description.fixes) + .build(); + } + } } diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 98450f30bd..118475dbea 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -118,28 +118,12 @@ private static void applyMatches( ImmutableRangeSet ranges = getReplacementRanges(description, endPositions); if (ranges.asRanges().stream().noneMatch(replacedSections::intersects)) { /* This suggested fix does not overlap with any ("larger") replacement seen until now. Apply it. */ - state.reportMatch(withCustomLink(description)); + state.reportMatch(description); replacedSections.addAll(ranges); } } } - // XXX: Move method down. - // XXX: Give method better name. - // XXX: Make URL configurable using custom annotation. - // XXX: Revisit URL format. Right now this produces - // https://error-prone.picnic.tech/refastertemplates/tech.picnic.errorprone.refastertemplates.OptionalTemplates.OptionalOrElseThrow - private static Description withCustomLink(Description description) { - return Description.builder( - description.position, - description.checkName, - "https://error-prone.picnic.tech/refastertemplates/" + description.checkName, - description.severity, - description.getMessage()) - .addAllFixes(description.fixes) - .build(); - } - private static int getReplacedCodeSize(Description description, EndPosTable endPositions) { return getReplacements(description, endPositions).mapToInt(Replacement::length).sum(); } From 827880bc454921a1bf64020c86db2e6c7851a750 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 22 Sep 2022 18:05:36 +0200 Subject: [PATCH 03/53] WIP: Some plumbing for annotation support To be used for custom links, custom error messages, custom other stuff... --- refaster-compiler/pom.xml | 4 ++ .../RefasterRuleCompilerTaskListener.java | 52 ++++++++++++------- refaster-runner/pom.xml | 4 ++ .../refaster/runner/CodeTransformers.java | 7 +++ .../AnnotatedCompositeCodeTransformer.java | 37 +++++++++++++ .../annotation/TemplateCollection.java | 12 +++++ refaster-test-support/pom.xml | 4 ++ .../test/MatchInWrongMethodTemplates.java | 4 ++ 8 files changed, 104 insertions(+), 20 deletions(-) create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java diff --git a/refaster-compiler/pom.xml b/refaster-compiler/pom.xml index 049521617d..9a6fbd9798 100644 --- a/refaster-compiler/pom.xml +++ b/refaster-compiler/pom.xml @@ -27,6 +27,10 @@ ${groupId.error-prone} error_prone_core + + ${project.groupId} + refaster-support + com.google.auto.service auto-service-annotations diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java index e2940b1422..f7e5f42693 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java @@ -1,12 +1,11 @@ package tech.picnic.errorprone.refaster.plugin; -import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.ImmutableListMultimap; -import com.google.common.collect.ListMultimap; -import com.google.common.collect.Multimaps; +import com.google.common.collect.ImmutableClassToInstanceMap; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.errorprone.CodeTransformer; -import com.google.errorprone.CompositeCodeTransformer; import com.google.errorprone.refaster.RefasterRuleBuilderScanner; +import com.google.errorprone.refaster.UTemplater; import com.google.errorprone.refaster.annotation.BeforeTemplate; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; @@ -26,12 +25,13 @@ import java.io.ObjectOutput; import java.io.ObjectOutputStream; import java.io.UncheckedIOException; -import java.util.List; +import java.lang.annotation.Annotation; import java.util.Map; import javax.annotation.Nullable; import javax.tools.FileObject; import javax.tools.JavaFileManager; import javax.tools.StandardLocation; +import tech.picnic.errorprone.refaster.AnnotatedCompositeCodeTransformer; /** * A variant of {@code com.google.errorprone.refaster.RefasterRuleCompilerAnalyzer} that stores @@ -58,10 +58,10 @@ public void finished(TaskEvent taskEvent) { return; } - ImmutableListMultimap rules = compileRefasterTemplates(tree); - for (Map.Entry> rule : Multimaps.asMap(rules).entrySet()) { + ImmutableMap rules = compileRefasterTemplates(tree); + for (Map.Entry rule : rules.entrySet()) { try { - outputCodeTransformers(rule.getValue(), getOutputFile(taskEvent, rule.getKey())); + outputCodeTransformer(rule.getValue(), getOutputFile(taskEvent, rule.getKey())); } catch (IOException e) { throw new UncheckedIOException("Failed to persist compiled Refaster templates", e); } @@ -87,18 +87,30 @@ public Boolean reduce(Boolean r1, Boolean r2) { }.scan(tree, null)); } - private ImmutableListMultimap compileRefasterTemplates( - ClassTree tree) { - ListMultimap rules = ArrayListMultimap.create(); - new TreeScanner() { + private ImmutableMap compileRefasterTemplates(ClassTree tree) { + ImmutableMap.Builder rules = ImmutableMap.builder(); + new TreeScanner>() { @Nullable @Override - public Void visitClass(ClassTree node, @Nullable Void v) { - rules.putAll(node, RefasterRuleBuilderScanner.extractRules(node, context)); - return super.visitClass(node, null); + public Void visitClass(ClassTree node, ImmutableClassToInstanceMap annotations) { + ImmutableList transformers = + ImmutableList.copyOf(RefasterRuleBuilderScanner.extractRules(node, context)); + if (!transformers.isEmpty()) { + rules.put(node, new AnnotatedCompositeCodeTransformer(transformers, annotations)); + } + + return super.visitClass( + node, merge(annotations, UTemplater.annotationMap(ASTHelpers.getSymbol(node)))); } - }.scan(tree, null); - return ImmutableListMultimap.copyOf(rules); + }.scan(tree, ImmutableClassToInstanceMap.of()); + return rules.buildOrThrow(); + } + + // XXX: Move down? + private static ImmutableClassToInstanceMap merge( + ImmutableClassToInstanceMap left, ImmutableClassToInstanceMap right) { + // XXX: Handle duplicates! + return ImmutableClassToInstanceMap.builder().putAll(left).putAll(right).build(); } private FileObject getOutputFile(TaskEvent taskEvent, ClassTree tree) throws IOException { @@ -118,10 +130,10 @@ private static CharSequence toSimpleFlatName(ClassSymbol classSymbol) { return lastDot < 0 ? flatName : flatName.subSequence(lastDot + 1, flatName.length()); } - private static void outputCodeTransformers(List rules, FileObject target) + private static void outputCodeTransformer(CodeTransformer codeTransformer, FileObject target) throws IOException { try (ObjectOutput output = new ObjectOutputStream(target.openOutputStream())) { - output.writeObject(CompositeCodeTransformer.compose(rules)); + output.writeObject(codeTransformer); } } } diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index 7bccf546c5..f328b024e0 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -37,6 +37,10 @@ `annotationProcessorPaths` configuration below. --> provided + + ${project.groupId} + refaster-support + com.google.auto.service auto-service-annotations diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java index f264ebb85b..64a56751e0 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java @@ -19,6 +19,7 @@ import java.util.NoSuchElementException; import java.util.Optional; import java.util.function.Supplier; +import tech.picnic.errorprone.refaster.annotation.TemplateCollection; /** * Scans the classpath for {@value #REFASTER_TEMPLATE_SUFFIX} files and loads them as {@link @@ -120,6 +121,12 @@ private CodeTransformerDescriptionAdapter(String name, CodeTransformer delegate) @Override public void apply(TreePath path, Context context, DescriptionListener listener) { + TemplateCollection coll = annotations().getInstance(TemplateCollection.class); + + if (coll != null) { + coll.linkPattern(); + } + delegate.apply( path, context, description -> listener.onDescribed(augmentDescription(description))); } diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java new file mode 100644 index 0000000000..d97d30e1f8 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java @@ -0,0 +1,37 @@ +package tech.picnic.errorprone.refaster; + +import com.google.common.collect.ImmutableClassToInstanceMap; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.CodeTransformer; +import com.google.errorprone.DescriptionListener; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.util.Context; +import java.io.Serializable; +import java.lang.annotation.Annotation; + +// XXX: Move? Rename? Use `@AutoValue`? +// XXX: This is a bit of an implementation detail. Alternative idea: move this to `refaster-runner`, +// and have `refaster-compiler` depend on `refaster-runner`. (Or the other way around?) +public final class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { + private final ImmutableList transformers; + private final ImmutableClassToInstanceMap annotations; + + public AnnotatedCompositeCodeTransformer( + ImmutableList transformers, + ImmutableClassToInstanceMap annotations) { + this.transformers = transformers; + this.annotations = annotations; + } + + @Override + public ImmutableClassToInstanceMap annotations() { + return annotations; + } + + @Override + public void apply(TreePath path, Context context, DescriptionListener listener) { + for (CodeTransformer transformer : transformers) { + transformer.apply(path, context, listener); + } + } +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java new file mode 100644 index 0000000000..a1e3f5109e --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java @@ -0,0 +1,12 @@ +package tech.picnic.errorprone.refaster.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.SOURCE) +public @interface TemplateCollection { + String linkPattern(); +} diff --git a/refaster-test-support/pom.xml b/refaster-test-support/pom.xml index 05960ceff8..10d8722307 100644 --- a/refaster-test-support/pom.xml +++ b/refaster-test-support/pom.xml @@ -40,6 +40,10 @@ ${project.groupId} refaster-runner + + ${project.groupId} + refaster-support + com.google.auto.service auto-service-annotations diff --git a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java index 3e202f17dd..85551d6be2 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java @@ -2,13 +2,17 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import tech.picnic.errorprone.refaster.annotation.TemplateCollection; /** * Refaster template collection to validate reporting of a match occurring in an unexpected place. */ +@TemplateCollection(linkPattern = "XXX") final class MatchInWrongMethodTemplates { private MatchInWrongMethodTemplates() {} + // XXX: Test merging/overriding. + // @TemplateCollection(linkPattern = "XXX") static final class StringIsEmpty { @BeforeTemplate boolean before(String string) { From 0f44844c51a5df57c1813e71d304f8c89af794a4 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 22 Sep 2022 21:46:44 +0200 Subject: [PATCH 04/53] Another round --- .../RefasterRuleCompilerTaskListener.java | 14 ++++- refaster-runner/pom.xml | 4 -- .../refaster/runner/CodeTransformers.java | 60 +----------------- .../AnnotatedCompositeCodeTransformer.java | 63 ++++++++++++++++++- .../annotation/TemplateCollection.java | 14 ++++- refaster-test-support/pom.xml | 1 + .../test/MatchInWrongMethodTemplates.java | 6 +- 7 files changed, 92 insertions(+), 70 deletions(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java index f7e5f42693..6759bb5d34 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java @@ -3,6 +3,7 @@ import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; import com.google.errorprone.CodeTransformer; import com.google.errorprone.refaster.RefasterRuleBuilderScanner; import com.google.errorprone.refaster.UTemplater; @@ -96,7 +97,12 @@ public Void visitClass(ClassTree node, ImmutableClassToInstanceMap a ImmutableList transformers = ImmutableList.copyOf(RefasterRuleBuilderScanner.extractRules(node, context)); if (!transformers.isEmpty()) { - rules.put(node, new AnnotatedCompositeCodeTransformer(transformers, annotations)); + rules.put( + node, + new AnnotatedCompositeCodeTransformer( + String.valueOf(toSimpleFlatName(ASTHelpers.getSymbol(node))), + transformers, + annotations)); } return super.visitClass( @@ -109,8 +115,10 @@ public Void visitClass(ClassTree node, ImmutableClassToInstanceMap a // XXX: Move down? private static ImmutableClassToInstanceMap merge( ImmutableClassToInstanceMap left, ImmutableClassToInstanceMap right) { - // XXX: Handle duplicates! - return ImmutableClassToInstanceMap.builder().putAll(left).putAll(right).build(); + return ImmutableClassToInstanceMap.builder() + .putAll(Maps.filterKeys(left, k -> !right.containsKey(k))) + .putAll(right) + .build(); } private FileObject getOutputFile(TaskEvent taskEvent, ClassTree tree) throws IOException { diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index f328b024e0..7bccf546c5 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -37,10 +37,6 @@ `annotationProcessorPaths` configuration below. --> provided - - ${project.groupId} - refaster-support - com.google.auto.service auto-service-annotations diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java index 64a56751e0..f3bde9fd0c 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java @@ -1,25 +1,18 @@ package tech.picnic.errorprone.refaster.runner; import com.google.common.base.Suppliers; -import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; import com.google.common.reflect.ClassPath; import com.google.common.reflect.ClassPath.ResourceInfo; import com.google.errorprone.CodeTransformer; -import com.google.errorprone.DescriptionListener; -import com.google.errorprone.matchers.Description; -import com.sun.source.util.TreePath; -import com.sun.tools.javac.util.Context; import java.io.IOException; import java.io.InputStream; import java.io.ObjectInputStream; import java.io.UncheckedIOException; -import java.lang.annotation.Annotation; import java.util.NoSuchElementException; import java.util.Optional; import java.util.function.Supplier; -import tech.picnic.errorprone.refaster.annotation.TemplateCollection; /** * Scans the classpath for {@value #REFASTER_TEMPLATE_SUFFIX} files and loads them as {@link @@ -58,12 +51,7 @@ private static ImmutableListMultimap loadAllCodeTransfo .ifPresent( templateName -> loadCodeTransformer(resource) - .ifPresent( - transformer -> - transformers.put( - templateName, - new CodeTransformerDescriptionAdapter( - templateName, transformer)))); + .ifPresent(transformer -> transformers.put(templateName, transformer))); } return transformers.build(); @@ -107,50 +95,4 @@ private static Optional loadCodeTransformer(ResourceInfo resour throw new IllegalStateException("Can't load `CodeTransformer` from " + resource, e); } } - - // XXX: Move to separate file? - // XXX: Can we find a better class name? - private static final class CodeTransformerDescriptionAdapter implements CodeTransformer { - private final String name; - private final CodeTransformer delegate; - - private CodeTransformerDescriptionAdapter(String name, CodeTransformer delegate) { - this.name = name; - this.delegate = delegate; - } - - @Override - public void apply(TreePath path, Context context, DescriptionListener listener) { - TemplateCollection coll = annotations().getInstance(TemplateCollection.class); - - if (coll != null) { - coll.linkPattern(); - } - - delegate.apply( - path, context, description -> listener.onDescribed(augmentDescription(description))); - } - - @Override - public ImmutableClassToInstanceMap annotations() { - return delegate.annotations(); - } - - private Description augmentDescription(Description description) { - // XXX: Make this configurable based on a Refaster annotation. (E.g. by allowing users to - // specify an optional URL pattern.) - // XXX: Replace only the first `$`. - // XXX: Review URL format. Currently produced format: - // https://error-prone.picnic.tech/refastertemplates/OptionalTemplates#OptionalOrElseThrow - // XXX: Test this. - return Description.builder( - description.position, - description.checkName, - "https://error-prone.picnic.tech/refastertemplates/" + name.replace('$', '#'), - description.severity, - "Refactoring opportunity") - .addAllFixes(description.fixes) - .build(); - } - } } diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java index d97d30e1f8..5598481927 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java @@ -2,23 +2,38 @@ import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.CodeTransformer; import com.google.errorprone.DescriptionListener; +import com.google.errorprone.matchers.Description; import com.sun.source.util.TreePath; import com.sun.tools.javac.util.Context; import java.io.Serializable; import java.lang.annotation.Annotation; +import java.util.Optional; +import java.util.function.Function; +import tech.picnic.errorprone.refaster.annotation.TemplateCollection; // XXX: Move? Rename? Use `@AutoValue`? // XXX: This is a bit of an implementation detail. Alternative idea: move this to `refaster-runner`, // and have `refaster-compiler` depend on `refaster-runner`. (Or the other way around?) +// XXX: If we go this route, do we need `CodeTransformer#annotations()` at all? We can track the +// meta-data we're interested in in a custom format (like we already do for the name). +// XXX: Or should the name _also_ be derived from an annotation? Upshot is limited. +// XXX: ^ Name could be dropped if we derive it from `description.checkName`, with the assumption +// that package and class names follow idiomatic Java naming convention. Kinda icky... public final class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { + private static final long serialVersionUID = 1L; + + private final String name; private final ImmutableList transformers; private final ImmutableClassToInstanceMap annotations; public AnnotatedCompositeCodeTransformer( + String name, ImmutableList transformers, ImmutableClassToInstanceMap annotations) { + this.name = name; this.transformers = transformers; this.annotations = annotations; } @@ -31,7 +46,53 @@ public ImmutableClassToInstanceMap annotations() { @Override public void apply(TreePath path, Context context, DescriptionListener listener) { for (CodeTransformer transformer : transformers) { - transformer.apply(path, context, listener); + transformer.apply( + path, + context, + description -> listener.onDescribed(augmentDescription(description, transformer))); } } + + private Description augmentDescription(Description description, CodeTransformer delegate) { + String linkPattern = + extract(AnnotatedCompositeCodeTransformer::getLinkPattern, delegate, this).orElse(""); + SeverityLevel severityLevel = + extract(AnnotatedCompositeCodeTransformer::getSeverity, delegate, this) + .orElse(SeverityLevel.SUGGESTION); + String message = + extract(AnnotatedCompositeCodeTransformer::getDescription, delegate, this) + .orElse("Refactoring opportunity"); + + // XXX: Replace only the first `$`. + // XXX: Test this. + return Description.builder( + description.position, + description.checkName, + String.format(linkPattern, name.replace('$', '#')), + severityLevel, + message) + .addAllFixes(description.fixes) + .build(); + } + + // XXX: Deduplicate the code below. + + private static Optional extract(Function> extractor, S first, S second) { + return extractor.apply(first).or(() -> extractor.apply(second)); + } + + private static Optional getLinkPattern(CodeTransformer codeTransformer) { + return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class)) + .map(TemplateCollection::linkPattern); + } + + private static Optional getSeverity(CodeTransformer codeTransformer) { + return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class)) + .map(TemplateCollection::severity); + } + + private static Optional getDescription(CodeTransformer codeTransformer) { + return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class)) + .map(TemplateCollection::description); + } } diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java index a1e3f5109e..bf0e70262b 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java @@ -1,12 +1,24 @@ package tech.picnic.errorprone.refaster.annotation; +import com.google.errorprone.BugPattern.SeverityLevel; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +// XXX: By grouping multiple properties this annotation does not lend itself to overriding. Perhaps +// have separate annotations for each? +// XXX: ^ Additional argument: the current setup "requires" defaults, which then causes duplication +// with `AnnotatedCompositeCodeTransformer`. +// XXX: The name `TemplateCollection` isn't appropriate if used directly on a Refaster template. +// Find a more neutral name. @Target(ElementType.TYPE) @Retention(RetentionPolicy.SOURCE) public @interface TemplateCollection { - String linkPattern(); + // XXX: This default is Error Prone Support-specific. Appropriate? + String linkPattern() default "https://error-prone.picnic.tech/refastertemplates/%s"; + + SeverityLevel severity() default SeverityLevel.SUGGESTION; + + String description() default "Refactoring opportunity"; } diff --git a/refaster-test-support/pom.xml b/refaster-test-support/pom.xml index 10d8722307..81732bf9f2 100644 --- a/refaster-test-support/pom.xml +++ b/refaster-test-support/pom.xml @@ -43,6 +43,7 @@ ${project.groupId} refaster-support + test com.google.auto.service diff --git a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java index 85551d6be2..af4537eda7 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java @@ -1,5 +1,7 @@ package tech.picnic.errorprone.refaster.test; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; + import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; import tech.picnic.errorprone.refaster.annotation.TemplateCollection; @@ -11,8 +13,8 @@ final class MatchInWrongMethodTemplates { private MatchInWrongMethodTemplates() {} - // XXX: Test merging/overriding. - // @TemplateCollection(linkPattern = "XXX") + // XXX: Demo: nesting overrides. + @TemplateCollection(linkPattern = "YYY", severity = ERROR, description = "Foo") static final class StringIsEmpty { @BeforeTemplate boolean before(String string) { From 0e46f9cf5fdb913f1aabf68d3c107f35833e4f90 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 23 Sep 2022 17:00:55 +0200 Subject: [PATCH 05/53] Tweaks --- .../RefasterRuleCompilerTaskListener.java | 57 ++++++++++--------- refaster-runner/pom.xml | 8 +++ 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java index 6759bb5d34..5ba370d602 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java @@ -69,25 +69,6 @@ public void finished(TaskEvent taskEvent) { } } - private boolean containsRefasterTemplates(ClassTree tree) { - return Boolean.TRUE.equals( - new TreeScanner() { - @Override - public Boolean visitAnnotation(AnnotationTree node, @Nullable Void v) { - Symbol sym = ASTHelpers.getSymbol(node); - return (sym != null - && sym.getQualifiedName() - .contentEquals(BeforeTemplate.class.getCanonicalName())) - || super.visitAnnotation(node, v); - } - - @Override - public Boolean reduce(Boolean r1, Boolean r2) { - return Boolean.TRUE.equals(r1) || Boolean.TRUE.equals(r2); - } - }.scan(tree, null)); - } - private ImmutableMap compileRefasterTemplates(ClassTree tree) { ImmutableMap.Builder rules = ImmutableMap.builder(); new TreeScanner>() { @@ -112,15 +93,6 @@ public Void visitClass(ClassTree node, ImmutableClassToInstanceMap a return rules.buildOrThrow(); } - // XXX: Move down? - private static ImmutableClassToInstanceMap merge( - ImmutableClassToInstanceMap left, ImmutableClassToInstanceMap right) { - return ImmutableClassToInstanceMap.builder() - .putAll(Maps.filterKeys(left, k -> !right.containsKey(k))) - .putAll(right) - .build(); - } - private FileObject getOutputFile(TaskEvent taskEvent, ClassTree tree) throws IOException { ClassSymbol symbol = ASTHelpers.getSymbol(tree); PackageSymbol enclosingPackage = ASTHelpers.enclosingPackage(symbol); @@ -132,6 +104,35 @@ private FileObject getOutputFile(TaskEvent taskEvent, ClassTree tree) throws IOE StandardLocation.CLASS_OUTPUT, packageName, relativeName, taskEvent.getSourceFile()); } + private static boolean containsRefasterTemplates(ClassTree tree) { + return Boolean.TRUE.equals( + new TreeScanner() { + @Override + public Boolean visitAnnotation(AnnotationTree node, @Nullable Void unused) { + Symbol sym = ASTHelpers.getSymbol(node); + return (sym != null + && sym.getQualifiedName() + .contentEquals(BeforeTemplate.class.getCanonicalName())) + || super.visitAnnotation(node, unused); + } + + @Override + public Boolean reduce(Boolean r1, Boolean r2) { + return Boolean.TRUE.equals(r1) || Boolean.TRUE.equals(r2); + } + }.scan(tree, null)); + } + + /** Merges two annotation mappings, preferring the second over the first in case of conflicts. */ + private static ImmutableClassToInstanceMap merge( + ImmutableClassToInstanceMap first, + ImmutableClassToInstanceMap second) { + return ImmutableClassToInstanceMap.builder() + .putAll(Maps.filterKeys(first, k -> !second.containsKey(k))) + .putAll(second) + .build(); + } + private static CharSequence toSimpleFlatName(ClassSymbol classSymbol) { Name flatName = classSymbol.flatName(); int lastDot = flatName.lastIndexOf((byte) '.'); diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index 7bccf546c5..17f1a61d39 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -37,6 +37,14 @@ `annotationProcessorPaths` configuration below. --> provided + + + ${project.groupId} + refaster-support + runtime + com.google.auto.service auto-service-annotations From 914d30a7edb794ff4b43f95108b4930af112c7f0 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 23 Sep 2022 21:01:48 +0200 Subject: [PATCH 06/53] Better annotation support, simplify setup --- .../refastertemplates/OptionalTemplates.java | 5 +- refaster-compiler/pom.xml | 4 + .../AnnotatedCompositeCodeTransformer.java | 96 ++++++++++++++++++ .../RefasterRuleCompilerTaskListener.java | 1 - refaster-runner/pom.xml | 11 --- .../AnnotatedCompositeCodeTransformer.java | 98 ------------------- .../refaster/annotation/Description.java | 22 +++++ .../annotation/OnlineDocumentation.java | 33 +++++++ .../refaster/annotation/Severity.java | 24 +++++ .../annotation/TemplateCollection.java | 24 ----- refaster-test-support/pom.xml | 1 + .../test/MatchInWrongMethodTemplates.java | 10 +- 12 files changed, 189 insertions(+), 140 deletions(-) create mode 100644 refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java delete mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java delete mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java index 113159d98c..55a1ee2b3d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java @@ -17,8 +17,11 @@ import java.util.function.Supplier; import java.util.stream.Stream; import javax.annotation.Nullable; +import tech.picnic.errorprone.refaster.annotation.Description; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster templates related to expressions dealing with {@link Optional}s. */ +@OnlineDocumentation final class OptionalTemplates { private OptionalTemplates() {} @@ -63,7 +66,7 @@ boolean after(Optional optional) { } } - /** Prefer {@link Optional#orElseThrow()} over the less explicit {@link Optional#get()}. */ + @Description("Prefer `Optional#orElseThrow()` over the less explicit `Optional#get()`") static final class OptionalOrElseThrow { @BeforeTemplate @SuppressWarnings("NullAway") diff --git a/refaster-compiler/pom.xml b/refaster-compiler/pom.xml index 9a6fbd9798..8a48ea66c2 100644 --- a/refaster-compiler/pom.xml +++ b/refaster-compiler/pom.xml @@ -14,6 +14,10 @@ A Java compiler plugin that identifies and compiles Refaster templates, storing them as resource files on the classpath. + + ${groupId.error-prone} + error_prone_annotation + ${groupId.error-prone} error_prone_annotations diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java new file mode 100644 index 0000000000..9cae0e1cfe --- /dev/null +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -0,0 +1,96 @@ +package tech.picnic.errorprone.refaster.plugin; + +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; + +import com.google.common.collect.ImmutableClassToInstanceMap; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.CodeTransformer; +import com.google.errorprone.DescriptionListener; +import com.google.errorprone.matchers.Description; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.util.Context; +import java.io.Serializable; +import java.lang.annotation.Annotation; +import java.util.Optional; +import java.util.function.Function; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.annotation.Severity; + +// XXX: Rename? Use `@AutoValue`? +// XXX: Should the name _also_ be derived from an annotation? Upshot is limited. +// XXX: ^ Name could be dropped if we derive it from `description.checkName`, with the assumption +// that package and class names follow idiomatic Java naming convention. Kinda icky... +final class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { + private static final long serialVersionUID = 1L; + + private final String name; + private final ImmutableList transformers; + private final ImmutableClassToInstanceMap annotations; + + AnnotatedCompositeCodeTransformer( + String name, + ImmutableList transformers, + ImmutableClassToInstanceMap annotations) { + this.name = name; + this.transformers = transformers; + this.annotations = annotations; + } + + @Override + public ImmutableClassToInstanceMap annotations() { + return annotations; + } + + @Override + public void apply(TreePath path, Context context, DescriptionListener listener) { + for (CodeTransformer transformer : transformers) { + transformer.apply( + path, + context, + description -> listener.onDescribed(augmentDescription(description, transformer))); + } + } + + private Description augmentDescription(Description description, CodeTransformer delegate) { + // XXX: Replace only the first `$`. + // XXX: Test this. + return Description.builder( + description.position, + description.checkName, + String.format(getLinkPattern(delegate), name.replace('$', '#')), + getSeverity(delegate), + getDescription(delegate)) + .addAllFixes(description.fixes) + .build(); + } + + private String getLinkPattern(CodeTransformer delegate) { + return getAnnotationValue(OnlineDocumentation.class, OnlineDocumentation::value, delegate, ""); + } + + private SeverityLevel getSeverity(CodeTransformer delegate) { + return getAnnotationValue(Severity.class, Severity::value, delegate, SUGGESTION); + } + + private String getDescription(CodeTransformer delegate) { + return getAnnotationValue( + tech.picnic.errorprone.refaster.annotation.Description.class, + tech.picnic.errorprone.refaster.annotation.Description::value, + delegate, + "Refactoring opportunity"); + } + + private T getAnnotationValue( + Class annotation, Function extractor, CodeTransformer delegate, T defaultValue) { + return getAnnotationValue(delegate, annotation) + .or(() -> getAnnotationValue(this, annotation)) + .map(extractor) + .orElse(defaultValue); + } + + private static Optional getAnnotationValue( + CodeTransformer codeTransformer, Class annotation) { + return Optional.ofNullable(codeTransformer.annotations().getInstance(annotation)); + } +} diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java index 5ba370d602..4b448ddc84 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java @@ -32,7 +32,6 @@ import javax.tools.FileObject; import javax.tools.JavaFileManager; import javax.tools.StandardLocation; -import tech.picnic.errorprone.refaster.AnnotatedCompositeCodeTransformer; /** * A variant of {@code com.google.errorprone.refaster.RefasterRuleCompilerAnalyzer} that stores diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index 17f1a61d39..62b536b822 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -32,17 +32,6 @@ ${project.groupId} refaster-compiler - - provided - - - - ${project.groupId} - refaster-support runtime diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java deleted file mode 100644 index 5598481927..0000000000 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java +++ /dev/null @@ -1,98 +0,0 @@ -package tech.picnic.errorprone.refaster; - -import com.google.common.collect.ImmutableClassToInstanceMap; -import com.google.common.collect.ImmutableList; -import com.google.errorprone.BugPattern.SeverityLevel; -import com.google.errorprone.CodeTransformer; -import com.google.errorprone.DescriptionListener; -import com.google.errorprone.matchers.Description; -import com.sun.source.util.TreePath; -import com.sun.tools.javac.util.Context; -import java.io.Serializable; -import java.lang.annotation.Annotation; -import java.util.Optional; -import java.util.function.Function; -import tech.picnic.errorprone.refaster.annotation.TemplateCollection; - -// XXX: Move? Rename? Use `@AutoValue`? -// XXX: This is a bit of an implementation detail. Alternative idea: move this to `refaster-runner`, -// and have `refaster-compiler` depend on `refaster-runner`. (Or the other way around?) -// XXX: If we go this route, do we need `CodeTransformer#annotations()` at all? We can track the -// meta-data we're interested in in a custom format (like we already do for the name). -// XXX: Or should the name _also_ be derived from an annotation? Upshot is limited. -// XXX: ^ Name could be dropped if we derive it from `description.checkName`, with the assumption -// that package and class names follow idiomatic Java naming convention. Kinda icky... -public final class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { - private static final long serialVersionUID = 1L; - - private final String name; - private final ImmutableList transformers; - private final ImmutableClassToInstanceMap annotations; - - public AnnotatedCompositeCodeTransformer( - String name, - ImmutableList transformers, - ImmutableClassToInstanceMap annotations) { - this.name = name; - this.transformers = transformers; - this.annotations = annotations; - } - - @Override - public ImmutableClassToInstanceMap annotations() { - return annotations; - } - - @Override - public void apply(TreePath path, Context context, DescriptionListener listener) { - for (CodeTransformer transformer : transformers) { - transformer.apply( - path, - context, - description -> listener.onDescribed(augmentDescription(description, transformer))); - } - } - - private Description augmentDescription(Description description, CodeTransformer delegate) { - String linkPattern = - extract(AnnotatedCompositeCodeTransformer::getLinkPattern, delegate, this).orElse(""); - SeverityLevel severityLevel = - extract(AnnotatedCompositeCodeTransformer::getSeverity, delegate, this) - .orElse(SeverityLevel.SUGGESTION); - String message = - extract(AnnotatedCompositeCodeTransformer::getDescription, delegate, this) - .orElse("Refactoring opportunity"); - - // XXX: Replace only the first `$`. - // XXX: Test this. - return Description.builder( - description.position, - description.checkName, - String.format(linkPattern, name.replace('$', '#')), - severityLevel, - message) - .addAllFixes(description.fixes) - .build(); - } - - // XXX: Deduplicate the code below. - - private static Optional extract(Function> extractor, S first, S second) { - return extractor.apply(first).or(() -> extractor.apply(second)); - } - - private static Optional getLinkPattern(CodeTransformer codeTransformer) { - return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class)) - .map(TemplateCollection::linkPattern); - } - - private static Optional getSeverity(CodeTransformer codeTransformer) { - return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class)) - .map(TemplateCollection::severity); - } - - private static Optional getDescription(CodeTransformer codeTransformer) { - return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class)) - .map(TemplateCollection::description); - } -} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java new file mode 100644 index 0000000000..bb2ecc1b9f --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java @@ -0,0 +1,22 @@ +package tech.picnic.errorprone.refaster.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Describes the intent of a Refaster template or group of Refaster templates. + * + *

Annotations on nested classes override the description associated with any enclosing class. + */ +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.SOURCE) +public @interface Description { + /** + * A description of the annotated Refaster template(s). + * + * @return A non-{@code null} string. + */ + String value(); +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java new file mode 100644 index 0000000000..f213bcbb73 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java @@ -0,0 +1,33 @@ +package tech.picnic.errorprone.refaster.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Signals that a Refaster template or group of Refaster templates comes with online documentation. + * + *

The provided value may be a full URL, or a URL pattern containing a single {@code %s} + * placeholder. If a placeholder is present, then it will be replaced with the name of the + * associated Refaster template any time the URL is rendered. + * + *

By default it is assumed that the Refaster template(s) are documented on the Error Prone + * Support website. Annotations on nested classes override the documentation URL associated with any + * enclosing class. + */ +// XXX: Is the `%s` protocol sufficiently generic for non-Picnic use cases? +// XXX: The documentation is misleading, in that the generated anchor isn't mentioned. +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.SOURCE) +public @interface OnlineDocumentation { + /** + * The URL or URL pattern of the website at which the annotated Refaster template(s) are + * documented. + * + * @return A non-{@code null} string. + */ + // XXX: This default is Error Prone Support-specific. Appropriate? (The alternative is to repeat + // this URL pattern in many places.) If we drop this, also update the class documentation. + String value() default "https://error-prone.picnic.tech/refastertemplates/%s"; +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java new file mode 100644 index 0000000000..9bf4398181 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java @@ -0,0 +1,24 @@ +package tech.picnic.errorprone.refaster.annotation; + +import com.google.errorprone.BugPattern.SeverityLevel; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Describes the severity of a Refaster template or group of Refaster templates. + * + *

The default severity is {@link SeverityLevel#SUGGESTION}. Annotations on nested classes + * override the severity associated with any enclosing class. + */ +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.SOURCE) +public @interface Severity { + /** + * The expected severity of any match of the annotated Refaster template(s). + * + * @return An Error Prone severity level. + */ + SeverityLevel value(); +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java deleted file mode 100644 index bf0e70262b..0000000000 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java +++ /dev/null @@ -1,24 +0,0 @@ -package tech.picnic.errorprone.refaster.annotation; - -import com.google.errorprone.BugPattern.SeverityLevel; -import java.lang.annotation.ElementType; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; - -// XXX: By grouping multiple properties this annotation does not lend itself to overriding. Perhaps -// have separate annotations for each? -// XXX: ^ Additional argument: the current setup "requires" defaults, which then causes duplication -// with `AnnotatedCompositeCodeTransformer`. -// XXX: The name `TemplateCollection` isn't appropriate if used directly on a Refaster template. -// Find a more neutral name. -@Target(ElementType.TYPE) -@Retention(RetentionPolicy.SOURCE) -public @interface TemplateCollection { - // XXX: This default is Error Prone Support-specific. Appropriate? - String linkPattern() default "https://error-prone.picnic.tech/refastertemplates/%s"; - - SeverityLevel severity() default SeverityLevel.SUGGESTION; - - String description() default "Refactoring opportunity"; -} diff --git a/refaster-test-support/pom.xml b/refaster-test-support/pom.xml index 81732bf9f2..7e2359f72b 100644 --- a/refaster-test-support/pom.xml +++ b/refaster-test-support/pom.xml @@ -40,6 +40,7 @@ ${project.groupId} refaster-runner + ${project.groupId} refaster-support diff --git a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java index af4537eda7..543b24308a 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java @@ -1,20 +1,20 @@ package tech.picnic.errorprone.refaster.test; -import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; - import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; -import tech.picnic.errorprone.refaster.annotation.TemplateCollection; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster template collection to validate reporting of a match occurring in an unexpected place. */ -@TemplateCollection(linkPattern = "XXX") +@OnlineDocumentation final class MatchInWrongMethodTemplates { private MatchInWrongMethodTemplates() {} // XXX: Demo: nesting overrides. - @TemplateCollection(linkPattern = "YYY", severity = ERROR, description = "Foo") + // @Website("YYY") + // @Severity(ERROR) + // @Description("Fooo!") static final class StringIsEmpty { @BeforeTemplate boolean before(String string) { From 458fb99d4ecde133e349d7dbb0c280df173772c8 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 23 Sep 2022 22:26:58 +0200 Subject: [PATCH 07/53] Flag why build currently doesn't fail, while it should. --- .../plugin/AnnotatedCompositeCodeTransformer.java | 9 +++++++++ .../errorprone/refaster/annotation/package-info.java | 8 ++++++++ 2 files changed, 17 insertions(+) create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index 9cae0e1cfe..ddfb3d1bde 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -1,12 +1,15 @@ package tech.picnic.errorprone.refaster.plugin; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.CodeTransformer; import com.google.errorprone.DescriptionListener; +import com.google.errorprone.ErrorProneOptions; import com.google.errorprone.matchers.Description; import com.sun.source.util.TreePath; import com.sun.tools.javac.util.Context; @@ -44,6 +47,12 @@ public ImmutableClassToInstanceMap annotations() { @Override public void apply(TreePath path, Context context, DescriptionListener listener) { + // XXX: Reflectively access `suggestionsAsWarnings`! + SeverityLevel minSeverity = WARNING; + SeverityLevel maxSeverity = + context.get(ErrorProneOptions.class).isDropErrorsToWarnings() ? WARNING : ERROR; + // XXX: Use these ^ in `getSeverity(...)`. + for (CodeTransformer transformer : transformers) { transformer.apply( path, diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java new file mode 100644 index 0000000000..b829101a80 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java @@ -0,0 +1,8 @@ +/** + * A collection annotations that may be placed on Refaster template classes and Refaster template + * collection classes, thus influencing the way in which associated template matches are reported in + * non-patch mode. + */ +@com.google.errorprone.annotations.CheckReturnValue +@javax.annotation.ParametersAreNonnullByDefault +package tech.picnic.errorprone.refaster.annotation; From db8cf3c8601aa52e4d4f1274eec9b5cb4fa081d2 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 24 Sep 2022 15:21:03 +0200 Subject: [PATCH 08/53] Improve logic and test coverage --- .../bugpatterns/MethodReferenceUsage.java | 3 +- .../AnnotatedCompositeCodeTransformer.java | 69 +++++++--- .../RefasterRuleCompilerTaskListener.java | 26 ++-- refaster-runner/pom.xml | 5 + .../errorprone/refaster/runner/Refaster.java | 55 +++++++- .../refaster/runner/CodeTransformersTest.java | 9 +- .../refaster/runner/FooTemplates.java | 84 +++++++++++- .../refaster/runner/RefasterTest.java | 120 ++++++++++++++++++ .../annotation/OnlineDocumentation.java | 6 +- refaster-test-support/pom.xml | 6 - .../test/RefasterTemplateCollection.java | 10 +- .../test/MatchInWrongMethodTemplates.java | 6 - 12 files changed, 340 insertions(+), 59 deletions(-) create mode 100644 refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java index 0bcd71fee7..437fb6db1f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java @@ -150,8 +150,7 @@ private static Optional constructMethodRef( } Type lhsType = ASTHelpers.getType(subTree.getExpression()); - // XXX: Using `.get()` instead of `.orElseThrows()` for demonstration purposes. - if (lhsType == null || !expectedInstance.get().equals(lhs)) { + if (lhsType == null || !expectedInstance.orElseThrow().equals(lhs)) { return Optional.empty(); } diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index ddfb3d1bde..cfb701fc7e 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -1,11 +1,15 @@ package tech.picnic.errorprone.refaster.plugin; +import static com.google.common.base.Preconditions.checkState; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import com.google.common.base.Splitter; +import com.google.common.collect.Comparators; import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterators; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.CodeTransformer; import com.google.errorprone.DescriptionListener; @@ -15,27 +19,26 @@ import com.sun.tools.javac.util.Context; import java.io.Serializable; import java.lang.annotation.Annotation; +import java.util.Iterator; import java.util.Optional; import java.util.function.Function; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.annotation.Severity; // XXX: Rename? Use `@AutoValue`? -// XXX: Should the name _also_ be derived from an annotation? Upshot is limited. -// XXX: ^ Name could be dropped if we derive it from `description.checkName`, with the assumption -// that package and class names follow idiomatic Java naming convention. Kinda icky... final class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { private static final long serialVersionUID = 1L; + private static final Splitter CLASS_NAME_SPLITTER = Splitter.on('.').limit(2); - private final String name; + private final String packageName; private final ImmutableList transformers; private final ImmutableClassToInstanceMap annotations; AnnotatedCompositeCodeTransformer( - String name, + String packageName, ImmutableList transformers, ImmutableClassToInstanceMap annotations) { - this.name = name; + this.packageName = packageName; this.transformers = transformers; this.annotations = annotations; } @@ -47,35 +50,52 @@ public ImmutableClassToInstanceMap annotations() { @Override public void apply(TreePath path, Context context, DescriptionListener listener) { - // XXX: Reflectively access `suggestionsAsWarnings`! - SeverityLevel minSeverity = WARNING; - SeverityLevel maxSeverity = - context.get(ErrorProneOptions.class).isDropErrorsToWarnings() ? WARNING : ERROR; - // XXX: Use these ^ in `getSeverity(...)`. - for (CodeTransformer transformer : transformers) { transformer.apply( path, context, - description -> listener.onDescribed(augmentDescription(description, transformer))); + description -> + listener.onDescribed(augmentDescription(description, transformer, context))); } } - private Description augmentDescription(Description description, CodeTransformer delegate) { - // XXX: Replace only the first `$`. + private Description augmentDescription( + Description description, CodeTransformer delegate, Context context) { // XXX: Test this. + String shortCheckName = getShortCheckName(description.checkName); return Description.builder( description.position, - description.checkName, - String.format(getLinkPattern(delegate), name.replace('$', '#')), - getSeverity(delegate), + shortCheckName, + getLinkPattern(delegate, shortCheckName), + overrideSeverity(getSeverity(delegate), context), getDescription(delegate)) .addAllFixes(description.fixes) .build(); } - private String getLinkPattern(CodeTransformer delegate) { - return getAnnotationValue(OnlineDocumentation.class, OnlineDocumentation::value, delegate, ""); + private String getShortCheckName(String fullCheckName) { + if (packageName.isEmpty()) { + return fullCheckName; + } + + String prefix = packageName + '.'; + checkState( + fullCheckName.startsWith(prefix), + "Refaster template class '%s' is not located in package '%s'", + fullCheckName, + packageName); + + return fullCheckName.substring(prefix.length()); + } + + private String getLinkPattern(CodeTransformer delegate, String checkName) { + String urlPattern = + getAnnotationValue(OnlineDocumentation.class, OnlineDocumentation::value, delegate, ""); + + Iterator nameComponents = CLASS_NAME_SPLITTER.splitToList(checkName).iterator(); + return urlPattern + .replace("${topLevelClassName}", nameComponents.next()) + .replace("${nestedClassName}", Iterators.getNext(nameComponents, "")); } private SeverityLevel getSeverity(CodeTransformer delegate) { @@ -102,4 +122,13 @@ private static Optional getAnnotationValue( CodeTransformer codeTransformer, Class annotation) { return Optional.ofNullable(codeTransformer.annotations().getInstance(annotation)); } + + private static SeverityLevel overrideSeverity(SeverityLevel severity, Context context) { + // XXX: Respect `-XepAllSuggestionsAsWarnings` when using the Picnic Error Prone Fork! + SeverityLevel minSeverity = SUGGESTION; + SeverityLevel maxSeverity = + context.get(ErrorProneOptions.class).isDropErrorsToWarnings() ? WARNING : ERROR; + + return Comparators.max(Comparators.min(severity, minSeverity), maxSeverity); + } } diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java index 4b448ddc84..6eeeb1f318 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java @@ -74,19 +74,18 @@ private ImmutableMap compileRefasterTemplates(ClassT @Nullable @Override public Void visitClass(ClassTree node, ImmutableClassToInstanceMap annotations) { + ClassSymbol symbol = ASTHelpers.getSymbol(node); + ImmutableList transformers = ImmutableList.copyOf(RefasterRuleBuilderScanner.extractRules(node, context)); if (!transformers.isEmpty()) { rules.put( node, new AnnotatedCompositeCodeTransformer( - String.valueOf(toSimpleFlatName(ASTHelpers.getSymbol(node))), - transformers, - annotations)); + toPackageName(symbol), transformers, annotations)); } - return super.visitClass( - node, merge(annotations, UTemplater.annotationMap(ASTHelpers.getSymbol(node)))); + return super.visitClass(node, merge(annotations, UTemplater.annotationMap(symbol))); } }.scan(tree, ImmutableClassToInstanceMap.of()); return rules.buildOrThrow(); @@ -94,13 +93,13 @@ public Void visitClass(ClassTree node, ImmutableClassToInstanceMap a private FileObject getOutputFile(TaskEvent taskEvent, ClassTree tree) throws IOException { ClassSymbol symbol = ASTHelpers.getSymbol(tree); - PackageSymbol enclosingPackage = ASTHelpers.enclosingPackage(symbol); - String packageName = enclosingPackage == null ? "" : enclosingPackage.toString(); - String relativeName = toSimpleFlatName(symbol) + ".refaster"; JavaFileManager fileManager = context.get(JavaFileManager.class); return fileManager.getFileForOutput( - StandardLocation.CLASS_OUTPUT, packageName, relativeName, taskEvent.getSourceFile()); + StandardLocation.CLASS_OUTPUT, + toPackageName(symbol), + toSimpleFlatName(symbol) + ".refaster", + taskEvent.getSourceFile()); } private static boolean containsRefasterTemplates(ClassTree tree) { @@ -132,8 +131,13 @@ private static ImmutableClassToInstanceMap merge( .build(); } - private static CharSequence toSimpleFlatName(ClassSymbol classSymbol) { - Name flatName = classSymbol.flatName(); + private static String toPackageName(ClassSymbol symbol) { + PackageSymbol enclosingPackage = ASTHelpers.enclosingPackage(symbol); + return enclosingPackage == null ? "" : enclosingPackage.toString(); + } + + private static CharSequence toSimpleFlatName(ClassSymbol symbol) { + Name flatName = symbol.flatName(); int lastDot = flatName.lastIndexOf((byte) '.'); return lastDot < 0 ? flatName : flatName.subSequence(lastDot + 1, flatName.length()); } diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index 62b536b822..9e43b5cc68 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -29,6 +29,11 @@ error_prone_check_api provided + + ${groupId.error-prone} + error_prone_test_helpers + test + ${project.groupId} refaster-compiler diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 118475dbea..9b2a780f1f 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -3,7 +3,9 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableRangeSet.toImmutableRangeSet; import static com.google.errorprone.BugPattern.LinkType.NONE; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; import static java.util.function.Predicate.not; @@ -16,9 +18,11 @@ import com.google.common.collect.RangeSet; import com.google.common.collect.TreeRangeSet; import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.CodeTransformer; import com.google.errorprone.CompositeCodeTransformer; import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.ErrorProneOptions.Severity; import com.google.errorprone.SubContext; import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.CanIgnoreReturnValue; @@ -33,6 +37,7 @@ import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -40,9 +45,9 @@ * A {@link BugChecker} that flags code that can be simplified using Refaster templates located on * the classpath. * - *

This checker locates all {@code *.refaster} classpath resources and assumes they contain a - * {@link CodeTransformer}. The set of loaded Refaster templates can be restricted by passing {@code - * -XepOpt:Refaster:NamePattern=}. + *

This checker locates all {@code *.refaster} classpath resources and assumes that they contain + * a {@link CodeTransformer}. The set of loaded Refaster templates can be restricted by passing + * {@code -XepOpt:Refaster:NamePattern=}. */ @AutoService(BugChecker.class) @BugPattern( @@ -104,7 +109,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s */ // XXX: This selection logic solves an issue described in // https://github.com/google/error-prone/issues/559. Consider contributing it back upstream. - private static void applyMatches( + private void applyMatches( Iterable allMatches, EndPosTable endPositions, VisitorState state) { ImmutableList byReplacementSize = ImmutableList.sortedCopyOf( @@ -118,12 +123,52 @@ private static void applyMatches( ImmutableRangeSet ranges = getReplacementRanges(description, endPositions); if (ranges.asRanges().stream().noneMatch(replacedSections::intersects)) { /* This suggested fix does not overlap with any ("larger") replacement seen until now. Apply it. */ - state.reportMatch(description); + state.reportMatch(augmentDescription(description, getSeverityOverride(state))); replacedSections.addAll(ranges); } } } + private Optional getSeverityOverride(VisitorState state) { + return Optional.ofNullable(state.errorProneOptions().getSeverityMap().get(canonicalName())) + .flatMap(Refaster::toSeverityLevel); + } + + private static Optional toSeverityLevel(Severity severity) { + switch (severity) { + case DEFAULT: + return Optional.empty(); + case WARN: + return Optional.of(WARNING); + case ERROR: + return Optional.of(ERROR); + default: + throw new IllegalStateException(String.format("Unsupported severity='%s'", severity)); + } + } + + /** + * Updates the given {@link Description}'s details such that {@link + * VisitorState#reportMatch(Description)} will override the reported severity only if this bug + * checker's severity was explicitly configured. + * + *

The original check name (i.e. the Refaster template name) is prepended to the {@link + * Description}'s message. The replacement check name ("Refaster Template") is chosen such that it + * is guaranteed not to match any canonical bug checker name (as that could cause {@link + * VisitorState#reportMatch(Description)}} to override the reported severity). + */ + private static Description augmentDescription( + Description description, Optional severityOverride) { + return Description.builder( + description.position, + "Refaster Rule", + description.getLink(), + severityOverride.orElse(description.severity), + String.format("%s: %s", description.checkName, description.getRawMessage())) + .addAllFixes(description.fixes) + .build(); + } + private static int getReplacedCodeSize(Description description, EndPosTable endPositions) { return getReplacements(description, endPositions).mapToInt(Replacement::length).sum(); } diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/CodeTransformersTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/CodeTransformersTest.java index b4a2732102..40ccbc61f5 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/CodeTransformersTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/CodeTransformersTest.java @@ -6,12 +6,17 @@ final class CodeTransformersTest { /** - * Verifies that {@link CodeTransformers#getAllCodeTransformers()} finds the code transformer + * Verifies that {@link CodeTransformers#getAllCodeTransformers()} finds the code transformers * compiled from {@link FooTemplates} on the classpath. */ @Test void getAllCodeTransformers() { assertThat(CodeTransformers.getAllCodeTransformers().keySet()) - .containsExactly("FooTemplates$SimpleTemplate"); + .containsExactlyInAnyOrder( + "FooTemplates$StringOfSizeZeroTemplate", + "FooTemplates$StringOfSizeZeroVerboseTemplate", + "FooTemplates$StringOfSizeOneTemplate", + "FooTemplates$ExtraGrouping$StringOfSizeTwoTemplate", + "FooTemplates$ExtraGrouping$StringOfSizeThreeTemplate"); } } diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooTemplates.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooTemplates.java index 0625275d9f..2b7ccd0d85 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooTemplates.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooTemplates.java @@ -1,14 +1,21 @@ package tech.picnic.errorprone.refaster.runner; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; + import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import tech.picnic.errorprone.refaster.annotation.Description; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.annotation.Severity; -/** An example template collection used to test {@link CodeTransformers}. */ +/** An example template collection used to test {@link CodeTransformers} and {@link Refaster}. */ final class FooTemplates { private FooTemplates() {} - /** Simple template for testing purposes. */ - static final class SimpleTemplate { + /** A simple template for testing purposes, lacking any custom annotations. */ + static final class StringOfSizeZeroTemplate { @BeforeTemplate boolean before(String string) { return string.toCharArray().length == 0; @@ -19,4 +26,75 @@ boolean after(String string) { return string.isEmpty(); } } + + /** + * A simple template for testing purposes, matching the same set of expressions as {@link + * StringOfSizeZeroTemplate}, but producing a larger replacement string. + */ + static final class StringOfSizeZeroVerboseTemplate { + @BeforeTemplate + boolean before(String string) { + return string.toCharArray().length == 0; + } + + @AfterTemplate + boolean after(String string) { + return string.length() + 1 == 1; + } + } + + /** A simple template for testing purposes, having several custom annotations. */ + @Description("A custom description about matching single-char strings") + @OnlineDocumentation + @Severity(WARNING) + static final class StringOfSizeOneTemplate { + @BeforeTemplate + boolean before(String string) { + return string.toCharArray().length == 1; + } + + @AfterTemplate + boolean after(String string) { + return string.length() == 1; + } + } + + /** + * A nested class with annotations that are inherited by the Refaster templates contained in it. + */ + @Description("A custom subgroup description") + @OnlineDocumentation("https://example.com/template/${topLevelClassName}#${nestedClassName}") + @Severity(ERROR) + static final class ExtraGrouping { + private ExtraGrouping() {} + + /** A simple template for testing purposes, inheriting custom annotations. */ + static final class StringOfSizeTwoTemplate { + @BeforeTemplate + boolean before(String string) { + return string.toCharArray().length == 2; + } + + @AfterTemplate + boolean after(String string) { + return string.length() == 2; + } + } + + /** A simple template for testing purposes, overriding custom annotations. */ + @Description("A custom description about matching three-char strings") + @OnlineDocumentation("https://example.com/custom") + @Severity(SUGGESTION) + static final class StringOfSizeThreeTemplate { + @BeforeTemplate + boolean before(String string) { + return string.toCharArray().length == 3; + } + + @AfterTemplate + boolean after(String string) { + return string.length() == 3; + } + } + } } diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java new file mode 100644 index 0000000000..95047a059c --- /dev/null +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -0,0 +1,120 @@ +package tech.picnic.errorprone.refaster.runner; + +import static com.google.common.base.Predicates.containsPattern; +import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +// XXX: Verify the reported severity level. There does not appear to be a straightforward way to +// distinguish these. +// XXX: Also verify that overriding the severity level works. +final class RefasterTest { + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(Refaster.class, getClass()) + .matchAllDiagnostics() + .expectErrorMessage( + "StringOfSizeZeroTemplate", + containsPattern( + "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeZeroTemplate: Refactoring opportunity\\s+.+\\s+null")) + .expectErrorMessage( + "StringOfSizeOneTemplate", + containsPattern( + "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeOneTemplate: " + + "A custom description about matching single-char strings\\s+.+\\s+" + + "\\(see https://error-prone.picnic.tech/refastertemplates/FooTemplates#StringOfSizeOneTemplate\\)")) + .expectErrorMessage( + "StringOfSizeTwoTemplate", + containsPattern( + "\\[Refaster Rule\\] FooTemplates\\.ExtraGrouping\\.StringOfSizeTwoTemplate: " + + "A custom subgroup description\\s+.+\\s+" + + "\\(see https://example.com/template/FooTemplates#ExtraGrouping.StringOfSizeTwoTemplate\\)")) + .expectErrorMessage( + "StringOfSizeThreeTemplate", + containsPattern( + "\\[Refaster Rule\\] FooTemplates\\.ExtraGrouping\\.StringOfSizeThreeTemplate: " + + "A custom description about matching three-char strings\\s+.+\\s+" + + "\\(see https://example.com/custom\\)")); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(Refaster.class, getClass()); + private final BugCheckerRefactoringTestHelper restrictedRefactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(Refaster.class, getClass()) + .setArgs( + "-XepOpt:Refaster:NamePattern=.*\\$(StringOfSizeZeroVerboseTemplate|StringOfSizeTwoTemplate)$"); + + @Test + void identification() { + compilationHelper + .addSourceLines( + "A.java", + "class A {", + " void m() {", + " // BUG: Diagnostic matches: StringOfSizeZeroTemplate", + " boolean b1 = \"foo\".toCharArray().length == 0;", + "", + " // BUG: Diagnostic matches: StringOfSizeOneTemplate", + " boolean b2 = \"bar\".toCharArray().length == 1;", + "", + " // BUG: Diagnostic matches: StringOfSizeTwoTemplate", + " boolean b3 = \"baz\".toCharArray().length == 2;", + "", + " // BUG: Diagnostic matches: StringOfSizeThreeTemplate", + " boolean b4 = \"qux\".toCharArray().length == 3;", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + refactoringTestHelper + .addInputLines( + "A.java", + "class A {", + " void m() {", + " boolean b1 = \"foo\".toCharArray().length == 0;", + " boolean b2 = \"bar\".toCharArray().length == 1;", + " boolean b3 = \"baz\".toCharArray().length == 2;", + " boolean b4 = \"qux\".toCharArray().length == 3;", + " }", + "}") + .addOutputLines( + "A.java", + "class A {", + " void m() {", + " boolean b1 = \"foo\".isEmpty();", + " boolean b2 = \"bar\".length() == 1;", + " boolean b3 = \"baz\".length() == 2;", + " boolean b4 = \"qux\".length() == 3;", + " }", + "}") + .doTest(TEXT_MATCH); + } + + @Test + void restrictedReplacement() { + restrictedRefactoringTestHelper + .addInputLines( + "A.java", + "class A {", + " void m() {", + " boolean b1 = \"foo\".toCharArray().length == 0;", + " boolean b2 = \"bar\".toCharArray().length == 1;", + " boolean b3 = \"baz\".toCharArray().length == 2;", + " boolean b4 = \"qux\".toCharArray().length == 3;", + " }", + "}") + .addOutputLines( + "A.java", + "class A {", + " void m() {", + " boolean b1 = \"foo\".length() + 1 == 1;", + " boolean b2 = \"bar\".toCharArray().length == 1;", + " boolean b3 = \"baz\".length() == 2;", + " boolean b4 = \"qux\".toCharArray().length == 3;", + " }", + "}") + .doTest(TEXT_MATCH); + } +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java index f213bcbb73..44421d4aa5 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java @@ -16,11 +16,12 @@ * Support website. Annotations on nested classes override the documentation URL associated with any * enclosing class. */ -// XXX: Is the `%s` protocol sufficiently generic for non-Picnic use cases? // XXX: The documentation is misleading, in that the generated anchor isn't mentioned. @Target(ElementType.TYPE) @Retention(RetentionPolicy.SOURCE) public @interface OnlineDocumentation { + // XXX: Introduce placeholder constants! + /** * The URL or URL pattern of the website at which the annotated Refaster template(s) are * documented. @@ -29,5 +30,6 @@ */ // XXX: This default is Error Prone Support-specific. Appropriate? (The alternative is to repeat // this URL pattern in many places.) If we drop this, also update the class documentation. - String value() default "https://error-prone.picnic.tech/refastertemplates/%s"; + String value() default + "https://error-prone.picnic.tech/refastertemplates/${topLevelClassName}#${nestedClassName}"; } diff --git a/refaster-test-support/pom.xml b/refaster-test-support/pom.xml index 7e2359f72b..05960ceff8 100644 --- a/refaster-test-support/pom.xml +++ b/refaster-test-support/pom.xml @@ -40,12 +40,6 @@ ${project.groupId} refaster-runner - - - ${project.groupId} - refaster-support - test - com.google.auto.service auto-service-annotations diff --git a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java index e3d6f3567e..c5a1eace5f 100644 --- a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java +++ b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java @@ -182,8 +182,7 @@ private static ImmutableRangeMap indexTemplateMatches( Set replacements = Iterables.getOnlyElement(description.fixes).getReplacements(endPositions); for (Replacement replacement : replacements) { - templateMatches.put( - replacement.range(), getSubstringAfterFinalDelimiter('.', description.checkName)); + templateMatches.put(replacement.range(), extractRefasterTemplateName(description)); } } @@ -230,6 +229,13 @@ private void reportViolations( state.reportMatch(describeMatch(tree, fixWithComment)); } + private static String extractRefasterTemplateName(Description description) { + String message = description.getRawMessage(); + int index = message.indexOf(':'); + checkState(index >= 0, "String '%s' does not contain character '%s'", message, ':'); + return getSubstringAfterFinalDelimiter('.', message.substring(0, index)); + } + private static String getSubstringAfterFinalDelimiter(char delimiter, String value) { int index = value.lastIndexOf(delimiter); checkState(index >= 0, "String '%s' does not contain character '%s'", value, delimiter); diff --git a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java index 543b24308a..3e202f17dd 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java @@ -2,19 +2,13 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; -import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster template collection to validate reporting of a match occurring in an unexpected place. */ -@OnlineDocumentation final class MatchInWrongMethodTemplates { private MatchInWrongMethodTemplates() {} - // XXX: Demo: nesting overrides. - // @Website("YYY") - // @Severity(ERROR) - // @Description("Fooo!") static final class StringIsEmpty { @BeforeTemplate boolean before(String string) { From abb6cea861bb0375030e07cd5d170b83ab63dca0 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 24 Sep 2022 19:57:44 +0200 Subject: [PATCH 09/53] Properly document URL placeholder usage --- .../AnnotatedCompositeCodeTransformer.java | 8 +++-- .../annotation/OnlineDocumentation.java | 32 +++++++++++++------ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index cfb701fc7e..08c2926188 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -4,6 +4,8 @@ import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static tech.picnic.errorprone.refaster.annotation.OnlineDocumentation.NESTED_CLASS_URL_PLACEHOLDER; +import static tech.picnic.errorprone.refaster.annotation.OnlineDocumentation.TOP_LEVEL_CLASS_URL_PLACEHOLDER; import com.google.common.base.Splitter; import com.google.common.collect.Comparators; @@ -92,10 +94,10 @@ private String getLinkPattern(CodeTransformer delegate, String checkName) { String urlPattern = getAnnotationValue(OnlineDocumentation.class, OnlineDocumentation::value, delegate, ""); - Iterator nameComponents = CLASS_NAME_SPLITTER.splitToList(checkName).iterator(); + Iterator nameComponents = CLASS_NAME_SPLITTER.splitToStream(checkName).iterator(); return urlPattern - .replace("${topLevelClassName}", nameComponents.next()) - .replace("${nestedClassName}", Iterators.getNext(nameComponents, "")); + .replace(TOP_LEVEL_CLASS_URL_PLACEHOLDER, nameComponents.next()) + .replace(NESTED_CLASS_URL_PLACEHOLDER, Iterators.getNext(nameComponents, "")); } private SeverityLevel getSeverity(CodeTransformer delegate) { diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java index 44421d4aa5..ae56f3d621 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java @@ -8,28 +8,42 @@ /** * Signals that a Refaster template or group of Refaster templates comes with online documentation. * - *

The provided value may be a full URL, or a URL pattern containing a single {@code %s} - * placeholder. If a placeholder is present, then it will be replaced with the name of the - * associated Refaster template any time the URL is rendered. + *

The provided value may be a full URL, or a URL pattern containing either or both of the + * {@value TOP_LEVEL_CLASS_URL_PLACEHOLDER} and {@value NESTED_CLASS_URL_PLACEHOLDER} placeholders. * *

By default it is assumed that the Refaster template(s) are documented on the Error Prone * Support website. Annotations on nested classes override the documentation URL associated with any * enclosing class. */ -// XXX: The documentation is misleading, in that the generated anchor isn't mentioned. @Target(ElementType.TYPE) @Retention(RetentionPolicy.SOURCE) public @interface OnlineDocumentation { - // XXX: Introduce placeholder constants! + /** + * The URL placeholder value that will be replaced with the name of the top-level class in which + * the annotated Refaster template is located. + */ + String TOP_LEVEL_CLASS_URL_PLACEHOLDER = "${topLevelClassName}"; + /** + * The URL placeholder value that will be replaced with the name of the nested class in which the + * annotated Refaster template is located, if applicable. + * + *

If the Refaster template is not defined in a nested class then this placeholder will be + * replaced with the empty string. In case the Refaster template is syntactically nested inside a + * deeper hierarchy of classes, then this placeholder will be replaced with concatenation of the + * names of all these classes (except the top-level class name), separated by dots. + */ + String NESTED_CLASS_URL_PLACEHOLDER = "${nestedClassName}"; /** * The URL or URL pattern of the website at which the annotated Refaster template(s) are * documented. * - * @return A non-{@code null} string. + * @return A non-{@code null} string, optionally containing the {@value + * TOP_LEVEL_CLASS_URL_PLACEHOLDER} and {@value NESTED_CLASS_URL_PLACEHOLDER} placeholders. */ - // XXX: This default is Error Prone Support-specific. Appropriate? (The alternative is to repeat - // this URL pattern in many places.) If we drop this, also update the class documentation. String value() default - "https://error-prone.picnic.tech/refastertemplates/${topLevelClassName}#${nestedClassName}"; + "https://error-prone.picnic.tech/refastertemplates/" + + TOP_LEVEL_CLASS_URL_PLACEHOLDER + + '#' + + NESTED_CLASS_URL_PLACEHOLDER; } From cf772c47cdc6dba0679bf4820178de8e77ed15ac Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 24 Sep 2022 22:47:54 +0200 Subject: [PATCH 10/53] Expand test coverage --- .../AnnotatedCompositeCodeTransformer.java | 5 +- refaster-runner/pom.xml | 5 + .../refaster/runner/RefasterTest.java | 98 +++++++++++++++++-- 3 files changed, 100 insertions(+), 8 deletions(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index 08c2926188..8c683156d9 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -27,7 +27,9 @@ import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.annotation.Severity; -// XXX: Rename? Use `@AutoValue`? +// XXX: Can we find a better name for thi class? +// XXX: Use `@AutoValue`? +// XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.) final class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { private static final long serialVersionUID = 1L; private static final Splitter CLASS_NAME_SPLITTER = Splitter.on('.').limit(2); @@ -63,7 +65,6 @@ public void apply(TreePath path, Context context, DescriptionListener listener) private Description augmentDescription( Description description, CodeTransformer delegate, Context context) { - // XXX: Test this. String shortCheckName = getShortCheckName(description.checkName); return Description.builder( description.position, diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index 9e43b5cc68..a27dec810b 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -69,6 +69,11 @@ junit-jupiter-engine test + + org.junit.jupiter + junit-jupiter-params + test + diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index 95047a059c..7239a2751b 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -1,15 +1,23 @@ package tech.picnic.errorprone.refaster.runner; import static com.google.common.base.Predicates.containsPattern; +import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap; import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; +import static java.util.Comparator.naturalOrder; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.params.provider.Arguments.arguments; +import com.google.common.collect.ImmutableList; import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.CompilationTestHelper; +import java.util.regex.Pattern; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; -// XXX: Verify the reported severity level. There does not appear to be a straightforward way to -// distinguish these. -// XXX: Also verify that overriding the severity level works. final class RefasterTest { private final CompilationTestHelper compilationHelper = CompilationTestHelper.newInstance(Refaster.class, getClass()) @@ -52,13 +60,10 @@ void identification() { " void m() {", " // BUG: Diagnostic matches: StringOfSizeZeroTemplate", " boolean b1 = \"foo\".toCharArray().length == 0;", - "", " // BUG: Diagnostic matches: StringOfSizeOneTemplate", " boolean b2 = \"bar\".toCharArray().length == 1;", - "", " // BUG: Diagnostic matches: StringOfSizeTwoTemplate", " boolean b3 = \"baz\".toCharArray().length == 2;", - "", " // BUG: Diagnostic matches: StringOfSizeThreeTemplate", " boolean b4 = \"qux\".toCharArray().length == 3;", " }", @@ -66,6 +71,87 @@ void identification() { .doTest(); } + // XXX: Add test cases for `-XepAllSuggestionsAsWarnings`, conditional on the tests running + // against the Picnic Error Prone fork. + private static Stream reportedSeverityTestCases() { + /* { arguments, expectedSeverities } */ + return Stream.of( + arguments(ImmutableList.of(), ImmutableList.of("Note", "warning", "error", "Note")), + arguments(ImmutableList.of("-Xep:Refaster:OFF"), ImmutableList.of()), + arguments( + ImmutableList.of("-Xep:Refaster:DEFAULT"), + ImmutableList.of("Note", "warning", "error", "Note")), + arguments( + ImmutableList.of("-Xep:Refaster:WARN"), + ImmutableList.of("warning", "warning", "warning", "warning")), + arguments( + ImmutableList.of("-Xep:Refaster:ERROR"), + ImmutableList.of("error", "error", "error", "error")), + arguments( + ImmutableList.of("-XepAllErrorsAsWarnings"), + ImmutableList.of("Note", "warning", "warning", "Note")), + arguments( + ImmutableList.of("-Xep:Refaster:OFF", "-XepAllErrorsAsWarnings"), ImmutableList.of()), + arguments( + ImmutableList.of("-Xep:Refaster:DEFAULT", "-XepAllErrorsAsWarnings"), + ImmutableList.of("Note", "warning", "warning", "Note")), + arguments( + ImmutableList.of("-Xep:Refaster:WARN", "-XepAllErrorsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning")), + arguments( + ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllErrorsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning"))); + } + + /** + * Verifies that the bug checker flags the refactoring opportunities with the appropriate severity + * level. + * + * @implNote This test setup is rather awkward, because {@link CompilationTestHelper} does not + * enable direct assertions against the severity of collected diagnostics output. + */ + @MethodSource("reportedSeverityTestCases") + @ParameterizedTest + void defaultSeverities( + ImmutableList arguments, ImmutableList expectedSeverities) { + assertThatThrownBy( + () -> + compilationHelper + .setArgs(arguments) + .addSourceLines( + "A.java", + "class A {", + " void m() {", + " boolean[] bs = {", + " \"foo\".toCharArray().length == 0,", + " \"bar\".toCharArray().length == 1,", + " \"baz\".toCharArray().length == 2,", + " \"qux\".toCharArray().length == 3", + " };", + " }", + "}") + .doTest()) + .isInstanceOf(AssertionError.class) + .message() + .satisfies( + message -> + assertThat(extractRefasterSeverities("A.java", message)) + .containsExactlyElementsOf(expectedSeverities)); + } + + private static ImmutableList extractRefasterSeverities(String fileName, String message) { + return Pattern.compile( + String.format( + "/%s:(\\d+): (Note|warning|error): \\[Refaster Rule\\]", Pattern.quote(fileName))) + .matcher(message) + .results() + .collect( + toImmutableSortedMap( + naturalOrder(), r -> Integer.parseInt(r.group(1)), r -> r.group(2))) + .values() + .asList(); + } + @Test void replacement() { refactoringTestHelper From d26bc18b0c7b5a76e465168cd8529f5bc2068a22 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 25 Sep 2022 11:00:08 +0200 Subject: [PATCH 11/53] Flag classpath issue --- refaster-runner/pom.xml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index a27dec810b..d71c745250 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -37,6 +37,14 @@ ${project.groupId} refaster-compiler + runtime From 01b7e5b78d81e9518e0de7ae6a7defa74cfc6c87 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 27 Sep 2022 15:21:19 +0200 Subject: [PATCH 12/53] Improve text and minor improvements --- .../errorprone/refaster/runner/RefasterTest.java | 10 +++++----- .../errorprone/refaster/annotation/package-info.java | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index 7239a2751b..3202849d01 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -2,7 +2,6 @@ import static com.google.common.base.Predicates.containsPattern; import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap; -import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; import static java.util.Comparator.naturalOrder; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -10,6 +9,7 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -107,8 +107,8 @@ private static Stream reportedSeverityTestCases() { * Verifies that the bug checker flags the refactoring opportunities with the appropriate severity * level. * - * @implNote This test setup is rather awkward, because {@link CompilationTestHelper} does not - * enable direct assertions against the severity of collected diagnostics output. + * @implNote This test setup is rather cumbersome, because the {@link CompilationTestHelper} does + * not enable direct assertions against the severity of collected diagnostics output. */ @MethodSource("reportedSeverityTestCases") @ParameterizedTest @@ -175,7 +175,7 @@ void replacement() { " boolean b4 = \"qux\".length() == 3;", " }", "}") - .doTest(TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } @Test @@ -201,6 +201,6 @@ void restrictedReplacement() { " boolean b4 = \"qux\".toCharArray().length == 3;", " }", "}") - .doTest(TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } } diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java index b829101a80..3169332926 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java @@ -1,5 +1,5 @@ /** - * A collection annotations that may be placed on Refaster template classes and Refaster template + * A collection of annotations that may be placed on Refaster template classes and Refaster template * collection classes, thus influencing the way in which associated template matches are reported in * non-patch mode. */ From 34826413da4f483b5e3df6792b994b319bcffc79 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 29 Sep 2022 13:35:32 +0200 Subject: [PATCH 13/53] Use `@AutoValue` for the `AnnotatedCompositeCodeTransformer` --- pom.xml | 10 ++++++ refaster-compiler/pom.xml | 4 +++ .../AnnotatedCompositeCodeTransformer.java | 34 ++++++++----------- .../RefasterRuleCompilerTaskListener.java | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/pom.xml b/pom.xml index 85afe299a2..1f5f04399c 100644 --- a/pom.xml +++ b/pom.xml @@ -821,6 +821,11 @@ error_prone_core ${version.error-prone} + + com.google.auto.value + auto-value + ${version.auto-value} + com.google.auto.service auto-service @@ -1534,6 +1539,11 @@ failing the build on the first error encountered. --> -XepAllErrorsAsWarnings + + -XepDisableWarningsInGeneratedCode diff --git a/refaster-compiler/pom.xml b/refaster-compiler/pom.xml index 8a48ea66c2..9eee2b94e1 100644 --- a/refaster-compiler/pom.xml +++ b/refaster-compiler/pom.xml @@ -40,6 +40,10 @@ auto-service-annotations provided + + com.google.auto.value + auto-value-annotations + com.google.code.findbugs jsr305 diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index 8c683156d9..fdd88bfb72 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -7,6 +7,7 @@ import static tech.picnic.errorprone.refaster.annotation.OnlineDocumentation.NESTED_CLASS_URL_PLACEHOLDER; import static tech.picnic.errorprone.refaster.annotation.OnlineDocumentation.TOP_LEVEL_CLASS_URL_PLACEHOLDER; +import com.google.auto.value.AutoValue; import com.google.common.base.Splitter; import com.google.common.collect.Comparators; import com.google.common.collect.ImmutableClassToInstanceMap; @@ -27,34 +28,29 @@ import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.annotation.Severity; -// XXX: Can we find a better name for thi class? -// XXX: Use `@AutoValue`? // XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.) -final class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { +@AutoValue +abstract class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { private static final long serialVersionUID = 1L; private static final Splitter CLASS_NAME_SPLITTER = Splitter.on('.').limit(2); - private final String packageName; - private final ImmutableList transformers; - private final ImmutableClassToInstanceMap annotations; + abstract String packageName(); - AnnotatedCompositeCodeTransformer( + abstract ImmutableList transformers(); + + @Override + public abstract ImmutableClassToInstanceMap annotations(); + + static AnnotatedCompositeCodeTransformer create( String packageName, ImmutableList transformers, ImmutableClassToInstanceMap annotations) { - this.packageName = packageName; - this.transformers = transformers; - this.annotations = annotations; - } - - @Override - public ImmutableClassToInstanceMap annotations() { - return annotations; + return new AutoValue_AnnotatedCompositeCodeTransformer(packageName, transformers, annotations); } @Override public void apply(TreePath path, Context context, DescriptionListener listener) { - for (CodeTransformer transformer : transformers) { + for (CodeTransformer transformer : transformers()) { transformer.apply( path, context, @@ -77,16 +73,16 @@ private Description augmentDescription( } private String getShortCheckName(String fullCheckName) { - if (packageName.isEmpty()) { + if (packageName().isEmpty()) { return fullCheckName; } - String prefix = packageName + '.'; + String prefix = packageName() + '.'; checkState( fullCheckName.startsWith(prefix), "Refaster template class '%s' is not located in package '%s'", fullCheckName, - packageName); + packageName()); return fullCheckName.substring(prefix.length()); } diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java index 6eeeb1f318..515753cc0f 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java @@ -81,7 +81,7 @@ public Void visitClass(ClassTree node, ImmutableClassToInstanceMap a if (!transformers.isEmpty()) { rules.put( node, - new AnnotatedCompositeCodeTransformer( + AnnotatedCompositeCodeTransformer.create( toPackageName(symbol), transformers, annotations)); } From 12d09ad496a64357aa6c649ae0f57af8a09037f9 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 29 Sep 2022 20:22:46 +0200 Subject: [PATCH 14/53] Support `AllSuggestionsAsWarnings` and add a suggestion --- .../AnnotatedCompositeCodeTransformer.java | 26 +++++- .../refaster/runner/RefasterTest.java | 93 +++++++++++++------ .../test/RefasterTemplateCollection.java | 2 +- 3 files changed, 88 insertions(+), 33 deletions(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index fdd88bfb72..9a03e34436 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -22,6 +22,10 @@ import com.sun.tools.javac.util.Context; import java.io.Serializable; import java.lang.annotation.Annotation; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; import java.util.Iterator; import java.util.Optional; import java.util.function.Function; @@ -123,11 +127,25 @@ private static Optional getAnnotationValue( } private static SeverityLevel overrideSeverity(SeverityLevel severity, Context context) { - // XXX: Respect `-XepAllSuggestionsAsWarnings` when using the Picnic Error Prone Fork! - SeverityLevel minSeverity = SUGGESTION; - SeverityLevel maxSeverity = - context.get(ErrorProneOptions.class).isDropErrorsToWarnings() ? WARNING : ERROR; + ErrorProneOptions errorProneOptions = context.get(ErrorProneOptions.class); + SeverityLevel minSeverity = allSuggestionsAsWarnings(errorProneOptions) ? WARNING : SUGGESTION; + SeverityLevel maxSeverity = errorProneOptions.isDropErrorsToWarnings() ? WARNING : ERROR; return Comparators.max(Comparators.min(severity, minSeverity), maxSeverity); } + + private static boolean allSuggestionsAsWarnings(ErrorProneOptions errorProneOptions) { + try { + Optional isSuggestionsAsWarningsMethod = + Arrays.stream(errorProneOptions.getClass().getDeclaredMethods()) + .filter(m -> Modifier.isPublic(m.getModifiers())) + .filter(m -> m.getName().equals("isSuggestionsAsWarnings")) + .findFirst(); + return isSuggestionsAsWarningsMethod.isPresent() + && Boolean.TRUE.equals( + isSuggestionsAsWarningsMethod.orElseThrow().invoke(errorProneOptions)); + } catch (IllegalAccessException | InvocationTargetException e) { + return false; + } + } } diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index 3202849d01..d73aa924d4 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -11,6 +11,8 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; +import java.lang.reflect.Modifier; +import java.util.Arrays; import java.util.regex.Pattern; import java.util.stream.Stream; import org.junit.jupiter.api.Test; @@ -71,36 +73,55 @@ void identification() { .doTest(); } - // XXX: Add test cases for `-XepAllSuggestionsAsWarnings`, conditional on the tests running - // against the Picnic Error Prone fork. private static Stream reportedSeverityTestCases() { /* { arguments, expectedSeverities } */ - return Stream.of( - arguments(ImmutableList.of(), ImmutableList.of("Note", "warning", "error", "Note")), - arguments(ImmutableList.of("-Xep:Refaster:OFF"), ImmutableList.of()), - arguments( - ImmutableList.of("-Xep:Refaster:DEFAULT"), - ImmutableList.of("Note", "warning", "error", "Note")), - arguments( - ImmutableList.of("-Xep:Refaster:WARN"), - ImmutableList.of("warning", "warning", "warning", "warning")), - arguments( - ImmutableList.of("-Xep:Refaster:ERROR"), - ImmutableList.of("error", "error", "error", "error")), - arguments( - ImmutableList.of("-XepAllErrorsAsWarnings"), - ImmutableList.of("Note", "warning", "warning", "Note")), - arguments( - ImmutableList.of("-Xep:Refaster:OFF", "-XepAllErrorsAsWarnings"), ImmutableList.of()), - arguments( - ImmutableList.of("-Xep:Refaster:DEFAULT", "-XepAllErrorsAsWarnings"), - ImmutableList.of("Note", "warning", "warning", "Note")), - arguments( - ImmutableList.of("-Xep:Refaster:WARN", "-XepAllErrorsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning")), - arguments( - ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllErrorsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning"))); + + Stream forkTestCases = + isBuiltWithErrorProneFork() + ? Stream.of( + arguments( + ImmutableList.of("-Xep:Refaster:OFF", "-XepAllSuggestionsAsWarnings"), + ImmutableList.of()), + arguments( + ImmutableList.of("-Xep:Refaster:DEFAULT", "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("warning", "warning", "error", "warning")), + arguments( + ImmutableList.of("-Xep:Refaster:WARN", "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning")), + arguments( + ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("error", "error", "error", "error"))) + : Stream.empty(); + + return Stream.concat( + Stream.of( + arguments(ImmutableList.of(), ImmutableList.of("Note", "warning", "error", "Note")), + arguments(ImmutableList.of("-Xep:Refaster:OFF"), ImmutableList.of()), + arguments( + ImmutableList.of("-Xep:Refaster:DEFAULT"), + ImmutableList.of("Note", "warning", "error", "Note")), + arguments( + ImmutableList.of("-Xep:Refaster:WARN"), + ImmutableList.of("warning", "warning", "warning", "warning")), + arguments( + ImmutableList.of("-Xep:Refaster:ERROR"), + ImmutableList.of("error", "error", "error", "error")), + arguments( + ImmutableList.of("-XepAllErrorsAsWarnings"), + ImmutableList.of("Note", "warning", "warning", "Note")), + arguments( + ImmutableList.of("-Xep:Refaster:OFF", "-XepAllErrorsAsWarnings"), + ImmutableList.of()), + arguments( + ImmutableList.of("-Xep:Refaster:DEFAULT", "-XepAllErrorsAsWarnings"), + ImmutableList.of("Note", "warning", "warning", "Note")), + arguments( + ImmutableList.of("-Xep:Refaster:WARN", "-XepAllErrorsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning")), + arguments( + ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllErrorsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning"))), + forkTestCases); } /** @@ -203,4 +224,20 @@ void restrictedReplacement() { "}") .doTest(TestMode.TEXT_MATCH); } + + private static boolean isBuiltWithErrorProneFork() { + Class clazz; + try { + clazz = + Class.forName( + "com.google.errorprone.ErrorProneOptions", + /* initialize= */ false, + Thread.currentThread().getContextClassLoader()); + } catch (ClassNotFoundException e) { + return false; + } + return Arrays.stream(clazz.getDeclaredMethods()) + .filter(m -> Modifier.isPublic(m.getModifiers())) + .anyMatch(m -> m.getName().equals("isSuggestionsAsWarnings")); + } } diff --git a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java index c5a1eace5f..811a631883 100644 --- a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java +++ b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java @@ -232,7 +232,7 @@ private void reportViolations( private static String extractRefasterTemplateName(Description description) { String message = description.getRawMessage(); int index = message.indexOf(':'); - checkState(index >= 0, "String '%s' does not contain character '%s'", message, ':'); + checkState(index >= 0, "String '%s' does not contain character ':'", message); return getSubstringAfterFinalDelimiter('.', message.substring(0, index)); } From 74d6c9a46b9670196f13dbe30fbddbd9d3001ce5 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 29 Sep 2022 22:42:25 +0200 Subject: [PATCH 15/53] Tweak --- .../tech/picnic/errorprone/refaster/runner/RefasterTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index d73aa924d4..4c99a90032 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -234,7 +234,7 @@ private static boolean isBuiltWithErrorProneFork() { /* initialize= */ false, Thread.currentThread().getContextClassLoader()); } catch (ClassNotFoundException e) { - return false; + throw new IllegalStateException("Can't load `ErrorProneOptions` class", e); } return Arrays.stream(clazz.getDeclaredMethods()) .filter(m -> Modifier.isPublic(m.getModifiers())) From c4b6a5fbe4aa13677e05be8a5b6c376ea5bab03c Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 29 Sep 2022 23:24:05 +0200 Subject: [PATCH 16/53] Suggestions, introduce `ErrorProneFork` --- pom.xml | 7 ++ .../AnnotatedCompositeCodeTransformer.java | 27 ++----- .../refaster/plugin/ErrorProneFork.java | 58 ++++++++++++++ .../refaster/runner/RefasterTest.java | 77 ++++++++++--------- .../test/RefasterTemplateCollection.java | 5 +- 5 files changed, 113 insertions(+), 61 deletions(-) create mode 100644 refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java diff --git a/pom.xml b/pom.xml index 1f5f04399c..c5b589a1da 100644 --- a/pom.xml +++ b/pom.xml @@ -622,6 +622,10 @@ + + + @@ -1262,6 +1266,9 @@ pitest-maven 1.9.7 + + *.AutoValue_* + diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index 9a03e34436..3ab7a7e17d 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -22,16 +22,13 @@ import com.sun.tools.javac.util.Context; import java.io.Serializable; import java.lang.annotation.Annotation; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; -import java.lang.reflect.Modifier; -import java.util.Arrays; import java.util.Iterator; import java.util.Optional; import java.util.function.Function; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.annotation.Severity; +// XXX: Can we find a better name for this class? `CompositeAnnotatedCodeTransformer`, ...? // XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.) @AutoValue abstract class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { @@ -127,25 +124,11 @@ private static Optional getAnnotationValue( } private static SeverityLevel overrideSeverity(SeverityLevel severity, Context context) { - ErrorProneOptions errorProneOptions = context.get(ErrorProneOptions.class); - SeverityLevel minSeverity = allSuggestionsAsWarnings(errorProneOptions) ? WARNING : SUGGESTION; - SeverityLevel maxSeverity = errorProneOptions.isDropErrorsToWarnings() ? WARNING : ERROR; + ErrorProneOptions options = context.get(ErrorProneOptions.class); + SeverityLevel minSeverity = + ErrorProneFork.isSuggestionsAsWarningsEnabled(options) ? WARNING : SUGGESTION; + SeverityLevel maxSeverity = options.isDropErrorsToWarnings() ? WARNING : ERROR; return Comparators.max(Comparators.min(severity, minSeverity), maxSeverity); } - - private static boolean allSuggestionsAsWarnings(ErrorProneOptions errorProneOptions) { - try { - Optional isSuggestionsAsWarningsMethod = - Arrays.stream(errorProneOptions.getClass().getDeclaredMethods()) - .filter(m -> Modifier.isPublic(m.getModifiers())) - .filter(m -> m.getName().equals("isSuggestionsAsWarnings")) - .findFirst(); - return isSuggestionsAsWarningsMethod.isPresent() - && Boolean.TRUE.equals( - isSuggestionsAsWarningsMethod.orElseThrow().invoke(errorProneOptions)); - } catch (IllegalAccessException | InvocationTargetException e) { - return false; - } - } } diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java new file mode 100644 index 0000000000..3b8711fc5a --- /dev/null +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java @@ -0,0 +1,58 @@ +package tech.picnic.errorprone.refaster.plugin; + +import com.google.errorprone.ErrorProneOptions; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.Optional; + +/** + * Utility class that enables the runtime to determine whether Picnic's fork of Error Prone is on + * the classpath. + * + * @see Picnic's Error Prone fork + */ +// XXX: Add tests for this class. We can at least test `#isErrorProneForkAvailable()` by having +// Maven inject a corresponding system property. +public final class ErrorProneFork { + private static final Optional ERROR_PRONE_OPTIONS_IS_SUGGESTIONS_AS_WARNINGS_METHOD = + Arrays.stream(ErrorProneOptions.class.getDeclaredMethods()) + .filter(m -> Modifier.isPublic(m.getModifiers())) + .filter(m -> "isSuggestionsAsWarnings".equals(m.getName())) + .findFirst(); + + private ErrorProneFork() {} + + /** + * Tells whether Picnic's fork of Error Prone is available. + * + * @return {@code true} iff classpath introspection indicates the presence of Error Prone + * modifications that are assumed to be present only in Picnic's fork. + */ + public static boolean isErrorProneForkAvailable() { + return ERROR_PRONE_OPTIONS_IS_SUGGESTIONS_AS_WARNINGS_METHOD.isPresent(); + } + + /** + * Tells whether the custom {@code -XepAllSuggestionsAsWarnings} flag is set. + * + * @param options The currently active Error Prone options. + * @return {@code true} iff {@link #isErrorProneForkAvailable() the Error Prone fork is available} + * and the aforementioned flag is configured. + * @see google/error-prone#3301 + */ + public static boolean isSuggestionsAsWarningsEnabled(ErrorProneOptions options) { + return ERROR_PRONE_OPTIONS_IS_SUGGESTIONS_AS_WARNINGS_METHOD + .filter(m -> Boolean.TRUE.equals(invoke(m, options))) + .isPresent(); + } + + private static Object invoke(Method method, Object obj, Object... args) { + try { + return method.invoke(obj, args); + } catch (IllegalAccessException | InvocationTargetException e) { + throw new IllegalStateException(String.format("Failed to invoke method '%s'", method), e); + } + } +} diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index 4c99a90032..26d221f3cf 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -11,14 +11,13 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; -import java.lang.reflect.Modifier; -import java.util.Arrays; import java.util.regex.Pattern; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import tech.picnic.errorprone.refaster.plugin.ErrorProneFork; final class RefasterTest { private final CompilationTestHelper compilationHelper = @@ -75,24 +74,6 @@ void identification() { private static Stream reportedSeverityTestCases() { /* { arguments, expectedSeverities } */ - - Stream forkTestCases = - isBuiltWithErrorProneFork() - ? Stream.of( - arguments( - ImmutableList.of("-Xep:Refaster:OFF", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of()), - arguments( - ImmutableList.of("-Xep:Refaster:DEFAULT", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("warning", "warning", "error", "warning")), - arguments( - ImmutableList.of("-Xep:Refaster:WARN", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning")), - arguments( - ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("error", "error", "error", "error"))) - : Stream.empty(); - return Stream.concat( Stream.of( arguments(ImmutableList.of(), ImmutableList.of("Note", "warning", "error", "Note")), @@ -121,7 +102,45 @@ private static Stream reportedSeverityTestCases() { arguments( ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllErrorsAsWarnings"), ImmutableList.of("warning", "warning", "warning", "warning"))), - forkTestCases); + ErrorProneFork.isErrorProneForkAvailable() + ? Stream.of( + arguments( + ImmutableList.of("-Xep:Refaster:OFF", "-XepAllSuggestionsAsWarnings"), + ImmutableList.of()), + arguments( + ImmutableList.of("-Xep:Refaster:DEFAULT", "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("warning", "warning", "error", "warning")), + arguments( + ImmutableList.of("-Xep:Refaster:WARN", "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning")), + arguments( + ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("error", "error", "error", "error")), + arguments( + ImmutableList.of( + "-Xep:Refaster:OFF", + "-XepAllErrorsAsWarnings", + "-XepAllSuggestionsAsWarnings"), + ImmutableList.of()), + arguments( + ImmutableList.of( + "-Xep:Refaster:DEFAULT", + "-XepAllErrorsAsWarnings", + "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning")), + arguments( + ImmutableList.of( + "-Xep:Refaster:WARN", + "-XepAllErrorsAsWarnings", + "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning")), + arguments( + ImmutableList.of( + "-Xep:Refaster:ERROR", + "-XepAllErrorsAsWarnings", + "-XepAllSuggestionsAsWarnings"), + ImmutableList.of("warning", "warning", "warning", "warning"))) + : Stream.empty()); } /** @@ -224,20 +243,4 @@ void restrictedReplacement() { "}") .doTest(TestMode.TEXT_MATCH); } - - private static boolean isBuiltWithErrorProneFork() { - Class clazz; - try { - clazz = - Class.forName( - "com.google.errorprone.ErrorProneOptions", - /* initialize= */ false, - Thread.currentThread().getContextClassLoader()); - } catch (ClassNotFoundException e) { - throw new IllegalStateException("Can't load `ErrorProneOptions` class", e); - } - return Arrays.stream(clazz.getDeclaredMethods()) - .filter(m -> Modifier.isPublic(m.getModifiers())) - .anyMatch(m -> m.getName().equals("isSuggestionsAsWarnings")); - } } diff --git a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java index 811a631883..7cf467c647 100644 --- a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java +++ b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java @@ -179,10 +179,11 @@ private static ImmutableRangeMap indexTemplateMatches( ImmutableRangeMap.Builder templateMatches = ImmutableRangeMap.builder(); for (Description description : matches) { + String templateName = extractRefasterTemplateName(description); Set replacements = Iterables.getOnlyElement(description.fixes).getReplacements(endPositions); for (Replacement replacement : replacements) { - templateMatches.put(replacement.range(), extractRefasterTemplateName(description)); + templateMatches.put(replacement.range(), templateName); } } @@ -232,7 +233,7 @@ private void reportViolations( private static String extractRefasterTemplateName(Description description) { String message = description.getRawMessage(); int index = message.indexOf(':'); - checkState(index >= 0, "String '%s' does not contain character ':'", message); + checkState(index >= 0, "Failed to extract Refaster template name from string '%s'", message); return getSubstringAfterFinalDelimiter('.', message.substring(0, index)); } From d899a8a10c9aa2286c2c9513d97a43ade2acd03f Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 29 Sep 2022 23:34:54 +0200 Subject: [PATCH 17/53] Also this --- .../picnic/errorprone/refaster/annotation/package-info.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java index 3169332926..7178ba1cb9 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java @@ -1,5 +1,5 @@ /** - * A collection of annotations that may be placed on Refaster template classes and Refaster template + * A collection of annotations that can be placed on Refaster template classes and Refaster template * collection classes, thus influencing the way in which associated template matches are reported in * non-patch mode. */ From 20d194b4d46c90823468f065fa7296a0e0d1ec68 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 30 Sep 2022 06:46:22 +0200 Subject: [PATCH 18/53] Clarify how the default Refaster hit severity comes to be Plus other tweaks. --- .../AnnotatedCompositeCodeTransformer.java | 8 +- .../errorprone/refaster/runner/Refaster.java | 7 +- .../refaster/runner/RefasterTest.java | 78 ++++++++++++------- .../refaster/annotation/Severity.java | 5 +- 4 files changed, 66 insertions(+), 32 deletions(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index 3ab7a7e17d..658ab289bb 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -28,7 +28,8 @@ import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.annotation.Severity; -// XXX: Can we find a better name for this class? `CompositeAnnotatedCodeTransformer`, ...? +// XXX: Can we find a better name for this class? `CompositeAnnotatedCodeTransformer`, +// `AugmentedCompositeCodeTransformer`, ...? // XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.) @AutoValue abstract class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { @@ -99,6 +100,11 @@ private String getLinkPattern(CodeTransformer delegate, String checkName) { } private SeverityLevel getSeverity(CodeTransformer delegate) { + /* + * The default severity should be kept in sync with the default severity of the + * `tech.picnic.errorprone.refaster.runner.Refaster` bug checker. (The associated + * `RefasterTest#severityAssignment` test verifies this invariant.) + */ return getAnnotationValue(Severity.class, Severity::value, delegate, SUGGESTION); } diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 9b2a780f1f..e9b54ce201 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -153,9 +153,10 @@ private static Optional toSeverityLevel(Severity severity) { * checker's severity was explicitly configured. * *

The original check name (i.e. the Refaster template name) is prepended to the {@link - * Description}'s message. The replacement check name ("Refaster Template") is chosen such that it - * is guaranteed not to match any canonical bug checker name (as that could cause {@link - * VisitorState#reportMatch(Description)}} to override the reported severity). + * Description}'s message. The replacement check name ("Refaster Template", a name which includes + * a space) is chosen such that it is guaranteed not to match any canonical bug checker name (as + * that could cause {@link VisitorState#reportMatch(Description)}} to override the reported + * severity). */ private static Description augmentDescription( Description description, Optional severityOverride) { diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index 26d221f3cf..284fc60246 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -1,15 +1,20 @@ package tech.picnic.errorprone.refaster.runner; import static com.google.common.base.Predicates.containsPattern; -import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap; -import static java.util.Comparator.naturalOrder; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static java.util.Comparator.comparingInt; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.params.provider.Arguments.arguments; import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugCheckerInfo; import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.CompilationTestHelper; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -72,36 +77,44 @@ void identification() { .doTest(); } - private static Stream reportedSeverityTestCases() { + private static Stream severityAssignmentTestCases() { + /* + * The _actual_ default severity is assigned by the `CodeTransformer`s to which the `Refaster` + * bug checker delegates. Here we verify that the absence of an `@Severity` annotation yields + * the same severity as the bug checker's declared severity. + */ + SeverityLevel defaultSeverity = BugCheckerInfo.create(Refaster.class).defaultSeverity(); + /* { arguments, expectedSeverities } */ return Stream.concat( Stream.of( - arguments(ImmutableList.of(), ImmutableList.of("Note", "warning", "error", "Note")), + arguments( + ImmutableList.of(), ImmutableList.of(defaultSeverity, WARNING, ERROR, SUGGESTION)), arguments(ImmutableList.of("-Xep:Refaster:OFF"), ImmutableList.of()), arguments( ImmutableList.of("-Xep:Refaster:DEFAULT"), - ImmutableList.of("Note", "warning", "error", "Note")), + ImmutableList.of(defaultSeverity, WARNING, ERROR, SUGGESTION)), arguments( ImmutableList.of("-Xep:Refaster:WARN"), - ImmutableList.of("warning", "warning", "warning", "warning")), + ImmutableList.of(WARNING, WARNING, WARNING, WARNING)), arguments( ImmutableList.of("-Xep:Refaster:ERROR"), - ImmutableList.of("error", "error", "error", "error")), + ImmutableList.of(ERROR, ERROR, ERROR, ERROR)), arguments( ImmutableList.of("-XepAllErrorsAsWarnings"), - ImmutableList.of("Note", "warning", "warning", "Note")), + ImmutableList.of(defaultSeverity, WARNING, WARNING, SUGGESTION)), arguments( ImmutableList.of("-Xep:Refaster:OFF", "-XepAllErrorsAsWarnings"), ImmutableList.of()), arguments( ImmutableList.of("-Xep:Refaster:DEFAULT", "-XepAllErrorsAsWarnings"), - ImmutableList.of("Note", "warning", "warning", "Note")), + ImmutableList.of(defaultSeverity, WARNING, WARNING, SUGGESTION)), arguments( ImmutableList.of("-Xep:Refaster:WARN", "-XepAllErrorsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning")), + ImmutableList.of(WARNING, WARNING, WARNING, WARNING)), arguments( ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllErrorsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning"))), + ImmutableList.of(WARNING, WARNING, WARNING, WARNING))), ErrorProneFork.isErrorProneForkAvailable() ? Stream.of( arguments( @@ -109,13 +122,13 @@ private static Stream reportedSeverityTestCases() { ImmutableList.of()), arguments( ImmutableList.of("-Xep:Refaster:DEFAULT", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("warning", "warning", "error", "warning")), + ImmutableList.of(WARNING, WARNING, ERROR, WARNING)), arguments( ImmutableList.of("-Xep:Refaster:WARN", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning")), + ImmutableList.of(WARNING, WARNING, WARNING, WARNING)), arguments( ImmutableList.of("-Xep:Refaster:ERROR", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("error", "error", "error", "error")), + ImmutableList.of(ERROR, ERROR, ERROR, ERROR)), arguments( ImmutableList.of( "-Xep:Refaster:OFF", @@ -127,19 +140,19 @@ private static Stream reportedSeverityTestCases() { "-Xep:Refaster:DEFAULT", "-XepAllErrorsAsWarnings", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning")), + ImmutableList.of(WARNING, WARNING, WARNING, WARNING)), arguments( ImmutableList.of( "-Xep:Refaster:WARN", "-XepAllErrorsAsWarnings", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning")), + ImmutableList.of(WARNING, WARNING, WARNING, WARNING)), arguments( ImmutableList.of( "-Xep:Refaster:ERROR", "-XepAllErrorsAsWarnings", "-XepAllSuggestionsAsWarnings"), - ImmutableList.of("warning", "warning", "warning", "warning"))) + ImmutableList.of(WARNING, WARNING, WARNING, WARNING))) : Stream.empty()); } @@ -150,10 +163,10 @@ private static Stream reportedSeverityTestCases() { * @implNote This test setup is rather cumbersome, because the {@link CompilationTestHelper} does * not enable direct assertions against the severity of collected diagnostics output. */ - @MethodSource("reportedSeverityTestCases") + @MethodSource("severityAssignmentTestCases") @ParameterizedTest - void defaultSeverities( - ImmutableList arguments, ImmutableList expectedSeverities) { + void severityAssignment( + ImmutableList arguments, ImmutableList expectedSeverities) { assertThatThrownBy( () -> compilationHelper @@ -179,17 +192,30 @@ void defaultSeverities( .containsExactlyElementsOf(expectedSeverities)); } - private static ImmutableList extractRefasterSeverities(String fileName, String message) { + private static ImmutableList extractRefasterSeverities( + String fileName, String message) { return Pattern.compile( String.format( "/%s:(\\d+): (Note|warning|error): \\[Refaster Rule\\]", Pattern.quote(fileName))) .matcher(message) .results() - .collect( - toImmutableSortedMap( - naturalOrder(), r -> Integer.parseInt(r.group(1)), r -> r.group(2))) - .values() - .asList(); + .sorted(comparingInt(r -> Integer.parseInt(r.group(1)))) + .map(r -> toSeverityLevel(r.group(2))) + .collect(toImmutableList()); + } + + private static SeverityLevel toSeverityLevel(String compilerDiagnosticsPrefix) { + switch (compilerDiagnosticsPrefix) { + case "Note": + return SUGGESTION; + case "warning": + return WARNING; + case "error": + return ERROR; + default: + throw new IllegalStateException( + String.format("Unrecognized diagnostics prefix '%s'", compilerDiagnosticsPrefix)); + } } @Test diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java index 9bf4398181..bb493fda04 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java @@ -9,8 +9,9 @@ /** * Describes the severity of a Refaster template or group of Refaster templates. * - *

The default severity is {@link SeverityLevel#SUGGESTION}. Annotations on nested classes - * override the severity associated with any enclosing class. + *

The default severity is the severity assigned to the {@code Refaster} bug checker, which may + * be controlled explicitly by running Error Prone with e.g. {@code -Xep:Refaster:WARN}. Annotations + * on nested classes override the severity associated with any enclosing class. */ @Target(ElementType.TYPE) @Retention(RetentionPolicy.SOURCE) From ad1c98d3eb9afd47e8f0b9ded6debf07d0bca04b Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Fri, 30 Sep 2022 12:02:42 +0200 Subject: [PATCH 19/53] Align documentation of reported description names by the Refaster check --- .../java/tech/picnic/errorprone/refaster/runner/Refaster.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index e9b54ce201..810c761fcd 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -153,8 +153,8 @@ private static Optional toSeverityLevel(Severity severity) { * checker's severity was explicitly configured. * *

The original check name (i.e. the Refaster template name) is prepended to the {@link - * Description}'s message. The replacement check name ("Refaster Template", a name which includes - * a space) is chosen such that it is guaranteed not to match any canonical bug checker name (as + * Description}'s message. The replacement check name ("Refaster Rule", a name which includes a + * space) is chosen such that it is guaranteed not to match any canonical bug checker name (as * that could cause {@link VisitorState#reportMatch(Description)}} to override the reported * severity). */ From 50443d179fbe8d32ebb08f5430cd7aa30dedb237 Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Fri, 30 Sep 2022 15:19:45 +0200 Subject: [PATCH 20/53] Suggestion --- .../tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java index 3b8711fc5a..0751e0425e 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java @@ -39,7 +39,7 @@ public static boolean isErrorProneForkAvailable() { * * @param options The currently active Error Prone options. * @return {@code true} iff {@link #isErrorProneForkAvailable() the Error Prone fork is available} - * and the aforementioned flag is configured. + * and the aforementioned flag is set. * @see google/error-prone#3301 */ public static boolean isSuggestionsAsWarningsEnabled(ErrorProneOptions options) { From 7e49b08a7e9c9e3f454a941b62b65dd43b8ec003 Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Fri, 30 Sep 2022 18:44:21 +0200 Subject: [PATCH 21/53] Pass null for urlLink to Description.Builder --- .../AnnotatedCompositeCodeTransformer.java | 35 ++++++++++--------- .../refaster/runner/RefasterTest.java | 2 +- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index 658ab289bb..ff4a136318 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -67,7 +67,7 @@ private Description augmentDescription( return Description.builder( description.position, shortCheckName, - getLinkPattern(delegate, shortCheckName), + getLinkPattern(delegate, shortCheckName).orElse(null), overrideSeverity(getSeverity(delegate), context), getDescription(delegate)) .addAllFixes(description.fixes) @@ -89,14 +89,16 @@ private String getShortCheckName(String fullCheckName) { return fullCheckName.substring(prefix.length()); } - private String getLinkPattern(CodeTransformer delegate, String checkName) { - String urlPattern = - getAnnotationValue(OnlineDocumentation.class, OnlineDocumentation::value, delegate, ""); - + private Optional getLinkPattern(CodeTransformer delegate, String checkName) { Iterator nameComponents = CLASS_NAME_SPLITTER.splitToStream(checkName).iterator(); - return urlPattern - .replace(TOP_LEVEL_CLASS_URL_PLACEHOLDER, nameComponents.next()) - .replace(NESTED_CLASS_URL_PLACEHOLDER, Iterators.getNext(nameComponents, "")); + return getAnnotationValue(OnlineDocumentation.class, OnlineDocumentation::value, delegate) + .map( + urlPattern -> + urlPattern.replace(TOP_LEVEL_CLASS_URL_PLACEHOLDER, nameComponents.next())) + .map( + urlPattern -> + urlPattern.replace( + NESTED_CLASS_URL_PLACEHOLDER, Iterators.getNext(nameComponents, ""))); } private SeverityLevel getSeverity(CodeTransformer delegate) { @@ -105,23 +107,22 @@ private SeverityLevel getSeverity(CodeTransformer delegate) { * `tech.picnic.errorprone.refaster.runner.Refaster` bug checker. (The associated * `RefasterTest#severityAssignment` test verifies this invariant.) */ - return getAnnotationValue(Severity.class, Severity::value, delegate, SUGGESTION); + return getAnnotationValue(Severity.class, Severity::value, delegate).orElse(SUGGESTION); } private String getDescription(CodeTransformer delegate) { return getAnnotationValue( - tech.picnic.errorprone.refaster.annotation.Description.class, - tech.picnic.errorprone.refaster.annotation.Description::value, - delegate, - "Refactoring opportunity"); + tech.picnic.errorprone.refaster.annotation.Description.class, + tech.picnic.errorprone.refaster.annotation.Description::value, + delegate) + .orElse("Refactoring opportunity"); } - private T getAnnotationValue( - Class annotation, Function extractor, CodeTransformer delegate, T defaultValue) { + private Optional getAnnotationValue( + Class annotation, Function extractor, CodeTransformer delegate) { return getAnnotationValue(delegate, annotation) .or(() -> getAnnotationValue(this, annotation)) - .map(extractor) - .orElse(defaultValue); + .map(extractor); } private static Optional getAnnotationValue( diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index 284fc60246..fa965e12ea 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -31,7 +31,7 @@ final class RefasterTest { .expectErrorMessage( "StringOfSizeZeroTemplate", containsPattern( - "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeZeroTemplate: Refactoring opportunity\\s+.+\\s+null")) + "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeZeroTemplate: Refactoring opportunity\\s+.+\\s+")) .expectErrorMessage( "StringOfSizeOneTemplate", containsPattern( From 594f51c9d00bb03e92013293269e7b1fb3abdf5f Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 30 Sep 2022 21:54:50 +0200 Subject: [PATCH 22/53] Tweaks --- .../plugin/AnnotatedCompositeCodeTransformer.java | 9 +++------ .../tech/picnic/errorprone/refaster/runner/Refaster.java | 8 +++++--- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java index ff4a136318..515e569d4d 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java @@ -92,13 +92,10 @@ private String getShortCheckName(String fullCheckName) { private Optional getLinkPattern(CodeTransformer delegate, String checkName) { Iterator nameComponents = CLASS_NAME_SPLITTER.splitToStream(checkName).iterator(); return getAnnotationValue(OnlineDocumentation.class, OnlineDocumentation::value, delegate) + .map(url -> url.replace(TOP_LEVEL_CLASS_URL_PLACEHOLDER, nameComponents.next())) .map( - urlPattern -> - urlPattern.replace(TOP_LEVEL_CLASS_URL_PLACEHOLDER, nameComponents.next())) - .map( - urlPattern -> - urlPattern.replace( - NESTED_CLASS_URL_PLACEHOLDER, Iterators.getNext(nameComponents, ""))); + url -> + url.replace(NESTED_CLASS_URL_PLACEHOLDER, Iterators.getNext(nameComponents, ""))); } private SeverityLevel getSeverity(CodeTransformer delegate) { diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 810c761fcd..a058470386 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -148,9 +148,11 @@ private static Optional toSeverityLevel(Severity severity) { } /** - * Updates the given {@link Description}'s details such that {@link - * VisitorState#reportMatch(Description)} will override the reported severity only if this bug - * checker's severity was explicitly configured. + * Updates the given {@link Description}'s details by standardizing the reported check name, + * updating the associated message, and optionally overriding its severity. + * + *

The assigned severity is overridden only if this bug checker's severity was explicitly + * configured. * *

The original check name (i.e. the Refaster template name) is prepended to the {@link * Description}'s message. The replacement check name ("Refaster Rule", a name which includes a From e6caceba606ddead2e14eadd9e03a846823db031 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 12:44:17 +0200 Subject: [PATCH 23/53] Move `AnnotatedCompositeCodeTransformer` and `ErrorProneFork` to `refaster-support` --- refaster-compiler/pom.xml | 8 ------- .../RefasterRuleCompilerTaskListener.java | 1 + refaster-runner/pom.xml | 14 +++++------- .../refaster/runner/RefasterTest.java | 2 +- refaster-support/pom.xml | 5 +++++ .../AnnotatedCompositeCodeTransformer.java | 22 ++++++++++++++++--- .../errorprone/refaster}/ErrorProneFork.java | 2 +- 7 files changed, 32 insertions(+), 22 deletions(-) rename {refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin => refaster-support/src/main/java/tech/picnic/errorprone/refaster}/AnnotatedCompositeCodeTransformer.java (85%) rename {refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin => refaster-support/src/main/java/tech/picnic/errorprone/refaster}/ErrorProneFork.java (97%) diff --git a/refaster-compiler/pom.xml b/refaster-compiler/pom.xml index 9eee2b94e1..9a6fbd9798 100644 --- a/refaster-compiler/pom.xml +++ b/refaster-compiler/pom.xml @@ -14,10 +14,6 @@ A Java compiler plugin that identifies and compiles Refaster templates, storing them as resource files on the classpath. - - ${groupId.error-prone} - error_prone_annotation - ${groupId.error-prone} error_prone_annotations @@ -40,10 +36,6 @@ auto-service-annotations provided - - com.google.auto.value - auto-value-annotations - com.google.code.findbugs jsr305 diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java index 515753cc0f..ac8dccb5d1 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java +++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java @@ -32,6 +32,7 @@ import javax.tools.FileObject; import javax.tools.JavaFileManager; import javax.tools.StandardLocation; +import tech.picnic.errorprone.refaster.AnnotatedCompositeCodeTransformer; /** * A variant of {@code com.google.errorprone.refaster.RefasterRuleCompilerAnalyzer} that stores diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index d71c745250..553d699067 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -37,15 +37,11 @@ ${project.groupId} refaster-compiler - - runtime + provided + + + ${project.groupId} + refaster-support com.google.auto.service diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index fa965e12ea..3dbc78738b 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -22,7 +22,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; -import tech.picnic.errorprone.refaster.plugin.ErrorProneFork; +import tech.picnic.errorprone.refaster.ErrorProneFork; final class RefasterTest { private final CompilationTestHelper compilationHelper = diff --git a/refaster-support/pom.xml b/refaster-support/pom.xml index a4e65ea140..8131ec25bd 100644 --- a/refaster-support/pom.xml +++ b/refaster-support/pom.xml @@ -46,6 +46,11 @@ error_prone_test_helpers test + + com.google.auto.value + auto-value-annotations + provided + com.google.code.findbugs jsr305 diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java similarity index 85% rename from refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java rename to refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java index 515e569d4d..d49ac4030b 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java @@ -1,4 +1,4 @@ -package tech.picnic.errorprone.refaster.plugin; +package tech.picnic.errorprone.refaster; import static com.google.common.base.Preconditions.checkState; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; @@ -15,6 +15,7 @@ import com.google.common.collect.Iterators; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.CodeTransformer; +import com.google.errorprone.CompositeCodeTransformer; import com.google.errorprone.DescriptionListener; import com.google.errorprone.ErrorProneOptions; import com.google.errorprone.matchers.Description; @@ -28,11 +29,17 @@ import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.annotation.Severity; +/** + * A {@link CompositeCodeTransformer} that can be augmented with annotations. + * + *

The information of the annotations can be used for enriched compilation warnings or errors, + * and documentation purposes. + */ // XXX: Can we find a better name for this class? `CompositeAnnotatedCodeTransformer`, // `AugmentedCompositeCodeTransformer`, ...? // XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.) @AutoValue -abstract class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { +public abstract class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { private static final long serialVersionUID = 1L; private static final Splitter CLASS_NAME_SPLITTER = Splitter.on('.').limit(2); @@ -43,13 +50,22 @@ abstract class AnnotatedCompositeCodeTransformer implements CodeTransformer, Ser @Override public abstract ImmutableClassToInstanceMap annotations(); - static AnnotatedCompositeCodeTransformer create( + /** + * Creates an instance of an {@link AnnotatedCompositeCodeTransformer}. + * + * @param packageName The package in which the {@link CodeTransformer} resides. + * @param transformers The {@link CodeTransformer}s in this {@link CompositeCodeTransformer}. + * @param annotations The annotations that are applicable on this {@link CodeTransformer}. + * @return An {@link AnnotatedCompositeCodeTransformer} that may be augmented with annotations. + */ + public static AnnotatedCompositeCodeTransformer create( String packageName, ImmutableList transformers, ImmutableClassToInstanceMap annotations) { return new AutoValue_AnnotatedCompositeCodeTransformer(packageName, transformers, annotations); } + /** {@inheritDoc} */ @Override public void apply(TreePath path, Context context, DescriptionListener listener) { for (CodeTransformer transformer : transformers()) { diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/ErrorProneFork.java similarity index 97% rename from refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java rename to refaster-support/src/main/java/tech/picnic/errorprone/refaster/ErrorProneFork.java index 0751e0425e..287abe7952 100644 --- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/ErrorProneFork.java @@ -1,4 +1,4 @@ -package tech.picnic.errorprone.refaster.plugin; +package tech.picnic.errorprone.refaster; import com.google.errorprone.ErrorProneOptions; import java.lang.reflect.InvocationTargetException; From b2ef63107b06cdd9941b54ede6f1c258b615cf79 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 13:24:01 +0200 Subject: [PATCH 24/53] `s/information/content/` --- .../refaster/AnnotatedCompositeCodeTransformer.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java index d49ac4030b..d831533823 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java @@ -32,8 +32,8 @@ /** * A {@link CompositeCodeTransformer} that can be augmented with annotations. * - *

The information of the annotations can be used for enriched compilation warnings or errors, - * and documentation purposes. + *

The content of the annotations can be used for enriched compilation warnings or errors, and + * documentation purposes. */ // XXX: Can we find a better name for this class? `CompositeAnnotatedCodeTransformer`, // `AugmentedCompositeCodeTransformer`, ...? From 8bd88bbe01335a1bfeded47945662d5aae139132 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 13:54:45 +0200 Subject: [PATCH 25/53] Improve Javadoc `AnnotatedCompositeCodeTransformer` --- .../errorprone/refaster/AnnotatedCompositeCodeTransformer.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java index d831533823..64023fbbeb 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java @@ -32,8 +32,7 @@ /** * A {@link CompositeCodeTransformer} that can be augmented with annotations. * - *

The content of the annotations can be used for enriched compilation warnings or errors, and - * documentation purposes. + *

The annotations are used to augment the description of Refaster rule matches. */ // XXX: Can we find a better name for this class? `CompositeAnnotatedCodeTransformer`, // `AugmentedCompositeCodeTransformer`, ...? From 302e20b212b56beaf4161a4e34b6cf7e1f7ab2b0 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 14:08:09 +0200 Subject: [PATCH 26/53] Revert changes in `OptionalTemplates` --- .../errorprone/refastertemplates/OptionalTemplates.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java index 55a1ee2b3d..09882bb2e1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java @@ -17,11 +17,8 @@ import java.util.function.Supplier; import java.util.stream.Stream; import javax.annotation.Nullable; -import tech.picnic.errorprone.refaster.annotation.Description; -import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster templates related to expressions dealing with {@link Optional}s. */ -@OnlineDocumentation final class OptionalTemplates { private OptionalTemplates() {} @@ -66,7 +63,7 @@ boolean after(Optional optional) { } } - @Description("Prefer `Optional#orElseThrow()` over the less explicit `Optional#get()`") + /** Prefer `Optional#orElseThrow()` over the less explicit `Optional#get()` */ static final class OptionalOrElseThrow { @BeforeTemplate @SuppressWarnings("NullAway") From 32300ff2e5c1f161c51bf8828394dc8c85485d34 Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Tue, 4 Oct 2022 16:17:06 +0200 Subject: [PATCH 27/53] Tweak `AnnotatedCompositeCodeTransformer` Javadoc --- .../refaster/AnnotatedCompositeCodeTransformer.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java index 64023fbbeb..adee4e2d5a 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java @@ -30,12 +30,13 @@ import tech.picnic.errorprone.refaster.annotation.Severity; /** - * A {@link CompositeCodeTransformer} that can be augmented with annotations. + * A {@link CompositeCodeTransformer} that augments the {@link Description} of Refaster rule + * matches. * - *

The annotations are used to augment the description of Refaster rule matches. + *

The content is augmented based on custom {@link tech.picnic.errorprone.refaster.annotation + * annotations} available on the matching {@link CodeTransformer} or on this {@link + * CompositeCodeTransformer} as a fallback, if any. */ -// XXX: Can we find a better name for this class? `CompositeAnnotatedCodeTransformer`, -// `AugmentedCompositeCodeTransformer`, ...? // XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.) @AutoValue public abstract class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { From d21ac59cb56f291c9410f580d3cc31e2095fe3e7 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 7 Oct 2022 13:27:50 +0200 Subject: [PATCH 28/53] Apply `StringJoin` suggestion --- .../java/tech/picnic/errorprone/refaster/runner/Refaster.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index a058470386..7c49bd09a9 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -167,7 +167,7 @@ private static Description augmentDescription( "Refaster Rule", description.getLink(), severityOverride.orElse(description.severity), - String.format("%s: %s", description.checkName, description.getRawMessage())) + String.join(": ", description.checkName, description.getRawMessage())) .addAllFixes(description.fixes) .build(); } From 04847628f5b361aa1a76bd608b58f90a8a105574 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 8 Oct 2022 12:38:29 +0200 Subject: [PATCH 29/53] Suggestions --- .../refastertemplates/OptionalTemplates.java | 2 +- pom.xml | 17 +++++ refaster-runner/pom.xml | 3 + .../refaster/runner/RefasterTest.java | 6 +- refaster-support/pom.xml | 5 ++ .../AnnotatedCompositeCodeTransformer.java | 11 ++-- .../errorprone/refaster/ErrorProneFork.java | 2 - .../errorprone/refaster/package-info.java | 4 ++ .../refaster/ErrorProneForkTest.java | 65 +++++++++++++++++++ 9 files changed, 103 insertions(+), 12 deletions(-) create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/package-info.java create mode 100644 refaster-support/src/test/java/tech/picnic/errorprone/refaster/ErrorProneForkTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java index 09882bb2e1..113159d98c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java @@ -63,7 +63,7 @@ boolean after(Optional optional) { } } - /** Prefer `Optional#orElseThrow()` over the less explicit `Optional#get()` */ + /** Prefer {@link Optional#orElseThrow()} over the less explicit {@link Optional#get()}. */ static final class OptionalOrElseThrow { @BeforeTemplate @SuppressWarnings("NullAway") diff --git a/pom.xml b/pom.xml index c5b589a1da..636a17b91b 100644 --- a/pom.xml +++ b/pom.xml @@ -1331,6 +1331,23 @@ + + + + + org.apache.maven.plugins + maven-surefire-plugin + + + + true + + + + + + provided diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java index 3dbc78738b..0ddad27c10 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java @@ -157,11 +157,11 @@ private static Stream severityAssignmentTestCases() { } /** - * Verifies that the bug checker flags the refactoring opportunities with the appropriate severity + * Verifies that the bug checker flags refactoring opportunities with the appropriate severity * level. * - * @implNote This test setup is rather cumbersome, because the {@link CompilationTestHelper} does - * not enable direct assertions against the severity of collected diagnostics output. + * @implNote This test setup is rather cumbersome, because {@link CompilationTestHelper} does not + * enable direct assertions against the severity of collected diagnostics output. */ @MethodSource("severityAssignmentTestCases") @ParameterizedTest diff --git a/refaster-support/pom.xml b/refaster-support/pom.xml index 8131ec25bd..918986e91a 100644 --- a/refaster-support/pom.xml +++ b/refaster-support/pom.xml @@ -61,6 +61,11 @@ guava provided + + org.assertj + assertj-core + test + org.junit.jupiter junit-jupiter-api diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java index adee4e2d5a..6d2fc60386 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java @@ -53,10 +53,10 @@ public abstract class AnnotatedCompositeCodeTransformer implements CodeTransform /** * Creates an instance of an {@link AnnotatedCompositeCodeTransformer}. * - * @param packageName The package in which the {@link CodeTransformer} resides. - * @param transformers The {@link CodeTransformer}s in this {@link CompositeCodeTransformer}. - * @param annotations The annotations that are applicable on this {@link CodeTransformer}. - * @return An {@link AnnotatedCompositeCodeTransformer} that may be augmented with annotations. + * @param packageName The package in which the wrapped {@link CodeTransformer}s reside. + * @param transformers The {@link CodeTransformer}s to which to delegate. + * @param annotations The annotations that are applicable to this {@link CodeTransformer}. + * @return A non-{@code null} {@link AnnotatedCompositeCodeTransformer}. */ public static AnnotatedCompositeCodeTransformer create( String packageName, @@ -65,9 +65,8 @@ public static AnnotatedCompositeCodeTransformer create( return new AutoValue_AnnotatedCompositeCodeTransformer(packageName, transformers, annotations); } - /** {@inheritDoc} */ @Override - public void apply(TreePath path, Context context, DescriptionListener listener) { + public final void apply(TreePath path, Context context, DescriptionListener listener) { for (CodeTransformer transformer : transformers()) { transformer.apply( path, diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/ErrorProneFork.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/ErrorProneFork.java index 287abe7952..38d29c403c 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/ErrorProneFork.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/ErrorProneFork.java @@ -13,8 +13,6 @@ * * @see Picnic's Error Prone fork */ -// XXX: Add tests for this class. We can at least test `#isErrorProneForkAvailable()` by having -// Maven inject a corresponding system property. public final class ErrorProneFork { private static final Optional ERROR_PRONE_OPTIONS_IS_SUGGESTIONS_AS_WARNINGS_METHOD = Arrays.stream(ErrorProneOptions.class.getDeclaredMethods()) diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/package-info.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/package-info.java new file mode 100644 index 0000000000..4955569986 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/package-info.java @@ -0,0 +1,4 @@ +/** Assorted classes that aid the compilation or evaluation of Refaster rules. */ +@com.google.errorprone.annotations.CheckReturnValue +@javax.annotation.ParametersAreNonnullByDefault +package tech.picnic.errorprone.refaster; diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/ErrorProneForkTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/ErrorProneForkTest.java new file mode 100644 index 0000000000..cfe6888943 --- /dev/null +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/ErrorProneForkTest.java @@ -0,0 +1,65 @@ +package tech.picnic.errorprone.refaster; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.ErrorProneOptions; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.ClassTree; +import org.junit.jupiter.api.Test; + +final class ErrorProneForkTest { + @Test + void isErrorProneForkAvailable() { + assertThat(ErrorProneFork.isErrorProneForkAvailable()) + .isEqualTo(Boolean.TRUE.toString().equals(System.getProperty("error-prone-fork-in-use"))); + } + + @Test + void isSuggestionsAsWarningsEnabledWithoutFlag() { + CompilationTestHelper.newInstance(TestChecker.class, getClass()) + .addSourceLines( + "A.java", + "// BUG: Diagnostic contains: Suggestions as warnings enabled: false", + "class A {}") + .doTest(); + } + + @Test + void isSuggestionsAsWarningsEnabledWithFlag() { + assumeTrue(ErrorProneFork.isErrorProneForkAvailable()); + + CompilationTestHelper.newInstance(TestChecker.class, getClass()) + .setArgs("-XepAllSuggestionsAsWarnings") + .addSourceLines( + "A.java", + "// BUG: Diagnostic contains: Suggestions as warnings enabled: true", + "class A {}") + .doTest(); + } + + /** + * A {@link BugChecker} that reports the result of {@link + * ErrorProneFork#isSuggestionsAsWarningsEnabled(ErrorProneOptions)}. + */ + @BugPattern(summary = "Flags classes with a custom error message", severity = ERROR) + public static final class TestChecker extends BugChecker implements ClassTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + return buildDescription(tree) + .setMessage( + String.format( + "Suggestions as warnings enabled: %s", + ErrorProneFork.isSuggestionsAsWarningsEnabled(state.errorProneOptions()))) + .build(); + } + } +} From 1624ebf4c67f9e0dac2fdd4d09d22a6b5ef2efdd Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 1 May 2022 16:53:25 +0200 Subject: [PATCH 30/53] WIP: Speed up `Refaster` check This code assumes a modification of Error Prone; see the `sschroevers/refaster-tree-tweaks` branch on the fork. --- pom.xml | 4 + refaster-runner/pom.xml | 10 + .../errorprone/refaster/runner/Refaster.java | 237 +++++++++++++++++- 3 files changed, 240 insertions(+), 11 deletions(-) diff --git a/pom.xml b/pom.xml index 85afe299a2..55560e7117 100644 --- a/pom.xml +++ b/pom.xml @@ -622,6 +622,10 @@ + + + diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index 7bccf546c5..a6f79662a6 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -29,6 +29,11 @@ error_prone_check_api provided + + ${groupId.error-prone} + error_prone_core + provided + ${project.groupId} refaster-compiler @@ -37,6 +42,11 @@ `annotationProcessorPaths` configuration below. --> provided + + com.google.auto + auto-common + provided + com.google.auto.service auto-service-annotations diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 118475dbea..a82284765e 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -1,17 +1,21 @@ package tech.picnic.errorprone.refaster.runner; +import static com.google.auto.common.MoreStreams.toImmutableSet; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableRangeSet.toImmutableRangeSet; 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.function.Predicate.not; +import static java.util.stream.Collectors.toCollection; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableRangeSet; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Multimaps; import com.google.common.collect.Range; import com.google.common.collect.RangeSet; import com.google.common.collect.TreeRangeSet; @@ -26,15 +30,33 @@ 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.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.util.TreeScanner; import com.sun.tools.javac.tree.EndPosTable; import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; import java.util.ArrayList; +import java.util.Collection; import java.util.Comparator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.function.Consumer; import java.util.regex.Pattern; import java.util.stream.Stream; +import javax.annotation.Nullable; /** * A {@link BugChecker} that flags code that can be simplified using Refaster templates located on @@ -56,7 +78,10 @@ public final class Refaster extends BugChecker implements CompilationUnitTreeMat private static final long serialVersionUID = 1L; - private final CodeTransformer codeTransformer; + // XXX: Drop this field. + // private final CodeTransformer codeTransformer; + private final ImmutableListMultimap>, RefasterRule> + refasterRules; /** Instantiates the default {@link Refaster}. */ public Refaster() { @@ -69,31 +94,211 @@ public Refaster() { * @param flags Any provided command line flags. */ public Refaster(ErrorProneFlags flags) { - codeTransformer = createCompositeCodeTransformer(flags); + // XXX: Drop this assignment. + // codeTransformer = createCompositeCodeTransformer(flags); + refasterRules = loadIndexedRules(flags); } @CanIgnoreReturnValue @Override public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + ImmutableSet sourceIdentifiers = extractSourceIdentifiers(tree); + /* 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 (Map.Entry>, Collection>> e : + refasterRules.asMap().entrySet()) { + if (e.getKey().stream().anyMatch(sourceIdentifiers::containsAll)) { + for (RefasterRule rule : e.getValue()) { + 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. + } + } + } } + /* Then apply them. */ applyMatches(matches, ((JCCompilationUnit) tree).endPositions, state); + // XXX: drop code below. + // /* 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; + // } + // /* Then apply them. */ + // applyMatches(matches, ((JCCompilationUnit) tree).endPositions, state); + /* Any matches were already reported by the code above, directly to the `VisitorState`. */ return Description.NO_MATCH; } + 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); + } + } + + // XXX: Log `else` case? + } + + 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)); + } + // XXX: error if other kind of template. + } + + return results.build(); + } + + // XXX: Consider interning the strings (once a benchmark is in place). + private static ImmutableSet> extractTemplateIdentifiers( + ImmutableList trees) { + // XXX: Here and below: replace `LinkedHashSet`s with `HashSet` once done. + List> identifierCombinations = new ArrayList<>(); + identifierCombinations.add(new LinkedHashSet<>()); + + // XXX: Make the scanner static, then make also its helper methods static. + new TreeScanner>>() { + @Nullable + @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); + } + + @Nullable + @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; + } + + @Nullable + @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; + } + + @Nullable + @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; + } + + private List> copy(List> identifierCombinations) { + return identifierCombinations.stream() + .map(LinkedHashSet::new) + .collect(toCollection(ArrayList::new)); + } + }.scan(trees, identifierCombinations); + + return identifierCombinations.stream().map(ImmutableSet::copyOf).collect(toImmutableSet()); + } + + // XXX: Consider interning! + private static ImmutableSet extractSourceIdentifiers(Tree tree) { + // XXX: Replace `LinkedHashSet`s with `HashSet` once done. + Set identifiers = new LinkedHashSet<>(); + + // XXX: Make the scanner static. + new TreeScanner>() { + @Nullable + @Override + public Void visitIdentifier(IdentifierTree node, Set identifiers) { + // XXX: Can we be more precise? + identifiers.add(node.getName().toString()); + return null; + } + + @Nullable + @Override + public Void visitMemberReference(MemberReferenceTree node, Set identifiers) { + super.visitMemberReference(node, identifiers); + identifiers.add(node.getName().toString()); + return null; + } + + @Nullable + @Override + public Void visitMemberSelect(MemberSelectTree node, Set identifiers) { + super.visitMemberSelect(node, identifiers); + identifiers.add(node.getIdentifier().toString()); + return null; + } + }.scan(tree, identifiers); + + return ImmutableSet.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. @@ -146,6 +351,16 @@ private static Stream getReplacements( return description.fixes.stream().flatMap(fix -> fix.getReplacements(endPositions).stream()); } + // XXX: Move to separate class, in order to be compatible with vanilla EP. + private static ImmutableListMultimap>, RefasterRule> + loadIndexedRules(ErrorProneFlags flags) { + List> refasterRules = new ArrayList<>(); + collectRefasterRules(createCompositeCodeTransformer(flags), refasterRules::add); + + return Multimaps.index(refasterRules, Refaster::extractTemplateIdentifiers); + } + + // XXX: instead create an `ImmutableList` private static CodeTransformer createCompositeCodeTransformer(ErrorProneFlags flags) { ImmutableListMultimap allTransformers = CodeTransformers.getAllCodeTransformers(); From ee74279ef9e63cdf3d9e8d4415f054557c227986 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 12 Sep 2022 13:38:13 +0200 Subject: [PATCH 31/53] Compatibility with stock Error Prone --- pom.xml | 5 +- .../errorprone/refaster/runner/Refaster.java | 95 +++++++++++++++++-- 2 files changed, 90 insertions(+), 10 deletions(-) diff --git a/pom.xml b/pom.xml index 55560e7117..c0396b0529 100644 --- a/pom.xml +++ b/pom.xml @@ -475,7 +475,6 @@ jdk-internal - jdk-reflection jdk-system-out -XepAllDisabledChecksAsWarnings + + -XepDisableWarningsInGeneratedCode -Xep:AndroidJdkLibsChecker:OFF + 1.0.1 0.10.2 com.jakewharton.nopen - nopen-checker - ${version.nopen-checker} + nopen-annotations + ${version.nopen} com.newrelic.agent.java @@ -401,6 +403,11 @@ pom import + + org.openjdk.jmh + jmh-core + ${version.jmh} + org.slf4j slf4j-api @@ -846,7 +853,7 @@ com.jakewharton.nopen nopen-checker - ${version.nopen-checker} + ${version.nopen} GPL-2.0-with-classpath-exception | CDDL/GPLv2+CE + | GNU General Public License (GPL), version 2, with the Classpath exception | GNU General Public License, version 2 (GPL2), with the classpath exception | GNU General Public License, version 2, with the Classpath Exception | GPL2 w/ CPE @@ -1558,6 +1576,7 @@ avoid that, so we simply tell Error Prone not to warn about generated code. --> -XepDisableWarningsInGeneratedCode + -XepExcludedPaths:\Q${project.build.directory}${file.separator}\E.* -Xep:AndroidJdkLibsChecker:OFF + run-jmh-benchmark + + + jmh.run-benchmark + + + + + + org.apache.maven.plugins + maven-dependency-plugin + + + build-jmh-runtime-classpath + + build-classpath + + + testClasspath + + + + + + org.codehaus.mojo + exec-maven-plugin + + + run-jmh-benchmark + + java + + process-test-classes + + test + ${jmh.run-benchmark} + + + + java.class.path + ${project.build.testOutputDirectory}${path.separator}${project.build.outputDirectory}${path.separator}${testClasspath} + + + + + + + + + diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index 0f4658522b..3b46a19056 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -66,6 +66,11 @@ guava provided + + com.jakewharton.nopen + nopen-annotations + provided + org.assertj assertj-core @@ -86,6 +91,11 @@ junit-jupiter-params test + + org.openjdk.jmh + jmh-core + test + diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java index ca27cec79c..5df83f47b9 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java @@ -8,6 +8,7 @@ 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; @@ -35,6 +36,8 @@ static Node create( // XXX: Consider having `RefasterRuleSelector` already collect the candidate edges into a // `SortedSet`, as that would likely speed up `ImmutableSortedSet#copyOf`. + // XXX: If this ^ proves worthwhile, then the test code and benchmark should be updated + // accordingly. void collectReachableValues(Set candidateEdges, Consumer sink) { collectReachableValues(ImmutableSortedSet.copyOf(candidateEdges).asList(), sink); } @@ -86,9 +89,9 @@ private static BuildNode create() { private void register( List values, Function>> pathsExtractor) { for (T value : values) { - pathsExtractor.apply(value).stream() - .sorted(comparingInt(Set::size)) - .forEach(path -> registerPath(value, ImmutableList.sortedCopyOf(path))); + List> paths = new ArrayList<>(pathsExtractor.apply(value)); + Collections.sort(paths, comparingInt(Set::size)); + paths.forEach(path -> registerPath(value, ImmutableList.sortedCopyOf(path))); } } @@ -98,14 +101,13 @@ private void registerPath(T value, ImmutableList path) { return; } - path.stream() - .findFirst() - .ifPresentOrElse( - edge -> - children() - .computeIfAbsent(edge, k -> create()) - .registerPath(value, path.subList(1, path.size())), - () -> values().add(value)); + if (path.isEmpty()) { + values().add(value); + } else { + children() + .computeIfAbsent(path.get(0), k -> create()) + .registerPath(value, path.subList(1, path.size())); + } } private Node immutable() { diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 0b88ea21d3..523f723c86 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -157,6 +157,8 @@ private static Stream getReplacements( return description.fixes.stream().flatMap(fix -> fix.getReplacements(endPositions).stream()); } + // XXX: Add a flag to disable the optimized `RefasterRuleSelector`. That would allow us to verify + // that we're not prematurely pruning rules. private static RefasterRuleSelector createRefasterRuleSelector(ErrorProneFlags flags) { ImmutableListMultimap allTransformers = CodeTransformers.getAllCodeTransformers(); diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java index 81e49e163f..eb2dd6c02e 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java @@ -19,14 +19,18 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.ClassTree; 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; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.PackageTree; import com.sun.source.tree.Tree; import com.sun.source.tree.UnaryTree; +import com.sun.source.tree.VariableTree; import com.sun.source.util.TreeScanner; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -183,7 +187,7 @@ private static Set extractSourceIdentifiers(Tree tree) { * @throws IllegalArgumentException If the given input is not supported. */ // XXX: Extend list to cover remaining cases; at least for any `Kind` that may appear in a - // Refaster template. + // Refaster template. (E.g. keywords such as `if`, `instanceof`, `new`, ...) private static String treeKindToString(Tree.Kind kind) { switch (kind) { case ASSIGNMENT: @@ -458,6 +462,40 @@ private static List> copy(List> identifierCombinations) } private static class SourceIdentifierExtractor extends TreeScanner> { + @Nullable + @Override + public Void visitPackage(PackageTree node, Set identifiers) { + /* Refaster rules never match package declarations. */ + return null; + } + + @Nullable + @Override + public Void visitClass(ClassTree node, Set identifiers) { + /* + * Syntactic details of a class declaration other than the definition of its members do not + * need to be reflected in a Refaster rule for it to apply to the class's code. + */ + return scan(node.getMembers(), identifiers); + } + + @Nullable + @Override + public Void visitMethod(MethodTree node, Set identifiers) { + /* + * Syntactic details of a method declaration other than its body do not need to be reflected + * in a Refaster rule for it to apply to the method's code. + */ + return scan(node.getBody(), identifiers); + } + + @Nullable + @Override + public Void visitVariable(VariableTree node, Set identifiers) { + /* A variable's modifiers and name do not influence where a Refaster rule matches. */ + return reduce(scan(node.getInitializer(), identifiers), scan(node.getType(), identifiers)); + } + @Nullable @Override public Void visitIdentifier(IdentifierTree node, Set identifiers) { diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeBenchmark.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeBenchmark.java new file mode 100644 index 0000000000..206893c0af --- /dev/null +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeBenchmark.java @@ -0,0 +1,81 @@ +package tech.picnic.errorprone.refaster.runner; + +import static com.google.common.collect.ImmutableListMultimap.flatteningToImmutableListMultimap; +import static java.util.function.Function.identity; + +import com.google.common.collect.ImmutableListMultimap; +import com.jakewharton.nopen.annotation.Open; +import java.util.Collection; +import java.util.Map; +import java.util.Random; +import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; +import java.util.stream.Stream; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.OptionsBuilder; +import tech.picnic.errorprone.refaster.runner.NodeTestCase.NodeTestCaseEntry; + +@Open +@State(Scope.Benchmark) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MILLISECONDS) +@Fork(jvmArgs = {"-Xms1G", "-Xmx1G"}) +@Warmup(iterations = 5) +@Measurement(iterations = 10) +public class NodeBenchmark { + @SuppressWarnings("NullAway" /* Initialized by `@Setup` method. */) + private ImmutableListMultimap, NodeTestCaseEntry> testCases; + + public static void main(String[] args) throws RunnerException { + String testRegex = Pattern.quote(NodeBenchmark.class.getCanonicalName()); + new Runner(new OptionsBuilder().include(testRegex).forks(1).build()).run(); + } + + @Setup + public final void setUp() { + Random random = new Random(0); + + testCases = + Stream.of( + NodeTestCase.generate(100, 5, 10, 10, random), + NodeTestCase.generate(100, 5, 10, 100, random), + NodeTestCase.generate(100, 5, 10, 1000, random), + NodeTestCase.generate(1000, 10, 20, 10, random), + NodeTestCase.generate(1000, 10, 20, 100, random), + NodeTestCase.generate(1000, 10, 20, 1000, random), + NodeTestCase.generate(1000, 10, 20, 10000, random)) + .collect( + flatteningToImmutableListMultimap( + identity(), testCase -> testCase.generateTestCaseEntries(random))); + } + + @Benchmark + public final void create(Blackhole bh) { + for (NodeTestCase testCase : testCases.keySet()) { + bh.consume(testCase.buildTree()); + } + } + + @Benchmark + public final void collectReachableValues(Blackhole bh) { + for (Map.Entry, Collection>> e : + testCases.asMap().entrySet()) { + Node tree = e.getKey().buildTree(); + for (NodeTestCaseEntry testCaseEntry : e.getValue()) { + tree.collectReachableValues(testCaseEntry.candidateEdges(), bh::consume); + } + } + } +} diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTest.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTest.java index 5b91e1199e..82a9f2be6c 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTest.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTest.java @@ -1,176 +1,47 @@ package tech.picnic.errorprone.refaster.runner; -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.common.collect.ImmutableSetMultimap.flatteningToImmutableSetMultimap; -import static java.util.function.Function.identity; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.params.provider.Arguments.arguments; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSetMultimap; import java.util.ArrayList; -import java.util.Collections; -import java.util.HashSet; +import java.util.Collection; import java.util.List; -import java.util.Map; import java.util.Random; -import java.util.Set; import java.util.stream.Stream; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; final class NodeTest { - private static Stream verifyTestCases() { + private static Stream collectReachableValuesTestCases() { Random random = new Random(0); - /* { treeInput, random } */ return Stream.of( - arguments(generateTestInput(random, 0, 0, 0, 0), random), - arguments(generateTestInput(random, 1, 1, 1, 1), random), - arguments(generateTestInput(random, 2, 2, 2, 10), random), - arguments(generateTestInput(random, 2, 2, 2, 100), random), - arguments(generateTestInput(random, 2, 2, 5, 10), random), - arguments(generateTestInput(random, 2, 2, 5, 100), random), - arguments(generateTestInput(random, 2, 2, 10, 10), random), - arguments(generateTestInput(random, 2, 2, 10, 100), random), - arguments(generateTestInput(random, 100, 1, 1, 10), random), - arguments(generateTestInput(random, 100, 1, 1, 100), random), - arguments(generateTestInput(random, 100, 1, 5, 10), random), - arguments(generateTestInput(random, 100, 1, 5, 100), random), - arguments(generateTestInput(random, 100, 5, 5, 10), random), - arguments(generateTestInput(random, 100, 5, 5, 100), random), - arguments(generateTestInput(random, 100, 5, 5, 1000), random), - arguments(generateTestInput(random, 100, 5, 10, 10), random), - arguments(generateTestInput(random, 100, 5, 10, 100), random), - arguments(generateTestInput(random, 100, 5, 10, 1000), random), - arguments(generateTestInput(random, 1000, 1, 5, 10), random), - arguments(generateTestInput(random, 1000, 1, 5, 100), random), - arguments(generateTestInput(random, 1000, 1, 5, 1000), random), - arguments(generateTestInput(random, 1000, 5, 5, 10), random), - arguments(generateTestInput(random, 1000, 5, 5, 100), random), - arguments(generateTestInput(random, 1000, 5, 5, 1000), random), - arguments(generateTestInput(random, 1000, 10, 5, 10), random), - arguments(generateTestInput(random, 1000, 10, 5, 100), random), - arguments(generateTestInput(random, 1000, 10, 5, 1000), random), - arguments(generateTestInput(random, 1000, 10, 5, 10000), random), - arguments(generateTestInput(random, 1000, 5, 10, 10), random), - arguments(generateTestInput(random, 1000, 5, 10, 100), random), - arguments(generateTestInput(random, 1000, 5, 10, 1000), random), - arguments(generateTestInput(random, 1000, 5, 10, 10000), random)); - } - - @MethodSource("verifyTestCases") + NodeTestCase.generate(0, 0, 0, 0, random), + NodeTestCase.generate(1, 1, 1, 1, random), + NodeTestCase.generate(2, 2, 2, 10, random), + NodeTestCase.generate(10, 2, 5, 10, random), + NodeTestCase.generate(10, 2, 5, 100, random), + NodeTestCase.generate(100, 5, 10, 100, random), + NodeTestCase.generate(100, 5, 10, 1000, random)) + .flatMap( + testCase -> { + Node tree = testCase.buildTree(); + return testCase + .generateTestCaseEntries(random) + .map(e -> arguments(tree, e.candidateEdges(), e.reachableValues())); + }); + } + + @MethodSource("collectReachableValuesTestCases") @ParameterizedTest - void verify(ImmutableSetMultimap> treeInput, Random random) { - Node tree = Node.create(treeInput.keySet().asList(), treeInput::get); - - // XXX: Drop. - // System.out.println(size(tree)); - - verifyConstruction(tree, treeInput, random); - } - - // XXX: Drop. - // private static int size(Node t) { - // return t.values().size() - // + t.children().size() - // + t.children().values().stream().mapToInt(NodeTest::size).sum(); - // } - - private static void verifyConstruction( + void collectReachableValues( Node tree, - ImmutableSetMultimap> treeInput, - Random random) { - ImmutableSet allPathValues = - treeInput.values().stream().flatMap(ImmutableSet::stream).collect(toImmutableSet()); - - for (Map.Entry> e : treeInput.entries()) { - verifyReachability(tree, e.getKey(), shuffle(e.getValue(), random), allPathValues, random); - } - } - - private static void verifyReachability( - Node tree, - T leaf, - ImmutableSet unorderedEdgesToLeaf, - ImmutableSet allEdges, - Random random) { - String unknownEdge = "unknown"; - - assertThat(isReachable(tree, leaf, unorderedEdgesToLeaf)).isTrue(); - assertThat(isReachable(tree, leaf, insertValue(unorderedEdgesToLeaf, unknownEdge, random))) - .isTrue(); - if (!allEdges.isEmpty()) { - String knownEdge = selectRandomElement(allEdges, random); - assertThat(isReachable(tree, leaf, insertValue(unorderedEdgesToLeaf, knownEdge, random))) - .isTrue(); - } - - // XXX: Strictly speaking this is wrong: these paths _could_ exist. - // XXX: Implement something better. - if (!unorderedEdgesToLeaf.isEmpty()) { - // assertThat(isReachable(tree, leaf, randomStrictSubset(unorderedEdgesToLeaf, random))) - // .isFalse(); - // assertThat( - // isReachable( - // tree, - // leaf, - // insertValue( - // randomStrictSubset(unorderedEdgesToLeaf, random), unknownEdge, random))) - // .isFalse(); - } - } - - private static ImmutableSet shuffle(ImmutableSet values, Random random) { - List allValues = new ArrayList<>(values); - Collections.shuffle(allValues, random); - return ImmutableSet.copyOf(allValues); - } - - private static ImmutableSet insertValue( - ImmutableSet values, T extraValue, Random random) { - List allValues = new ArrayList<>(values); - allValues.add(random.nextInt(values.size() + 1), extraValue); - return ImmutableSet.copyOf(allValues); - } - - private static T selectRandomElement(ImmutableSet collection, Random random) { - return collection.asList().get(random.nextInt(collection.size())); - } - - // XXX: Use or drop. - // private static ImmutableSet randomStrictSubset(ImmutableSet values, Random random) { - // checkArgument(!values.isEmpty(), "Cannot select strict subset of random collection"); - // - // List allValues = new ArrayList<>(values); - // Collections.shuffle(allValues, random); - // return ImmutableSet.copyOf(allValues.subList(0, random.nextInt(allValues.size()))); - // } - - private static boolean isReachable( - Node tree, T target, ImmutableSet candidateEdges) { - Set matches = new HashSet<>(); - tree.collectReachableValues(candidateEdges, matches::add); - return matches.contains(target); - } - - private static ImmutableSetMultimap> generateTestInput( - Random random, int entryCount, int maxPathCount, int maxPathLength, int pathValueDomainSize) { - return random - .ints(entryCount) - .boxed() - .collect( - flatteningToImmutableSetMultimap( - identity(), - i -> - random - .ints(random.nextInt(maxPathCount + 1)) - .mapToObj( - p -> - random - .ints(random.nextInt(maxPathLength + 1), 0, pathValueDomainSize) - .mapToObj(String::valueOf) - .collect(toImmutableSet())))); + ImmutableSet candidateEdges, + Collection expectedReachable) { + List actualReachable = new ArrayList<>(); + tree.collectReachableValues(candidateEdges, actualReachable::add); + assertThat(actualReachable).hasSameElementsAs(expectedReachable); } } diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTestCase.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTestCase.java new file mode 100644 index 0000000000..8739d1846e --- /dev/null +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTestCase.java @@ -0,0 +1,138 @@ +package tech.picnic.errorprone.refaster.runner; + +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.common.collect.ImmutableSetMultimap.flatteningToImmutableSetMultimap; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.collectingAndThen; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableCollection; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSetMultimap; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Random; +import java.util.stream.Stream; + +@AutoValue +abstract class NodeTestCase { + static NodeTestCase generate( + int entryCount, int maxPathCount, int maxPathLength, int pathValueDomainSize, Random random) { + return random + .ints(entryCount) + .boxed() + .collect( + collectingAndThen( + flatteningToImmutableSetMultimap( + identity(), + i -> + random + .ints(random.nextInt(maxPathCount + 1)) + .mapToObj( + p -> + random + .ints( + random.nextInt(maxPathLength + 1), + 0, + pathValueDomainSize) + .mapToObj(String::valueOf) + .collect(toImmutableSet()))), + AutoValue_NodeTestCase::new)); + } + + abstract ImmutableSetMultimap> input(); + + final Node buildTree() { + return Node.create(input().keySet().asList(), input()::get); + } + + final Stream> generateTestCaseEntries(Random random) { + return generatePathTestCases(input(), random); + } + + private static Stream> generatePathTestCases( + ImmutableSetMultimap> treeInput, Random random) { + ImmutableSet allEdges = + treeInput.values().stream().flatMap(ImmutableSet::stream).collect(toImmutableSet()); + + return Stream.concat( + Stream.of(ImmutableSet.of()), shuffle(treeInput.values(), random).stream()) + .limit(random.nextInt(20, 100)) + .flatMap(edges -> generateVariations(edges, allEdges, "unused", random)) + .distinct() + .map(edges -> createTestCaseEntry(treeInput, edges)); + } + + private static Stream> generateVariations( + ImmutableSet baseEdges, ImmutableSet allEdges, T unusedEdge, Random random) { + Optional knownEdge = selectRandomElement(allEdges, random); + + return Stream.of( + random.nextBoolean() ? null : baseEdges, + random.nextBoolean() ? null : shuffle(baseEdges, random), + random.nextBoolean() ? null : insertValue(baseEdges, unusedEdge, random), + baseEdges.isEmpty() || random.nextBoolean() + ? null + : randomStrictSubset(baseEdges, random), + baseEdges.isEmpty() || random.nextBoolean() + ? null + : insertValue(randomStrictSubset(baseEdges, random), unusedEdge, random), + baseEdges.isEmpty() || random.nextBoolean() + ? null + : knownEdge + .map(edge -> insertValue(randomStrictSubset(baseEdges, random), edge, random)) + .orElse(null)) + .filter(Objects::nonNull); + } + + private static Optional selectRandomElement(ImmutableSet collection, Random random) { + return collection.isEmpty() + ? Optional.empty() + : Optional.of(collection.asList().get(random.nextInt(collection.size()))); + } + + private static ImmutableSet shuffle(ImmutableCollection values, Random random) { + List allValues = new ArrayList<>(values); + Collections.shuffle(allValues, random); + return ImmutableSet.copyOf(allValues); + } + + private static ImmutableSet insertValue( + ImmutableSet values, T extraValue, Random random) { + List allValues = new ArrayList<>(values); + allValues.add(random.nextInt(values.size() + 1), extraValue); + return ImmutableSet.copyOf(allValues); + } + + private static ImmutableSet randomStrictSubset(ImmutableSet values, Random random) { + checkArgument(!values.isEmpty(), "Cannot select strict subset of random collection"); + + List allValues = new ArrayList<>(values); + Collections.shuffle(allValues, random); + return ImmutableSet.copyOf(allValues.subList(0, random.nextInt(allValues.size()))); + } + + private static NodeTestCaseEntry createTestCaseEntry( + ImmutableSetMultimap> treeInput, ImmutableSet edges) { + return new AutoValue_NodeTestCase_NodeTestCaseEntry<>( + edges, + treeInput.asMap().entrySet().stream() + .filter(e -> e.getValue().stream().anyMatch(edges::containsAll)) + .map(Map.Entry::getKey) + .collect(toImmutableList())); + } + + @AutoValue + abstract static class NodeTestCaseEntry { + abstract ImmutableSet candidateEdges(); + + abstract ImmutableList reachableValues(); + } +} From 457a8e229ee72393e11b2e454ef0234090bf40f1 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 2 Oct 2022 10:57:41 +0200 Subject: [PATCH 46/53] Comment style, explain both performance-only pieces of code Since PIT correctly flags these as "redundant". Also for JDK 11 compatibility. --- .../tech/picnic/errorprone/refaster/runner/Node.java | 12 +++++++++--- .../errorprone/refaster/runner/NodeTestCase.java | 4 +++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java index 5df83f47b9..04887065c0 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java @@ -49,9 +49,11 @@ private void collectReachableValues(ImmutableList candidateEdges, Consum return; } - // For performance reasons we iterate over the smallest set of edges. In case there are fewer - // children than candidate edges we iterate over the former, at the cost of not pruning the - // set of candidate edges if a transition is made. + /* + * For performance reasons we iterate over the smallest set of edges. In case there are fewer + * children than candidate edges we iterate over the former, at the cost of not pruning the set + * of candidate edges if a transition is made. + */ int candidateEdgeCount = candidateEdges.size(); if (children().size() < candidateEdgeCount) { for (Map.Entry> e : children().entrySet()) { @@ -90,6 +92,10 @@ private void register( List values, Function>> pathsExtractor) { for (T value : values) { List> paths = new ArrayList<>(pathsExtractor.apply(value)); + /* + * 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.forEach(path -> registerPath(value, ImmutableList.sortedCopyOf(path))); } diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTestCase.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTestCase.java index 8739d1846e..4de94f5541 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTestCase.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/NodeTestCase.java @@ -64,7 +64,9 @@ private static Stream> generatePathTestCases( return Stream.concat( Stream.of(ImmutableSet.of()), shuffle(treeInput.values(), random).stream()) - .limit(random.nextInt(20, 100)) + // XXX: Use `random.nextInt(20, 100)` once we no longer target JDK 11. (And consider + // introducing a Refaster template for this case.) + .limit(20 + random.nextInt(80)) .flatMap(edges -> generateVariations(edges, allEdges, "unused", random)) .distinct() .map(edges -> createTestCaseEntry(treeInput, edges)); From 63ad14e76e706073fcc77558b9a50a5aee51b218 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 16:44:19 +0200 Subject: [PATCH 47/53] Fix typo --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index de41aaf04a..9069ea3998 100644 --- a/pom.xml +++ b/pom.xml @@ -157,7 +157,7 @@ 3.8.6 4.8.0 1.0.1 0.10.2 From 0cf891c87b3249726b0f7592fae5f9ce48b226c5 Mon Sep 17 00:00:00 2001 From: Ivan Babiankou Date: Fri, 7 Oct 2022 15:02:32 +0200 Subject: [PATCH 48/53] Suggestions --- .../errorprone/refaster/runner/Node.java | 7 ++- .../errorprone/refaster/runner/Refaster.java | 28 ++++++++- .../refaster/runner/RefasterRuleSelector.java | 61 ++++++------------- 3 files changed, 48 insertions(+), 48 deletions(-) diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java index 04887065c0..b83972363a 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java @@ -3,6 +3,7 @@ import static java.util.Comparator.comparingInt; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedSet; @@ -24,7 +25,8 @@ @AutoValue abstract class Node { static Node create( - List values, Function>> pathExtractor) { + ImmutableCollection values, + Function>> pathExtractor) { BuildNode tree = BuildNode.create(); tree.register(values, pathExtractor); return tree.immutable(); @@ -89,7 +91,8 @@ private static BuildNode create() { * leads to the same value. */ private void register( - List values, Function>> pathsExtractor) { + ImmutableCollection values, + Function>> pathsExtractor) { for (T value : values) { List> paths = new ArrayList<>(pathsExtractor.apply(value)); /* diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 523f723c86..ce905f791e 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -17,6 +17,7 @@ import com.google.common.collect.TreeRangeSet; import com.google.errorprone.BugPattern; import com.google.errorprone.CodeTransformer; +import com.google.errorprone.CompositeCodeTransformer; import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.SubContext; import com.google.errorprone.VisitorState; @@ -34,6 +35,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Consumer; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -162,13 +164,35 @@ private static Stream getReplacements( private static RefasterRuleSelector createRefasterRuleSelector(ErrorProneFlags flags) { ImmutableListMultimap allTransformers = CodeTransformers.getAllCodeTransformers(); - return RefasterRuleSelector.create( + List> refasterRules = new ArrayList<>(); + collectRefasterRules( flags .get(INCLUDED_TEMPLATES_PATTERN_FLAG) .map(Pattern::compile) .>map( nameFilter -> filterCodeTransformers(allTransformers, nameFilter)) - .orElseGet(allTransformers::values)); + .orElseGet(allTransformers::values), + refasterRules::add); + return RefasterRuleSelector.create(ImmutableList.copyOf(refasterRules)); + } + + private static void collectRefasterRules( + ImmutableCollection transformers, Consumer> sink) { + for (CodeTransformer t : transformers) { + collectRefasterRules(t, sink); + } + } + + private static void collectRefasterRules( + CodeTransformer transformer, Consumer> sink) { + if (transformer instanceof RefasterRule) { + sink.accept((RefasterRule) transformer); + } else if (transformer instanceof CompositeCodeTransformer) { + collectRefasterRules(((CompositeCodeTransformer) transformer).transformers(), sink); + } else { + throw new IllegalStateException( + String.format("Can't handle `CodeTransformer` of type '%s'", transformer.getClass())); + } } private static ImmutableList filterCodeTransformers( diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java index eb2dd6c02e..45021c165f 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java @@ -7,8 +7,6 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.errorprone.CodeTransformer; -import com.google.errorprone.CompositeCodeTransformer; import com.google.errorprone.refaster.BlockTemplate; import com.google.errorprone.refaster.ExpressionTemplate; import com.google.errorprone.refaster.RefasterRule; @@ -39,7 +37,6 @@ import java.util.IdentityHashMap; import java.util.List; import java.util.Set; -import java.util.function.Consumer; import javax.annotation.Nullable; // XXX: Add some examples of which source files would match what templates in the tree. @@ -60,8 +57,8 @@ *

  • Extract all identifiers from the {@link CompilationUnitTree} and sort them * lexicographically. *
  • Traverse the tree based on the identifiers from the {@link CompilationUnitTree}. Every node - * can contain Refaster templates. Once a node is we found a candidate Refaster template and - * will therefore be added to the list of candidates. + * can contain Refaster templates. Once a node is we found a candidate Refaster template that + * might match some code and will therefore be added to the list of candidates. * * *

    This is an example to explain the algorithm. Consider the templates with identifiers; {@code @@ -77,14 +74,14 @@ * └── D -- T3 * } * - *

    The tree is traversed based on the identifiers in the {@link CompilationUnitTree}. When a leaf - * contains a template and is reached, we can be certain that the identifiers from the {@link + *

    The tree is traversed based on the identifiers in the {@link CompilationUnitTree}. When a node + * containing a template is reached, we can be certain that the identifiers from the {@link * BeforeTemplate} are at least present in the {@link CompilationUnitTree}. * - *

    Since the identifiers are sorted, we can prune parts of the {@link Node tree} while we are + *

    Since the identifiers are sorted, we can skip parts of the {@link Node tree} while we are * traversing it. Instead of trying to match all Refaster templates against every expression in a - * {@link CompilationUnitTree} we now only return a subset of the templates that at least have a - * chance of matching. As a result, the performance of Refaster significantly increases. + * {@link CompilationUnitTree} we now only matching a subset of the templates that at least have a + * chance of matching. As a result, the performance of Refaster increases significantly. */ final class RefasterRuleSelector { private final Node> treeRules; @@ -93,36 +90,12 @@ private RefasterRuleSelector(Node> treeRules) { this.treeRules = treeRules; } - /** - * Instantiates a new {@link RefasterRuleSelector} backed by the {@link RefasterRule}s referenced - * by the given collection of {@link CodeTransformer}s. - */ - static RefasterRuleSelector create(ImmutableCollection transformers) { - List> refasterRules = new ArrayList<>(); - collectRefasterRules(transformers, refasterRules::add); + /** Instantiates a new {@link RefasterRuleSelector} backed by the given {@link RefasterRule}s. */ + static RefasterRuleSelector create(ImmutableCollection> refasterRules) { return new RefasterRuleSelector( Node.create(refasterRules, RefasterRuleSelector::extractTemplateIdentifiers)); } - private static void collectRefasterRules( - ImmutableCollection transformers, Consumer> sink) { - for (CodeTransformer t : transformers) { - collectRefasterRules(t, sink); - } - } - - private static void collectRefasterRules( - CodeTransformer transformer, Consumer> sink) { - if (transformer instanceof RefasterRule) { - sink.accept((RefasterRule) transformer); - } else if (transformer instanceof CompositeCodeTransformer) { - collectRefasterRules(((CompositeCodeTransformer) transformer).transformers(), sink); - } else { - throw new IllegalStateException( - String.format("Can't handle `CodeTransformer` of type '%s'", transformer.getClass())); - } - } - /** * Retrieve a set of Refaster templates that can possibly match based on a {@link * CompilationUnitTree}. @@ -135,7 +108,6 @@ private static void collectRefasterRules( Set> selectCandidateRules(CompilationUnitTree tree) { Set> candidateRules = newSetFromMap(new IdentityHashMap<>()); treeRules.collectReachableValues(extractSourceIdentifiers(tree), candidateRules::add); - return candidateRules; } @@ -166,17 +138,13 @@ private static ImmutableSet> extractTemplateIdentifiers( ImmutableList trees) { List> identifierCombinations = new ArrayList<>(); identifierCombinations.add(new HashSet<>()); - - new TemplateIdentifierExtractor().scan(trees, identifierCombinations); - + TemplateIdentifierExtractor.INSTANCE.scan(trees, identifierCombinations); return identifierCombinations.stream().map(ImmutableSet::copyOf).collect(toImmutableSet()); } private static Set extractSourceIdentifiers(Tree tree) { Set identifiers = new HashSet<>(); - - new SourceIdentifierExtractor().scan(tree, identifiers); - + SourceIdentifierExtractor.INSTANCE.scan(tree, identifiers); return identifiers; } @@ -354,6 +322,8 @@ private static T invokeMethod(Method method, Object instance) { } private static class TemplateIdentifierExtractor extends TreeScanner>> { + private static final TemplateIdentifierExtractor INSTANCE = new TemplateIdentifierExtractor(); + @Nullable @Override public Void visitIdentifier(IdentifierTree node, List> identifierCombinations) { @@ -434,7 +404,8 @@ public Void visitBinary(BinaryTree node, List> identifierCombination private static void registerOperator( ExpressionTree node, List> identifierCombinations) { - identifierCombinations.forEach(ids -> ids.add(treeKindToString(node.getKind()))); + String id = treeKindToString(node.getKind()); + identifierCombinations.forEach(ids -> ids.add(id)); } @Nullable @@ -462,6 +433,8 @@ private static List> copy(List> identifierCombinations) } private static class SourceIdentifierExtractor extends TreeScanner> { + private static final SourceIdentifierExtractor INSTANCE = new SourceIdentifierExtractor(); + @Nullable @Override public Void visitPackage(PackageTree node, Set identifiers) { From 2cb4bd0e57ac700bc061c366261ef76049e51007 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 8 Oct 2022 19:20:04 +0200 Subject: [PATCH 49/53] Suggestions --- .../main/java/tech/picnic/errorprone/refaster/runner/Node.java | 2 +- .../errorprone/refaster/runner/RefasterRuleSelector.java | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java index b83972363a..d541b5fae4 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java @@ -25,7 +25,7 @@ @AutoValue abstract class Node { static Node create( - ImmutableCollection values, + ImmutableList values, Function>> pathExtractor) { BuildNode tree = BuildNode.create(); tree.register(values, pathExtractor); diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java index 45021c165f..8d413f0652 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java @@ -4,7 +4,6 @@ import static java.util.Collections.newSetFromMap; import static java.util.stream.Collectors.toCollection; -import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.errorprone.refaster.BlockTemplate; @@ -91,7 +90,7 @@ private RefasterRuleSelector(Node> treeRules) { } /** Instantiates a new {@link RefasterRuleSelector} backed by the given {@link RefasterRule}s. */ - static RefasterRuleSelector create(ImmutableCollection> refasterRules) { + static RefasterRuleSelector create(ImmutableList> refasterRules) { return new RefasterRuleSelector( Node.create(refasterRules, RefasterRuleSelector::extractTemplateIdentifiers)); } From c34fa4decd2f45756fa1448bf81ee86d2e617548 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Sun, 9 Oct 2022 10:53:28 +0200 Subject: [PATCH 50/53] Fix typo --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 636a17b91b..06b0a46d57 100644 --- a/pom.xml +++ b/pom.xml @@ -1340,7 +1340,7 @@ + to verify fork identification. --> true From 5ba70753f6deca89abe1c1946a700ab8e212717a Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 9 Oct 2022 15:52:43 +0200 Subject: [PATCH 51/53] Introduce `AnnotatedCompositeCodeTransformerTest` --- refaster-support/pom.xml | 10 + .../AnnotatedCompositeCodeTransformer.java | 1 - ...AnnotatedCompositeCodeTransformerTest.java | 203 ++++++++++++++++++ 3 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java diff --git a/refaster-support/pom.xml b/refaster-support/pom.xml index 918986e91a..b7a93479b5 100644 --- a/refaster-support/pom.xml +++ b/refaster-support/pom.xml @@ -76,5 +76,15 @@ junit-jupiter-engine test + + org.junit.jupiter + junit-jupiter-params + test + + + org.mockito + mockito-core + test + diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java index 6d2fc60386..1d2871f9b3 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java @@ -37,7 +37,6 @@ * annotations} available on the matching {@link CodeTransformer} or on this {@link * CompositeCodeTransformer} as a fallback, if any. */ -// XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.) @AutoValue public abstract class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable { private static final long serialVersionUID = 1L; diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java new file mode 100644 index 0000000000..73de215d7e --- /dev/null +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java @@ -0,0 +1,203 @@ +package tech.picnic.errorprone.refaster; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.notNull; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.google.auto.value.AutoAnnotation; +import com.google.common.collect.ImmutableClassToInstanceMap; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Maps; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.CodeTransformer; +import com.google.errorprone.DescriptionListener; +import com.google.errorprone.ErrorProneOptions; +import com.google.errorprone.fixes.Fix; +import com.google.errorprone.matchers.Description; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.util.Context; +import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import java.lang.annotation.Annotation; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.annotation.Severity; + +// XXX: Test the `ErrorProneOptions`-based severity override logic. (Right now that logic is tested +// through `RefasterTest`, but ideally it is covered by tests in this class, closer to the code that +// implements the relevant logic.) +final class AnnotatedCompositeCodeTransformerTest { + private static final DiagnosticPosition DUMMY_POSITION = mock(DiagnosticPosition.class); + private static final Fix DUMMY_FIX = mock(Fix.class); + private static final TreePath DUMMY_PATH = mock(TreePath.class); + private static final String DEFAULT_PACKAGE = ""; + private static final String CUSTOM_PACKAGE = "com.example"; + private static final String SIMPLE_CLASS_NAME = "MyRefasterRule"; + + private static Stream applyTestCases() { + /* { context, packageName, ruleName compositeAnnotations, delegateAnnotations, expectedDescription } */ + return Stream.of( + arguments( + context(), + DEFAULT_PACKAGE, + SIMPLE_CLASS_NAME, + ImmutableSet.of(), + ImmutableSet.of(), + description( + SIMPLE_CLASS_NAME, Optional.empty(), SUGGESTION, "Refactoring opportunity")), + arguments( + context(), + CUSTOM_PACKAGE, + CUSTOM_PACKAGE + '.' + SIMPLE_CLASS_NAME, + ImmutableSet.of( + descriptionAnnotation("Composite description"), + documentationAnnotation("https://example.com"), + severityAnnotation(ERROR)), + ImmutableSet.of(), + description( + SIMPLE_CLASS_NAME, + Optional.of("https://example.com"), + ERROR, + "Composite description")), + arguments( + context(), + DEFAULT_PACKAGE, + SIMPLE_CLASS_NAME, + ImmutableSet.of(), + ImmutableSet.of( + descriptionAnnotation("Rule description"), + documentationAnnotation("https://example.com/rule/${topLevelClassName}"), + severityAnnotation(WARNING)), + description( + SIMPLE_CLASS_NAME, + Optional.of("https://example.com/rule/" + SIMPLE_CLASS_NAME), + WARNING, + "Rule description")), + arguments( + context(), + CUSTOM_PACKAGE, + CUSTOM_PACKAGE + '.' + SIMPLE_CLASS_NAME + ".SomeInnerClass.NestedEvenDeeper", + ImmutableSet.of( + descriptionAnnotation("Some description"), + documentationAnnotation("https://example.com"), + severityAnnotation(ERROR)), + ImmutableSet.of( + descriptionAnnotation("Overriding description"), + documentationAnnotation( + "https://example.com/rule/${topLevelClassName}/${nestedClassName}"), + severityAnnotation(SUGGESTION)), + description( + SIMPLE_CLASS_NAME + ".SomeInnerClass.NestedEvenDeeper", + Optional.of( + "https://example.com/rule/" + + SIMPLE_CLASS_NAME + + "/SomeInnerClass.NestedEvenDeeper"), + SUGGESTION, + "Overriding description"))); + } + + @MethodSource("applyTestCases") + @ParameterizedTest + void apply( + Context context, + String packageName, + String ruleName, + ImmutableSet compositeAnnotations, + ImmutableSet delegateAnnotations, + Description expectedDescription) { + CodeTransformer codeTransformer = + AnnotatedCompositeCodeTransformer.create( + packageName, + ImmutableList.of( + delegateCodeTransformer( + delegateAnnotations, context, refasterDescription(ruleName))), + indexAnnotations(compositeAnnotations)); + + List collected = new ArrayList<>(); + codeTransformer.apply(DUMMY_PATH, context, collected::add); + assertThat(collected) + .satisfiesExactly( + actual -> { + assertThat(actual.position).isEqualTo(expectedDescription.position); + assertThat(actual.checkName).isEqualTo(expectedDescription.checkName); + assertThat(actual.fixes).containsExactlyElementsOf(expectedDescription.fixes); + assertThat(actual.getLink()).isEqualTo(expectedDescription.getLink()); + assertThat(actual.getRawMessage()).isEqualTo(expectedDescription.getRawMessage()); + }); + } + + private static ImmutableClassToInstanceMap indexAnnotations( + ImmutableSet annotations) { + return ImmutableClassToInstanceMap.copyOf( + Maps.uniqueIndex(annotations, Annotation::annotationType)); + } + + private static CodeTransformer delegateCodeTransformer( + ImmutableSet annotations, + Context expectedContext, + Description returnedDescription) { + CodeTransformer codeTransformer = mock(CodeTransformer.class); + + when(codeTransformer.annotations()).thenReturn(indexAnnotations(annotations)); + doAnswer( + inv -> { + inv.getArgument(2).onDescribed(returnedDescription); + return null; + }) + .when(codeTransformer) + .apply(eq(DUMMY_PATH), eq(expectedContext), notNull()); + + return codeTransformer; + } + + /** + * Returns a {@link Description} with some default values as produced by {@link + * com.google.errorprone.refaster.RefasterScanner}. + */ + private static Description refasterDescription(String name) { + return description(name, Optional.of(""), WARNING, ""); + } + + private static Description description( + String name, Optional link, SeverityLevel severityLevel, String message) { + return Description.builder(DUMMY_POSITION, name, link.orElse(null), severityLevel, message) + .addFix(DUMMY_FIX) + .build(); + } + + private static Context context() { + Context context = mock(Context.class); + when(context.get(ErrorProneOptions.class)) + .thenReturn(ErrorProneOptions.processArgs(ImmutableList.of())); + return context; + } + + @AutoAnnotation + private static tech.picnic.errorprone.refaster.annotation.Description descriptionAnnotation( + String value) { + return new AutoAnnotation_AnnotatedCompositeCodeTransformerTest_descriptionAnnotation(value); + } + + @AutoAnnotation + private static OnlineDocumentation documentationAnnotation(String value) { + return new AutoAnnotation_AnnotatedCompositeCodeTransformerTest_documentationAnnotation(value); + } + + @AutoAnnotation + private static Severity severityAnnotation(SeverityLevel value) { + return new AutoAnnotation_AnnotatedCompositeCodeTransformerTest_severityAnnotation(value); + } +} From 72f888125a62f79ef623aa9093060fdafd61c24b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 9 Oct 2022 18:45:26 +0200 Subject: [PATCH 52/53] Suggestions --- pom.xml | 3 --- .../errorprone/refaster/runner/Node.java | 19 +++++++++---------- .../errorprone/refaster/runner/Refaster.java | 5 ----- .../refaster/runner/RefasterRuleSelector.java | 14 ++++---------- 4 files changed, 13 insertions(+), 28 deletions(-) diff --git a/pom.xml b/pom.xml index 39a79754df..4eb7e4cdf2 100644 --- a/pom.xml +++ b/pom.xml @@ -1296,9 +1296,6 @@ 4 4 false - - *.AutoValue_* - diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java index 16bd484cee..f5c9553139 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Node.java @@ -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; @@ -27,9 +26,9 @@ abstract class Node { // there be such an overload? static Node create( Set values, Function>> pathExtractor) { - BuildNode tree = BuildNode.create(); + Builder tree = Builder.create(); tree.register(values, pathExtractor); - return tree.immutable(); + return tree.build(); } abstract ImmutableMap> children(); @@ -75,12 +74,12 @@ private void collectReachableValues(ImmutableList candidateEdges, Consum @AutoValue @SuppressWarnings("AutoValueImmutableFields" /* Type is used only during `Node` construction. */) - abstract static class BuildNode { - private static BuildNode create() { - return new AutoValue_Node_BuildNode<>(new HashMap<>(), new ArrayList<>()); + abstract static class Builder { + private static Builder create() { + return new AutoValue_Node_Builder<>(new HashMap<>(), new ArrayList<>()); } - abstract Map> children(); + abstract Map> children(); abstract List values(); @@ -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))); } } @@ -118,9 +117,9 @@ private void registerPath(T value, ImmutableList path) { } } - private Node immutable() { + private Node build() { return new AutoValue_Node<>( - ImmutableMap.copyOf(Maps.transformValues(children(), BuildNode::immutable)), + ImmutableMap.copyOf(Maps.transformValues(children(), Builder::build)), ImmutableList.copyOf(values())); } } diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index e70a8d8a51..09b7c7ee47 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java @@ -82,11 +82,6 @@ public Refaster(ErrorProneFlags flags) { public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { Set 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 matches = new ArrayList<>(); diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java index 98b7088dc6..1a72b609e4 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java +++ b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterRuleSelector.java @@ -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> extractRuleIdentifiers( RefasterRule refasterRule) { ImmutableSet.Builder> results = ImmutableSet.builder(); @@ -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 = @@ -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) { - return UCLASS_IDENT.equals(tree.getClass()) || UCLASS_AUTOVALUE_IDENT.equals(tree.getClass()); + return UCLASS_IDENT.isAssignableFrom(tree.getClass()); } static ImmutableList getBeforeTemplates(RefasterRule refasterRule) { @@ -320,12 +315,11 @@ static ImmutableList getTemplateStatements(BlockTemplate template) { return invokeMethod(METHOD_BLOCK_TEMPLATE_TEMPLATE_STATEMENTS, template); } - // Actually UClassIdent. static IdentifierTree getClassIdent(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`. static String getTopLevelClass(IdentifierTree uClassIdent) { return invokeMethod(METHOD_UCLASS_IDENT_GET_TOP_LEVEL_CLASS, uClassIdent); } From 8d99611af729181d869bf1b778a0b2fb20586cea Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 24 Dec 2024 13:35:11 +0100 Subject: [PATCH 53/53] Post-merge-fix --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index fdef6bd505..f4035c4aed 100644 --- a/pom.xml +++ b/pom.xml @@ -1945,7 +1945,7 @@ com.jakewharton.nopen nopen-checker - ${version.nopen-checker} + ${version.nopen} com.uber.nullaway