From b5d73cb4f62a5251a0b8d3c0d3c8ce3e42105fd9 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 23 Sep 2022 21:01:48 +0200 Subject: [PATCH] Better annotation support, simplify setup --- .../refastertemplates/OptionalTemplates.java | 5 +- .../AnnotatedCompositeCodeTransformer.java | 96 ++++++++++++++++++ .../RefasterRuleCompilerTaskListener.java | 1 - refaster-runner/pom.xml | 12 --- .../AnnotatedCompositeCodeTransformer.java | 98 ------------------- .../refaster/annotation/Description.java | 22 +++++ .../annotation/OnlineDocumentation.java | 33 +++++++ .../refaster/annotation/Severity.java | 24 +++++ .../annotation/TemplateCollection.java | 24 ----- refaster-test-support/pom.xml | 1 + .../test/MatchInWrongMethodTemplates.java | 10 +- 11 files changed, 185 insertions(+), 141 deletions(-) create mode 100644 refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java delete mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/OnlineDocumentation.java create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java delete mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java index 1e216ed121a..9d11699d192 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java @@ -17,8 +17,11 @@ import java.util.function.Supplier; import java.util.stream.Stream; import javax.annotation.Nullable; +import tech.picnic.errorprone.refaster.annotation.Description; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** Refaster templates related to expressions dealing with {@link Optional}s. */ +@OnlineDocumentation final class OptionalTemplates { private OptionalTemplates() {} @@ -63,7 +66,7 @@ boolean after(Optional optional) { } } - /** Prefer {@link Optional#orElseThrow()} over the less explicit {@link Optional#get()}. */ + @Description("Prefer `Optional#orElseThrow()` over the less explicit `Optional#get()`") static final class OptionalOrElseThrow { @BeforeTemplate @SuppressWarnings("NullAway") diff --git a/refaster-compiler/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 00000000000..9cae0e1cfef --- /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 12128330825..9f9aaa0dff1 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} which stores diff --git a/refaster-runner/pom.xml b/refaster-runner/pom.xml index 17f1a61d395..edd255da65a 100644 --- a/refaster-runner/pom.xml +++ b/refaster-runner/pom.xml @@ -32,18 +32,6 @@ ${project.groupId} refaster-compiler - - provided - - - - ${project.groupId} - refaster-support - runtime com.google.auto.service 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 5598481927d..00000000000 --- 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 00000000000..bb2ecc1b9f6 --- /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 00000000000..f213bcbb731 --- /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 00000000000..9bf43981810 --- /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 bf0e70262be..00000000000 --- 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 81732bf9f22..7e2359f72b5 100644 --- a/refaster-test-support/pom.xml +++ b/refaster-test-support/pom.xml @@ -40,6 +40,7 @@ ${project.groupId} refaster-runner + ${project.groupId} refaster-support diff --git a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java index af4537eda74..543b24308a7 100644 --- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java +++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java @@ -1,20 +1,20 @@ package tech.picnic.errorprone.refaster.test; -import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; - import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; -import tech.picnic.errorprone.refaster.annotation.TemplateCollection; +import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster template collection to validate reporting of a match occurring in an unexpected place. */ -@TemplateCollection(linkPattern = "XXX") +@OnlineDocumentation final class MatchInWrongMethodTemplates { private MatchInWrongMethodTemplates() {} // XXX: Demo: nesting overrides. - @TemplateCollection(linkPattern = "YYY", severity = ERROR, description = "Foo") + // @Website("YYY") + // @Severity(ERROR) + // @Description("Fooo!") static final class StringIsEmpty { @BeforeTemplate boolean before(String string) {