Generate classpath entries for FQN references (#19)

* Generate classpath entries for FQN references

* Detect classpath and add to generated templates

* Simplify test cases; we're testing imports not a full replacement

* Reduce visitor duplication by reusing imports

* Fix detection

* Adopt a TreeSet to sort classpath entries

* Sort after filtering

src/main/java/org/openrewrite/java/template/TemplateProcessor.java

@@ -27,6 +27,7 @@
 import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;
 import com.sun.tools.javac.tree.TreeScanner;
 import com.sun.tools.javac.util.Context;
+import org.openrewrite.java.template.internal.ClasspathJarNameDetector;
 import org.openrewrite.java.template.internal.ImportDetector;
 import org.openrewrite.java.template.internal.JavacResolution;
 import org.openrewrite.java.template.internal.Permit;
@@ -47,11 +48,8 @@
 import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.Method;
 import java.lang.reflect.Proxy;
-import java.net.URI;
 import java.util.*;
 import java.util.concurrent.atomic.AtomicReference;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 import static java.util.Collections.*;
 
@@ -247,8 +245,6 @@ public void visitIdent(JCTree.JCIdent ident) {
                                 out.write("package " + classDecl.sym.packge().toString() + ";\n");
                                 out.write("import org.openrewrite.java.*;\n");
 
-                                boolean hasParserClasspath = false;
-                                StringJoiner parserClasspath = new StringJoiner(", ");
 
                                 for (JCTree.JCVariableDecl parameter : parameters) {
                                     if (parameter.type.tsym instanceof Symbol.ClassSymbol) {
@@ -266,31 +262,10 @@ public void visitIdent(JCTree.JCIdent ident) {
                                 out.write("                .builder(\"" + templateSource + "\")");
 
                                 List<Symbol> imports = ImportDetector.imports(resolved.get(template));
-                                for (Symbol anImport : imports) {
-                                    Symbol.ClassSymbol enclClass = anImport instanceof Symbol.ClassSymbol ? (Symbol.ClassSymbol) anImport : anImport.enclClass();
-                                    while (enclClass.enclClass() != null && enclClass.enclClass() != enclClass) {
-                                        enclClass = enclClass.enclClass();
-                                    }
-                                    JavaFileObject classfile = enclClass.classfile;
-                                    if (classfile == null) {
-                                        // TODO this shouldn't really happen, but it does. Need to investigate why.
-                                        continue;
-                                    }
-                                    URI uri = classfile.toUri();
-                                    if (uri.toString().contains(".jar!/")) {
-                                        Matcher matcher = Pattern.compile("([^/]*)?\\.jar!/").matcher(uri.toString());
-                                        if (matcher.find()) {
-                                            hasParserClasspath = true;
-                                            String jarName = matcher.group(1);
-                                            jarName = jarName.replaceAll("-\\d.*$", "");
-                                            parserClasspath.add("\"" + jarName + "\"");
-                                        }
-                                    }
-                                }
-
-                                if (hasParserClasspath) {
+                                String classpath = ClasspathJarNameDetector.classpathFor(resolved.get(template), imports);
+                                if (!classpath.isEmpty()) {
                                     out.write("\n                .javaParser(JavaParser.fromJavaVersion().classpath(" +
-                                            parserClasspath + "))");
+                                              classpath + "))");
                                 }
 
                                 for (Symbol anImport : imports) {

src/main/java/org/openrewrite/java/template/internal/ClasspathJarNameDetector.java

@@ -0,0 +1,84 @@
+/*
+ * Copyright 2022 the original author or authors.
+ * <p>
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * https://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.openrewrite.java.template.internal;
+
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.tree.JCTree;
+import com.sun.tools.javac.tree.JCTree.JCFieldAccess;
+import com.sun.tools.javac.tree.TreeScanner;
+
+import javax.tools.JavaFileObject;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+public class ClasspathJarNameDetector {
+
+    /**
+     * Locate types that are directly referred to by name in the
+     * given tree and therefore need an import in the template.
+     *
+     * @return The list of imports to add.
+     */
+    public static String classpathFor(JCTree input, List<Symbol> imports) {
+        Set<String> jarNames = new LinkedHashSet<>();
+
+        for (Symbol anImport : imports) {
+            jarNames.add(jarNameFor(anImport));
+        }
+
+        // Detect fully qualified classes
+        new TreeScanner() {
+            @Override
+            public void scan(JCTree tree) {
+                if (tree instanceof JCFieldAccess &&
+                    ((JCFieldAccess) tree).sym instanceof Symbol.ClassSymbol &&
+                    Character.isUpperCase(((JCFieldAccess) tree).getIdentifier().toString().charAt(0))) {
+                    jarNames.add(jarNameFor(((JCFieldAccess) tree).sym));
+                }
+                super.scan(tree);
+            }
+        }.scan(input);
+
+        return jarNames.stream()
+                .filter(Objects::nonNull)
+                .map(jarName -> '"' + jarName + '"')
+                .sorted()
+                .collect(Collectors.joining(", "));
+    }
+
+
+    private static String jarNameFor(Symbol anImport) {
+        Symbol.ClassSymbol enclClass = anImport instanceof Symbol.ClassSymbol ? (Symbol.ClassSymbol) anImport : anImport.enclClass();
+        while (enclClass.enclClass() != null && enclClass.enclClass() != enclClass) {
+            enclClass = enclClass.enclClass();
+        }
+        JavaFileObject classfile = enclClass.classfile;
+        if (classfile != null) {
+            String uriStr = classfile.toUri().toString();
+            Matcher matcher = Pattern.compile("([^/]*)?\\.jar!/").matcher(uriStr);
+            if (matcher.find()) {
+                String jarName = matcher.group(1);
+                return jarName.replaceAll("-\\d.*$", "");
+            }
+        }
+        return null;
+    }
+}

src/test/java/org/openrewrite/java/template/TemplateProcessorTest.java

@@ -20,7 +20,8 @@
 import com.google.testing.compile.Compilation;
 import com.google.testing.compile.JavaFileObjects;
 import org.jetbrains.annotations.NotNull;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import java.io.File;
 import java.net.URL;
@@ -32,8 +33,12 @@
 
 class TemplateProcessorTest {
 
-    @Test    
-    void generateRecipeTemplates() {
+    @ParameterizedTest
+    @ValueSource(strings = {
+      "Unqualified",
+      "FullyQualified",
+    })
+    void generateRecipeTemplates(String qualifier) {
         // As per https://github.com/google/compile-testing/blob/v0.21.0/src/main/java/com/google/testing/compile/package-info.java#L53-L55
         Compilation compilation = javac()
           .withProcessors(new RefasterTemplateProcessor(), new TemplateProcessor())
@@ -42,11 +47,11 @@ void generateRecipeTemplates() {
         assertThat(compilation).succeeded();
         compilation.generatedSourceFiles().forEach(System.out::println);
         assertThat(compilation)
-          .generatedSourceFile("foo/ShouldAddClasspathRecipe$1_before")
-          .hasSourceEquivalentTo(JavaFileObjects.forResource("recipes/ShouldAddClasspathRecipe$1_before.java"));
+          .generatedSourceFile("foo/ShouldAddClasspathRecipes$" + qualifier + "Recipe$1_before")
+          .hasSourceEquivalentTo(JavaFileObjects.forResource("recipes/ShouldAddClasspathRecipe$" + qualifier + "Recipe$1_before.java"));
         assertThat(compilation)
-          .generatedSourceFile("foo/ShouldAddClasspathRecipe$1_after")
-          .hasSourceEquivalentTo(JavaFileObjects.forResource("recipes/ShouldAddClasspathRecipe$1_after.java"));
+          .generatedSourceFile("foo/ShouldAddClasspathRecipes$" + qualifier + "Recipe$1_after")
+          .hasSourceEquivalentTo(JavaFileObjects.forResource("recipes/ShouldAddClasspathRecipe$" + qualifier + "Recipe$1_after.java"));
     }
 
     @NotNull

src/test/resources/recipes/ShouldAddClasspath.java

@@ -6,14 +6,28 @@
 
 public class ShouldAddClasspath {
 
-    @BeforeTemplate
-    void before(String message) {
-        System.out.println(message);
+    class Unqualified {
+        @BeforeTemplate
+        void before(String message) {
+            System.out.println(message);
+        }
+
+        @AfterTemplate
+        void after(String message) {
+            LoggerFactory.getLogger(message);
+        }
     }
 
-    @AfterTemplate
-    void after(String message) {
-        LoggerFactory.getLogger("ROOT").info(message);
+    class FullyQualified {
+        @BeforeTemplate
+        void before(String message) {
+            System.out.println(message);
+        }
+
+        @AfterTemplate
+        void after(String message) {
+            org.slf4j.LoggerFactory.getLogger(message);
+        }
     }
 
-}
+}

src/test/resources/recipes/ShouldAddClasspathRecipe$FullyQualifiedRecipe$1_after.java

@@ -0,0 +1,10 @@
+package foo;
+import org.openrewrite.java.*;
+
+public class ShouldAddClasspathRecipes$FullyQualifiedRecipe$1_after {
+    public static JavaTemplate.Builder getTemplate(JavaVisitor<?> visitor) {
+        return JavaTemplate
+                .builder("org.slf4j.LoggerFactory.getLogger(#{any(java.lang.String)})")
+                .javaParser(JavaParser.fromJavaVersion().classpath("slf4j-api"));
+    }
+}

src/test/resources/recipes/ShouldAddClasspathRecipe$FullyQualifiedRecipe$1_before.java

@@ -0,0 +1,9 @@
+package foo;
+import org.openrewrite.java.*;
+
+public class ShouldAddClasspathRecipes$FullyQualifiedRecipe$1_before {
+    public static JavaTemplate.Builder getTemplate(JavaVisitor<?> visitor) {
+        return JavaTemplate
+                .builder("System.out.println(#{any(java.lang.String)})");
+    }
+}

src/test/resources/recipes/ShouldAddClasspathRecipe$1_after.java → src/test/resources/recipes/ShouldAddClasspathRecipe$UnqualifiedRecipe$1_after.java

@@ -1,10 +1,10 @@
 package foo;
 import org.openrewrite.java.*;
 
-public class ShouldAddClasspathRecipe$1_after {
+public class ShouldAddClasspathRecipes$UnqualifiedRecipe$1_after {
     public static JavaTemplate.Builder getTemplate(JavaVisitor<?> visitor) {
         return JavaTemplate
-                .builder("LoggerFactory.getLogger(\"ROOT\").inf#{any(java.lang.String)}ge)")
+                .builder("LoggerFactory.getLogger(#{any(java.lang.String)})")
                 .javaParser(JavaParser.fromJavaVersion().classpath("slf4j-api"))
                 .imports("org.slf4j.LoggerFactory");
     }

src/test/resources/recipes/ShouldAddClasspathRecipe$1_before.java → src/test/resources/recipes/ShouldAddClasspathRecipe$UnqualifiedRecipe$1_before.java

@@ -1,7 +1,7 @@
 package foo;
 import org.openrewrite.java.*;
 
-public class ShouldAddClasspathRecipe$1_before {
+public class ShouldAddClasspathRecipes$UnqualifiedRecipe$1_before {
     public static JavaTemplate.Builder getTemplate(JavaVisitor<?> visitor) {
         return JavaTemplate
                 .builder("System.out.println(#{any(java.lang.String)})");