From 230a4f84ff2671c5d1f99a64ffef956e8654fc11 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 21 Sep 2022 22:15:36 +0200 Subject: [PATCH 01/43] 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 203e2fe25b..c02022f926 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 ba435707574d1189ff3966d80f3c0c7aa8aea994 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 22 Sep 2022 09:55:23 +0200 Subject: [PATCH 02/43] 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 cda3004c0e..d4cbbd90ee 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( ruleName -> loadCodeTransformer(resource) - .ifPresent(transformer -> transformers.put(ruleName, transformer))); + .ifPresent( + transformer -> + transformers.put( + ruleName, + new CodeTransformerDescriptionAdapter( + ruleName, 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 c02022f926..203e2fe25b 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 a9141e20df0520deec86562100b34e65bee4ee2b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 22 Sep 2022 18:05:36 +0200 Subject: [PATCH 03/43] 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 | 51 ++++++++++++------- refaster-runner/pom.xml | 4 ++ .../refaster/runner/CodeTransformers.java | 7 +++ .../AnnotatedCompositeCodeTransformer.java | 37 ++++++++++++++ .../annotation/TemplateCollection.java | 12 +++++ refaster-test-support/pom.xml | 4 ++ .../test/MatchInWrongMethodRules.java | 8 +-- 8 files changed, 105 insertions(+), 22 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 c494ab1dc0..dc43b58dfa 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 7731ee0810..4279a076f1 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 = compileRefasterRules(tree); - for (Map.Entry> rule : Multimaps.asMap(rules).entrySet()) { + ImmutableMap rules = compileRefasterRules(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 rules", e); } @@ -87,17 +87,30 @@ public Boolean reduce(Boolean r1, Boolean r2) { }.scan(tree, null)); } - private ImmutableListMultimap compileRefasterRules(ClassTree tree) { - ListMultimap rules = ArrayListMultimap.create(); - new TreeScanner() { + private ImmutableMap compileRefasterRules(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 { @@ -117,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 c0265fc3a7..3726633d40 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 d4cbbd90ee..662b8e1432 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_RULE_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 92a3b9bd3f..fb6b84a6e9 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/MatchInWrongMethodRules.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java index ff25d1b74f..90a734f971 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java @@ -2,11 +2,13 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import tech.picnic.errorprone.refaster.annotation.TemplateCollection; /** Refaster rule collection to validate reporting of a match occurring in an unexpected place. */ -final class MatchInWrongMethodRules { - private MatchInWrongMethodRules() {} - +@TemplateCollection(linkPattern = "XXX") +final class MatchInWrongMethodTemplates { + // XXX: Test merging/overriding. + // @TemplateCollection(linkPattern = "XXX") static final class StringIsEmpty { @BeforeTemplate boolean before(String string) { From 1477c5a4f6cb34339750991fdc956f44d46b154a Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 22 Sep 2022 21:46:44 +0200 Subject: [PATCH 04/43] 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/MatchInWrongMethodRules.java | 8 ++- 7 files changed, 94 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 4279a076f1..ff279be3c9 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 3726633d40..c0265fc3a7 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 662b8e1432..cda3004c0e 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_RULE_SUFFIX} files and loads them as {@link @@ -58,12 +51,7 @@ private static ImmutableListMultimap loadAllCodeTransfo .ifPresent( ruleName -> loadCodeTransformer(resource) - .ifPresent( - transformer -> - transformers.put( - ruleName, - new CodeTransformerDescriptionAdapter( - ruleName, transformer)))); + .ifPresent(transformer -> transformers.put(ruleName, 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 fb6b84a6e9..5a0134f803 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/MatchInWrongMethodRules.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java index 90a734f971..40e460c098 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.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; @@ -7,8 +9,10 @@ /** Refaster rule collection to validate reporting of a match occurring in an unexpected place. */ @TemplateCollection(linkPattern = "XXX") final class MatchInWrongMethodTemplates { - // XXX: Test merging/overriding. - // @TemplateCollection(linkPattern = "XXX") + private MatchInWrongMethodTemplates() {} + + // XXX: Demo: nesting overrides. + @TemplateCollection(linkPattern = "YYY", severity = ERROR, description = "Foo") static final class StringIsEmpty { @BeforeTemplate boolean before(String string) { From e89472afef0b84da70f7ae569cd7f631ca43e000 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 23 Sep 2022 17:00:55 +0200 Subject: [PATCH 05/43] Tweaks --- .../RefasterRuleCompilerTaskListener.java | 38 ++++++++++++++----- refaster-runner/pom.xml | 8 ++++ 2 files changed, 37 insertions(+), 9 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 ff279be3c9..ae352eb66f 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 @@ -112,15 +112,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 +123,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 c0265fc3a7..29fcbbb983 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 c4beed3d3f6691220e0bfd75f0cc644d0c6f3371 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 23 Sep 2022 21:01:48 +0200 Subject: [PATCH 06/43] Better annotation support, simplify setup --- .../refasterrules/OptionalRules.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/MatchInWrongMethodRules.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/refasterrules/OptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java index 7c990060a5..b030cca40f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.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 rules related to expressions dealing with {@link Optional}s. */ +@OnlineDocumentation final class OptionalRules { private OptionalRules() {} @@ -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 dc43b58dfa..11e78cf58d 100644 --- a/refaster-compiler/pom.xml +++ b/refaster-compiler/pom.xml @@ -14,6 +14,10 @@ A Java compiler plugin that identifies and compiles Refaster rules, 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 ae352eb66f..18b9f03dd9 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 29fcbbb983..9640462eec 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 5a0134f803..2843d8f1f6 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/MatchInWrongMethodRules.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java index 40e460c098..07f8866242 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java @@ -1,18 +1,18 @@ 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 rule 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 9622b43fe7c4c8189fefce7c4735d8b436022146 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 23 Sep 2022 22:26:58 +0200 Subject: [PATCH 07/43] 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 3238a0950cdcc8c20b9443284b6fce8c029ef37b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 24 Sep 2022 15:21:03 +0200 Subject: [PATCH 08/43] 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 | 53 +++++++- .../refaster/runner/CodeTransformersTest.java | 7 +- .../errorprone/refaster/runner/FooRules.java | 84 +++++++++++- .../refaster/runner/RefasterTest.java | 120 ++++++++++++++++++ .../annotation/OnlineDocumentation.java | 6 +- refaster-test-support/pom.xml | 6 - .../refaster/test/RefasterRuleCollection.java | 10 +- .../test/MatchInWrongMethodRules.java | 4 - 12 files changed, 338 insertions(+), 55 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 18b9f03dd9..426ce8baeb 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 @@ -93,19 +93,18 @@ private ImmutableMap compileRefasterRules(ClassTree @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(); @@ -113,13 +112,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) { @@ -151,8 +150,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 9640462eec..807b58e314 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 203e2fe25b..99ed10ee6c 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,8 +45,8 @@ * A {@link BugChecker} that flags code that can be simplified using Refaster rules located on the * classpath. * - *

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

This checker locates all {@code *.refaster} classpath resources and assumes that they contain + * a {@link CodeTransformer}. The set of loaded Refaster rules can be restricted by passing {@code * -XepOpt:Refaster:NamePattern=}. */ @AutoService(BugChecker.class) @@ -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 5888f03422..ba3898fdab 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 @@ -12,6 +12,11 @@ final class CodeTransformersTest { @Test void getAllCodeTransformers() { assertThat(CodeTransformers.getAllCodeTransformers().keySet()) - .containsExactly("FooRules$SimpleRule"); + .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/FooRules.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.java index be79875a47..a892141770 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.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 rule collection used to test {@link CodeTransformers}. */ +/** An example template collection used to test {@link CodeTransformers} and {@link Refaster}. */ final class FooRules { private FooRules() {} - /** Simple rule for testing purposes. */ - static final class SimpleRule { + /** 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 2843d8f1f6..92a3b9bd3f 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/RefasterRuleCollection.java b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java index ce01ec9919..221c3a7844 100644 --- a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java +++ b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java @@ -178,8 +178,7 @@ private static ImmutableRangeMap indexRuleMatches( Set replacements = Iterables.getOnlyElement(description.fixes).getReplacements(endPositions); for (Replacement replacement : replacements) { - ruleMatches.put( - replacement.range(), getSubstringAfterFinalDelimiter('.', description.checkName)); + ruleMatches.put(replacement.range(), extractRefasterTemplateName(description)); } } @@ -226,6 +225,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/MatchInWrongMethodRules.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java index 07f8866242..ee47a66737 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java @@ -9,10 +9,6 @@ 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 0c1f824a9d4327ddb71c8caeee8bc4a97774ac20 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 24 Sep 2022 19:57:44 +0200 Subject: [PATCH 09/43] 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 41cf97d550f1c310693afe83e14e8f4acb446bcf Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 24 Sep 2022 22:47:54 +0200 Subject: [PATCH 10/43] 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 807b58e314..d7ece6290f 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 aeda8b6eb89465eba987c24145d6e2b2285d351d Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 25 Sep 2022 11:00:08 +0200 Subject: [PATCH 11/43] 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 d7ece6290f..545e2f24de 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -37,6 +37,14 @@ ${project.groupId} refaster-compiler + runtime From d9842955f7829cf0bd17879312d064bf2838fd61 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 27 Sep 2022 15:21:19 +0200 Subject: [PATCH 12/43] 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 006a43e210b85f031026ad9737c7cbdf1526b35f Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 29 Sep 2022 13:35:32 +0200 Subject: [PATCH 13/43] 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 67001d1494..3d4d0d7d70 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 @@ -1533,6 +1538,11 @@ failing the build on the first error encountered. --> -XepAllErrorsAsWarnings + + -XepDisableWarningsInGeneratedCode diff --git a/refaster-compiler/pom.xml b/refaster-compiler/pom.xml index 11e78cf58d..86d0ecc678 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 426ce8baeb..4abd64fd25 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 @@ -100,7 +100,7 @@ public Void visitClass(ClassTree node, ImmutableClassToInstanceMap a if (!transformers.isEmpty()) { rules.put( node, - new AnnotatedCompositeCodeTransformer( + AnnotatedCompositeCodeTransformer.create( toPackageName(symbol), transformers, annotations)); } From e4ab1d07ada4ad72bafa3bddfc72f1693b44a27a Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 29 Sep 2022 20:22:46 +0200 Subject: [PATCH 14/43] Support `AllSuggestionsAsWarnings` and add a suggestion --- .../AnnotatedCompositeCodeTransformer.java | 26 +++++- .../refaster/runner/RefasterTest.java | 93 +++++++++++++------ .../refaster/test/RefasterRuleCollection.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/RefasterRuleCollection.java b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java index 221c3a7844..d62ccc4ab8 100644 --- a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java +++ b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java @@ -228,7 +228,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 0b62ef904a08b82397f31ba91c23dcd4569603d1 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 29 Sep 2022 22:42:25 +0200 Subject: [PATCH 15/43] 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 544b5d80c63e34cde7da35b39fa8b0edebe6ae9e Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 29 Sep 2022 23:24:05 +0200 Subject: [PATCH 16/43] Suggestions, introduce `ErrorProneFork` --- pom.xml | 7 ++ .../AnnotatedCompositeCodeTransformer.java | 27 ++----- .../refaster/plugin/ErrorProneFork.java | 58 ++++++++++++++ .../refaster/runner/RefasterTest.java | 77 ++++++++++--------- .../refaster/test/RefasterRuleCollection.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 3d4d0d7d70..27b3b697ad 100644 --- a/pom.xml +++ b/pom.xml @@ -622,6 +622,10 @@ + + + @@ -1261,6 +1265,9 @@ pitest-maven 1.9.8 + + *.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/RefasterRuleCollection.java b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java index d62ccc4ab8..7f380a60e4 100644 --- a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java +++ b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java @@ -175,10 +175,11 @@ private static ImmutableRangeMap indexRuleMatches( ImmutableRangeMap.Builder ruleMatches = ImmutableRangeMap.builder(); for (Description description : matches) { + String templateName = extractRefasterTemplateName(description); Set replacements = Iterables.getOnlyElement(description.fixes).getReplacements(endPositions); for (Replacement replacement : replacements) { - ruleMatches.put(replacement.range(), extractRefasterTemplateName(description)); + ruleMatches.put(replacement.range(), templateName); } } @@ -228,7 +229,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 27cf20a0cc138c5033d53430a3ebc7b9cc9b7e58 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 29 Sep 2022 23:34:54 +0200 Subject: [PATCH 17/43] 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 95db08fd303bf4d791a6bfc66cab43a3ed1cfc46 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 30 Sep 2022 06:46:22 +0200 Subject: [PATCH 18/43] 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 99ed10ee6c..26aad0680f 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 b1ac4acfe1905072169fb12e3d3b0e396c6d6e5c Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Fri, 30 Sep 2022 12:02:42 +0200 Subject: [PATCH 19/43] 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 26aad0680f..f0e1ee4853 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 36445644e8afe2265d5b33c97cf33aefc14adcd3 Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Fri, 30 Sep 2022 15:19:45 +0200 Subject: [PATCH 20/43] 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 258e3d20a8d29d9f4a30ff09dd5811385360072c Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Fri, 30 Sep 2022 18:44:21 +0200 Subject: [PATCH 21/43] 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 1888b63a71a0016112daca8a88a282838345d6d5 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 30 Sep 2022 21:54:50 +0200 Subject: [PATCH 22/43] 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 f0e1ee4853..36ad85a31b 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 76c6941755d46d85ce7b7bf46f9d7f9339d26f07 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 12:44:17 +0200 Subject: [PATCH 23/43] 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 86d0ecc678..dc43b58dfa 100644 --- a/refaster-compiler/pom.xml +++ b/refaster-compiler/pom.xml @@ -14,10 +14,6 @@ A Java compiler plugin that identifies and compiles Refaster rules, 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 4abd64fd25..1190ddb926 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 545e2f24de..8437ab7d95 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 51680fc3f4..a4a8267408 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 c440910ce46ae94d72543a6a1f2b090cfe4f346b Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 13:24:01 +0200 Subject: [PATCH 24/43] `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 be7863165b9c73287f0ebaa35c535acd51d019e0 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 13:54:45 +0200 Subject: [PATCH 25/43] 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 248590efff92981db994a85dc7af5f8def13e767 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 4 Oct 2022 14:08:09 +0200 Subject: [PATCH 26/43] Revert changes in `OptionalTemplates` --- .../tech/picnic/errorprone/refasterrules/OptionalRules.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java index b030cca40f..86be998ccb 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.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 rules related to expressions dealing with {@link Optional}s. */ -@OnlineDocumentation final class OptionalRules { private OptionalRules() {} @@ -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 8d5621a6f518929583168a385817d433b86387a5 Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Tue, 4 Oct 2022 16:17:06 +0200 Subject: [PATCH 27/43] 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 0a373c3a3b3b1ebbe7dec5378393fb5f83201a3b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 7 Oct 2022 13:27:50 +0200 Subject: [PATCH 28/43] 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 36ad85a31b..75f41ee375 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 58d882c060a0bdd6080f24a55cdad4d3054acdbb Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 8 Oct 2022 12:38:29 +0200 Subject: [PATCH 29/43] Suggestions --- .../refasterrules/OptionalRules.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/refasterrules/OptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java index 86be998ccb..7c990060a5 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.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 27b3b697ad..fb0bbb993c 100644 --- a/pom.xml +++ b/pom.xml @@ -1330,6 +1330,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 a4a8267408..b76a4b9c93 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 1e9253ea0316ce4e0afb5712497c8188ab0ca28b Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Sun, 9 Oct 2022 10:53:28 +0200 Subject: [PATCH 30/43] Fix typo --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index fb0bbb993c..84a6a5572a 100644 --- a/pom.xml +++ b/pom.xml @@ -1339,7 +1339,7 @@ + to verify fork identification. --> true From d6f8106ad80b8edc8df5a7f722a488e341d6a0a2 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 9 Oct 2022 15:52:43 +0200 Subject: [PATCH 31/43] 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 b76a4b9c93..b8a1835630 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 e00e4b0ca6d93dd10c6e5464c5889a9c451ae089 Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Sun, 9 Oct 2022 21:41:13 +0200 Subject: [PATCH 32/43] Improve test assumption reasoning --- .../tech/picnic/errorprone/refaster/ErrorProneForkTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index cfe6888943..9b7703f4e7 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/ErrorProneForkTest.java +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/ErrorProneForkTest.java @@ -33,7 +33,8 @@ void isSuggestionsAsWarningsEnabledWithoutFlag() { @Test void isSuggestionsAsWarningsEnabledWithFlag() { - assumeTrue(ErrorProneFork.isErrorProneForkAvailable()); + assumeTrue( + ErrorProneFork.isErrorProneForkAvailable(), "The Error Prone fork is not on the classpath"); CompilationTestHelper.newInstance(TestChecker.class, getClass()) .setArgs("-XepAllSuggestionsAsWarnings") From 7c2cbabdd57aca1321fd97a82fae659cbaf1a698 Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Sun, 9 Oct 2022 22:04:08 +0200 Subject: [PATCH 33/43] Add missing comma --- .../refaster/AnnotatedCompositeCodeTransformerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 73de215d7e..1de0fbccc5 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java @@ -48,7 +48,7 @@ final class AnnotatedCompositeCodeTransformerTest { private static final String SIMPLE_CLASS_NAME = "MyRefasterRule"; private static Stream applyTestCases() { - /* { context, packageName, ruleName compositeAnnotations, delegateAnnotations, expectedDescription } */ + /* { context, packageName, ruleName, compositeAnnotations, delegateAnnotations, expectedDescription } */ return Stream.of( arguments( context(), From 432c954550748b46c3325eae412ac89055126e6f Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 10 Oct 2022 05:39:30 +0200 Subject: [PATCH 34/43] Tweak --- .../tech/picnic/errorprone/refaster/ErrorProneForkTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index 9b7703f4e7..06b11b8501 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/ErrorProneForkTest.java +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/ErrorProneForkTest.java @@ -34,7 +34,8 @@ void isSuggestionsAsWarningsEnabledWithoutFlag() { @Test void isSuggestionsAsWarningsEnabledWithFlag() { assumeTrue( - ErrorProneFork.isErrorProneForkAvailable(), "The Error Prone fork is not on the classpath"); + ErrorProneFork.isErrorProneForkAvailable(), + "Picnic's Error Prone fork is not on the classpath"); CompilationTestHelper.newInstance(TestChecker.class, getClass()) .setArgs("-XepAllSuggestionsAsWarnings") From ec1895bcf4244f2459c93ca2d2a4e4b1d17a160b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 10 Oct 2022 08:32:21 +0200 Subject: [PATCH 35/43] Some instructions for our future selves --- .../refaster/AnnotatedCompositeCodeTransformerTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 index 1de0fbccc5..d49dcf3006 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java @@ -38,7 +38,7 @@ // 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.) +// implements the relevant logic.) See the comment in `#context()` below. final class AnnotatedCompositeCodeTransformerTest { private static final DiagnosticPosition DUMMY_POSITION = mock(DiagnosticPosition.class); private static final Fix DUMMY_FIX = mock(Fix.class); @@ -179,9 +179,10 @@ private static Description description( } private static Context context() { + // XXX: Use `ErrorProneOptions#processArgs` to test the + // `AnnotatedCompositeCodeTransformer#overrideSeverity` logic. Context context = mock(Context.class); - when(context.get(ErrorProneOptions.class)) - .thenReturn(ErrorProneOptions.processArgs(ImmutableList.of())); + when(context.get(ErrorProneOptions.class)).thenReturn(ErrorProneOptions.empty()); return context; } From 1c5514ede70f4a8f647200df4574b677d6e6ef44 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 10 Oct 2022 11:04:02 +0200 Subject: [PATCH 36/43] `s/Refaster template/Refaster rule/` in `OnlineDocumentation` --- .../errorprone/refaster/annotation/OnlineDocumentation.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 ae56f3d621..9d725b9a5d 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 @@ -6,7 +6,7 @@ import java.lang.annotation.Target; /** - * Signals that a Refaster template or group of Refaster templates comes with online documentation. + * Signals that a Refaster rule or group of Refaster rule comes with online documentation. * *

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. @@ -42,7 +42,7 @@ * TOP_LEVEL_CLASS_URL_PLACEHOLDER} and {@value NESTED_CLASS_URL_PLACEHOLDER} placeholders. */ String value() default - "https://error-prone.picnic.tech/refastertemplates/" + "https://error-prone.picnic.tech/refasterrules/" + TOP_LEVEL_CLASS_URL_PLACEHOLDER + '#' + NESTED_CLASS_URL_PLACEHOLDER; From 54bba33043761d74c9f05e6f2e958b14c764925e Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 10 Oct 2022 11:13:29 +0200 Subject: [PATCH 37/43] Sorry forgot the test --- .../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 0ddad27c10..2c84a4f954 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 @@ -37,7 +37,7 @@ final class RefasterTest { containsPattern( "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeOneTemplate: " + "A custom description about matching single-char strings\\s+.+\\s+" - + "\\(see https://error-prone.picnic.tech/refastertemplates/FooTemplates#StringOfSizeOneTemplate\\)")) + + "\\(see https://error-prone.picnic.tech/refasterrules/FooTemplates#StringOfSizeOneTemplate\\)")) .expectErrorMessage( "StringOfSizeTwoTemplate", containsPattern( From 74835eff9c4907c1de0fee85fbd21bc6bc50645c Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 11 Oct 2022 15:47:03 +0200 Subject: [PATCH 38/43] `s/templates/rule/` for some classes, still WIP --- .../RefasterRuleCompilerTaskListener.java | 21 +------------------ .../annotation/OnlineDocumentation.java | 21 +++++++++---------- 2 files changed, 11 insertions(+), 31 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 1190ddb926..3c13b74d67 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 containsRefasterRules(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 compileRefasterRules(ClassTree tree) { ImmutableMap.Builder rules = ImmutableMap.builder(); new TreeScanner>() { @@ -122,7 +103,7 @@ private FileObject getOutputFile(TaskEvent taskEvent, ClassTree tree) throws IOE taskEvent.getSourceFile()); } - private static boolean containsRefasterTemplates(ClassTree tree) { + private static boolean containsRefasterRules(ClassTree tree) { return Boolean.TRUE.equals( new TreeScanner() { @Override 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 9d725b9a5d..f41a2f1b67 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 @@ -6,13 +6,13 @@ import java.lang.annotation.Target; /** - * Signals that a Refaster rule or group of Refaster rule comes with online documentation. + * Signals that a Refaster rule or group of Refaster rules comes with online documentation. * *

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 + *

By default it is assumed that the Refaster rule(s) are documented on the Error Prone Support + * website. Annotations on nested classes override the documentation URL associated with any * enclosing class. */ @Target(ElementType.TYPE) @@ -20,23 +20,22 @@ public @interface OnlineDocumentation { /** * The URL placeholder value that will be replaced with the name of the top-level class in which - * the annotated Refaster template is located. + * the annotated Refaster rule 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. + * annotated Refaster rule 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. + *

If the Refaster rule is not defined in a nested class then this placeholder will be replaced + * with the empty string. In case the Refaster rule 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. + * The URL or URL pattern of the website at which the annotated Refaster rule(s) are documented. * * @return A non-{@code null} string, optionally containing the {@value * TOP_LEVEL_CLASS_URL_PLACEHOLDER} and {@value NESTED_CLASS_URL_PLACEHOLDER} placeholders. From e8af23928c8341db4fc732d698750cb0097ccfe1 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 11 Oct 2022 16:42:01 +0200 Subject: [PATCH 39/43] Make last files consistent with `refaster rules` --- .../errorprone/refaster/runner/Refaster.java | 2 +- .../refaster/runner/CodeTransformersTest.java | 10 +++---- .../errorprone/refaster/runner/FooRules.java | 30 +++++++++---------- .../refaster/runner/RefasterTest.java | 30 +++++++++---------- .../AnnotatedCompositeCodeTransformer.java | 2 +- .../refaster/annotation/Description.java | 4 +-- .../refaster/annotation/Severity.java | 4 +-- .../refaster/annotation/package-info.java | 4 +-- .../refaster/test/RefasterRuleCollection.java | 8 ++--- .../test/MatchInWrongMethodRules.java | 4 +-- 10 files changed, 48 insertions(+), 50 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 75f41ee375..ef3b70161f 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 @@ -154,7 +154,7 @@ private static Optional toSeverityLevel(Severity 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 + *

The original check name (i.e. the Refaster rule name) is prepended to the {@link * 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 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 ba3898fdab..66b3a082dd 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 @@ -13,10 +13,10 @@ final class CodeTransformersTest { void getAllCodeTransformers() { assertThat(CodeTransformers.getAllCodeTransformers().keySet()) .containsExactlyInAnyOrder( - "FooTemplates$StringOfSizeZeroTemplate", - "FooTemplates$StringOfSizeZeroVerboseTemplate", - "FooTemplates$StringOfSizeOneTemplate", - "FooTemplates$ExtraGrouping$StringOfSizeTwoTemplate", - "FooTemplates$ExtraGrouping$StringOfSizeThreeTemplate"); + "FooRules$StringOfSizeZeroRule", + "FooRules$StringOfSizeZeroVerboseRule", + "FooRules$StringOfSizeOneRule", + "FooRules$ExtraGrouping$StringOfSizeTwoRule", + "FooRules$ExtraGrouping$StringOfSizeThreeRule"); } } diff --git a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.java b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.java index a892141770..df20e66eb7 100644 --- a/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.java +++ b/refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.java @@ -10,12 +10,12 @@ import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.annotation.Severity; -/** An example template collection used to test {@link CodeTransformers} and {@link Refaster}. */ +/** An example rule collection used to test {@link CodeTransformers} and {@link Refaster}. */ final class FooRules { private FooRules() {} - /** A simple template for testing purposes, lacking any custom annotations. */ - static final class StringOfSizeZeroTemplate { + /** A simple rule for testing purposes, lacking any custom annotations. */ + static final class StringOfSizeZeroRule { @BeforeTemplate boolean before(String string) { return string.toCharArray().length == 0; @@ -28,10 +28,10 @@ boolean after(String string) { } /** - * A simple template for testing purposes, matching the same set of expressions as {@link - * StringOfSizeZeroTemplate}, but producing a larger replacement string. + * A simple rule for testing purposes, matching the same set of expressions as {@link + * StringOfSizeZeroRule}, but producing a larger replacement string. */ - static final class StringOfSizeZeroVerboseTemplate { + static final class StringOfSizeZeroVerboseRule { @BeforeTemplate boolean before(String string) { return string.toCharArray().length == 0; @@ -43,11 +43,11 @@ boolean after(String string) { } } - /** A simple template for testing purposes, having several custom annotations. */ + /** A simple rule for testing purposes, having several custom annotations. */ @Description("A custom description about matching single-char strings") @OnlineDocumentation @Severity(WARNING) - static final class StringOfSizeOneTemplate { + static final class StringOfSizeOneRule { @BeforeTemplate boolean before(String string) { return string.toCharArray().length == 1; @@ -59,17 +59,15 @@ boolean after(String string) { } } - /** - * A nested class with annotations that are inherited by the Refaster templates contained in it. - */ + /** A nested class with annotations that are inherited by the Refaster rules contained in it. */ @Description("A custom subgroup description") - @OnlineDocumentation("https://example.com/template/${topLevelClassName}#${nestedClassName}") + @OnlineDocumentation("https://example.com/rule/${topLevelClassName}#${nestedClassName}") @Severity(ERROR) static final class ExtraGrouping { private ExtraGrouping() {} - /** A simple template for testing purposes, inheriting custom annotations. */ - static final class StringOfSizeTwoTemplate { + /** A simple rule for testing purposes, inheriting custom annotations. */ + static final class StringOfSizeTwoRule { @BeforeTemplate boolean before(String string) { return string.toCharArray().length == 2; @@ -81,11 +79,11 @@ boolean after(String string) { } } - /** A simple template for testing purposes, overriding custom annotations. */ + /** A simple rule 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 { + static final class StringOfSizeThreeRule { @BeforeTemplate boolean before(String string) { return string.toCharArray().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 index 2c84a4f954..8ec4e5122c 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 @@ -29,25 +29,25 @@ final class RefasterTest { CompilationTestHelper.newInstance(Refaster.class, getClass()) .matchAllDiagnostics() .expectErrorMessage( - "StringOfSizeZeroTemplate", + "StringOfSizeZeroRule", containsPattern( - "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeZeroTemplate: Refactoring opportunity\\s+.+\\s+")) + "\\[Refaster Rule\\] FooRules\\.StringOfSizeZeroRule: Refactoring opportunity\\s+.+\\s+")) .expectErrorMessage( - "StringOfSizeOneTemplate", + "StringOfSizeOneRule", containsPattern( - "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeOneTemplate: " + "\\[Refaster Rule\\] FooRules\\.StringOfSizeOneRule: " + "A custom description about matching single-char strings\\s+.+\\s+" - + "\\(see https://error-prone.picnic.tech/refasterrules/FooTemplates#StringOfSizeOneTemplate\\)")) + + "\\(see https://error-prone.picnic.tech/refasterrules/FooRules#StringOfSizeOneRule\\)")) .expectErrorMessage( - "StringOfSizeTwoTemplate", + "StringOfSizeTwoRule", containsPattern( - "\\[Refaster Rule\\] FooTemplates\\.ExtraGrouping\\.StringOfSizeTwoTemplate: " + "\\[Refaster Rule\\] FooRules\\.ExtraGrouping\\.StringOfSizeTwoRule: " + "A custom subgroup description\\s+.+\\s+" - + "\\(see https://example.com/template/FooTemplates#ExtraGrouping.StringOfSizeTwoTemplate\\)")) + + "\\(see https://example.com/rule/FooRules#ExtraGrouping.StringOfSizeTwoRule\\)")) .expectErrorMessage( - "StringOfSizeThreeTemplate", + "StringOfSizeThreeRule", containsPattern( - "\\[Refaster Rule\\] FooTemplates\\.ExtraGrouping\\.StringOfSizeThreeTemplate: " + "\\[Refaster Rule\\] FooRules\\.ExtraGrouping\\.StringOfSizeThreeRule: " + "A custom description about matching three-char strings\\s+.+\\s+" + "\\(see https://example.com/custom\\)")); private final BugCheckerRefactoringTestHelper refactoringTestHelper = @@ -55,7 +55,7 @@ final class RefasterTest { private final BugCheckerRefactoringTestHelper restrictedRefactoringTestHelper = BugCheckerRefactoringTestHelper.newInstance(Refaster.class, getClass()) .setArgs( - "-XepOpt:Refaster:NamePattern=.*\\$(StringOfSizeZeroVerboseTemplate|StringOfSizeTwoTemplate)$"); + "-XepOpt:Refaster:NamePattern=.*\\$(StringOfSizeZeroVerboseRule|StringOfSizeTwoRule)$"); @Test void identification() { @@ -64,13 +64,13 @@ void identification() { "A.java", "class A {", " void m() {", - " // BUG: Diagnostic matches: StringOfSizeZeroTemplate", + " // BUG: Diagnostic matches: StringOfSizeZeroRule", " boolean b1 = \"foo\".toCharArray().length == 0;", - " // BUG: Diagnostic matches: StringOfSizeOneTemplate", + " // BUG: Diagnostic matches: StringOfSizeOneRule", " boolean b2 = \"bar\".toCharArray().length == 1;", - " // BUG: Diagnostic matches: StringOfSizeTwoTemplate", + " // BUG: Diagnostic matches: StringOfSizeTwoRule", " boolean b3 = \"baz\".toCharArray().length == 2;", - " // BUG: Diagnostic matches: StringOfSizeThreeTemplate", + " // BUG: Diagnostic matches: StringOfSizeThreeRule", " boolean b4 = \"qux\".toCharArray().length == 3;", " }", "}") 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 1d2871f9b3..e43f0ea4c5 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 @@ -96,7 +96,7 @@ private String getShortCheckName(String fullCheckName) { String prefix = packageName() + '.'; checkState( fullCheckName.startsWith(prefix), - "Refaster template class '%s' is not located in package '%s'", + "Refaster rule class '%s' is not located in package '%s'", fullCheckName, packageName()); 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 index bb2ecc1b9f..84af8a5727 100644 --- 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 @@ -6,7 +6,7 @@ import java.lang.annotation.Target; /** - * Describes the intent of a Refaster template or group of Refaster templates. + * Describes the intent of a Refaster rule or group of Refaster rules. * *

Annotations on nested classes override the description associated with any enclosing class. */ @@ -14,7 +14,7 @@ @Retention(RetentionPolicy.SOURCE) public @interface Description { /** - * A description of the annotated Refaster template(s). + * A description of the annotated Refaster rule(s). * * @return A non-{@code null} string. */ 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 bb493fda04..afa5a6c6e1 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 @@ -7,7 +7,7 @@ import java.lang.annotation.Target; /** - * Describes the severity of a Refaster template or group of Refaster templates. + * Describes the severity of a Refaster rule or group of Refaster rules. * *

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 @@ -17,7 +17,7 @@ @Retention(RetentionPolicy.SOURCE) public @interface Severity { /** - * The expected severity of any match of the annotated Refaster template(s). + * The expected severity of any match of the annotated Refaster rule(s). * * @return An Error Prone severity level. */ 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 7178ba1cb9..53e8e88739 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,6 +1,6 @@ /** - * 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 + * A collection of annotations that can be placed on Refaster rule classes and Refaster rule + * collection classes, thus influencing the way in which associated rule matches are reported in * non-patch mode. */ @com.google.errorprone.annotations.CheckReturnValue diff --git a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java index 7f380a60e4..daf2d52513 100644 --- a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java +++ b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java @@ -175,11 +175,11 @@ private static ImmutableRangeMap indexRuleMatches( ImmutableRangeMap.Builder ruleMatches = ImmutableRangeMap.builder(); for (Description description : matches) { - String templateName = extractRefasterTemplateName(description); + String ruleName = extractRefasterRuleName(description); Set replacements = Iterables.getOnlyElement(description.fixes).getReplacements(endPositions); for (Replacement replacement : replacements) { - ruleMatches.put(replacement.range(), templateName); + ruleMatches.put(replacement.range(), ruleName); } } @@ -226,10 +226,10 @@ private void reportViolations( state.reportMatch(describeMatch(tree, fixWithComment)); } - private static String extractRefasterTemplateName(Description description) { + private static String extractRefasterRuleName(Description description) { String message = description.getRawMessage(); int index = message.indexOf(':'); - checkState(index >= 0, "Failed to extract Refaster template name from string '%s'", message); + checkState(index >= 0, "Failed to extract Refaster rule name from string '%s'", message); return getSubstringAfterFinalDelimiter('.', message.substring(0, index)); } diff --git a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java index ee47a66737..b8fccf5cdf 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java @@ -6,8 +6,8 @@ /** Refaster rule collection to validate reporting of a match occurring in an unexpected place. */ @OnlineDocumentation -final class MatchInWrongMethodTemplates { - private MatchInWrongMethodTemplates() {} +final class MatchInWrongMethodRules { + private MatchInWrongMethodRules() {} static final class StringIsEmpty { @BeforeTemplate From ea33a0b0147812c5f4aedf3980c2b5dc13f974d1 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 11 Oct 2022 16:58:56 +0200 Subject: [PATCH 40/43] Add `@OnlineDocumentation` to the Refaster rule collections in `error-prone-contrib` --- .../errorprone/refasterrules/AssertJBigDecimalRules.java | 2 ++ .../errorprone/refasterrules/AssertJBigIntegerRules.java | 2 ++ .../errorprone/refasterrules/AssertJBooleanRules.java | 2 ++ .../picnic/errorprone/refasterrules/AssertJByteRules.java | 2 ++ .../errorprone/refasterrules/AssertJCharSequenceRules.java | 2 ++ .../errorprone/refasterrules/AssertJComparableRules.java | 2 ++ .../picnic/errorprone/refasterrules/AssertJDoubleRules.java | 2 ++ .../errorprone/refasterrules/AssertJEnumerableRules.java | 2 ++ .../picnic/errorprone/refasterrules/AssertJFloatRules.java | 2 ++ .../errorprone/refasterrules/AssertJIntegerRules.java | 2 ++ .../picnic/errorprone/refasterrules/AssertJLongRules.java | 2 ++ .../picnic/errorprone/refasterrules/AssertJMapRules.java | 2 ++ .../picnic/errorprone/refasterrules/AssertJNumberRules.java | 2 ++ .../picnic/errorprone/refasterrules/AssertJObjectRules.java | 2 ++ .../errorprone/refasterrules/AssertJOptionalRules.java | 2 ++ .../errorprone/refasterrules/AssertJPrimitiveRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/AssertJRules.java | 6 ++++-- .../picnic/errorprone/refasterrules/AssertJShortRules.java | 2 ++ .../picnic/errorprone/refasterrules/AssertJStringRules.java | 2 ++ .../refasterrules/AssertJThrowingCallableRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/AssortedRules.java | 2 ++ .../picnic/errorprone/refasterrules/BigDecimalRules.java | 2 ++ .../picnic/errorprone/refasterrules/CollectionRules.java | 2 ++ .../picnic/errorprone/refasterrules/ComparatorRules.java | 2 ++ .../picnic/errorprone/refasterrules/DoubleStreamRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/EqualityRules.java | 2 ++ .../refasterrules/ImmutableListMultimapRules.java | 2 ++ .../picnic/errorprone/refasterrules/ImmutableListRules.java | 2 ++ .../picnic/errorprone/refasterrules/ImmutableMapRules.java | 2 ++ .../errorprone/refasterrules/ImmutableMultisetRules.java | 2 ++ .../errorprone/refasterrules/ImmutableSetMultimapRules.java | 2 ++ .../picnic/errorprone/refasterrules/ImmutableSetRules.java | 2 ++ .../errorprone/refasterrules/ImmutableSortedMapRules.java | 2 ++ .../refasterrules/ImmutableSortedMultisetRules.java | 2 ++ .../errorprone/refasterrules/ImmutableSortedSetRules.java | 2 ++ .../picnic/errorprone/refasterrules/IntStreamRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/JUnitRules.java | 2 ++ .../picnic/errorprone/refasterrules/LongStreamRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/MapEntryRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/MockitoRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/MultimapRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/NullRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/OptionalRules.java | 2 ++ .../picnic/errorprone/refasterrules/PrimitiveRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/ReactorRules.java | 2 ++ .../errorprone/refasterrules/RxJava2AdapterRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/StreamRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/StringRules.java | 2 ++ .../errorprone/refasterrules/TestNGToAssertJRules.java | 2 ++ .../tech/picnic/errorprone/refasterrules/TimeRules.java | 2 ++ .../picnic/errorprone/refasterrules/WebClientRules.java | 2 ++ 51 files changed, 104 insertions(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBigDecimalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBigDecimalRules.java index 272b3ab194..576e8bde2b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBigDecimalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBigDecimalRules.java @@ -9,6 +9,7 @@ import java.math.BigDecimal; import org.assertj.core.api.AbstractBigDecimalAssert; import org.assertj.core.api.BigDecimalAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster rules related to AssertJ assertions over {@link BigDecimal}s. @@ -20,6 +21,7 @@ * instances, but also their scale. As a result various seemingly straightforward transformations * would actually subtly change the assertion's semantics. */ +@OnlineDocumentation final class AssertJBigDecimalRules { private AssertJBigDecimalRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBigIntegerRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBigIntegerRules.java index 7b3ad736ca..3f9ca69b4c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBigIntegerRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBigIntegerRules.java @@ -8,9 +8,11 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import java.math.BigInteger; import org.assertj.core.api.AbstractBigIntegerAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; // XXX: If we add a rule that drops unnecessary `L` suffixes from literal longs, then the `0L`/`1L` // cases below can go. +@OnlineDocumentation final class AssertJBigIntegerRules { private AssertJBigIntegerRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBooleanRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBooleanRules.java index 9c28353c63..cc582f1a1a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBooleanRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJBooleanRules.java @@ -8,7 +8,9 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import com.google.errorprone.refaster.annotation.UseImportPolicy; import org.assertj.core.api.AbstractBooleanAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJBooleanRules { private AssertJBooleanRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJByteRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJByteRules.java index 1c1134c387..7f4d2d5150 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJByteRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJByteRules.java @@ -7,7 +7,9 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; import org.assertj.core.api.AbstractByteAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJByteRules { private AssertJByteRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJCharSequenceRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJCharSequenceRules.java index 545b7fec08..573f9efa02 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJCharSequenceRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJCharSequenceRules.java @@ -8,7 +8,9 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import com.google.errorprone.refaster.annotation.UseImportPolicy; import org.assertj.core.api.AbstractAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJCharSequenceRules { private AssertJCharSequenceRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJComparableRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJComparableRules.java index ea92d58fb6..cd76d5d8ff 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJComparableRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJComparableRules.java @@ -8,7 +8,9 @@ import com.google.errorprone.refaster.annotation.UseImportPolicy; import org.assertj.core.api.AbstractComparableAssert; import org.assertj.core.api.AbstractIntegerAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJComparableRules { private AssertJComparableRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJDoubleRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJDoubleRules.java index 624018ffd9..8d640235b9 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJDoubleRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJDoubleRules.java @@ -8,7 +8,9 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import org.assertj.core.api.AbstractDoubleAssert; import org.assertj.core.data.Offset; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJDoubleRules { private AssertJDoubleRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJEnumerableRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJEnumerableRules.java index a891bfbc06..bc19fd72dc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJEnumerableRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJEnumerableRules.java @@ -6,7 +6,9 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import java.util.Collection; import org.assertj.core.api.EnumerableAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJEnumerableRules { private AssertJEnumerableRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJFloatRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJFloatRules.java index 8952ca4f2b..0d7d92e436 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJFloatRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJFloatRules.java @@ -8,7 +8,9 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import org.assertj.core.api.AbstractFloatAssert; import org.assertj.core.data.Offset; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJFloatRules { private AssertJFloatRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJIntegerRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJIntegerRules.java index d134bfe6c8..6495f8bb3e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJIntegerRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJIntegerRules.java @@ -7,7 +7,9 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; import org.assertj.core.api.AbstractIntegerAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJIntegerRules { private AssertJIntegerRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJLongRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJLongRules.java index 3ac9275fc2..35c16db3da 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJLongRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJLongRules.java @@ -7,7 +7,9 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; import org.assertj.core.api.AbstractLongAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJLongRules { private AssertJLongRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJMapRules.java index dfbd303ea3..59f99e9504 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJMapRules.java @@ -5,7 +5,9 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import java.util.Map; import org.assertj.core.api.AbstractMapAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJMapRules { private AssertJMapRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJNumberRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJNumberRules.java index fac67d6068..43453d3031 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJNumberRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJNumberRules.java @@ -19,8 +19,10 @@ import org.assertj.core.api.AbstractLongAssert; import org.assertj.core.api.AbstractShortAssert; import org.assertj.core.api.NumberAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.matchers.IsCharacter; +@OnlineDocumentation final class AssertJNumberRules { private AssertJNumberRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJObjectRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJObjectRules.java index b5122b7296..41b1a51c61 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJObjectRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJObjectRules.java @@ -10,7 +10,9 @@ import org.assertj.core.api.AbstractBooleanAssert; import org.assertj.core.api.AbstractStringAssert; import org.assertj.core.api.ObjectAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJObjectRules { private AssertJObjectRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJOptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJOptionalRules.java index f14bef43ad..2cab238072 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJOptionalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJOptionalRules.java @@ -14,7 +14,9 @@ import org.assertj.core.api.AbstractOptionalAssert; import org.assertj.core.api.ObjectAssert; import org.assertj.core.api.OptionalAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJOptionalRules { private AssertJOptionalRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJPrimitiveRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJPrimitiveRules.java index 7635e37037..e3285067ab 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJPrimitiveRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJPrimitiveRules.java @@ -9,7 +9,9 @@ import com.google.errorprone.refaster.annotation.UseImportPolicy; import org.assertj.core.api.AbstractBooleanAssert; import org.assertj.core.api.AbstractDoubleAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJPrimitiveRules { private AssertJPrimitiveRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java index 4da5ba9c92..8ee7396d9a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java @@ -53,6 +53,7 @@ import org.assertj.core.api.OptionalDoubleAssert; import org.assertj.core.api.OptionalIntAssert; import org.assertj.core.api.OptionalLongAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.matchers.IsArray; /** Refaster rules related to AssertJ expressions and statements. */ @@ -118,8 +119,9 @@ // XXX: `assertThat(ImmutableList.sortedCopyOf(cmp, values)).somethingExactOrder` -> just compare // "in any order". // XXX: Turns out a lot of this is also covered by https://github.com/palantir/assertj-automation. -// See how we can combine these things. Do note that (at present) their Refaster rules don't show up -// as Error Prone checks. So we'd have to build an integration for that. +// See how we can combine these things. Do note that (at present) their Refaster rules don't +// show up as Error Prone checks. So we'd have to build an integration for that. +@OnlineDocumentation final class AssertJRules { private AssertJRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJShortRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJShortRules.java index a09e34ca2b..4818920357 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJShortRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJShortRules.java @@ -7,7 +7,9 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; import org.assertj.core.api.AbstractShortAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJShortRules { private AssertJShortRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJStringRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJStringRules.java index d149d3b1ff..0f3fee47ba 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJStringRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJStringRules.java @@ -8,7 +8,9 @@ import com.google.errorprone.refaster.annotation.UseImportPolicy; import org.assertj.core.api.AbstractAssert; import org.assertj.core.api.AbstractStringAssert; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +@OnlineDocumentation final class AssertJStringRules { private AssertJStringRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJThrowingCallableRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJThrowingCallableRules.java index f221f66425..3bfb7502ee 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJThrowingCallableRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJThrowingCallableRules.java @@ -16,6 +16,7 @@ import org.assertj.core.api.AbstractObjectAssert; import org.assertj.core.api.AbstractThrowableAssert; import org.assertj.core.api.ThrowableAssert.ThrowingCallable; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster rules related to AssertJ assertions over expressions that may throw a {@link Throwable} @@ -26,6 +27,7 @@ * types. Note that only the most common assertion expressions are rewritten here; covering all * cases would require the implementation of an Error Prone check instead. */ +@OnlineDocumentation final class AssertJThrowingCallableRules { private AssertJThrowingCallableRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java index 872f9eced8..1d5523736f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java @@ -28,11 +28,13 @@ import java.util.Set; import java.util.stream.Stream; import javax.annotation.Nullable; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Assorted Refaster rules that do not (yet) belong in one of the other classes with more topical * Refaster rules. */ +@OnlineDocumentation final class AssortedRules { private AssortedRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BigDecimalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BigDecimalRules.java index 7f27917660..b23406170b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BigDecimalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BigDecimalRules.java @@ -4,8 +4,10 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; import java.math.BigDecimal; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link BigDecimal}s. */ +@OnlineDocumentation final class BigDecimalRules { private BigDecimalRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java index e02c12c2a2..19df9107db 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java @@ -19,10 +19,12 @@ import java.util.SortedSet; import java.util.function.IntFunction; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with (arbitrary) collections. */ // XXX: There are other Guava `Iterables` methods that should not be called if the input is known to // be a `Collection`. Add those here. +@OnlineDocumentation final class CollectionRules { private CollectionRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java index 3a1e94d32b..83ea4c7378 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java @@ -24,8 +24,10 @@ import java.util.function.ToDoubleFunction; import java.util.function.ToIntFunction; import java.util.function.ToLongFunction; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link Comparator}s. */ +@OnlineDocumentation final class ComparatorRules { private ComparatorRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/DoubleStreamRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/DoubleStreamRules.java index 7e8e007b19..ca64f929d8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/DoubleStreamRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/DoubleStreamRules.java @@ -12,8 +12,10 @@ import java.util.function.DoubleUnaryOperator; import java.util.stream.DoubleStream; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link DoubleStream}s. */ +@OnlineDocumentation final class DoubleStreamRules { private DoubleStreamRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java index 6aaccd1d4d..244a9eceec 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java @@ -6,8 +6,10 @@ import com.google.errorprone.refaster.annotation.BeforeTemplate; import java.util.Objects; import java.util.function.Predicate; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with (in)equalities. */ +@OnlineDocumentation final class EqualityRules { private EqualityRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java index 87fc03db95..b68a7a1601 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java @@ -24,8 +24,10 @@ import java.util.Map; import java.util.function.Function; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableListMultimap}s. */ +@OnlineDocumentation final class ImmutableListMultimapRules { private ImmutableListMultimapRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java index de9aca4deb..1848c268b9 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java @@ -20,8 +20,10 @@ import java.util.Iterator; import java.util.List; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableList}s. */ +@OnlineDocumentation final class ImmutableListRules { private ImmutableListRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index 2d1906983f..aa68506b83 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -21,8 +21,10 @@ import java.util.Set; import java.util.function.Function; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableMap}s. */ +@OnlineDocumentation final class ImmutableMapRules { private ImmutableMapRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java index 07802c7741..fee76fd89d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java @@ -13,8 +13,10 @@ import java.util.Collection; import java.util.Iterator; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableMultiset}s. */ +@OnlineDocumentation final class ImmutableMultisetRules { private ImmutableMultisetRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java index 8272c05b57..1c704943fc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java @@ -21,8 +21,10 @@ import java.util.Map; import java.util.function.Function; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableSetMultimap}s. */ +@OnlineDocumentation final class ImmutableSetMultimapRules { private ImmutableSetMultimapRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java index b7159b886a..f4d5ba78b7 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java @@ -17,8 +17,10 @@ import java.util.Iterator; import java.util.Set; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableSet}s. */ +@OnlineDocumentation final class ImmutableSetRules { private ImmutableSetRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java index ff03163cad..36975c7025 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java @@ -13,8 +13,10 @@ import java.util.Comparator; import java.util.Map; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableSortedMap}s. */ +@OnlineDocumentation final class ImmutableSortedMapRules { private ImmutableSortedMapRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMultisetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMultisetRules.java index 0e8a252223..c66f86e7d1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMultisetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMultisetRules.java @@ -15,8 +15,10 @@ import java.util.Comparator; import java.util.Iterator; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableSortedMultiset}s. */ +@OnlineDocumentation final class ImmutableSortedMultisetRules { private ImmutableSortedMultisetRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedSetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedSetRules.java index 627fdf7c0f..7117818569 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedSetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedSetRules.java @@ -15,8 +15,10 @@ import java.util.Comparator; import java.util.Iterator; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link ImmutableSortedSet}s. */ +@OnlineDocumentation final class ImmutableSortedSetRules { private ImmutableSortedSetRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/IntStreamRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/IntStreamRules.java index 382f5d332b..2c43fc3cc1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/IntStreamRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/IntStreamRules.java @@ -12,8 +12,10 @@ import java.util.function.IntUnaryOperator; import java.util.stream.IntStream; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link IntStream}s. */ +@OnlineDocumentation final class IntStreamRules { private IntStreamRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitRules.java index b2c5942856..218213ab9a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitRules.java @@ -8,8 +8,10 @@ import com.google.errorprone.refaster.annotation.Repeated; import com.google.errorprone.refaster.annotation.UseImportPolicy; import org.junit.jupiter.params.provider.Arguments; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to JUnit expressions and statements. */ +@OnlineDocumentation final class JUnitRules { private JUnitRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/LongStreamRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/LongStreamRules.java index 45b4c0da30..363157c55f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/LongStreamRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/LongStreamRules.java @@ -12,8 +12,10 @@ import java.util.function.LongUnaryOperator; import java.util.stream.LongStream; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link LongStream}s. */ +@OnlineDocumentation final class LongStreamRules { private LongStreamRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MapEntryRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MapEntryRules.java index 27d3af80c3..22c2cfe67f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MapEntryRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MapEntryRules.java @@ -14,8 +14,10 @@ import java.util.AbstractMap; import java.util.Comparator; import java.util.Map; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link Map.Entry} instances. */ +@OnlineDocumentation final class MapEntryRules { private MapEntryRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MockitoRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MockitoRules.java index b17af9b514..dbb7b16770 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MockitoRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MockitoRules.java @@ -10,8 +10,10 @@ import com.google.errorprone.refaster.annotation.UseImportPolicy; import org.mockito.Mockito; import org.mockito.verification.VerificationMode; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to Mockito expressions and statements. */ +@OnlineDocumentation final class MockitoRules { private MockitoRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MultimapRules.java index ff64ad784c..576508d745 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MultimapRules.java @@ -8,8 +8,10 @@ import java.util.Collection; import java.util.Set; import javax.annotation.Nullable; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link Multimap}s. */ +@OnlineDocumentation final class MultimapRules { private MultimapRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/NullRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/NullRules.java index 770843886c..0182547ef9 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/NullRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/NullRules.java @@ -10,8 +10,10 @@ import java.util.Objects; import java.util.function.Predicate; import javax.annotation.Nullable; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with (possibly) null values. */ +@OnlineDocumentation final class NullRules { private NullRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java index 7c990060a5..6a1ec4d951 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java @@ -17,8 +17,10 @@ import java.util.function.Supplier; import java.util.stream.Stream; import javax.annotation.Nullable; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link Optional}s. */ +@OnlineDocumentation final class OptionalRules { private OptionalRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PrimitiveRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PrimitiveRules.java index 041abec002..b0c6c138d2 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PrimitiveRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PrimitiveRules.java @@ -3,8 +3,10 @@ import com.google.common.primitives.Ints; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with primitives. */ +@OnlineDocumentation final class PrimitiveRules { private PrimitiveRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index e03b521533..500bd40e34 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -25,9 +25,11 @@ import reactor.core.publisher.Mono; import reactor.test.StepVerifier; import reactor.test.publisher.PublisherProbe; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.matchers.ThrowsCheckedException; /** Refaster rules related to Reactor expressions and statements. */ +@OnlineDocumentation final class ReactorRules { private ReactorRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RxJava2AdapterRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RxJava2AdapterRules.java index b03245c7b1..ad75d06fc5 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RxJava2AdapterRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RxJava2AdapterRules.java @@ -13,8 +13,10 @@ import reactor.adapter.rxjava.RxJava2Adapter; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link RxJava2Adapter}. */ +@OnlineDocumentation final class RxJava2AdapterRules { private RxJava2AdapterRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java index 336393e151..17abcf97c3 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java @@ -22,8 +22,10 @@ import java.util.stream.Collector; import java.util.stream.Collectors; import java.util.stream.Stream; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link Stream}s. */ +@OnlineDocumentation final class StreamRules { private StreamRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java index 3e01463f68..a18b289565 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java @@ -17,9 +17,11 @@ import java.util.Optional; import java.util.function.Function; import javax.annotation.Nullable; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with {@link String}s. */ // XXX: Should we prefer `s -> !s.isEmpty()` or `not(String::isEmpty)`? +@OnlineDocumentation final class StringRules { private StringRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java index e49014fb0f..8c9521ca3a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java @@ -28,6 +28,7 @@ import org.assertj.core.api.ThrowableAssert.ThrowingCallable; import org.testng.Assert; import org.testng.Assert.ThrowingRunnable; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster rules that replace TestNG assertions with equivalent AssertJ assertions. @@ -72,6 +73,7 @@ // XXX: As-is these rules do not result in a complete migration: // - Expressions containing comments are skipped due to a limitation of Refaster. // - Assertions inside lambda expressions are also skipped. Unclear why. +@OnlineDocumentation final class TestNGToAssertJRules { private TestNGToAssertJRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java index 6b33428dc6..5ff77ff8fa 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java @@ -21,8 +21,10 @@ import java.time.chrono.ChronoZonedDateTime; import java.time.temporal.ChronoUnit; import java.time.temporal.TemporalUnit; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rules related to expressions dealing with time. */ +@OnlineDocumentation final class TimeRules { private TimeRules() {} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/WebClientRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/WebClientRules.java index 8452f00d17..62e43e7050 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/WebClientRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/WebClientRules.java @@ -20,11 +20,13 @@ import org.springframework.web.reactive.function.client.WebClient.RequestBodyUriSpec; import org.springframework.web.reactive.function.client.WebClient.RequestHeadersSpec; import org.springframework.web.reactive.function.client.WebClient.RequestHeadersUriSpec; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster rules related to expressions dealing with {@link * org.springframework.web.reactive.function.client.WebClient} and related types. */ +@OnlineDocumentation final class WebClientRules { private WebClientRules() {} From 66398ab8568fd9a5ccd61d5d0566ccc9fb96e519 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Tue, 11 Oct 2022 18:39:37 +0200 Subject: [PATCH 41/43] Tweaks --- .../picnic/errorprone/refaster/runner/CodeTransformersTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 66b3a082dd..5a2a71980d 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,7 +6,7 @@ final class CodeTransformersTest { /** - * Verifies that {@link CodeTransformers#getAllCodeTransformers()} finds the code transformer + * Verifies that {@link CodeTransformers#getAllCodeTransformers()} finds the code transformers * compiled from {@link FooRules} on the classpath. */ @Test From b07096a3429f7d963dd3ef21204602f1954cd86b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Tue, 11 Oct 2022 19:48:30 +0200 Subject: [PATCH 42/43] Drop it --- .../errorprone/refaster/test/MatchInWrongMethodRules.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java index b8fccf5cdf..ff25d1b74f 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java @@ -2,10 +2,8 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; -import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster rule collection to validate reporting of a match occurring in an unexpected place. */ -@OnlineDocumentation final class MatchInWrongMethodRules { private MatchInWrongMethodRules() {} From 9f1d28606cc34ead898f5d10d037128b0a0d62e7 Mon Sep 17 00:00:00 2001 From: Pieter Dirk Soels Date: Wed, 12 Oct 2022 09:13:26 +0200 Subject: [PATCH 43/43] Drop `@OnlineDocumentation` from `TestNGToAssertJRules` --- .../picnic/errorprone/refasterrules/TestNGToAssertJRules.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java index 8c9521ca3a..e49014fb0f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java @@ -28,7 +28,6 @@ import org.assertj.core.api.ThrowableAssert.ThrowingCallable; import org.testng.Assert; import org.testng.Assert.ThrowingRunnable; -import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster rules that replace TestNG assertions with equivalent AssertJ assertions. @@ -73,7 +72,6 @@ // XXX: As-is these rules do not result in a complete migration: // - Expressions containing comments are skipped due to a limitation of Refaster. // - Assertions inside lambda expressions are also skipped. Unclear why. -@OnlineDocumentation final class TestNGToAssertJRules { private TestNGToAssertJRules() {}