Another round

PicnicSupermarket · Sep 23, 2022 · b69750f · b69750f
1 parent ffee1c4
commit b69750f
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 70 deletions.
diff --git a/...rc/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/...rc/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<Annotation> a
         ImmutableList<CodeTransformer> 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<Annotation> a
   // XXX: Move down?
   private static ImmutableClassToInstanceMap<Annotation> merge(
       ImmutableClassToInstanceMap<Annotation> left, ImmutableClassToInstanceMap<Annotation> right) {
-    // XXX: Handle duplicates!
-    return ImmutableClassToInstanceMap.<Annotation>builder().putAll(left).putAll(right).build();
+    return ImmutableClassToInstanceMap.<Annotation>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
@@ -37,10 +37,6 @@
             `annotationProcessorPaths` configuration below. -->
             <scope>provided</scope>
         </dependency>
-        <dependency>
-            <groupId>${project.groupId}</groupId>
-            <artifactId>refaster-support</artifactId>
-        </dependency>
         <dependency>
             <groupId>com.google.auto.service</groupId>
             <artifactId>auto-service-annotations</artifactId>

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
@@ -1,25 +1,18 @@
 package tech.picnic.errorprone.refaster.runner;
 
 import com.google.common.base.Suppliers;
-import com.google.common.collect.ImmutableClassToInstanceMap;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.reflect.ClassPath;
 import com.google.common.reflect.ClassPath.ResourceInfo;
 import com.google.errorprone.CodeTransformer;
-import com.google.errorprone.DescriptionListener;
-import com.google.errorprone.matchers.Description;
-import com.sun.source.util.TreePath;
-import com.sun.tools.javac.util.Context;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.ObjectInputStream;
 import java.io.UncheckedIOException;
-import java.lang.annotation.Annotation;
 import java.util.NoSuchElementException;
 import java.util.Optional;
 import java.util.function.Supplier;
-import tech.picnic.errorprone.refaster.annotation.TemplateCollection;
 
 /**
  * Scans the classpath for {@value #REFASTER_TEMPLATE_SUFFIX} files and loads them as {@link
@@ -58,12 +51,7 @@ private static ImmutableListMultimap<String, CodeTransformer> loadAllCodeTransfo
           .ifPresent(
               templateName ->
                   loadCodeTransformer(resource)
-                      .ifPresent(
-                          transformer ->
-                              transformers.put(
-                                  templateName,
-                                  new CodeTransformerDescriptionAdapter(
-                                      templateName, transformer))));
+                      .ifPresent(transformer -> transformers.put(templateName, transformer)));
     }
 
     return transformers.build();
@@ -107,50 +95,4 @@ private static Optional<CodeTransformer> 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<Annotation> 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/...port/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java b/...port/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<CodeTransformer> transformers;
   private final ImmutableClassToInstanceMap<Annotation> annotations;
 
   public AnnotatedCompositeCodeTransformer(
+      String name,
       ImmutableList<CodeTransformer> transformers,
       ImmutableClassToInstanceMap<Annotation> annotations) {
+    this.name = name;
     this.transformers = transformers;
     this.annotations = annotations;
   }
@@ -31,7 +46,53 @@ public ImmutableClassToInstanceMap<Annotation> 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 <S, T> Optional<T> extract(Function<S, Optional<T>> extractor, S first, S second) {
+    return extractor.apply(first).or(() -> extractor.apply(second));
+  }
+
+  private static Optional<String> getLinkPattern(CodeTransformer codeTransformer) {
+    return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class))
+        .map(TemplateCollection::linkPattern);
+  }
+
+  private static Optional<SeverityLevel> getSeverity(CodeTransformer codeTransformer) {
+    return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class))
+        .map(TemplateCollection::severity);
+  }
+
+  private static Optional<String> getDescription(CodeTransformer codeTransformer) {
+    return Optional.ofNullable(codeTransformer.annotations().getInstance(TemplateCollection.class))
+        .map(TemplateCollection::description);
+  }
 }
diff --git a/...-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java b/...-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
@@ -43,6 +43,7 @@
         <dependency>
             <groupId>${project.groupId}</groupId>
             <artifactId>refaster-support</artifactId>
+            <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>com.google.auto.service</groupId>

diff --git a/...pport/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java b/...pport/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplates.java
@@ -1,5 +1,7 @@
 package tech.picnic.errorprone.refaster.test;
 
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+
 import com.google.errorprone.refaster.annotation.AfterTemplate;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
 import tech.picnic.errorprone.refaster.annotation.TemplateCollection;
@@ -11,8 +13,8 @@
 final class MatchInWrongMethodTemplates {
   private MatchInWrongMethodTemplates() {}
 
-  // XXX: Test merging/overriding.
-  //  @TemplateCollection(linkPattern = "XXX")
+  // XXX: Demo: nesting overrides.
+  @TemplateCollection(linkPattern = "YYY", severity = ERROR, description = "Foo")
   static final class StringIsEmpty {
     @BeforeTemplate
     boolean before(String string) {