Interpreter reply

[I3oris]

Dear crystal team.

I bring in this PR, at last, the integration of the shard REPLy as a term reader for the interpreter. The main purpose of it is to be an alternative to readline, that work on windows (#11340).
In addition, this shard is pure crystal and could be adjusted to fit exactly the compiler need.

Concretely, this PR adds the following things using the REPLy features:

• Imports a copy of v0.3.0 of the repository.
• Implements a REPLy reader at interpreter/repl_reader.cr.
  • The reader highlights in time the expression.
  • It supports multiline expression.
  • The prompt look isn't changed from current version, except that the value of the nest is not showed anymore. (It was hard to do it with a multiline expression)
  • It auto-indents when going to a new line. (Note that some indentation are not taken in account, because the parser doesn't count them yet, e.g. after an if. I think the patch for this could be added later)
  • It reformats the expression after submitting.
  • It keep a local history, which is not saved in a file yet.
  • It proposes auto-completion, but only for fixed crystal keywords.
• Implements a reader for pry at interpreter/pry_reader.cr.
  • The pry reader proposes the same feature that the repl reader.
  • It adds the following keybinding. (They sound to me rather natural, and are a considerable gain of time for debugging)
    • ctrl-down => next
    • ctrl-left => step
    • ctrl-right => finish

That's it. I hope everything is OK with this PR, don't hesitate to let me know in the comments for any question, improvement or typo.

Thank you much all! 😀

[asterite]

Thank you for this!

Here's a quick feedback. I'm on Mac OSX (M1). I just compiled this branch in release mode with the interpreter. I run bin/crystal i and no matter what I type, the first char I type I get this:

✗ bin/crystal i
Using compiled compiler at .build/crystal
Crystal interpreter 1.7.0-dev [d23eb066d] (2022-11-13).
EXPERIMENTAL SOFTWARE: if you find a bug, please consider opening an issue in
https://github.com/crystal-lang/crystal/issues/new/
icr:1> Division by 0 (DivisionByZeroError)
  from src/int.cr:145:7 in 'Reply::ExpressionEditor#line_height<String>:Int32'
  from lib/reply/src/expression_editor.cr:367:17 in 'height_above_cursor'
  from lib/reply/src/expression_editor.cr:701:25 in 'run'
  from src/compiler/crystal/command/repl.cr:39:7 in 'run'
  from src/compiler/crystal.cr:11:1 in '__crystal_main'
  from src/crystal/main.cr:115:5 in 'main'
Error: you've found a bug in the Crystal compiler. Please open an issue, including source code that will allow us to reproduce the bug: https://github.com/crystal-lang/crystal/issues

Any ideas? Probably something that computes the line height isn't implemented well for Mac OSX?

[asterite]

A bit more. This program:

require "reply"

p Reply::Term::Size.size

Gives me this:

Unhandled exception: Error retrieving terminal size: ioctl TIOCGWINSZ: EFAULT (Exception)
  from lib/reply/src/term_size.cr:1:1 in 'size'

Errno.value.message says "Bad address". I tried searching a bit about this but didn't find anything useful.

[I3oris]

Thank you @asterite for your feedback!

Probably something that computes the line height isn't implemented well for Mac OSX?

That it, it might be related to this issue, however the error code is different EFAULT/ENOTTY.

This need to be investigated more deeply!

[asterite]

Is there a default terminal size that could be used? I'm thinking it could maybe use a default if that function failed, maybe printing something to STDOUT information about this to the user. But of course fixing the issue might take a bit more time but solve the issue better.

[HertzDevil]

It doesn't set errno on my M2, but it keeps producing what looks like my stack address reinterpreted as a struct.

Your ioctl binding is wrong, it must be variadic and not hardcode Winsize*, otherwise the ABI will mess everything up. Also the members of Winsize are all unsigned:

lib LibC
  struct Winsize
    row : LibC::UShort
    col : LibC::UShort
    x_pixel : LibC::UShort
    y_pixel : LibC::UShort
  end

  TIOCGWINSZ = # ...

  fun ioctl(fd : Int, request : ULong, ...) : Int
end

screen_size = uninitialized LibC::Winsize
LibC.ioctl(1, LibC::TIOCGWINSZ, pointerof(screen_size))

[I3oris]

Thank you a lot @HertzDevil!

After some recherche, I find this awesome file created by @watzon which seem to handle every case!

In case of failure it fallback on several alternatives including tput or default environment variables, so it is a very robust implementation.

[I3oris]

I bring a new implementation of Term::Size based Hertz Devil's solution and crystal-term/screen. If every alternative fails, it defaults to a default size.

[I3oris]

I have also fixed a spec broken on Windows due to \n\r which I forgot to check on my last commit on REPLy.

[asterite]

It's working great now. This is awesome! ❤️

[straight-shoota]

Hey @I3oris, this PR is still in draft. Is there anything substantial missing?
I've been running this for a couple of weeks now for doing interpreter stuff, and it works great. I didn't get into the more advanced key bindings, but still.
I'd like to get this merged. It greatly improves the user experience with the interpreter 🚀

If you know there are missing bits, what's necessary to fill them?

[I3oris]

Hello @straight-shoota !

Yeah I think it's ready to be merged, apart from one thing (but I didn't got the time to share it, neither to solve it yet, so I'm a bit sorry for the delays).

The thing is the Crystal::Interpreter (different from the Crystal::Repl) depend of my shard REPLy. That mean, if one try to use require "compiler/crystal/interpreter" in his program, it won't work. He have to install REPLy in his project. Maybe it's not a problem, it's already the case for the markdown tool with the markd lib. However I found a bit annoying to have to install a dependency only to use the interpreter. We could imagine a usage in an IDE who wouldn't care about ask user input and doesn't need REPLy.

Ideally, REPLy would be used only Crystal::Repl and not in Crystal::Interpreter.

But the only reason the interpreter need REPLy now is when the debugger is triggered, which will open the debug prompt with REPLy.

To solve this I thinks we could do either:

1. Ship the REPLy (and markd) lib with the compiler. (But I don't know if it's a good solution)
2. Refactor the code to extract the debugger prompt outside the Crystal::Interpreter . The interpreter would expose a Proc for the action when debugging, default action could be to ignore, Crystal::Repl action would use REPLy, and a IDE tool could use a graphical API to handle it, etc.
3. Remove REPLy prompt from the debugger now and treat this subject later.
4. Nothing and merge if we consider is not a major problem for now.

[straight-shoota]

I think it would eventually make much sense to have the debugger action pluggable (option 2.)
But that can come as a later enhancement. I think it's fine to merge this as is for now (option 4). The interpreter is still in an experimental state, so I wouldn't consider it a big problem that it requires additional code outside of the compiler source. That should be easy to fix, you just need to install the shard. Alternatively, we could also consider a compile-time flag to build without debugger support. But that would really be unnecessary with a pluggable solution.
So I'd suggest to make this PR ready for merging as is. I think we can get it into 1.7 still, even after the feature freeze (because the interpreter is experimental). Just need a couple of reviews.

[asterite]

I agree. The goodies in this PR are greater than the mentioned issue. This PR is fantastic!

[straight-shoota]

LGTM.
I didn't dive into the details of the lib implementation. I trust that it works and makes sense from what I have been able to observe.
It should be great to get it merged and then we'll be able to iron out any issues detected while using it.

Thank you very much @I3oris for this library and its integration 🎉