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

Add readline for the compiler #11340

Closed
straight-shoota opened this issue Oct 19, 2021 · 41 comments
Closed

Add readline for the compiler #11340

straight-shoota opened this issue Oct 19, 2021 · 41 comments

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Oct 19, 2021

In interpreted mode (#11159), the compiler includes a REPL interface. For this we need a readline-like interface to provide support for typical console interaction such as cursor control.

There had previously been a Readline module in stdlib but it was removed in #8364 in order to keep the standard library slim (#5215). It has subsequently been moved to a shard at https://github.com/crystal-lang/crystal-readline.

We could consider reintroducing the module to stdlib. But since it's only used for the compiler, it would be a good candidate for pulling in the shard as a lib dependency. We already do this with markd for Markdown rendering (#11040) and I think it should work great with readline as well.

The shard could definitely use some polishing, but in the long run I wouldn't expect frequent changes to the code base.

There is however a problem with portability. As far as I am aware, libreadline is only compatible with POSIX platforms and it doesn't work on native Windows. There is a port of the GnuWin32 project, but it doesn't appear to be actively maintained (http://gnuwin32.sourceforge.net/packages/readline.htm last release is 5.0 from 2008).
Even though we don't have full Windows support yet, we have to keep compatibility in mind.

Of course, we could just consider using a different library on windows. But it would certainly be helpful if we could use the same interface on any platform. And this is not just for the compiler, but any Crystal project using readline-like functionality would benefit from portability.

A popular alternative to libreadline is libedit which provides a very similar interface. On MacOS, libreadline is actually just a wrapper around libedit (thus there can be slightly different behaviour on MacOS and Linux). A Windows port is available at http://mingweditline.sourceforge.net/

Alternatively, we could consider running our own implementation. This would eliminate all portability and dependency issues. Some programming languages such as Python and Java (those I'm aware of) ship their own implementations.

A Ruby implementation is available at https://github.com/ConnorAtherton/rb-readline. It seems to be used as readline implementation by Rubinius. It could serve as a basis for a native Crystal implementation.

By a quick count, rb-readline has about 9K LOC, libedit 20K LOC, libreadline 30K LOC.

@docelic
Copy link
Contributor

docelic commented Oct 19, 2021

Just as an idea maybe: https://github.com/Papierkorb/fancyline

@asterite
Copy link
Member

Nice! Running cloc in the src directory of fancyline:

-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Crystal                         17            233            340           1189
-------------------------------------------------------------------------------
SUM:                            17            233            340           1189
-------------------------------------------------------------------------------

@straight-shoota
Copy link
Member Author

@docelic Fancyline could indeed be a feasible option. Thanks for brining it up.

It is obviously not as mature and battle-tested as other implementations. This kind of software is really hard because it needs to adapt to many quirks of terminal emulators and that's hard to formalize and test.
It doesn't work on Windows either (yet). That can probably be improved, but I don't know how much effort that's going to be.

@vlazar
Copy link
Contributor

vlazar commented Oct 20, 2021

Could MPL-2.0 License used by fancyline be an issue? https://tldrlegal.com/license/mozilla-public-license-2.0-%28mpl-2%29

E.g. this line

You can distribute binaries under a proprietary license, as long as you make the source available under MPL.

Does it mean making the whole proprietary source available or just the MPL part?

@asterite
Copy link
Member

I think this library should be internal to the compiler and only have the features needed for the interpreter. Then it doesn't matter if it's battle tested for different scenarios. We only need it for the interpreter.

The Windows version could ship without readline. I added readline very late to the interpreter, so it's absolutely not needed for it.

@asterite
Copy link
Member

With that in mind we could use libreadline or libedit for Unix, and nothing or something else for Windows.

@straight-shoota
Copy link
Member Author

I think this library should [...] only have the features needed for the interpreter. Then it doesn't matter if it's battle tested for different scenarios.

The magnitude of scenarios doesn't necessarily come from using specific (enhanced) features. Of course, reducing that is a factor which can keep things simple.

But the most tricky thing is ensured compatibility across all weird kinds of terminal emulators and their quirks. The problem is that they show a lot of different behaviour and a readline-like library needs to work with all the flavours in order to provide a single, portable API. That's where it matters to be battle tested.

@straight-shoota
Copy link
Member Author

If we can treat readline functionality as optional, it's probably fine to ignore Windows support for now.

What exactly is it used for in the interpreter?
I suppose it's to allow more editing features in the REPL. Such as moving the cursor inside the prompt line and providing command history. I suppose that's not essential, even though it improves UX a lot. But we should be fine without it on incompatible platforms.

@asterite
Copy link
Member

Right now it's just for moving the cursor. I think also history.

The Idris repl doesn't have readline.

There's a program that will wrap any other program with readline, I can't remember the name.

Readline is totally optional and I would suggest spending zero time in this if we want to move forward with the interpreter. Removing readline for now and adding it much later could be a good step forward.

@straight-shoota
Copy link
Member Author

rlwrap is a program that wraps any other program with readline: https://github.com/hanslub42/rlwrap

@asterite
Copy link
Member

That's the one!

@beta-ziliani
Copy link
Member

+1 to removing readline for the interpreter and resort to rlwrap as alternative. If we find the need for more fancy input, we can always consider alternatives.

@straight-shoota
Copy link
Member Author

To be clear: I don't think rlwrap runs on Windows (remember it's just a wrapper for readline!). So it can't be an alternative option for Windows. If we get that to run, we can probably get it to work in the compiler directly.
Maybe we can find something similar for windows, but at that point I'd rather try to integrate it into the compiler directly instead of requiring a wrapper.

rlwrap could theroretically be an alternative option for Unix platforms if (for probably obscure reasons) the lib itself is not available, but the application is. Or if the compiler was built without readline support, you can add it at runtime. But I don't think those are common use cases. And for the latter it would be a better solution to make libreadline an optional runtime dependency via dlopen. The compiler has this capability anyways for interpreted mode.

@n-rodriguez
Copy link

https://github.com/ruby/reline full ruby

@n-rodriguez
Copy link

@oprypin what's wrong with reline?

@oprypin
Copy link
Member

oprypin commented Oct 23, 2021

@n-rodriguez It's written in a programming language that's not viable to integrate into Crystal distribution. So I'm not sure why you mention it.

@n-rodriguez
Copy link

Alternatively, we could consider running our own implementation. This would eliminate all portability and dependency issues. Some programming languages such as Python and Java (those I'm aware of) ship their own implementations.

@n-rodriguez
Copy link

n-rodriguez commented Oct 23, 2021

Crystal could ship its own implementation based on reline (just an idea).

@rdp
Copy link
Contributor

rdp commented Dec 22, 2021

https://stackoverflow.com/questions/17982633/lightweight-gnu-readline-alternative may be useful (basically: libedit, replxx). Or maybe you could put a fancy UI on it like crystal play? :) Not sure what that would look like... :)

@hugopl
Copy link
Contributor

hugopl commented Dec 24, 2021

Or maybe you could put a fancy UI on it like crystal play? :) Not sure what that would look like... :)

Probably not good 😛 , better keep with the terminal :-)

@watzon
Copy link
Contributor

watzon commented Jan 7, 2022

I assume this would be the fix for what currently happens in Windows Terminal and some other terminals (like Hyper)? Currently backspace doesn't work (the characters being deleted are printed instead in reverse) and I'm sure there are other issues too. This is also a problem with ICR though.

@straight-shoota
Copy link
Member Author

@watzon That sounds more like an issue with the terminal emulator than what's received by the acting program. I presume basic editing capabilities such as writing forward and deleting backward should be supported by any terminal.

@watzon
Copy link
Contributor

watzon commented Jan 7, 2022

Apparently it was an stty issue. Running the command stty sane to reset stty to it's default config fixed things. No idea what messed it up to begin with.

@paulocoghi
Copy link

@I3oris

@straight-shoota
Copy link
Member Author

straight-shoota commented Feb 7, 2022

While I recently realized that the compiler accidentally links libxml2 through libllvm, I also noticed that via the same dependency path, it already links libedit (and even curses). So I suppose we could just use that in the interpreter 🤷

That is, on my system libllvm links those libraries. They are optional dependencies so libllvm might not be linked against them on some systems. But we could just use libedit as an optional dependency as well.

@hugopl
Copy link
Contributor

hugopl commented Feb 8, 2022

on ArchLinux libedit is already there too, I guess this is true for most linux distros as well

$ ldd -r /bin/crystal
	linux-vdso.so.1 (0x00007fff831ce000)
	libLLVM-13.so => /usr/lib/libLLVM-13.so (0x00007f7690f0e000)
	libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x00007f7690cf8000)
	libpcre.so.1 => /usr/lib/libpcre.so.1 (0x00007f7690c81000)
	libm.so.6 => /usr/lib/libm.so.6 (0x00007f7690b3d000)
	libgc.so.1 => /usr/lib/libgc.so.1 (0x00007f7690ad0000)
	libpthread.so.0 => /usr/lib/libpthread.so.0 (0x00007f7690aaf000)
	libevent-2.1.so.7 => /usr/lib/libevent-2.1.so.7 (0x00007f7690a54000)
	libdl.so.2 => /usr/lib/libdl.so.2 (0x00007f7690a4d000)
	libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x00007f7690a32000)
	libc.so.6 => /usr/lib/libc.so.6 (0x00007f7690866000)
	/lib64/ld-linux-x86-64.so.2 => /usr/lib64/ld-linux-x86-64.so.2 (0x00007f76982dc000)
	libffi.so.8 => /usr/lib/../lib/libffi.so.8 (0x00007f769085a000)
	libedit.so.0 => /usr/lib/../lib/libedit.so.0 (0x00007f769081f000)
	libz.so.1 => /usr/lib/../lib/libz.so.1 (0x00007f7690803000)
	libncursesw.so.6 => /usr/lib/../lib/libncursesw.so.6 (0x00007f769078f000)
	libxml2.so.2 => /usr/lib/../lib/libxml2.so.2 (0x00007f7690605000)
	libicuuc.so.70 => /usr/lib/../lib/libicuuc.so.70 (0x00007f7690409000)
	liblzma.so.5 => /usr/lib/../lib/liblzma.so.5 (0x00007f76903e0000)
	libicudata.so.70 => /usr/lib/../lib/libicudata.so.70 (0x00007f768e7c2000)

@paulocoghi
Copy link

I don't know if we can learn something from the Interactive Crystal Project from I3oris, on github.com/I3oris/ic, which seems to have already resolved these issues (?)

@zw963
Copy link
Contributor

zw963 commented Sep 23, 2022

Just for confirm, @straight-shoota , if this issue can solve following issue?

When start crystal i in linux console, following hotkey not working. (Ctrl+A, Ctrl+E, Ctrl+B, Ctrl+F ...)

crystal> hello^A^A^E^B^F

Instead should moving cursor, it print a controller char.

@I3oris
Copy link
Contributor

I3oris commented Sep 24, 2022

@paulocoghi, so sorry i never see your messages on this thread (I discovered some month ago all my notifications was disabled!)

Work I've done for IC could be suitable for the compiler, or not..
It would require the following:

  • Window compatibility is not tested, well, now crystal is easy installable by scoop i should be able to test that.
  • Unicode characters not working well. The main difficulty is to know the print size of a char. This require at least a dependency like (https://github.com/naqvis/uni_text_seg)
  • The work need to be properly extracted from IC, to not include all feature like auto-completion, but just reading purposes. A dedicated shard should be feasible.
  • Need someone to maintain such shard, there is of course me, but currently I've have limited time to give to crystal. So I might be not as reliable as readline or libedit who have been heavily tested and are mature. In a other hand, I guess maintenance for that would not be huge, (largely feasible to one person).

I would be very glad to work one these points and integrate that to crystal i though, I just wonder it the compromise worth comparing to use libedit or other, in the point of view of the crystal team :)

@zw963, yes replacing readline would for sure fix this kind of issue.

@asterite
Copy link
Member

@I3oris does your shard implement readline functionality and auticompletion without relying on a C library? If so, to me that sounds more desire able that bonding to a C library (in my opinion)

@I3oris
Copy link
Contributor

I3oris commented Sep 24, 2022

Yes it does!

@I3oris
Copy link
Contributor

I3oris commented Sep 24, 2022

To be more precise, it relies only on ANSI escape (https://github.com/I3oris/ic/blob/master/src/term_cursor.cr) and a ioctl call
(https://github.com/I3oris/ic/blob/master/src/term_size.cr), and as I say, maybe (https://github.com/naqvis/uni_text_seg) in the future.

@I3oris
Copy link
Contributor

I3oris commented Sep 24, 2022

Sorry, wanted to talk about https://github.com/naqvis/uni_char_width not its dependency uni_text_seg.

It gives the size displayed on screen rather than char size or bytesize e.g. 💎=2.

This is required because the cursor you see should correspond to cursor on internal string.
If we have that: a💎b and the cursor at pos=1 (💎),
to move on the b we need to move by one char on the string, (or 4 bytes), but by 2 on the screen "\e[2C".
I believe its not possible to know this size in stdlib yet?
However I don't know if this lib take care user terminal, and problems you mention @straight-shoota . This makes the problem much harder!

@zw963
Copy link
Contributor

zw963 commented Sep 24, 2022

When starting use debugger in Crystal, really hope we can support at history feature(C-p, C-n), typing same expression again and again, quite boring.

@straight-shoota
Copy link
Member Author

straight-shoota commented Sep 26, 2022

Using a native-Crystal implementation similar to libreadline seems to be a good path forward.

https://github.com/Papierkorb/fancyline has already been mentioned. I haven't looked closely at it and do not know if it is already sufficient for the REPL use case or might use some enhancement. But it might be a good base to avoid starting from scratch.
Inspiration can be taken from other readline implementations. There are quite a few in different languages. A simple overview can be found here: https://github.com/kkawakam/rustyline#similar-projects

@I3oris I don't think there's much value in calculating the width of unicode characters. It depends on Unicode version and font support in the terminal app, which is impossible to take into account.

@I3oris
Copy link
Contributor

I3oris commented Sep 26, 2022

I tested fancyline but is doesn't seam to support unicode either ☹️ , however the irb/python/julia&node REPL all work correctly with unicode. Rustline tell it does also, It would worth to see their implementation and how they handle the problem, but rustline use something called unicode-width as dependency.

@asterite
Copy link
Member

Like all things, I would do this by steps. First I wouldn't worry about unicode. Unicode is real, but in my debugging sessions I rarely need to deal with unicode. Like, I won't be typing unicode chars. It's usually English because the programming language uses English.

Once we got a pure Crystal readline working, we can start improving things like adding good unicode support.

@I3oris
Copy link
Contributor

I3oris commented Sep 26, 2022

You're right, let handle unicode later. I'm working of extracting work from my repo to create a shard similar to fancyline.

@I3oris
Copy link
Contributor

I3oris commented Oct 8, 2022

Then it's here! see https://forum.crystal-lang.org/t/new-shard-reply-a-reader-for-your-repl/5005.

@paulocoghi
Copy link

Thank you, @I3oris !

@straight-shoota
Copy link
Member Author

Resolved by #12738

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

No branches or pull requests