8144095: jshell tool: normalize command parameter handling
8140265: jshell tool: /save saves rejected input
Reviewed-by: jlahoda
--- a/langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellTool.java Thu Dec 10 09:24:25 2015 -0800
+++ b/langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellTool.java Thu Dec 10 23:27:06 2015 -0800
@@ -86,10 +86,11 @@
import static java.nio.file.StandardOpenOption.CREATE;
import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
import static java.nio.file.StandardOpenOption.WRITE;
-import java.util.Locale;
import java.util.MissingResourceException;
import java.util.Optional;
import java.util.ResourceBundle;
+import java.util.Spliterators;
+import java.util.function.Supplier;
import static java.util.stream.Collectors.toList;
/**
@@ -600,6 +601,7 @@
}
private static final CompletionProvider EMPTY_COMPLETION_PROVIDER = new FixedCompletionProvider();
+ private static final CompletionProvider KEYWORD_COMPLETION_PROVIDER = new FixedCompletionProvider("all ", "start ", "history ");
private static final CompletionProvider FILE_COMPLETION_PROVIDER = fileCompletions(p -> true);
private final Map<String, Command> commands = new LinkedHashMap<>();
private void registerCommand(Command cmd) {
@@ -650,13 +652,21 @@
};
}
+ private CompletionProvider editKeywordCompletion() {
+ return (code, cursor, anchor) -> {
+ List<Suggestion> result = new ArrayList<>();
+ result.addAll(KEYWORD_COMPLETION_PROVIDER.completionSuggestions(code, cursor, anchor));
+ result.addAll(editCompletion().completionSuggestions(code, cursor, anchor));
+ return result;
+ };
+ }
+
private static CompletionProvider saveCompletion() {
- CompletionProvider keyCompletion = new FixedCompletionProvider("all ", "history ", "start ");
return (code, cursor, anchor) -> {
List<Suggestion> result = new ArrayList<>();
int space = code.indexOf(' ');
if (space == (-1)) {
- result.addAll(keyCompletion.completionSuggestions(code, cursor, anchor));
+ result.addAll(KEYWORD_COMPLETION_PROVIDER.completionSuggestions(code, cursor, anchor));
}
result.addAll(FILE_COMPLETION_PROVIDER.completionSuggestions(code.substring(space + 1), cursor - space - 1, anchor));
anchor[0] += space + 1;
@@ -667,9 +677,9 @@
// Table of commands -- with command forms, argument kinds, help message, implementation, ...
{
- registerCommand(new Command("/list", "[all]", "list the source you have typed",
+ registerCommand(new Command("/list", "[all|start|history|<name or id>]", "list the source you have typed",
arg -> cmdList(arg),
- new FixedCompletionProvider("all")));
+ editKeywordCompletion()));
registerCommand(new Command("/seteditor", "<executable>", "set the external editor command to use",
arg -> cmdSetEditor(arg),
EMPTY_COMPLETION_PROVIDER));
@@ -937,49 +947,67 @@
}
/**
- * Convert a user argument to a list of snippets referenced by that
- * argument (or lack of argument).
- * @param arg The user's argument to the command
- * @return a list of referenced snippets
+ * Avoid parameterized varargs possible heap pollution warning.
*/
- private List<Snippet> argToSnippets(String arg) {
- List<Snippet> snippets = new ArrayList<>();
- if (arg.isEmpty()) {
- // Default is all user snippets
- for (Snippet sn : state.snippets()) {
- if (notInStartUp(sn)) {
- snippets.add(sn);
- }
- }
- } else {
- // Look for all declarations with matching names
- for (Snippet key : state.snippets()) {
- switch (key.kind()) {
- case METHOD:
- case VAR:
- case TYPE_DECL:
- if (((DeclarationSnippet) key).name().equals(arg)) {
- snippets.add(key);
- }
- break;
- }
- }
- // If no declarations found, look for an id of this name
- if (snippets.isEmpty()) {
- for (Snippet sn : state.snippets()) {
- if (sn.id().equals(arg)) {
- snippets.add(sn);
- break;
- }
- }
- }
- // If still no matches found, give an error
- if (snippets.isEmpty()) {
- hard("No definition or id named %s found. See /classes /methods /vars or /list", arg);
- return null;
+ private interface SnippetPredicate extends Predicate<Snippet> { }
+
+ /**
+ * Apply filters to a stream until one that is non-empty is found.
+ * Adapted from Stuart Marks
+ *
+ * @param supplier Supply the Snippet stream to filter
+ * @param filters Filters to attempt
+ * @return The non-empty filtered Stream, or null
+ */
+ private static Stream<Snippet> nonEmptyStream(Supplier<Stream<Snippet>> supplier,
+ SnippetPredicate... filters) {
+ for (SnippetPredicate filt : filters) {
+ Iterator<Snippet> iterator = supplier.get().filter(filt).iterator();
+ if (iterator.hasNext()) {
+ return StreamSupport.stream(Spliterators.spliteratorUnknownSize(iterator, 0), false);
}
}
- return snippets;
+ return null;
+ }
+
+ private boolean mainActive(Snippet sn) {
+ return notInStartUp(sn) && state.status(sn).isActive;
+ }
+
+ private boolean matchingDeclaration(Snippet sn, String name) {
+ return sn instanceof DeclarationSnippet
+ && ((DeclarationSnippet) sn).name().equals(name);
+ }
+
+ /**
+ * Convert a user argument to a Stream of snippets referenced by that argument
+ * (or lack of argument).
+ *
+ * @param arg The user's argument to the command, maybe be the empty string
+ * @return a Stream of referenced snippets or null if no matches to specific arg
+ */
+ private Stream<Snippet> argToSnippets(String arg, boolean allowAll) {
+ List<Snippet> snippets = state.snippets();
+ if (allowAll && arg.equals("all")) {
+ // all snippets including start-up, failed, and overwritten
+ return snippets.stream();
+ } else if (arg.isEmpty()) {
+ // Default is all active user snippets
+ return snippets.stream()
+ .filter(this::mainActive);
+ } else {
+ Stream<Snippet> result =
+ nonEmptyStream(
+ () -> snippets.stream(),
+ // look for active user declarations matching the name
+ sn -> mainActive(sn) && matchingDeclaration(sn, arg),
+ // else, look for any declarations matching the name
+ sn -> matchingDeclaration(sn, arg),
+ // else, look for an id of this name
+ sn -> sn.id().equals(arg)
+ );
+ return result;
+ }
}
private void cmdDrop(String arg) {
@@ -988,39 +1016,40 @@
hard("Specify by id or name. Use /list to see ids. Use /reset to reset all state.");
return;
}
- List<Snippet> snippetSet = argToSnippets(arg);
- if (snippetSet == null) {
+ Stream<Snippet> stream = argToSnippets(arg, false);
+ if (stream == null) {
+ hard("No definition or id named %s found. See /classes, /methods, /vars, or /list", arg);
return;
}
- snippetSet = snippetSet.stream()
- .filter(sn -> state.status(sn).isActive)
+ List<Snippet> snippets = stream
+ .filter(sn -> state.status(sn).isActive && sn instanceof PersistentSnippet)
.collect(toList());
- snippetSet.removeIf(sn -> !(sn instanceof PersistentSnippet));
- if (snippetSet.isEmpty()) {
- hard("The argument did not specify an import, variable, method, or class to drop.");
+ if (snippets.isEmpty()) {
+ hard("The argument did not specify an active import, variable, method, or class to drop.");
return;
}
- if (snippetSet.size() > 1) {
+ if (snippets.size() > 1) {
hard("The argument references more than one import, variable, method, or class.");
hard("Try again with one of the ids below:");
- for (Snippet sn : snippetSet) {
+ for (Snippet sn : snippets) {
cmdout.printf("%4s : %s\n", sn.id(), sn.source().replace("\n", "\n "));
}
return;
}
- PersistentSnippet psn = (PersistentSnippet) snippetSet.iterator().next();
+ PersistentSnippet psn = (PersistentSnippet) snippets.get(0);
state.drop(psn).forEach(this::handleEvent);
}
private void cmdEdit(String arg) {
- List<Snippet> snippetSet = argToSnippets(arg);
- if (snippetSet == null) {
+ Stream<Snippet> stream = argToSnippets(arg, true);
+ if (stream == null) {
+ hard("No definition or id named %s found. See /classes, /methods, /vars, or /list", arg);
return;
}
Set<String> srcSet = new LinkedHashSet<>();
- for (Snippet key : snippetSet) {
- String src = key.source();
- switch (key.subKind()) {
+ stream.forEachOrdered(sn -> {
+ String src = sn.source();
+ switch (sn.subKind()) {
case VAR_VALUE_SUBKIND:
break;
case ASSIGNMENT_SUBKIND:
@@ -1035,7 +1064,7 @@
srcSet.add(src);
break;
}
- }
+ });
StringBuilder sb = new StringBuilder();
for (String s : srcSet) {
sb.append(s);
@@ -1107,31 +1136,30 @@
}
private void cmdList(String arg) {
- boolean all = false;
- switch (arg) {
- case "all":
- all = true;
- break;
- case "history":
- cmdHistory();
- return;
- case "":
- break;
- default:
- hard("Invalid /list argument: %s", arg);
- return;
+ if (arg.equals("history")) {
+ cmdHistory();
+ return;
}
- boolean hasOutput = false;
- for (Snippet sn : state.snippets()) {
- if (all || (notInStartUp(sn) && state.status(sn).isActive)) {
- if (!hasOutput) {
- cmdout.println();
- hasOutput = true;
- }
- cmdout.printf("%4s : %s\n", sn.id(), sn.source().replace("\n", "\n "));
+ Stream<Snippet> stream = argToSnippets(arg, true);
+ if (stream == null) {
+ // Check if there are any definitions at all
+ if (argToSnippets("", false).iterator().hasNext()) {
+ hard("No definition or id named %s found. Try /list without arguments.", arg);
+ } else {
+ hard("No definition or id named %s found. There are no active definitions.", arg);
+ }
+ return;
+ }
+ // prevent double newline on empty list
+ boolean[] hasOutput = new boolean[1];
+ stream.forEachOrdered(sn -> {
+ if (!hasOutput[0]) {
+ cmdout.println();
+ hasOutput[0] = true;
}
- }
+ cmdout.printf("%4s : %s\n", sn.id(), sn.source().replace("\n", "\n "));
+ });
}
private void cmdOpen(String filename) {
@@ -1166,12 +1194,12 @@
return;
}
boolean useHistory = false;
- boolean saveAll = false;
+ String saveAll = "";
boolean saveStart = false;
String cmd = mat.group("cmd");
if (cmd != null) switch (cmd) {
case "all":
- saveAll = true;
+ saveAll = "all";
break;
case "history":
useHistory = true;
@@ -1196,8 +1224,9 @@
} else if (saveStart) {
writer.append(DEFAULT_STARTUP);
} else {
- for (Snippet sn : state.snippets()) {
- if (saveAll || notInStartUp(sn)) {
+ Stream<Snippet> stream = argToSnippets(saveAll, true);
+ if (stream != null) {
+ for (Snippet sn : stream.collect(toList())) {
writer.write(sn.source());
writer.write("\n");
}
--- a/langtools/test/jdk/jshell/CommandCompletionTest.java Thu Dec 10 09:24:25 2015 -0800
+++ b/langtools/test/jdk/jshell/CommandCompletionTest.java Thu Dec 10 23:27:06 2015 -0800
@@ -23,6 +23,7 @@
/*
* @test
+ * @bug 8144095
* @summary Test Command Completion
* @library /tools/lib
* @build ReplToolTesting TestingInputStream Compiler ToolBox
@@ -56,15 +57,20 @@
}
public void testList() {
- assertCompletion("/l|", false, "/list ");
- assertCompletion("/list |", false, "all");
- assertCompletion("/list q|", false);
+ test(false, new String[] {"-nostartup"},
+ a -> assertCompletion(a, "/l|", false, "/list "),
+ a -> assertCompletion(a, "/list |", false, "all ", "history ", "start "),
+ a -> assertCompletion(a, "/list h|", false, "history "),
+ a -> assertCompletion(a, "/list q|", false),
+ a -> assertVariable(a, "int", "xray"),
+ a -> assertCompletion(a, "/list |", false, "1", "all ", "history ", "start ", "xray"),
+ a -> assertCompletion(a, "/list x|", false, "xray")
+ );
}
public void testDrop() {
- assertCompletion("/d|", false, "/drop ");
-
test(false, new String[] {"-nostartup"},
+ a -> assertCompletion(a, "/d|", false, "/drop "),
a -> assertClass(a, "class cTest {}", "class", "cTest"),
a -> assertMethod(a, "int mTest() { return 0; }", "()I", "mTest"),
a -> assertVariable(a, "int", "fTest"),
@@ -74,10 +80,9 @@
}
public void testEdit() {
- assertCompletion("/e|", false, "/edit ", "/exit ");
- assertCompletion("/ed|", false, "/edit ");
-
test(false, new String[]{"-nostartup"},
+ a -> assertCompletion(a, "/e|", false, "/edit ", "/exit "),
+ a -> assertCompletion(a, "/ed|", false, "/edit "),
a -> assertClass(a, "class cTest {}", "class", "cTest"),
a -> assertMethod(a, "int mTest() { return 0; }", "()I", "mTest"),
a -> assertVariable(a, "int", "fTest"),
--- a/langtools/test/jdk/jshell/EditorTestBase.java Thu Dec 10 09:24:25 2015 -0800
+++ b/langtools/test/jdk/jshell/EditorTestBase.java Thu Dec 10 23:27:06 2015 -0800
@@ -73,11 +73,11 @@
for (String edit : new String[] {"/e", "/edit"}) {
test(new String[]{"-nostartup"},
a -> assertCommand(a, edit + " 1",
- "| No definition or id named 1 found. See /classes /methods /vars or /list\n"),
+ "| No definition or id named 1 found. See /classes, /methods, /vars, or /list\n"),
a -> assertCommand(a, edit + " -1",
- "| No definition or id named -1 found. See /classes /methods /vars or /list\n"),
+ "| No definition or id named -1 found. See /classes, /methods, /vars, or /list\n"),
a -> assertCommand(a, edit + " unknown",
- "| No definition or id named unknown found. See /classes /methods /vars or /list\n")
+ "| No definition or id named unknown found. See /classes, /methods, /vars, or /list\n")
);
}
}
--- a/langtools/test/jdk/jshell/ReplToolTesting.java Thu Dec 10 09:24:25 2015 -0800
+++ b/langtools/test/jdk/jshell/ReplToolTesting.java Thu Dec 10 23:27:06 2015 -0800
@@ -26,6 +26,7 @@
import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -40,6 +41,7 @@
import jdk.internal.jshell.tool.JShellTool;
import jdk.jshell.SourceCodeAnalysis.Suggestion;
+import static java.util.stream.Collectors.toList;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertTrue;
@@ -48,6 +50,24 @@
public class ReplToolTesting {
private final static String DEFAULT_STARTUP_MESSAGE = "| Welcome to";
+ final static List<ImportInfo> START_UP_IMPORTS = Stream.of(
+ "java.util.*",
+ "java.io.*",
+ "java.math.*",
+ "java.net.*",
+ "java.util.concurrent.*",
+ "java.util.prefs.*",
+ "java.util.regex.*")
+ .map(s -> new ImportInfo("import " + s + ";", "", s))
+ .collect(toList());
+ final static List<MethodInfo> START_UP_METHODS = Stream.of(
+ new MethodInfo("void printf(String format, Object... args) { System.out.printf(format, args); }",
+ "(String,Object...)void", "printf"))
+ .collect(toList());
+ final static List<String> START_UP = Collections.unmodifiableList(
+ Stream.concat(START_UP_IMPORTS.stream(), START_UP_METHODS.stream())
+ .map(s -> s.getSource())
+ .collect(toList()));
private WaitingTestingInputStream cmdin = null;
private ByteArrayOutputStream cmdout = null;
@@ -190,18 +210,11 @@
classes = new HashMap<>();
imports = new HashMap<>();
if (isDefaultStartUp) {
- methods.put("printf (String,Object...)void",
- new MethodInfo("", "(String,Object...)void", "printf"));
+ methods.putAll(
+ START_UP_METHODS.stream()
+ .collect(Collectors.toMap(Object::toString, Function.identity())));
imports.putAll(
- Stream.of(
- "java.util.*",
- "java.io.*",
- "java.math.*",
- "java.net.*",
- "java.util.concurrent.*",
- "java.util.prefs.*",
- "java.util.regex.*")
- .map(s -> new ImportInfo("", "", s))
+ START_UP_IMPORTS.stream()
.collect(Collectors.toMap(Object::toString, Function.identity())));
}
}
--- a/langtools/test/jdk/jshell/ToolBasicTest.java Thu Dec 10 09:24:25 2015 -0800
+++ b/langtools/test/jdk/jshell/ToolBasicTest.java Thu Dec 10 23:27:06 2015 -0800
@@ -23,10 +23,10 @@
/*
* @test
- * @bug 8143037 8142447
+ * @bug 8143037 8142447 8144095 8140265
* @summary Tests for Basic tests for REPL tool
+ * @library /tools/lib
* @ignore 8139873
- * @library /tools/lib
* @build KullaTesting TestingInputStream ToolBox Compiler
* @run testng ToolBasicTest
*/
@@ -529,6 +529,7 @@
);
test(
(a) -> assertVariable(a, "int", "a"),
+ (a) -> assertCommand(a, "()", null, null, null, "", ""),
(a) -> assertClass(a, "class A { public String toString() { return \"A\"; } }", "class", "A"),
(a) -> assertCommand(a, "/save " + path.toString(), "")
);
@@ -537,6 +538,7 @@
List<String> output = new ArrayList<>();
test(
(a) -> assertCommand(a, "int a;", null),
+ (a) -> assertCommand(a, "()", null, null, null, "", ""),
(a) -> assertClass(a, "class A { public String toString() { return \"A\"; } }", "class", "A"),
(a) -> assertCommandCheckOutput(a, "/list all", (out) ->
output.addAll(Stream.of(out.split("\n"))
@@ -551,6 +553,7 @@
List<String> output = new ArrayList<>();
test(
(a) -> assertVariable(a, "int", "a"),
+ (a) -> assertCommand(a, "()", null, null, null, "", ""),
(a) -> assertClass(a, "class A { public String toString() { return \"A\"; } }", "class", "A"),
(a) -> assertCommandCheckOutput(a, "/history", (out) ->
output.addAll(Stream.of(out.split("\n"))
@@ -632,15 +635,7 @@
List<String> lines = Files.lines(startSave)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList());
- assertEquals(lines, Arrays.asList(
- "import java.util.*;",
- "import java.io.*;",
- "import java.math.*;",
- "import java.net.*;",
- "import java.util.concurrent.*;",
- "import java.util.prefs.*;",
- "import java.util.regex.*;",
- "void printf(String format, Object... args) { System.out.printf(format, args); }"));
+ assertEquals(lines, START_UP);
}
public void testConstrainedUpdates() {
@@ -665,15 +660,35 @@
);
}
+ // Check that each line of output contains the corresponding string from the list
+ private void checkLineToList(String in, List<String> match) {
+ String[] res = in.trim().split("\n");
+ assertEquals(res.length, match.size(), "Got: " + Arrays.asList(res));
+ for (int i = 0; i < match.size(); ++i) {
+ assertTrue(res[i].contains(match.get(i)));
+ }
+ }
+
public void testListArgs() {
- Consumer<String> assertList = s -> assertTrue(s.split("\n").length >= 4, s);
String arg = "qqqq";
- Consumer<String> assertError = s -> assertEquals(s, "| Invalid /list argument: " + arg + "\n");
+ List<String> startVarList = new ArrayList<>(START_UP);
+ startVarList.add("int aardvark");
test(
- a -> assertCommandCheckOutput(a, "/list all", assertList),
- a -> assertCommandCheckOutput(a, "/list " + arg, assertError),
- a -> assertVariable(a, "int", "a"),
- a -> assertCommandCheckOutput(a, "/list history", assertList)
+ a -> assertCommandCheckOutput(a, "/list all",
+ s -> checkLineToList(s, START_UP)),
+ a -> assertCommandCheckOutput(a, "/list " + arg,
+ s -> assertEquals(s, "| No definition or id named " + arg +
+ " found. There are no active definitions.\n")),
+ a -> assertVariable(a, "int", "aardvark"),
+ a -> assertCommandCheckOutput(a, "/list aardvark",
+ s -> assertTrue(s.contains("aardvark"))),
+ a -> assertCommandCheckOutput(a, "/list all",
+ s -> checkLineToList(s, startVarList)),
+ a -> assertCommandCheckOutput(a, "/list history",
+ s -> assertTrue(s.split("\n").length >= 4, s)),
+ a -> assertCommandCheckOutput(a, "/list " + arg,
+ s -> assertEquals(s, "| No definition or id named " + arg +
+ " found. Try /list without arguments.\n"))
);
}
@@ -806,13 +821,13 @@
public void testDropNegative() {
test(false, new String[]{"-nostartup"},
- a -> assertCommand(a, "/drop 0", "| No definition or id named 0 found. See /classes /methods /vars or /list\n"),
- a -> assertCommand(a, "/drop a", "| No definition or id named a found. See /classes /methods /vars or /list\n"),
+ a -> assertCommand(a, "/drop 0", "| No definition or id named 0 found. See /classes, /methods, /vars, or /list\n"),
+ a -> assertCommand(a, "/drop a", "| No definition or id named a found. See /classes, /methods, /vars, or /list\n"),
a -> assertCommandCheckOutput(a, "/drop",
assertStartsWith("| In the /drop argument, please specify an import, variable, method, or class to drop.")),
a -> assertVariable(a, "int", "a"),
a -> assertCommand(a, "a", "| Variable a of type int has value 0\n"),
- a -> assertCommand(a, "/drop 2", "| The argument did not specify an import, variable, method, or class to drop.\n")
+ a -> assertCommand(a, "/drop 2", "| The argument did not specify an active import, variable, method, or class to drop.\n")
);
}