Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REPL: upgrade to JLine 3 (benefits include multi-line editing) #8036

Merged
merged 5 commits into from Mar 25, 2020

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented May 6, 2019

JLine 3 supports multi-line editing, a better tab-completion UI, and more.

Configure keybindings with -Xjline:emacs (the default) or -Xjline:vi; disable with -Xjline:off

History file is now ~/.scala_history_jline3

scala-compiler.jar now depends on net.java.dev.jna#jna (instead of org.fusesource.jansi#jansi, as formerly).

Minimal changes to support new API, with basic completion and history.

Fixes scala/bug#11367. Rebased from #7645

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone May 6, 2019
@diesalbla diesalbla added the tool:REPL Changes in the Scala REPL Interpreter label May 19, 2019
@som-snytt

This comment has been minimized.

@SethTisue SethTisue added the WIP label Jun 18, 2019
@SethTisue
Copy link
Member

I salivate every time I run across this in the PR queue.

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jun 18, 2019
@adriaanm
Copy link
Contributor

Took care of one tiny little paper cut in the macro-annot tests. Sorry, I didn't mean to add myself as the committer for just a one line change, but don't know how to convey this to git.

@adriaanm adriaanm force-pushed the issue/jline3 branch 2 times, most recently from c9f4bf7 to 10260be Compare June 20, 2019 16:20
@som-snytt
Copy link
Contributor Author

I actually have a delta to push for this.

@adriaanm
Copy link
Contributor

Ok, I will hold off now that I have a ✅
Hope my push -f didn't cause any conflicts

@adriaanm
Copy link
Contributor

Actually, one more commit for the Reader header.

@som-snytt
Copy link
Contributor Author

Actually, that last force was it, so have at it. I have a sense that there are several things to do, but I didn't make a checklist.

@adriaanm
Copy link
Contributor

I didn't make a checklist.

I believe in fact you did, though in a different lifetime, as it were.

@adriaanm adriaanm removed the WIP label Jun 20, 2019
@adriaanm
Copy link
Contributor

I should add that I've been trying the REPL from this PR -- looks ready to ship in 2.13.1 to me! Really, really nice work. Multi-line history ftw!

@adriaanm
Copy link
Contributor

adriaanm commented Jul 1, 2019 via email

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just get this in so that it gets tested by those working on 2.13.x. Looking through the change at a high level, i only have one question, about the dependencies.

build.sbt Show resolved Hide resolved
@som-snytt
Copy link
Contributor Author

I guess Adriaan is in 👨 👶 🌫️, as in Karl the fog not as in head in the clouds, which also explains his heady enthusiasm, though we'd have to run some benchmarks to accurately compare to his previous levels.

I'll check out the previous commit and achieve minimal merge polish, with confidence that only @dwijnand is likely to complain about breakage.

@dwijnand
Copy link
Member

dwijnand commented Jul 2, 2019

What breakage am I likely to complain about? The only one that comes to mind is source-breakage of zinc's "console" abstraction of the REPL, which we'd have to address.

Other than that I don't have any PRs touching the same files (merge conflict breakage) or PRs looking to make changes to any behaviour here (semantics breakage). Did you mean the Zinc thing or am I forgetting something?

@som-snytt
Copy link
Contributor Author

Viewing my previous comment in Windows, I don't especially like 🌫 but maybe I'd like it more as a moustache.

Sorry @dwijnand let me add that adding your handle in FIrefox on Windows was painful, which was the whole reason we fought the browser wars.

Doubly sorry, now I don't remember what I was responding to. Please take my word that it was humorous to at least one of us, and that if I see a real issue in future I will report it.

@adriaanm
Copy link
Contributor

@SethTisue AFAICT the last action item before this can go into 2.13.1 is to see if we can restrict our additional dependencies to just jansi, and drop jna.

Even with my brain 🌩 starting to clear, I still think we should get this in 😁

@som-snytt
Copy link
Contributor Author

Tried it out. Did Jline completion always tab through alternatives? Some ordinary completion is broken, however, and will need repair.

(Actually I couldn't reproduce: c tab displayed cc and was stuck. Reminiscent of scala/bug#10914.)

I guess the val c: C used to be shown below the line as an explanatory result?

scala> class C
defined class C

scala> val c = new C
c: C = C@58a7dc4

scala> val c: C   // c tab

Also, manage jline logging and this condition:

Welcome to Scala 2.13.1-20190826-155501-3b73f99 (OpenJDK 64-Bit Server VM, Java 11.0.3).
Type in expressions for evaluation. Or try :help.
Aug 26, 2019 9:08:14 AM org.jline.utils.Log logr
WARNING: Failed to load history
java.lang.IllegalArgumentException: Bad history file syntax! The history file `/home/amarki/.scala_history` may be an older history: please remove it or use a different history file.
	at org.jline.reader.impl.history.DefaultHistory.addHistoryLine(DefaultHistory.java:185)
	at org.jline.reader.impl.history.DefaultHistory.addHistoryLine(DefaultHistory.java:169)
	at org.jline.reader.impl.history.DefaultHistory.lambda$load$0(DefaultHistory.java:86)
	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
	at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658)
	at org.jline.reader.impl.history.DefaultHistory.load(DefaultHistory.java:86)
	at org.jline.reader.impl.history.DefaultHistory.attach(DefaultHistory.java:69)
	at scala.tools.nsc.interpreter.jline.Reader$.apply(Reader.scala:94)
	at scala.tools.nsc.interpreter.shell.ILoop.defaultIn$lzycompute(ILoop.scala:71)
	at scala.tools.nsc.interpreter.shell.ILoop.defaultIn(ILoop.scala:66)
	at scala.tools.nsc.interpreter.shell.ILoop.run(ILoop.scala:953)
	at scala.tools.nsc.MainGenericRunner.runTarget$1(MainGenericRunner.scala:87)
	at scala.tools.nsc.MainGenericRunner.run$1(MainGenericRunner.scala:91)
	at scala.tools.nsc.MainGenericRunner.process(MainGenericRunner.scala:103)
	at scala.tools.nsc.MainGenericRunner$.main(MainGenericRunner.scala:108)
	at scala.tools.nsc.MainGenericRunner.main(MainGenericRunner.scala)

build.sbt Outdated Show resolved Hide resolved
@SethTisue
Copy link
Member

rebased and partially squashed. if all the commits pass CI, I think we could go ahead and merge this so that we'll all be testing it (and then address remaining blockers in followup PRs)

use a different filename than we used with JLine 2, since the format
is different (has timestamps) and because old files might have a
different character encoding, we encountered that in testing

also bring back code that restricts permissions on history file,
this went missing during the upgrade
based on review feedback from hvesalai
@lrytz
Copy link
Member

lrytz commented Mar 25, 2020

  • make REPL's JNA & JLine dependencies optional. most users of scala-compiler.jar just want the compiler and aren't embedding the REPL, so there's no need to pull REPL stuff onto their classpath (and doing so might even cause a conflict. if someone is embedding the REPL and does want JNA, they can take care of adding the optional dependency themselves.

I think this is too breaking, at least for JLine, but IMO also for JNA. The dependency on JLine is not optional, as we get

$> java -cp `cs fetch -p -r https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots org.scala-lang:scala-compiler:2.13.2-bin-7cb1bd9-SNAPSHOT` scala.tools.nsc.MainGenericRunner -usejavacp
Welcome to Scala 2.13.2-20200325-024252-7cb1bd9 (Java HotSpot(TM) 64-Bit GraalVM EE 19.2.1, Java 1.8.0_231).
Type in expressions for evaluation. Or try :help.
Exception in thread "main" java.lang.NoClassDefFoundError: org/jline/reader/Completer
	at scala.tools.nsc.interpreter.shell.ILoop.defaultIn$lzycompute(ILoop.scala:67)
        ...

The dependency for JNA is kind of optional. The REPL works fine without JNA on macOS after adding org.jline:jline:3.14.0 to cs fetch, I guess JLine uses a "child processes whenever the terminal is accessed", not sure if that comes with its own set of problems? On windows we get "creating a dumb terminal". That will likely break tools that start the repl on windows (including sbt? I didn't test).

Goal: avoid conflicting with sbt's own Jansi version

Ref scala/scala-dev#678

https://github.com/jline/jline3#jansi-vs-jna says

> On the Windows platform, relying on native calls is mandatory, so
> you need to have either Jansi or JNA library in your classpath along
> with the `jline-terminal-jansi` or `jline-terminal-jna` jar.

When I tested on Windows 7 or Windows 10 using `sbt console` I got
"WARNING: Unable to create a system terminal, creating a dumb terminal
(enable debug logging for more information)" and key input also didn't
work. (This could be more to do with sbt). I am guessing that Jansi
used by sbt itself maybe interfering with Jansi that gets loaded from
the compiler. Normally these duplicates are resolved by creating a
sandbox ClassLoader, but maybe it doesn't exactly work for native
libraries loaded into the same process.

additional changes by Seth:
* change Jansi -> JNA in legal info, Intellij config

Co-authored-by: Seth Tisue <seth@tisue.net>
@SethTisue
Copy link
Member

I think this is too breaking, at least for JLine, but IMO also for JNA

Okay, I've changed it back to regular dependencies, as the conservative choice, in the interest of getting this merged. (Eugene, if you think one or both dependencies should be optional, you could PR that separately and argue for it.)

We could consider it (we already consider it!) future work to move the whole REPL front end into its own JAR, and then put the JLine and JNA dependencies on that (instead of scala-compiler.jar).

@SethTisue SethTisue changed the title Use JLine3 REPL: upgrade to JLine 3 (benefits include multi-line editing) Mar 25, 2020
@SethTisue
Copy link
Member

SethTisue commented Mar 25, 2020

merging, based on Lukas's re-approval today

remaining blockers for 2.13.2

@SethTisue SethTisue merged commit bc1cad4 into scala:2.13.x Mar 25, 2020
@eed3si9n
Copy link
Member

Eugene, if you think one or both dependencies should be optional, you could PR that separately and argue for it.

If we are ok with I'm fine with that too. For the tooling maintainers, it would be good to document the full dependency JARs (both for 2.13.2 and 2.13.1) somewhere before we forget all this.

@som-snytt
Copy link
Contributor Author

Forget what? 😺

@som-snytt som-snytt deleted the issue/jline3 branch March 25, 2020 19:50
@SethTisue
Copy link
Member

For the tooling maintainers, it would be good to document the full dependency JARs (both for 2.13.2 and 2.13.1) somewhere before we forget all this

To the PR description above, I have now added:

scala-compiler.jar now depends on net.java.dev.jna#jna (instead of org.fusesource.jansi#jansi, as formerly).

I'm not sure what else you might have in mind that we should document?

@bishabosha
Copy link
Member

When i try to use :paste with the following

abstract class D { val d: Any; type T }

class DDep[D2 <: (D { type T = d.type }) { val d: Int }] {
  def test_==(d: D2)(t: d.d.type): Boolean = d.d == t
}

it formats pretty strangely:

scala> :paste
// Entering paste mode (ctrl-D to finish)

abstract class D { val d: Any; type T }
     | 
     | class DDep[D2 <: (D { type T = d.type }) { val d: Int }] {
     |   def test_==(d: D2)(t: d.d.type): Boolean = d.d == t
     | }

@eed3si9n
Copy link
Member

I'm not sure what else you might have in mind that we should document?

I was thinking somewhere more permanent like readme or scala-lang site, describing why/when JNA is needed.

@som-snytt
Copy link
Contributor Author

@bishabosha there was a "to do" for migrating the old line continuation code, but I can't find a ticket. Probably it's better to create an issue than comment here. (Some tickets are on scala-dev.)

@SethTisue
Copy link
Member

@bishabosha thanks for the report; opened scala/scala-dev#703 and marked it "blocker"

@som-snytt you're probably thinking of scala/scala-dev#689 ("Excise old continuation handling" — I'd assumed that was just cleanup, but now we have a case where the old code is actually interfering)

@SethTisue
Copy link
Member

I was thinking somewhere more permanent like readme or scala-lang site, describing why/when JNA is needed.

@eed3si9n suggestion recorded at https://github.com/scala/scala-dev/issues/685

@SethTisue
Copy link
Member

SethTisue commented Apr 5, 2020

#8848 sands down some rough spots in the behavior

UPDATE (Apr 21): also #8880, #8905

@twome

This comment was marked as spam.

@SethTisue

This comment was marked as resolved.

@twome

This comment was marked as spam.

@som-snytt

This comment was marked as resolved.

@scala scala locked as spam and limited conversation to collaborators Jul 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-notes worth highlighting in next release notes tool:REPL Changes in the Scala REPL Interpreter
Projects
None yet