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
--- 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<? extends ExpressionTree> 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<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 @@
} else {
header.append(p.asType());
}
- header.append(" ");
- header.append(p.getSimpleName());
+ if (includeParameterNames) {
+ header.append(" ");
+ header.append(p.getSimpleName());
+ }
sep = ", ";
}
header.append(")");
--- 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<? extends CompilationUnitTree> cuts;
- AnalyzeTask(final OuterWrap wrap) {
- this(Collections.singletonList(wrap));
+ AnalyzeTask(final OuterWrap wrap, String... extraArgs) {
+ 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 @@
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() {
--- 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<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;
--- 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("<T> 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("<T> 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<T> { A(T a) {} A(int a) {}}",
+ assertDocumentation("new A(|", "A()", "A(int i)");
+ 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 @@
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<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]);
+ }
}