REPL tool implementation code review
Paul Sandoz
paul.sandoz at oracle.com
Fri Sep 4 17:19:27 UTC 2015
Hi,
Here is the review of the REPL tool implementation.
Generally it all looks very good. I ran out of time to analyse the tests more closely. Quite a few comments are suggestions related to using newer APIs or techniques.
Paul.
Implementation:
jdk/internal/jshell/tool/JShellTool.java:91
> private static final String VERSION = "0.819”;
> …
> case "-version":
> cmdout.printf("jshell %s\n", VERSION);
> return null;
>
Should this version be the JDK version, as is the case for other Java tools?
jdk/internal/jshell/tool/JShellTool.java:128
> private IOContext input = null;
> private boolean regenerateOnDeath = true;
> private boolean live = false;
>
> SourceCodeAnalysis analysis;
> JShell state = null;
> Subscription shutdownSubscription = null;
>
> private boolean debug = false;
> private boolean displayPrompt = true;
> public boolean testPrompt = false;
> private Feedback feedback = Feedback.Default;
> private String cmdlineClasspath = null;
> private String cmdlineStartup = null;
> private String editor = null;
>
A small style thing, there is no need to initialize to default values (it’s also applied inconsistently)
jdk/internal/jshell/tool/JShellTool.java:307
> byte[] encoded = Files.readAllBytes(Paths.get(filename));
> cmdlineStartup = new String(encoded);
What character set is the file encoded in? should the platform's default charset be used or UTF-8? Same applies to processing the load list files.
jdk/internal/jshell/tool/JShellTool.java:582
> int lastSlash = code.lastIndexOf('/'); //XXX
Rogue comment.
jdk/internal/jshell/tool/JShellTool.java:587
> try (DirectoryStream<Path> dir = Files.newDirectoryStream(current)) {
> for (Path e : dir) {
> if (!accept.test(e) || !e.getFileName().toString().startsWith(prefix))
> continue;
> result.add(new Suggestion(e.getFileName() + (Files.isDirectory(e) ? "/" : ""), false));
> }
> } catch (IOException ex) {
> //ignore…
> }
See files.lines(path) that returns a stream with map.filter.collect(toCollection(ArrayList::new))
> if (path.isEmpty()) {
> for (Path root : FileSystems.getDefault().getRootDirectories()) {
> if (!accept.test(root) || !root.toString().startsWith(prefix))
> continue;
> result.add(new Suggestion(root.toString(), false));
> }
> }
Alas we don’t have a FileSystem.rootDirectories method that returns a stream.
jdk/internal/jshell/tool/JShellTool.java:645
> registerCommand(new Command("/list", "/l", "[all]", "list the source you have typed",
> arg -> cmdList(arg),
> new FixedCompletionProvider("all")));
You could use a method reference for "arg -> …”, same applies for other command declarations.
jdk/internal/jshell/tool/JShellTool.java:716
> public List<Suggestion> commandCompletionSuggestions(String code, int cursor, int[] anchor) {
> code = code.substring(0, cursor);
> int space = code.indexOf(' ');
> List<Suggestion> result = new ArrayList<>();
>
> if (space == (-1)) {
> for (Command cmd : new LinkedHashSet<>(commands.values())) {
> if (cmd.kind == CommandKind.HIDDEN || cmd.kind == CommandKind.HELP_ONLY)
> continue;
> String key = cmd.aliases[0];
> if (key.startsWith(code)) {
> result.add(new Suggestion(key + " ", false));
> }
> }
> anchor[0] = 0;
> } else {
> String arg = code.substring(space + 1);
> String cmd = code.substring(0, space);
> Command command = commands.get(cmd);
> if (command != null) {
> result.addAll(command.completions.completionSuggestions(arg, cursor - space, anchor));
> anchor[0] += space + 1;
> }
> }
>
> Collections.sort(result, (s1, s2) -> s1.continuation.compareTo(s2.continuation));
> return result;
> }
I believe you could use a stream here for both branches then do a sorted(…).collect(toList()) at the end of the method. For the first branch i think it might be something like values().distinct().filter(…).map(…).filter(…). For the second branch it is either an empty stream or the stream from the list.
FYI: you can also to List.sort now.
jdk/internal/jshell/tool/JShellTool.java:763
> if (arg == null || arg.isEmpty()) {
I dunno if it is possible but it would be nice to normalise to empty strings rather than checking for null or an empty string.
jdk/internal/jshell/tool/JShellTool.java:959
> Iterator<Snippet> it = keySet.iterator();
> while (it.hasNext()) {
> if (!(it.next() instanceof PersistentSnippet)) {
> it.remove();
> }
> }
See the bulk operation Collection.removeIf.
> PersistentSnippet key = (PersistentSnippet) keySet.iterator().next();
> state.drop(key).forEach(e -> handleEvent(e));
For kicks you could also do it like this (not suggesting you actually do unless you really want to, it’s just instructive for new APIs in Java 8):
keySet.stream().findFirst().
map(PersistentSnippet.class::cast).
ifPresent(k -> state.drop(k).forEach(this::handleEvent));
jdk/internal/jshell/tool/JShellTool.java:981
> private void cmdEdit(String arg) {
> Set<Snippet> keySet = argToKeySet(arg);
> if (keySet == null) {
> return;
> }
> Set<String> srcSet = new LinkedHashSet<>();
> for (Snippet key : keySet) {
> String src = key.source();
> switch (key.subKind()) {
> case VAR_VALUE_SUBKIND:
> break;
> case ASSIGNMENT_SUBKIND:
> case OTHER_EXPRESSION_SUBKIND:
> case TEMP_VAR_EXPRESSION_SUBKIND:
> if (!src.endsWith(";")) {
> src = src + ";";
> }
> srcSet.add(src);
> break;
> default:
> srcSet.add(src);
> break;
> }
> }
> StringBuilder sb = new StringBuilder();
> for (String s : srcSet) {
> sb.append(s);
> sb.append('\n');
> }
> String src = sb.toString();
There appears to be a reliance on an undefined iteration order, perhaps argToKeySet should return LinkedHashSet?
I think this might be convertible to a stream with filter(…).map(…).collect(joining(“\n”, “”, “\n"))
jdk/internal/jshell/tool/JShellTool.java:1150
> try (BufferedWriter writer = new BufferedWriter(new FileWriter(resolveUserHome(filename)))) {
See also Files.newBufferedWriter(…) i like that because it makes it more explicit whether you are appending to an existing file or not.
jdk/internal/jshell/tool/JShellTool.java:1266
> List<Diagnostic<String>> errorsOnly(List<Diagnostic<String>> diagnostics) {
> List<Diagnostic<String>> errors = new ArrayList<>();
> for (Diagnostic<String> diag : diagnostics) {
> switch (diag.getKind()) {
> case ERROR:
> errors.add(diag);
> break;
> case MANDATORY_WARNING:
> case WARNING:
> break;
> case NOTE:
> case OTHER:
> break;
> }
> }
> return errors;
> }
diagnostics.stream().filter(d -> d.getKind() == ERROR).collect(toList());
jdk/internal/jshell/tool/JShellTool.java:1710
> public ScannerIOContext(InputStream inStream, PrintStream pStream) {
> this(new Scanner(inStream), pStream);
> }
Unused constructor.
jdk/internal/jshell/tool/ExternalEditor.java:134
> private void saveFile() {
> List<String> lines;
> try {
> lines = Files.readAllLines(tmpfile);
> } catch (IOException ex) {
> errorHandler.accept("Failure read edit file: " + ex.getMessage());
> return ;
> }
> StringBuilder sb = new StringBuilder();
> for (String ln : lines) {
> sb.append(ln);
> sb.append('\n');
> }
> saveHandler.accept(sb.toString());
> }
Why don’t you just read as bytes then create a String from those bytes?
> if (!key.pollEvents().isEmpty()) {
> if (!input.terminalEditorRunning()) {
> saveFile();
> }
> }
Why does the watcher thread need to call saveFile when it is also performed after the process running the external editor has terminated? I am guessing every time the file is saved in the editor it is processed by the REPL?
jdk/internal/jshell/tool/EditPad.java:46
> public class EditPad extends JFrame implements Runnable {
> private final Consumer<String> errorHandler;
> private final String initialText;
> private final boolean[] closeLock;
> private final Consumer<String> saveHandler;
>
> EditPad(Consumer<String> errorHandler, String initialText,
> boolean[] closeLock, Consumer<String> saveHandler) {
> super("Edit Pad (Experimental)");
> this.errorHandler = errorHandler;
> this.initialText = initialText;
> this.closeLock = closeLock;
> this.saveHandler = saveHandler;
> }
Unused field errorHandle.
Is this still experimental? Is so should be it included?
jdk/internal/jshell/tool/EditPad.java:114
> private void notifyClose() {
> synchronized (closeLock) {
> closeLock[0] = true;
> closeLock.notify();
> }
> }
>
> public static void edit(Consumer<String> errorHandler, String initialText,
> Consumer<String> saveHandler) {
> boolean[] closeLock = new boolean[1];
> SwingUtilities.invokeLater(
> new EditPad(errorHandler, initialText, closeLock, saveHandler));
> synchronized (closeLock) {
> while (!closeLock[0]) {
> try {
> closeLock.wait();
> } catch (InterruptedException ex) {
> // ignore and loop
> }
> }
> }
> }
You could use a shared CountDownLatch, rather than notify/wait/synchronization with a boolean array of one element.
jdk/internal/jshell/tool/ConsoleIOContext.java:70
> TerminalFactory.registerFlavor(Flavor.WINDOWS, JShellWindowsTerminal :: new);
> TerminalFactory.registerFlavor(Flavor.UNIX, JShellUnixTerminal :: new);
The syntax for the constructor refs is a little unusual with the white space. Same further down when binding CRTL_UP/DOWN actions.
jdk/internal/jshell/tool/ConsoleIOContext.java:136
> .forEach(s -> { result.add(s); });
forEach(result::add);
> Optional<String> prefix =
> suggestions.stream()
> .map(s -> s.continuation)
> .reduce((s1, s2) -> commonPrefix(s1, s2));
.reduce(ConsoleIOContext::commonPrefix)
> try {
> Method setBuffer = in.getClass().getDeclaredMethod("setBuffer", String.class);
>
> setBuffer.setAccessible(true);
> setBuffer.invoke(in, in.getHistory().current().toString());
> in.flush();
> } catch (ReflectiveOperationException | IOException ex) {
> throw new IllegalStateException(ex);
> }
You might wanna comment more about the limitation in ConsoleReader and the necessity of calling a private method.
> private static String commonPrefix(String str1, String str2) {
> for (int i = 0; i < str2.length(); i++) {
> if (!str1.startsWith(str2.substring(0, i + 1))) {
> return str2.substring(0, i);
> }
> }
>
> return str2;
> }
What we need is a String mismatch method, from which to build a common prefix method, stay tuned….
jdk/internal/jshell/tool/ConsoleIOContext.java:505
> Set<Integer> snippetsStart = new HashSet<>();
> for (String start : prefs.get(HISTORY_SNIPPET_START, "").split(";")) {
> if (start.isEmpty())
> continue;
> snippetsStart.add(Integer.parseInt(start));
> }
Could streamify but perhaps not worth it. At least convert to
if(!start.isEmpty())
snippetsStart.add(Integer.parseInt(start));
> List<String> keys = new ArrayList<>(Arrays.asList(prefs.keys()));
> Collections.sort(keys);
Stream.of(prefs.keys()).sorted().collect(toList());
jdk/internal/jshell/tool/ConsoleIOContext.java:
I am struggling to understand the interactions of the before/afterUse code blocks, the CircularBuffer, and the thread created for writing (or pushing) read values to the buffer.
CircularBuffer feels like a blocking queue, but there is some additional communication signalling how much input is needed.
Tests
General point:
(a) -> …
Can be rewritten as:
a -> ...
ReplToolTesting.java:302
> private <T extends MemberInfo> void addKey(boolean after, T memberInfo, Map<String, T> map) {
> if (after) {
> for (Iterator<Entry<String, T>> iterator = map.entrySet().iterator(); iterator.hasNext(); ) {
> Entry<String, T> info = iterator.next();
> if (info.getValue().equals(memberInfo)) {
> iterator.remove();
> break;
> }
> }
> map.put(memberInfo.toString(), memberInfo);
> }
> }
Just do:
map.entrySet().removeIf(e -> e.getValue().equals(memberInfo));
ReplToolTesting.java:571
> public Consumer<String> checkOutput() {
> String fullType = type.equals("@interface")? "annotation interface" : type;
> String expectedOutput = String.format("\\| *\\w+ %s %s", fullType, name);
> Pattern outputPattern = Pattern.compile(expectedOutput);
> return (s) -> {
> Matcher matcher = outputPattern.matcher(s);
> assertTrue(matcher.find(), "Expected: '" + expectedOutput + "', actual: " + s);
> };
> }
See Pattern.asPredicate, capture the returned predicate in the lambda instead of the pattern and call predicate.test.
CommandCompletionTest.java:154
> private List<String> listFiles(Path path, DirectoryStream.Filter<? super Path> filter) throws IOException {
> List<String> paths = new ArrayList<>();
> try (DirectoryStream<Path> stream = Files.newDirectoryStream(path, filter)) {
> stream.iterator().forEachRemaining(p -> {
> String fileName = p.getFileName().toString();
> if (Files.isDirectory(p)) {
> fileName += "/";
> }
> paths.add(fileName);
> });
> }
> Collections.sort(paths);
> return paths;
> }
See Files.list which returns a Stream<Path>, you could probably reformulate using Predicate rather than DirectoryStream.Filter.
StartOptionTest.java:68
> private void start(Consumer<String> checkOutput, Consumer<String> checkError, String...args) throws Exception {
Space between … and parameter name. Same goes for other cases in this file.
More information about the kulla-dev
mailing list