8141092: JShell: Completion hangs on identifier completion

Summary: Avoiding recursive search when computing package completion.
Reviewed-by: mcimadamore, rfield

langtools/src/jdk.jshell/share/classes/jdk/jshell/JShell.java

@@ -93,7 +93,7 @@ public class JShell implements AutoCloseable {
 
 
     private ExecutionControl executionControl = null;
-    private SourceCodeAnalysis sourceCodeAnalysis = null;
+    private SourceCodeAnalysisImpl sourceCodeAnalysis = null;
 
 
     JShell(Builder b) {
@@ -378,6 +378,9 @@ public class JShell implements AutoCloseable {
     public void addToClasspath(String path) {
         taskFactory.addToClasspath(path);  // Compiler
         executionControl().commandAddToClasspath(path);       // Runtime
+        if (sourceCodeAnalysis != null) {
+            sourceCodeAnalysis.classpathChanged();
+        }
     }
 
     /**

langtools/src/jdk.jshell/share/classes/jdk/jshell/MemoryFileManager.java

@@ -42,6 +42,7 @@ import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Set;
 import java.util.TreeMap;
+
 import javax.tools.JavaFileObject.Kind;
 import static javax.tools.StandardLocation.CLASS_PATH;
 import javax.tools.FileObject;
@@ -49,6 +50,7 @@ import javax.tools.JavaFileManager;
 import javax.tools.JavaFileObject;
 import javax.tools.SimpleJavaFileObject;
 import javax.tools.StandardJavaFileManager;
+import javax.tools.StandardLocation;
 
 import com.sun.tools.javac.util.DefinedBy;
 import com.sun.tools.javac.util.DefinedBy.Api;
@@ -77,6 +79,10 @@ class MemoryFileManager implements JavaFileManager {
     private Method inferModuleNameMethod = null;
     private Method listModuleLocationsMethod = null;
 
+    Iterable<? extends Path> getLocationAsPaths(Location loc) {
+        return this.stdFileManager.getLocationAsPaths(loc);
+    }
+
     static abstract class MemoryJavaFileObject extends SimpleJavaFileObject {
 
         public MemoryJavaFileObject(String name, JavaFileObject.Kind kind) {

langtools/src/jdk.jshell/share/classes/jdk/jshell/SourceCodeAnalysisImpl.java

@@ -75,8 +75,14 @@ import javax.lang.model.type.TypeMirror;
 import static jdk.internal.jshell.debug.InternalDebugControl.DBG_COMPA;
 
 import java.io.IOException;
+import java.net.URI;
+import java.nio.file.DirectoryStream;
+import java.nio.file.FileSystem;
+import java.nio.file.FileSystems;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Comparator;
-import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.NoSuchElementException;
 import java.util.Set;
@@ -92,6 +98,7 @@ import static java.util.stream.Collectors.toSet;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 
+import javax.lang.model.SourceVersion;
 import javax.lang.model.element.ExecutableElement;
 import javax.lang.model.element.PackageElement;
 import javax.lang.model.element.QualifiedNameable;
@@ -100,10 +107,8 @@ import javax.lang.model.type.ArrayType;
 import javax.lang.model.type.ExecutableType;
 import javax.lang.model.type.TypeKind;
 import javax.lang.model.util.ElementFilter;
-import javax.lang.model.util.Elements;
 import javax.lang.model.util.Types;
 import javax.tools.JavaFileManager.Location;
-import javax.tools.JavaFileObject;
 import javax.tools.StandardLocation;
 
 import static jdk.jshell.Util.REPL_DOESNOTMATTER_CLASS_NAME;
@@ -584,42 +589,114 @@ class SourceCodeAnalysisImpl extends SourceCodeAnalysis {
                 .collect(toList());
     }
 
+    private Set<String> emptyContextPackages = null;
+
+    void classpathChanged() {
+        emptyContextPackages = null;
+    }
+
     private Set<PackageElement> listPackages(AnalyzeTask at, String enclosingPackage) {
-        Set<PackageElement> packs = new HashSet<>();
-        listPackages(at, StandardLocation.PLATFORM_CLASS_PATH, enclosingPackage, packs);
-        listPackages(at, StandardLocation.CLASS_PATH, enclosingPackage, packs);
-        listPackages(at, StandardLocation.SOURCE_PATH, enclosingPackage, packs);
-        return packs;
+        Set<String> packs;
+
+        if (enclosingPackage.isEmpty() && emptyContextPackages != null) {
+            packs = emptyContextPackages;
+        } else {
+            packs = new HashSet<>();
+
+            listPackages(StandardLocation.PLATFORM_CLASS_PATH, enclosingPackage, packs);
+            listPackages(StandardLocation.CLASS_PATH, enclosingPackage, packs);
+            listPackages(StandardLocation.SOURCE_PATH, enclosingPackage, packs);
+
+            if (enclosingPackage.isEmpty()) {
+                emptyContextPackages = packs;
+            }
+        }
+
+        return packs.stream()
+                    .map(pkg -> createPackageElement(at, pkg))
+                    .collect(Collectors.toSet());
     }
 
-    private void listPackages(AnalyzeTask at, Location loc, String currentPackage, Set<PackageElement> packs) {
+    private PackageElement createPackageElement(AnalyzeTask at, String packageName) {
+        Names names = Names.instance(at.getContext());
+        Symtab syms = Symtab.instance(at.getContext());
+        PackageElement existing = syms.enterPackage(names.fromString(packageName));
+
+        return existing;
+    }
+
+    private void listPackages(Location loc, String enclosing, Set<String> packs) {
+        Iterable<? extends Path> paths = proc.taskFactory.fileManager().getLocationAsPaths(loc);
+
+        if (paths == null)
+            return ;
+
+        for (Path p : paths) {
+            listPackages(p, enclosing, packs);
+        }
+    }
+
+    private void listPackages(Path path, String enclosing, Set<String> packages) {
         try {
-            MemoryFileManager fm = proc.taskFactory.fileManager();
-            for (JavaFileObject file : fm.list(loc, currentPackage, fileKinds, true)) {
-                String binaryName = fm.inferBinaryName(loc, file);
-                if (!currentPackage.isEmpty() && !binaryName.startsWith(currentPackage + "."))
-                    continue;
-                int nextDot = binaryName.indexOf('.', !currentPackage.isEmpty() ? currentPackage.length() + 1 : 0);
-                if (nextDot == (-1))
-                    continue;
-                Elements elements = at.getElements();
-                PackageElement pack =
-                        elements.getPackageElement(binaryName.substring(0, nextDot));
-                if (pack == null) {
-                    //if no types in the package have ever been seen, the package will be unknown
-                    //try to load a type, and then try to recognize the package again:
-                    elements.getTypeElement(binaryName);
-                    pack = elements.getPackageElement(binaryName.substring(0, nextDot));
+            if (path.equals(Paths.get("JRT_MARKER_FILE"))) {
+                FileSystem jrtfs = FileSystems.getFileSystem(URI.create("jrt:/"));
+                Path modules = jrtfs.getPath("modules");
+                try (DirectoryStream<Path> stream = Files.newDirectoryStream(modules)) {
+                    for (Path c : stream) {
+                        listDirectory(c, enclosing, packages);
+                    }
                 }
-                if (pack != null)
-                    packs.add(pack);
+            } else if (!Files.isDirectory(path)) {
+                if (Files.exists(path)) {
+                    ClassLoader cl = SourceCodeAnalysisImpl.class.getClassLoader();
+
+                    try (FileSystem zip = FileSystems.newFileSystem(path, cl)) {
+                        listDirectory(zip.getRootDirectories().iterator().next(), enclosing, packages);
+                    }
+                }
+            } else {
+                listDirectory(path, enclosing, packages);
             }
         } catch (IOException ex) {
-            //TODO: should log?
+            proc.debug(ex, "SourceCodeAnalysisImpl.listPackages(" + path.toString() + ", " + enclosing + ", " + packages + ")");
         }
     }
-    //where:
-        private final Set<JavaFileObject.Kind> fileKinds = EnumSet.of(JavaFileObject.Kind.CLASS);
+
+    private void listDirectory(Path path, String enclosing, Set<String> packages) throws IOException {
+        String separator = path.getFileSystem().getSeparator();
+        Path resolved = path.resolve(enclosing.replace(".", separator));
+
+        if (Files.isDirectory(resolved)) {
+            try (DirectoryStream<Path> ds = Files.newDirectoryStream(resolved)) {
+                for (Path entry : ds) {
+                    String name = pathName(entry);
+
+                    if (SourceVersion.isIdentifier(name) &&
+                        Files.isDirectory(entry) &&
+                        validPackageCandidate(entry)) {
+                        packages.add(enclosing + (enclosing.isEmpty() ? "" : ".") + name);
+                    }
+                }
+            }
+        }
+    }
+
+    private boolean validPackageCandidate(Path p) throws IOException {
+        try (Stream<Path> dir = Files.list(p)) {
+            return dir.anyMatch(e -> Files.isDirectory(e) && SourceVersion.isIdentifier(pathName(e)) ||
+                                e.getFileName().toString().endsWith(".class"));
+        }
+    }
+
+    private String pathName(Path p) {
+        String separator = p.getFileSystem().getSeparator();
+        String name = p.getFileName().toString();
+
+        if (name.endsWith(separator)) //jars have '/' appended
+            name = name.substring(0, name.length() - separator.length());
+
+        return name;
+    }
 
     private Element createArrayLengthSymbol(AnalyzeTask at, TypeMirror site) {
         Name length = Names.instance(at.getContext()).length;

langtools/test/jdk/jshell/CompletionSuggestionTest.java

@@ -23,6 +23,7 @@
 
 /*
  * @test
+ * @bug 8141092
  * @summary Test Completion
  * @library /tools/lib
  * @build KullaTesting TestingInputStream ToolBox Compiler
@@ -45,6 +46,9 @@ import static jdk.jshell.Snippet.Status.OVERWRITTEN;
 @Test
 public class CompletionSuggestionTest extends KullaTesting {
 
+    private final Compiler compiler = new Compiler();
+    private final Path outDir = Paths.get("completion_suggestion_test");
+
     public void testMemberExpr() {
         assertEval("class Test { static void test() { } }");
         assertCompletion("Test.t|", "test()");
@@ -232,6 +236,23 @@ public class CompletionSuggestionTest extends KullaTesting {
 
         assertCompletion("Object.notElement.|");
         assertCompletion("Object o = com.su|", "sun");
+
+        Path p1 = outDir.resolve("dir1");
+        compiler.compile(p1,
+                "package p1.p2;\n" +
+                "public class Test {\n" +
+                "}",
+                "package p1.p3;\n" +
+                "public class Test {\n" +
+                "}");
+        String jarName = "test.jar";
+        compiler.jar(p1, jarName, "p1/p2/Test.class", "p1/p3/Test.class");
+        addToClasspath(compiler.getPath(p1.resolve(jarName)));
+
+        assertCompletionIncludesExcludes("|", new HashSet<>(Collections.singletonList("p1")), Collections.emptySet());
+        assertCompletion("p1.|", "p2", "p3");
+        assertCompletion("p1.p2.|", "Test");
+        assertCompletion("p1.p3.|", "Test");
     }
 
     public void testCheckAccessibility() {