# HG changeset patch # User jlahoda # Date 1462445721 -7200 # Node ID 3eb8c2a89b77b353e1224cee7a65e82691f3b4bf # Parent 42baa89d2156c657a4fb657413be5bb29719c571 8153761: JShell: Completion -- Show parameter names if possible Summary: Compiling code with -parameters; keeping parameter names when reading classfiles; searching JDK sources if parameter names are not available. Reviewed-by: rfield diff -r 42baa89d2156 -r 3eb8c2a89b77 langtools/src/jdk.jshell/share/classes/jdk/jshell/SourceCodeAnalysisImpl.java --- a/langtools/src/jdk.jshell/share/classes/jdk/jshell/SourceCodeAnalysisImpl.java Mon May 09 16:52:15 2016 -0700 +++ b/langtools/src/jdk.jshell/share/classes/jdk/jshell/SourceCodeAnalysisImpl.java Thu May 05 12:55:21 2016 +0200 @@ -40,10 +40,13 @@ 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.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 @@ } } + //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 @@ 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,9 +996,11 @@ .collect(Collectors.toList()); } - return Util.stream(candidates) - .map(method -> Util.expunge(element2String(method.fst))) - .collect(joining("\n")); + try (SourceCache sourceCache = new SourceCache(at)) { + return Util.stream(candidates) + .map(method -> Util.expunge(element2String(sourceCache, method.fst))) + .collect(joining("\n")); + } } private boolean isEmptyArgumentsContext(List arguments) { @@ -996,19 +1011,173 @@ 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> topLevelName2Signature2Method = new HashMap<>(); + private final AnalyzeTask originalTask; + private final StandardJavaFileManager fm; + + public SourceCache(AnalyzeTask originalTask) { + this.originalTask = originalTask; + Iterable 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.(...)"); + 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 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 createMethodCache(String binaryName) throws IOException { + Pair source = findSource(binaryName); + + if (source == null) + return Collections.emptyMap(); + + Map signature2Method = new HashMap<>(); + Trees trees = Trees.instance(source.fst); + + new TreePathScanner() { + @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 findSource(String binaryName) throws IOException { + JavaFileObject jfo = fm.getJavaFileForInput(StandardLocation.SOURCE_PATH, + binaryName, + JavaFileObject.Kind.SOURCE); + + if (jfo == null) + return null; + + List jfos = Arrays.asList(jfo); + JavacTaskImpl task = (JavacTaskImpl) compiler.getTask(null, fm, d -> {}, null, null, jfos); + Iterable 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 availableSources; + + private Iterable findSources() { + if (availableSources != null) { + return availableSources; + } + List 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 @@ } else { header.append(p.asType()); } - header.append(" "); - header.append(p.getSimpleName()); + if (includeParameterNames) { + header.append(" "); + header.append(p.getSimpleName()); + } sep = ", "; } header.append(")"); diff -r 42baa89d2156 -r 3eb8c2a89b77 langtools/src/jdk.jshell/share/classes/jdk/jshell/TaskFactory.java --- a/langtools/src/jdk.jshell/share/classes/jdk/jshell/TaskFactory.java Mon May 09 16:52:15 2016 -0700 +++ b/langtools/src/jdk.jshell/share/classes/jdk/jshell/TaskFactory.java Thu May 05 12:55:21 2016 +0200 @@ -215,17 +215,21 @@ private final Iterable cuts; - AnalyzeTask(final OuterWrap wrap) { - this(Collections.singletonList(wrap)); + AnalyzeTask(final OuterWrap wrap, String... extraArgs) { + this(Collections.singletonList(wrap), extraArgs); } - AnalyzeTask(final Collection wraps) { + AnalyzeTask(final Collection 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)); } - AnalyzeTask(final Stream stream, SourceHandler sourceHandler, + private AnalyzeTask(final Stream stream, SourceHandler sourceHandler, String... extraOptions) { super(stream, sourceHandler, extraOptions); cuts = analyze(); @@ -264,7 +268,7 @@ CompileTask(final Collection 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() { diff -r 42baa89d2156 -r 3eb8c2a89b77 langtools/src/jdk.jshell/share/classes/jdk/jshell/Util.java --- a/langtools/src/jdk.jshell/share/classes/jdk/jshell/Util.java Mon May 09 16:52:15 2016 -0700 +++ b/langtools/src/jdk.jshell/share/classes/jdk/jshell/Util.java Thu May 05 12:55:21 2016 +0200 @@ -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 @@ return StreamSupport.stream(iterable.spliterator(), false); } + static String[] join(String[] a1, String[] a2) { + List result = new ArrayList<>(); + + result.addAll(Arrays.asList(a1)); + result.addAll(Arrays.asList(a2)); + + return result.toArray(new String[0]); + } + static class Pair { final T first; final U second; diff -r 42baa89d2156 -r 3eb8c2a89b77 langtools/test/jdk/jshell/CompletionSuggestionTest.java --- a/langtools/test/jdk/jshell/CompletionSuggestionTest.java Mon May 09 16:52:15 2016 -0700 +++ b/langtools/test/jdk/jshell/CompletionSuggestionTest.java Thu May 05 12:55:21 2016 +0200 @@ -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 @@ 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 arg0, java.lang.String arg1)"); + "java.lang.System.getProperty(java.lang.String key)", + "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 @@ "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 void testDocumentationOfUserDefinedMethods() { assertEval("void f() {}"); assertDocumentation("f(|", "f()"); - assertEval("void f(int a) {}"); - assertDocumentation("f(|", "f()", "f(int arg0)"); - assertEval(" void f(T... a) {}", DiagCheck.DIAG_WARNING, DiagCheck.DIAG_OK); - assertDocumentation("f(|", "f()", "f(int arg0)", "f(T... arg0)"); + assertEval("void f(int i) {}"); + assertDocumentation("f(|", "f()", "f(int i)"); + assertEval(" void f(T... ts) {}", DiagCheck.DIAG_WARNING, DiagCheck.DIAG_OK); + 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)"); - assertEval("class A { A(T a) {} A(int a) {}}", + assertDocumentation("new A(|", "A()", "A(int i)"); + assertEval("class A { 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 @@ 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(java.lang.Object arg0, int arg1)"); + "method(int n, java.lang.Object o)", + "method(java.lang.Object n, int o)"); assertDocumentation("method(boxed,|", - "method(int arg0, java.lang.Object arg1)", - "method(java.lang.Object arg0, int arg1)"); + "method(int n, java.lang.Object o)", + "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 @@ assertEval("class Foo { static void m(String str) {} static void m(Baz 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]); + } }