8153761: JShell: Completion -- Show parameter names if possible

Compiling code with -parameters; keeping parameter names when reading classfiles; searching JDK sources if parameter names are not available.

Reviewed-by: rfield

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

@@ -40,10 +40,13 @@ import com.sun.source.tree.Scope;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.Tree.Kind;
 import com.sun.source.tree.VariableTree;
+import com.sun.source.util.JavacTask;
 import com.sun.source.util.SourcePositions;
 import com.sun.source.util.TreePath;
 import com.sun.source.util.TreePathScanner;
+import com.sun.source.util.Trees;
 import com.sun.tools.javac.api.JavacScope;
+import com.sun.tools.javac.api.JavacTaskImpl;
 import com.sun.tools.javac.code.Flags;
 import com.sun.tools.javac.code.Symbol.CompletionFailure;
 import com.sun.tools.javac.code.Symbol.VarSymbol;
@@ -119,8 +122,12 @@ import javax.lang.model.type.ExecutableType;
 import javax.lang.model.type.TypeKind;
 import javax.lang.model.util.ElementFilter;
 import javax.lang.model.util.Types;
+import javax.tools.JavaCompiler;
 import javax.tools.JavaFileManager.Location;
+import javax.tools.JavaFileObject;
+import javax.tools.StandardJavaFileManager;
 import javax.tools.StandardLocation;
+import javax.tools.ToolProvider;
 
 import static jdk.jshell.Util.REPL_DOESNOTMATTER_CLASS_NAME;
 
@@ -932,6 +939,12 @@ class SourceCodeAnalysisImpl extends SourceCodeAnalysis {
         }
     }
 
+    //tweaked by tests to disable reading parameter names from classfiles so that tests using
+    //JDK's classes are stable for both release and fastdebug builds:
+    private final String[] keepParameterNames = new String[] {
+        "-XDsave-parameter-names=true"
+    };
+
     private String documentationImpl(String code, int cursor) {
         code = code.substring(0, cursor);
         if (code.trim().isEmpty()) { //TODO: comment handling
@@ -942,7 +955,7 @@ class SourceCodeAnalysisImpl extends SourceCodeAnalysis {
             return null;
 
         OuterWrap codeWrap = proc.outerMap.wrapInTrialClass(Wrap.methodWrap(code));
-        AnalyzeTask at = proc.taskFactory.new AnalyzeTask(codeWrap);
+        AnalyzeTask at = proc.taskFactory.new AnalyzeTask(codeWrap, keepParameterNames);
         SourcePositions sp = at.trees().getSourcePositions();
         CompilationUnitTree topLevel = at.firstCuTree();
         TreePath tp = pathFor(topLevel, sp, codeWrap.snippetIndexToWrapIndex(cursor));
@@ -983,10 +996,12 @@ class SourceCodeAnalysisImpl extends SourceCodeAnalysis {
                         .collect(Collectors.toList());
         }
 
+        try (SourceCache sourceCache = new SourceCache(at)) {
             return Util.stream(candidates)
-                .map(method -> Util.expunge(element2String(method.fst)))
+                    .map(method -> Util.expunge(element2String(sourceCache, method.fst)))
                     .collect(joining("\n"));
         }
+    }
 
     private boolean isEmptyArgumentsContext(List<? extends ExpressionTree> arguments) {
         if (arguments.size() == 1) {
@@ -996,19 +1011,173 @@ class SourceCodeAnalysisImpl extends SourceCodeAnalysis {
         return false;
     }
 
-    private String element2String(Element el) {
+    private String element2String(SourceCache sourceCache, Element el) {
+        try {
+            if (hasSyntheticParameterNames(el)) {
+                el = sourceCache.getSourceMethod(el);
+            }
+        } catch (IOException ex) {
+            proc.debug(ex, "SourceCodeAnalysisImpl.element2String(..., " + el + ")");
+        }
+
+        return Util.expunge(elementHeader(el));
+    }
+
+    private boolean hasSyntheticParameterNames(Element el) {
+        if (el.getKind() != ElementKind.CONSTRUCTOR && el.getKind() != ElementKind.METHOD)
+            return false;
+
+        ExecutableElement ee = (ExecutableElement) el;
+
+        if (ee.getParameters().isEmpty())
+            return false;
+
+        return ee.getParameters()
+                 .stream()
+                 .allMatch(param -> param.getSimpleName().toString().startsWith("arg"));
+    }
+
+    private final class SourceCache implements AutoCloseable {
+        private final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
+        private final Map<String, Map<String, Element>> topLevelName2Signature2Method = new HashMap<>();
+        private final AnalyzeTask originalTask;
+        private final StandardJavaFileManager fm;
+
+        public SourceCache(AnalyzeTask originalTask) {
+            this.originalTask = originalTask;
+            Iterable<? extends Path> sources = findSources();
+            if (sources.iterator().hasNext()) {
+                StandardJavaFileManager fm = compiler.getStandardFileManager(null, null, null);
+                try {
+                    fm.setLocationFromPaths(StandardLocation.SOURCE_PATH, sources);
+                } catch (IOException ex) {
+                    proc.debug(ex, "SourceCodeAnalysisImpl.SourceCache.<init>(...)");
+                    fm = null;
+                }
+                this.fm = fm;
+            } else {
+                //don't waste time if there are no sources
+                this.fm = null;
+            }
+        }
+
+        public Element getSourceMethod(Element method) throws IOException {
+            if (fm == null)
+                return method;
+
+            TypeElement type = topLevelType(method);
+
+            if (type == null)
+                return method;
+
+            String binaryName = originalTask.task.getElements().getBinaryName(type).toString();
+
+            Map<String, Element> cache = topLevelName2Signature2Method.get(binaryName);
+
+            if (cache == null) {
+                topLevelName2Signature2Method.put(binaryName, cache = createMethodCache(binaryName));
+            }
+
+            String handle = elementHeader(method, false);
+
+            return cache.getOrDefault(handle, method);
+        }
+
+        private TypeElement topLevelType(Element el) {
+            while (el != null && el.getEnclosingElement().getKind() != ElementKind.PACKAGE) {
+                el = el.getEnclosingElement();
+            }
+
+            return el != null && (el.getKind().isClass() || el.getKind().isInterface()) ? (TypeElement) el : null;
+        }
+
+        private Map<String, Element> createMethodCache(String binaryName) throws IOException {
+            Pair<JavacTask, CompilationUnitTree> source = findSource(binaryName);
+
+            if (source == null)
+                return Collections.emptyMap();
+
+            Map<String, Element> signature2Method = new HashMap<>();
+            Trees trees = Trees.instance(source.fst);
+
+            new TreePathScanner<Void, Void>() {
+                @Override @DefinedBy(Api.COMPILER_TREE)
+                public Void visitMethod(MethodTree node, Void p) {
+                    Element currentMethod = trees.getElement(getCurrentPath());
+
+                    if (currentMethod != null) {
+                        signature2Method.put(elementHeader(currentMethod, false), currentMethod);
+                    }
+
+                    return null;
+                }
+            }.scan(source.snd, null);
+
+            return signature2Method;
+        }
+
+        private Pair<JavacTask, CompilationUnitTree> findSource(String binaryName) throws IOException {
+            JavaFileObject jfo = fm.getJavaFileForInput(StandardLocation.SOURCE_PATH,
+                                                        binaryName,
+                                                        JavaFileObject.Kind.SOURCE);
+
+            if (jfo == null)
+                return null;
+
+            List<JavaFileObject> jfos = Arrays.asList(jfo);
+            JavacTaskImpl task = (JavacTaskImpl) compiler.getTask(null, fm, d -> {}, null, null, jfos);
+            Iterable<? extends CompilationUnitTree> cuts = task.parse();
+
+            task.enter();
+
+            return Pair.of(task, cuts.iterator().next());
+        }
+
+        @Override
+        public void close() {
+            try {
+                if (fm != null) {
+                    fm.close();
+                }
+            } catch (IOException ex) {
+                proc.debug(ex, "SourceCodeAnalysisImpl.SourceCache.close()");
+            }
+        }
+    }
+
+    private Iterable<? extends Path> availableSources;
+
+    private Iterable<? extends Path> findSources() {
+        if (availableSources != null) {
+            return availableSources;
+        }
+        List<Path> result = new ArrayList<>();
+        Path home = Paths.get(System.getProperty("java.home"));
+        Path srcZip = home.resolve("src.zip");
+        if (!Files.isReadable(srcZip))
+            srcZip = home.getParent().resolve("src.zip");
+        if (Files.isReadable(srcZip))
+            result.add(srcZip);
+        return availableSources = result;
+    }
+
+    private String elementHeader(Element el) {
+        return elementHeader(el, true);
+    }
+
+    private String elementHeader(Element el, boolean includeParameterNames) {
         switch (el.getKind()) {
             case ANNOTATION_TYPE: case CLASS: case ENUM: case INTERFACE:
                 return ((TypeElement) el).getQualifiedName().toString();
             case FIELD:
-                return element2String(el.getEnclosingElement()) + "." + el.getSimpleName() + ":" + el.asType();
+                return elementHeader(el.getEnclosingElement()) + "." + el.getSimpleName() + ":" + el.asType();
             case ENUM_CONSTANT:
-                return element2String(el.getEnclosingElement()) + "." + el.getSimpleName();
+                return elementHeader(el.getEnclosingElement()) + "." + el.getSimpleName();
             case EXCEPTION_PARAMETER: case LOCAL_VARIABLE: case PARAMETER: case RESOURCE_VARIABLE:
                 return el.getSimpleName() + ":" + el.asType();
             case CONSTRUCTOR: case METHOD:
                 StringBuilder header = new StringBuilder();
-                header.append(element2String(el.getEnclosingElement()));
+                header.append(elementHeader(el.getEnclosingElement()));
                 if (el.getKind() == ElementKind.METHOD) {
                     header.append(".");
                     header.append(el.getSimpleName());
@@ -1026,8 +1195,10 @@ class SourceCodeAnalysisImpl extends SourceCodeAnalysis {
                     } else {
                         header.append(p.asType());
                     }
+                    if (includeParameterNames) {
                         header.append(" ");
                         header.append(p.getSimpleName());
+                    }
                     sep = ", ";
                 }
                 header.append(")");

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

@@ -215,17 +215,21 @@ class TaskFactory {
 
         private final Iterable<? extends CompilationUnitTree> cuts;
 
-        AnalyzeTask(final OuterWrap wrap) {
+        AnalyzeTask(final OuterWrap wrap, String... extraArgs) {
-            this(Collections.singletonList(wrap));
+            this(Collections.singletonList(wrap), extraArgs);
         }
 
-        AnalyzeTask(final Collection<OuterWrap> wraps) {
+        AnalyzeTask(final Collection<OuterWrap> wraps, String... extraArgs) {
             this(wraps.stream(),
                     new WrapSourceHandler(),
-                    "-XDshouldStopPolicy=FLOW", "-Xlint:unchecked", "-XaddExports:jdk.jshell/jdk.internal.jshell.remote=ALL-UNNAMED", "-proc:none");
+                    Util.join(new String[] {
+                        "-XDshouldStopPolicy=FLOW", "-Xlint:unchecked",
+                        "-XaddExports:jdk.jshell/jdk.internal.jshell.remote=ALL-UNNAMED",
+                        "-proc:none"
+                    }, extraArgs));
         }
 
-        <T>AnalyzeTask(final Stream<T> stream, SourceHandler<T> sourceHandler,
+        private <T>AnalyzeTask(final Stream<T> stream, SourceHandler<T> sourceHandler,
                 String... extraOptions) {
             super(stream, sourceHandler, extraOptions);
             cuts = analyze();
@@ -264,7 +268,7 @@ class TaskFactory {
 
         CompileTask(final Collection<OuterWrap> wraps) {
             super(wraps.stream(), new WrapSourceHandler(),
-                    "-Xlint:unchecked", "-XaddExports:jdk.jshell/jdk.internal.jshell.remote=ALL-UNNAMED", "-proc:none");
+                    "-Xlint:unchecked", "-XaddExports:jdk.jshell/jdk.internal.jshell.remote=ALL-UNNAMED", "-proc:none", "-parameters");
         }
 
         boolean compile() {

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

@@ -25,6 +25,9 @@
 
 package jdk.jshell;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 import java.util.Locale;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
@@ -95,6 +98,15 @@ class Util {
         return StreamSupport.stream(iterable.spliterator(), false);
     }
 
+    static String[] join(String[] a1, String[] a2) {
+        List<String> result = new ArrayList<>();
+
+        result.addAll(Arrays.asList(a1));
+        result.addAll(Arrays.asList(a2));
+
+        return result.toArray(new String[0]);
+    }
+
     static class Pair<T, U> {
         final T first;
         final U second;

langtools/test/jdk/jshell/CompletionSuggestionTest.java

@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 8141092
+ * @bug 8141092 8153761
  * @summary Test Completion
  * @modules jdk.compiler/com.sun.tools.javac.api
  *          jdk.compiler/com.sun.tools.javac.main
@@ -34,14 +34,20 @@
  * @run testng CompletionSuggestionTest
  */
 
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Set;
 import java.util.HashSet;
+import java.util.jar.JarEntry;
+import java.util.jar.JarOutputStream;
 
 import jdk.jshell.Snippet;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import static jdk.jshell.Snippet.Status.VALID;
@@ -295,10 +301,11 @@ public class CompletionSuggestionTest extends KullaTesting {
         assertCompletion("import inner.|");
     }
 
-    public void testDocumentation() {
+    public void testDocumentation() throws Exception {
+        dontReadParameterNamesFromClassFile();
         assertDocumentation("System.getProperty(|",
-                "java.lang.System.getProperty(java.lang.String arg0)",
+                "java.lang.System.getProperty(java.lang.String key)",
-                "java.lang.System.getProperty(java.lang.String arg0, java.lang.String arg1)");
+                "java.lang.System.getProperty(java.lang.String key, java.lang.String def)");
         assertEval("char[] chars = null;");
         assertDocumentation("new String(chars, |",
                 "java.lang.String(char[] arg0, int arg1, int arg2)");
@@ -313,7 +320,8 @@ public class CompletionSuggestionTest extends KullaTesting {
                                                      "java.lang.String.getBytes(java.nio.charset.Charset arg0)");
     }
 
-    public void testMethodsWithNoArguments() {
+    public void testMethodsWithNoArguments() throws Exception {
+        dontReadParameterNamesFromClassFile();
         assertDocumentation("System.out.println(|",
                 "java.io.PrintStream.println()",
                 "java.io.PrintStream.println(boolean arg0)",
@@ -442,29 +450,30 @@ public class CompletionSuggestionTest extends KullaTesting {
     public void testDocumentationOfUserDefinedMethods() {
         assertEval("void f() {}");
         assertDocumentation("f(|", "f()");
-        assertEval("void f(int a) {}");
+        assertEval("void f(int i) {}");
-        assertDocumentation("f(|", "f()", "f(int arg0)");
+        assertDocumentation("f(|", "f()", "f(int i)");
-        assertEval("<T> void f(T... a) {}", DiagCheck.DIAG_WARNING, DiagCheck.DIAG_OK);
+        assertEval("<T> void f(T... ts) {}", DiagCheck.DIAG_WARNING, DiagCheck.DIAG_OK);
-        assertDocumentation("f(|", "f()", "f(int arg0)", "f(T... arg0)");
+        assertDocumentation("f(|", "f()", "f(int i)", "f(T... ts)");
         assertEval("class A {}");
         assertEval("void f(A a) {}");
-        assertDocumentation("f(|", "f()", "f(int arg0)", "f(T... arg0)", "f(A arg0)");
+        assertDocumentation("f(|", "f()", "f(int i)", "f(T... ts)", "f(A a)");
     }
 
     public void testDocumentationOfUserDefinedConstructors() {
         Snippet a = classKey(assertEval("class A {}"));
         assertDocumentation("new A(|", "A()");
-        Snippet a2 = classKey(assertEval("class A { A() {} A(int a) {}}",
+        Snippet a2 = classKey(assertEval("class A { A() {} A(int i) {}}",
                 ste(MAIN_SNIPPET, VALID, VALID, true, null),
                 ste(a, VALID, OVERWRITTEN, false, MAIN_SNIPPET)));
-        assertDocumentation("new A(|", "A()", "A(int arg0)");
+        assertDocumentation("new A(|", "A()", "A(int i)");
-        assertEval("class A<T> { A(T a) {} A(int a) {}}",
+        assertEval("class A<T> { A(T t) {} A(int i) {}}",
                 ste(MAIN_SNIPPET, VALID, VALID, true, null),
                 ste(a2, VALID, OVERWRITTEN, false, MAIN_SNIPPET));
-        assertDocumentation("new A(|", "A(T arg0)", "A(int arg0)");
+        assertDocumentation("new A(|", "A(T t)", "A(int i)");
     }
 
-    public void testDocumentationOfOverriddenMethods() {
+    public void testDocumentationOfOverriddenMethods() throws Exception {
+        dontReadParameterNamesFromClassFile();
         assertDocumentation("\"\".wait(|",
             "java.lang.Object.wait(long arg0)",
             "java.lang.Object.wait(long arg0, int arg1)",
@@ -502,13 +511,13 @@ public class CompletionSuggestionTest extends KullaTesting {
         assertEval("void method(int n, Object o) { }");
         assertEval("void method(Object n, int o) { }");
         assertDocumentation("method(primitive,|",
-                "method(int arg0, java.lang.Object arg1)",
+                "method(int n, java.lang.Object o)",
-                "method(java.lang.Object arg0, int arg1)");
+                "method(java.lang.Object n, int o)");
         assertDocumentation("method(boxed,|",
-                "method(int arg0, java.lang.Object arg1)",
+                "method(int n, java.lang.Object o)",
-                "method(java.lang.Object arg0, int arg1)");
+                "method(java.lang.Object n, int o)");
         assertDocumentation("method(object,|",
-                "method(java.lang.Object arg0, int arg1)");
+                "method(java.lang.Object n, int o)");
     }
 
     public void testVarArgs() {
@@ -546,4 +555,36 @@ public class CompletionSuggestionTest extends KullaTesting {
         assertEval("class Foo { static void m(String str) {} static void m(Baz<String> baz) {} }");
         assertCompletion("Foo.m(new Baz<>(|", true, "str");
     }
+
+    @BeforeMethod
+    public void setUp() {
+        super.setUp();
+
+        Path srcZip = Paths.get("src.zip");
+
+        try (JarOutputStream out = new JarOutputStream(Files.newOutputStream(srcZip))) {
+            out.putNextEntry(new JarEntry("java/lang/System.java"));
+            out.write(("package java.lang;\n" +
+                       "public class System {\n" +
+                       "    public String getProperty(String key) { return null; }\n" +
+                       "    public String getProperty(String key, String def) { return def; }\n" +
+                       "}\n").getBytes());
+        } catch (IOException ex) {
+            throw new IllegalStateException(ex);
+        }
+
+        try {
+            Field availableSources = getAnalysis().getClass().getDeclaredField("availableSources");
+            availableSources.setAccessible(true);
+            availableSources.set(getAnalysis(), Arrays.asList(srcZip));
+        } catch (NoSuchFieldException | IllegalArgumentException | IllegalAccessException ex) {
+            throw new IllegalStateException(ex);
+        }
+    }
+
+    private void dontReadParameterNamesFromClassFile() throws Exception {
+        Field keepParameterNames = getAnalysis().getClass().getDeclaredField("keepParameterNames");
+        keepParameterNames.setAccessible(true);
+        keepParameterNames.set(getAnalysis(), new String[0]);
+    }
 }