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

Use JLine3 #8036

Open
wants to merge 6 commits into
base: 2.13.x
from
Open

Use JLine3 #8036

wants to merge 6 commits into from

Conversation

@som-snytt
Copy link
Contributor

som-snytt commented May 6, 2019

Minimal changes to support new API,
with basic completion and history.
JLine handles continuation line edit.

Rebased but not reworked from #7645

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone May 6, 2019
@diesalbla diesalbla added the tool:REPL label May 19, 2019
@som-snytt som-snytt force-pushed the som-snytt:issue/jline3 branch from 8cc69ad to 8e15296 May 28, 2019
@som-snytt

This comment was marked as outdated.

Copy link
Contributor Author

som-snytt commented May 29, 2019

/rebuild

@SethTisue SethTisue added the WIP label Jun 18, 2019
@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Jun 18, 2019

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

@adriaanm adriaanm force-pushed the som-snytt:issue/jline3 branch from 8e15296 to 1363229 Jun 20, 2019
@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Jun 20, 2019

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 som-snytt:issue/jline3 branch 2 times, most recently from c9f4bf7 to 10260be Jun 20, 2019
@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Jun 20, 2019

I actually have a delta to push for this.

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Jun 20, 2019

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

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Jun 20, 2019

Actually, one more commit for the Reader header.

@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Jun 20, 2019

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

This comment has been minimized.

Copy link
Member

adriaanm commented Jun 20, 2019

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

This comment has been minimized.

Copy link
Member

adriaanm commented Jun 20, 2019

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

This comment has been minimized.

Copy link
Member

adriaanm commented Jul 1, 2019

@lrytz
lrytz approved these changes Jul 1, 2019
Copy link
Member

lrytz left a comment

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 Outdated
val jlineDep = "jline" % "jline" % versionProps("jline.version")
val jlineDep = "org.jline" % "jline" % versionProps("jline.version")
val jansiDep = "org.fusesource.jansi" % "jansi" % versionProps("jansi.version")
val jnaDep = "net.java.dev.jna" % "jna" % versionProps("jna.version")

This comment has been minimized.

Copy link
@lrytz

lrytz Jul 1, 2019

Member

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

If one of the Jansi or JNA library is present, it will be used and JLine will use native calls

There is no difference between JLine's support for Jansi and JNA. Both will provide the exact same behaviors. So it's a matter of preference

So do we need both?

This comment has been minimized.

Copy link
@som-snytt

som-snytt Jul 1, 2019

Author Contributor

I remember I tried the simple thing and it didn't work, but that was a long time ago. I'll try to refresh some memory cells and also experiment.

This comment has been minimized.

Copy link
@adriaanm

adriaanm Jul 24, 2019

Member

It looks like coursier uses just jansi -- maybe this can inspire?

@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Jul 1, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Jul 12, 2019

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.

som-snytt and others added 2 commits Aug 14, 2018
Minimal changes to support new API,
with basic completion and history.
JLine handles continuation line edit.
Add header to repl-frontend/scala/tools/nsc/interpreter/jline/Reader.scala

Still display parse errors
@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Aug 26, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Aug 26, 2019

Also relevant is the todo for separate REPL jar, and whether to use the "fine-grained" jline jars.


private def bool(name: String) = BooleanProp.keyExists(name)
private def int(name: String) = Prop[Int](name)

// This property is used in TypeDebugging. Let's recycle it.
val colorOk = Properties.coloredOutputEnabled

val historyFile = s"$userHome/.scala_history"

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Aug 26, 2019

Member

What do you think about picking a different name for this file, so we can keep using the old file name for older Scala version's REPL.

This comment has been minimized.

Copy link
@som-snytt

som-snytt Aug 26, 2019

Author Contributor

We can disable history timestamps for jline and share the (jline default) history file. There is no -D for history file name; 2.12 FileBackedHistory had -Dscala.shell.histfile=.

This comment has been minimized.

Copy link
@som-snytt

som-snytt Aug 27, 2019

Author Contributor

Yes, i.e., this was a quick hack. I don't know if there is a use case for timestamped REPL lines, but what people would want is versioned history. "I've been futzing with these type parameters for an hour, show me all the things I've tried so far." Or show me the versions that compiled and produced a result.

som-snytt added 2 commits Aug 26, 2019
@som-snytt som-snytt force-pushed the som-snytt:issue/jline3 branch from 760ca96 to 4346a44 Aug 26, 2019
@eed3si9n

This comment has been minimized.

Copy link
Member

eed3si9n commented Aug 26, 2019

Testing this PR on Windows 7 VM, using Command Prompt (as opposed to Cygwin) and sbt 1.2.8, on console task, I get:

WARNING: Unable to create a system terminal, creating a dumb terminal (enable debug logging for more information)

unable-to-create

I've attempted to get the debug logging by passing in logging.properties file, but it didn't seem to work. If I directly call build\quick\bin\scala from Command Prompt instead of going through sbt, it works as expected.

This could mean that right JARs are not on the Classloader on sbt's side, or JAnsi versions are interfering (JLine 2 insources JAnsi).

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 27, 2019

This basically works on my Mac (on both JDK 8 and 12, and both when run directly or using sbt's console), but as Som has already noted, the behavior when you hit the TAB key more than once is a regression. It seems like a blocker to me. (I expect to have time to help with it before too much longer, but perhaps not this week.)

Even once we get that straightened out, and the Windows thing straightened out, I would still suggest we hold this for 2.13.2, and let 2.13.1 be a bugfix release. I don't think we should risk getting into a situation where people want & need the 2.13.1 bugfixes but have to weigh that against REPL issues. This stuff is highly platform-dependent and environment-dependent and thus not very amenable to automated testing, so it's especially likely to have unexpected breakage despite all of our best efforts.

@@ -89,7 +89,7 @@ trait PresentationCompilation { self: IMain =>

private var lastCommonPrefixCompletion: Option[String] = None

abstract class PresentationCompileResult(val compiler: interactive.Global, val inputRange: Position, val cursor: Int, val buf: String) extends PresentationCompilationResult {
private abstract class PresentationCompileResult(val compiler: interactive.Global, val inputRange: Position, val cursor: Int, val buf: String) extends PresentationCompilationResult {

This comment has been minimized.

Copy link
@retronym

retronym Aug 27, 2019

Member

I intended this to be usable by tools build on top of the REPL (web UIs for instance) that need to be tightly coupled to compiler internals but want the convenience of this API. So I'd suggest to leave it public.

This comment has been minimized.

Copy link
@som-snytt

som-snytt Aug 27, 2019

Author Contributor

OK thanks. I assume I found it confusing because of the interface. All the names will be ironed out in the REPL protocol.

.completer(completer)
.history(history)
.parser(parser)
.terminal(terminal)

This comment has been minimized.

Copy link
@retronym

retronym Aug 27, 2019

Member

In our usage of JLine2, we picked up configuration from ~/.inputrc etc.

JLine3 does have support for parsing this file since jline/jline3@9ccfe0b, but it looks like we need to call InputRC.configure after determining which config file to use ourselves.

Furthermore, in the current configuration file, sections guarded with $if JLine .... $endif are interpreted by the Scala REPL. Under this change, config would need to be moved to $if scala (the appname used here. That's probably fine if release noted.

Here's my .inputrc for reference.

$if Bash
	"\e[A": history-search-backward
	"\e[B": history-search-forward
    Space: magic-space
$endif

#set blink-matching-paren on
"\C-xq": "\eb\"\ef\""

"\C-x)": "\ea\(\ee\)"

$if JLine
	"\C-xu": "import scala.tools.reflect.ToolBox; val m = scala.reflect.runtime.currentMirror; val u = m.universe; import u._; val tb = m.mkToolBox()"
	"\C-x)": "\C-a(\C-e)"
	"\C-x}": "\C-a{\C-e}"
	"\C-x{": "{}\C-b"
	"\C-x(": "()\C-b"

$endif

This comment has been minimized.

Copy link
@som-snytt

som-snytt Aug 27, 2019

Author Contributor

Thanks. I know there are tickets that turn on local .inputrc but this didn't come up.

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Aug 27, 2019

Previously: println<TAB><TAB> would show the definition string for the methods with names that exactly matched.

In the JLine3 repl, this suggestion shows up after a single <TAB>, and additional <TAB> lets cycles through those options with ZSH-like menu completion. Selecting one puts the def string into the buffer which isn't what we're after.

It was a hack to display this help through JLine2's completion system so we should rethink our UI options here with JLine3.

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Aug 27, 2019

Completion suggestions have changed when the cursor is midway through an identfier.

Before:

scala> 1.toD<TAB>XXXX
toDegrees   toDouble

scala> 1.toD<TAB>ouble
toDegrees   toDouble

This PR:

scala> 1.toD<TAB>XXXX. // no completions

scala> 1.toD<TAB>ouble. // "ouble" is completed
def backupHistory(): Unit = {
import java.nio.file.{Files, Paths, StandardCopyOption}, StandardCopyOption.REPLACE_EXISTING
val hf = Paths.get(config.historyFile)
val bk = Paths.get(config.historyFile + ".bk")

This comment has been minimized.

Copy link
@retronym

retronym Aug 27, 2019

Member

When creating the history file, we change its permissions to 600. This is a security measure for multi-user systems that don't secure the $HOME by default (I'm looking at you OS/X and Windows).

We should do the same with the backup and the new copy of the history file.

This comment has been minimized.

Copy link
@som-snytt

som-snytt Aug 27, 2019

Author Contributor

Yes, this was a quick hack.

@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Aug 27, 2019

I'll make an honest effort to steer back to previous functionality. Today I got a fresh checkout of jline as a sign of good faith. It sounds like this doesn't need to be done by morning.

But Seth has gone around with a bib for several months now.

@szeiger

This comment has been minimized.

Copy link
Contributor

szeiger commented Aug 27, 2019

This doesn't work for me on Windows under MSYS:

$ ./build/quick/bin/scala -nc
Welcome to Scala 2.13.1-20190826-185208-4346a44 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_121).
Type in expressions for evaluation. Or try :help.
Aug 27, 2019 4:58:38 PM org.jline.utils.Log logr
WARNING: Unable to retrieve infocmp for type msys
java.io.IOException: Cannot run program "infocmp.exe": CreateProcess error=2, The system cannot find the file specified
        at java.lang.ProcessBuilder.start(Unknown Source)
        at org.jline.utils.InfoCmp.getInfoCmp(InfoCmp.java:547)
        at org.jline.terminal.impl.AbstractTerminal.parseInfoCmp(AbstractTerminal.java:187)
        at org.jline.terminal.impl.AbstractWindowsTerminal.<init>(AbstractWindowsTerminal.java:91)
        at org.jline.terminal.impl.jansi.win.JansiWinSysTerminal.<init>(JansiWinSysTerminal.java:76)
        at org.jline.terminal.impl.jansi.win.JansiWinSysTerminal.createTerminal(JansiWinSysTerminal.java:67)
        at org.jline.terminal.impl.jansi.JansiSupportImpl.winSysTerminal(JansiSupportImpl.java:129)
        at org.jline.terminal.TerminalBuilder.doBuild(TerminalBuilder.java:345)
        at org.jline.terminal.TerminalBuilder.build(TerminalBuilder.java:259)
        at org.jline.terminal.TerminalBuilder.terminal(TerminalBuilder.java:75)
        at scala.tools.nsc.interpreter.jline.Reader$.apply(Reader.scala:58)
        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)
Caused by: java.io.IOException: CreateProcess error=2, The system cannot find the file specified
        at java.lang.ProcessImpl.create(Native Method)
        at java.lang.ProcessImpl.<init>(Unknown Source)
        at java.lang.ProcessImpl.start(Unknown Source)
        ... 19 more
@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Aug 27, 2019

I'll add an issue that REPL doesn't report deprecated options such as -nc. Edit: nm, it warns during the session. On every line of input. Maybe I'll add an issue.

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Aug 28, 2019

Another change in completion behaviour:

scala> "".getClass.getDM<TAB>

Used to expand to:

scala> "".getClass.getDeclaredMethod
getDeclaredMethod   getDeclaredMethods

But now offers no completions. AFAICT JLine is filtering candidates on our behalf. without an obvious extension point to add our own strategy.

@szeiger szeiger modified the milestones: 2.13.1, 2.13.2 Sep 3, 2019
@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Sep 6, 2019

TIL on 2.12 scala> java.util.concurrent.Execututors. unexpectedly offers completions for the misspelled member but repl doesn't coordinate to correct the spelling.

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Jan 31, 2020

It would be really nice to have this in 2.13.2! (4-5 weeks)

@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Jan 31, 2020

I looked at this recently, trying to close out PRs, and it would have been nice in 2.13.1 but is depressing to revisit again. I need it so that companions entered on different lines drops you into a multiline edit. But otherwise, the creaky old REPL won't move any needles.

SethTisue added 2 commits Feb 5, 2020
Scala 2.13.1
@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Feb 5, 2020

the next step here is to merge or rebase with #8600 — that's where the merge conflicts start.

As a new baseline, I've pushed merges that bring it to immediately before that point. (And I checked that the REPL still functions.)

@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Feb 5, 2020

Oh man, why do people keep messing with REPL...

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Feb 5, 2020

@som-snytt I'm just getting started with helping with this. more to come. I'm tasked with this now, it's no longer on an if-I-ever-get-around-to-it basis

@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Feb 5, 2020

I was just joking about how my old PRs keep stomping on my new PRs, clarifying for @dwijnand . Or maybe it's the other way around.

I'm not sure how best to help. I can spool up again.

It's like when Arrow says "Suit up!" except it's, "Spool up!"

@som-snytt

This comment has been minimized.

Copy link
Contributor Author

som-snytt commented Feb 5, 2020

I only hope they won't ask me to test it on windoze.

@martijnhoekstra

This comment has been minimized.

Copy link
Contributor

martijnhoekstra commented Feb 5, 2020

I only hope they won't ask me to test it on windoze.

I'm happy to test (and try to fix if broken) on windoze

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.