Skip to content

feat: add auto-completion preview for REPL #1832

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

Merged
merged 37 commits into from
Mar 25, 2024

Conversation

tudor-pagu
Copy link
Contributor

Resolves #1775 .

Description

What is the purpose of this pull request?

This pull request implements the preview for auto-completion feature, inspired by the node.js REPL. Whenever there is only one possible tab completion, it will be shown to the user as a gray preview. This preview is shown to the user regardless of where the cursor is. There are a few special interactions related to the preview:

  • If the user presses enter/return while a completion preview is showing, that completion will be filled before the line is submitted. This is shown below:
    Screencast from 11.03.2024 17:34:20.webm
  • If the user presses the right arrow to move into the preview, it is automatically filled in.
  • If the user presses tab while the preview is showing, it will be filled in. This is not actually implemented in this PR and is a natural result of tab completion. Note: if a preview is showing, the cursor is not at the end of the line, but there is also only one tab completion available from the point where the cursor is, and the user presses tab, the completion will be inserted where the cursor is positioned, not at the end of the line. This may seem like a bug, but the same behavior is present in the node.js REPL, so I did not change it. Pictured below:
    Screencast from 11.03.2024 17:37:36.webm

Related Issues

Does this pull request have any related issues?

#1794 aims to implement a related feature, and might depend on this PR (depending on the implementation).

Questions

Some things I was not sure about when implementing this:

  • The preview_completer.js uses a pattern I haven't seen used in the rest of the REPL code (returning an object with the needed functions). I did this to encapsulate the preview generation logic, and because both functions needed access to the completionCache variable. If there is a better way of structuring this I would be happy to implement it.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

  • In preview_completer.js, the no-underscore-dangle eslint rule is disabled in order to access the _rli attribute of the REPL object. This is also done in already existing code, such as in main.js and completer.js, for similar reasons.

  • In order to implement the auto-completion when the user presses enter, I had to override the internal _ttyWrite attribute of the readLine interface. This is because any other event listener only fires after the character is processed, so by that point the line is cleared and already submitted. While it is potentially dangerous to override internal methods, this is the only way I could find, and this has also been done in other established projects, such as this implementation of the node.js REPL:

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

Tudor Stefan Pagu added 3 commits March 11, 2024 11:39
Change repl to show a gray preview of the
possible tab completion, whenever there is
only one available. This feature is modeled
after the preview in the node.js REPL,
and should enhance the user experience.
Adds several special interactions inspired by
the node REPL, to enhance user experience. When
the user moves the caret into the preview
it is now automatically filled. Also, when the
user presses enter while the preview is showing,
it will be filled in before the line is processed.
Copy link
Contributor

@stdlib-bot stdlib-bot left a comment

Choose a reason for hiding this comment

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

👋 Hi there! 👋

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

@Planeshifter
Copy link
Member

/stdlib update-copyright-years

Refactor preview_completer from factory function
to small class. Also, use repeat from stdlib
instead of the string prototype, and make some
small style fixes.
@tudor-pagu
Copy link
Contributor Author

Thank you for the review @kgryte, I've made the changes you suggested.
I would also like to implement the related issue #1794. Would it be better to add the changes to this PR or create a separate one? (this is, of course, assuming that the change proposed there is accepted.)

@kgryte
Copy link
Member

kgryte commented Mar 12, 2024

@tudor-pagu A separate PR. We can discuss more on #1794, as that RFC is still under discussion.

Tudor Stefan Pagu added 3 commits March 14, 2024 12:38
Make the beforeKeypress and onKeypress methods be
defined in the prototype of PreviewCompleter so
that they are not created again for every instance.
The clearPreview function in completerCallback()
was accidentally removed, which caused some tests
to fail. This commit puts it back.
@tudor-pagu tudor-pagu requested a review from kgryte March 16, 2024 08:53
@kgryte kgryte added Needs Review A pull request which needs code review. REPL Issue or pull request specific to the project REPL. labels Mar 16, 2024
@kgryte
Copy link
Member

kgryte commented Mar 16, 2024

@tudor-pagu I've updated your PR. I encourage you to look through the changes in order to better understand expectations. This stated, the preview completion does work; however, there is a bug which I did not have time to investigate or fix. Namely, when a completion preview is displayed, if you hit space one or more times, the preview is still displayed. This should not be the case. Upon hitting space, the preview should be cleared.

I also did not update the tests, which also need some clean-up.

@kgryte kgryte removed the Needs Review A pull request which needs code review. label Mar 16, 2024
@tudor-pagu
Copy link
Contributor Author

tudor-pagu commented Mar 16, 2024

@kgryte I think there's 2 separate issues here:

  • Spaces in the middle of a function name still lead to auto-completion: if you type conso + TAB you get conso le, which is not valid JS.
    Should I change this completer behavior or just make the preview not show up anymore when there's trailing spaces?

  • Spaces before or after "the dot". For example, console . log(23) is of course valid javascript. So if the user types:
    console . l
    Should they not see a gray og after the l, signifying that there is a possible completion? (this is the current behavior)


Also, I've found another bug with the completer: (from before my commits)
if you type "let a = [" and then press TAB, the REPL crashes. This is a bigger issue now because my code checks for completions after every character, so that will crash my code. I will look into fixing this.

I also want to ask: If every possible completion has a common prefix, should we display that prefix as a preview? I noticed that typing

base.strided.

Leads to no preview, because the completions are ['unary','unaryBy','unary...', etc], but pressing tab completes to unary (common prefix). Should the preview also display unary here? My worry is it might be slow to check the common prefix of all completions when the number of completions is high, maybe we should only do this check if there are less than ~10 completions?

Fixes a bug where the preview would still show
after user pressed space.
@kgryte
Copy link
Member

kgryte commented Mar 16, 2024

Indeed, it looks like the introduction of the preview completer broke the ability to handle brackets.

@kgryte
Copy link
Member

kgryte commented Mar 16, 2024

Ah, okay, was able to reproduce the TAB crash behavior when using the existing TAB completion functionality.

@kgryte
Copy link
Member

kgryte commented Mar 16, 2024

Okay. I think I've fixed the various bugs associated with TAB completion which were affecting preview completion.

@kgryte
Copy link
Member

kgryte commented Mar 16, 2024

I also updated the logic for handling trailing whitespace. I had forgotten about the scenario such as

In [3]: var x = { '   a    b    c    ': 1 }
Out[3]: { '   a    b    c    ': 1 }

In [4]: x['   <|>

where you'd want to display a preview for properties containing whitespace.

@kgryte
Copy link
Member

kgryte commented Mar 16, 2024

@tudor-pagu Okay, so I think this is coming along.

Re: common prefix preview completion. Well, I'd say go ahead and try to implement it without restrictions and see how slow it is. If it makes the REPL unusable, then can try restricting to, say, when there are 10 completions or fewer.

Tudor Stefan Pagu added 2 commits March 18, 2024 20:21
Extract the logic for setting up the repl with
the debug streams into a separate fixture. Also,
add comments to the tests.
If there are multiple completion candidates, but
they all have some non-empty common prefix, that
prefix will now be shown as a preview. This works
well with the tab completion which also fills in
the common prefix when the user presses tab.
@tudor-pagu
Copy link
Contributor Author

@kgryte
I've added the preview for when all completion candidates have a common prefix. I haven't encountered any slowdowns.
I've also done my best to clean up the tests by extracting the setup logic to a fixture and adding more comments. Please let me know if there is more work I can do on the tests, I will happily improve them.

I've also found a small bug with the completer: variable names are only tab completed if they are initialized.
Steps to reproduce:

  1. write var abcdef, press enter
  2. write abcd, press tab. There is no completion.
  3. In a different repl instance type var abcdef = 1
  4. write abcd, press tab. The rest of the name will be completed.
    There is also no completion for variables defined with let/const.

@kgryte
Copy link
Member

kgryte commented Mar 19, 2024

I've also found a small bug with the completer

A couple of comments:

  1. For reasons I don't understand, uninitialized variables declared with var are not added to the contextified object managed by the Node.js vm. When the variable is declared and initialized, the variable is added to the contextified object (i.e., a property is created on the globalThis object). The reason for this difference I don't understand, as the property is available on the this variable (in both cases) which can be accessed in the REPL. Meaning, the property is on the this object, but not on the "contextified" object. That seems odd to me and is not something we directly control, as we delegate to the vm to update the context object.
  2. let/const variables do not create properties on the globalThis (w/in the REPL, the "context") object. Instead, they are stored in a declarative environment record, which we don't have access to. Or at least, I don't know of a way to directly access it.

In order to support TAB completion of const and let declared variables, we'd likely need to parse the program AST before execution and store in a separate cache, which we'd then need to search within the completer logic.

I think this is doable (albeit imperfect; e.g., const beep = 42; throw new Error(''); const boop = 43;), but should be left to another issue/PR. Feel free to open an issue describing the problem.

@tudor-pagu
Copy link
Contributor Author

tudor-pagu commented Mar 22, 2024

@kgryte
I see, it makes sense to me to keep those changes for a separate PR. Are there any other changes that need to be made here? I'm not sure if you've reviewed the tests I added, please let me know if I could improve on them.

@kgryte
Copy link
Member

kgryte commented Mar 22, 2024

@tudor-pagu Thanks for the ping. It is on my todos. Hopefully, I'll have a bit more feedback on the tests by this weekend, so we can get this over the finish line.

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@tudor-pagu I refactored and cleaned up the tests. I believe this PR should be good to merge. Will merge once CI passes. Thanks for working on this!

@kgryte kgryte merged commit ec283ec into stdlib-js:develop Mar 25, 2024
@kgryte kgryte removed the Needs Changes Pull request which needs changes before being merged. label Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Issue or pull request specific to the project REPL.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: Add auto-completion preview for REPL
4 participants