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

stdlib: enhances the shell completion #5924

Merged

Conversation

frazze-jobb
Copy link
Contributor

Have all functionality more or less in place, but I need to do some refactoring, and there are already 3 bugs that I know of.

I need to deal with c:memory/1 which have two type specs, I can use my fold function
but the fold function doesn't work. It can be seen in the new failing edlin_expand_SUITE testcases.
I also need to handle fun Mod:Fun/X, properly.

Please evaluate and see if the completions are helpful. I am thinking if a shell keyboard shortcut could show the whole type spec. Instead of just the current parameter of interest.
Also file:open("/filename", just gives ... because the suggestion is so long, file:open("/filename", [ will work. We need to decide on some heuristics when to print all the concrete types and when to print an abstract type.

@frazze-jobb frazze-jobb self-assigned this Apr 22, 2022
@frazze-jobb frazze-jobb added team:VM Assigned to OTP team VM enhancement labels Apr 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 22, 2022

CT Test Results

       8 files     318 suites   3h 31m 28s ⏱️
4 905 tests 4 661 ✔️ 243 💤 1
5 848 runs  5 538 ✔️ 309 💤 1

For more details on these failures, see this check.

Results for commit 542d97c.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@CLAassistant
Copy link

CLAassistant commented Apr 22, 2022

CLA assistant check
All committers have signed the CLA.

@garazdawi garazdawi added this to the OTP-26.0 milestone Apr 22, 2022
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch 3 times, most recently from 4a250d1 to f4da54e Compare April 29, 2022 09:58
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch from f4da54e to 12a5ba3 Compare May 2, 2022 08:48
@frazze-jobb frazze-jobb changed the title Frazze/stdlib/shell completion/otp 14835 stdlib: enhances the shell completion May 2, 2022
lib/kernel/src/code.erl Outdated Show resolved Hide resolved
lib/stdlib/test/edlin_expand_SUITE.erl Outdated Show resolved Hide resolved
lib/stdlib/test/edlin_expand_SUITE.erl Outdated Show resolved Hide resolved
lib/stdlib/test/edlin_expand_SUITE.erl Show resolved Hide resolved
@@ -18,12 +18,17 @@
%% %CopyrightEnd%
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add tests for Unicode names, filenames, fields variables etc? Just in case anyone runs beamoji on the source code and tries to autocomplete.

lib/stdlib/test/edlin_expand_SUITE.erl Outdated Show resolved Hide resolved
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch 3 times, most recently from 66e0510 to 3ac4520 Compare July 5, 2022 06:45
Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

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

Reviewed code and group so far.

lib/kernel/src/code.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/stdlib/src/erl_eval.erl Outdated Show resolved Hide resolved
lib/stdlib/src/shell_default.erl Outdated Show resolved Hide resolved
lib/stdlib/src/edlin_expand.erl Outdated Show resolved Hide resolved
lib/stdlib/src/edlin_expand.erl Outdated Show resolved Hide resolved
lib/stdlib/src/shell.erl Outdated Show resolved Hide resolved
lib/stdlib/src/shell.erl Outdated Show resolved Hide resolved
lib/stdlib/src/edlin_expand.erl Outdated Show resolved Hide resolved
lib/stdlib/src/edlin_expand.erl Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/stdlib/test/edlin_expand_SUITE.erl Outdated Show resolved Hide resolved
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch 3 times, most recently from 2e31321 to 05f0018 Compare August 22, 2022 12:41
@frazze-jobb frazze-jobb added the testing currently being tested, tag is used by OTP internal CI label Aug 23, 2022
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch from 10fbf29 to b713af7 Compare August 29, 2022 15:48
Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

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

Lots of great stuff in here now. Some details to polish though. For example:

No completions

If I type:

1> lists:ab<TAB>
functions

It prints an empty functions heading. I think it should not print anything as that is what it does when it cannot find a module that matches. Possibly the same thing applies when it does not find an argument type that matches. Right now it prints the slogan of the function, but should it print nothing?

Or maybe both of these should print the first suggestion that appears by erasing the characters before the cursor one by one? That is lists:ab would show the suggestions for lists:a and lists:all([{ would show the suggestions for lists:all(? Though I suppose that could potentially be very expensive to compute...

Multiple clauses

The completion for ssh:connect(<TAB> is not very intuitive:


16> ssh:connect(<TAB>
ssh:connect(Host, Port, Options, NegotiationTimeout)
ssh:connect(Var1, Var2, Var3)
ssh:host() :: {0..255, ...}      {0..65535, ...}    loopback           string()
ssh:connect(OpenTcpSocket, Options)
ssh:connect(Var1, Var2, Var3)
ssh:open_socket() :: {'$inet', ...}    port()

Where is the Var1 etc coming from? ssl:connect(<TAB> looks much better.


ssl:connect(Host, Port, TLSOptions)
ssl:connect(Host, Port, TLSOptions, Timeout)
ssh:host() :: {0..255, ...}      {0..65535, ...}    loopback           string()
ssl:connect(TCPSocket, TLSOptions)
ssl:connect(TCPSocket, TLSOptions, Timeout)
ssl:socket() :: {'$inet', ...}    port()

system_info

erlang:system_info is one of the most complex functions we have to document, so I played around with it and found some bugs in the completion code.

  1. erlang:system_info(<TAB> prints erlang:system_info(info), i.e. the wrong info is highlighted.
  2. erlang:system_info({allocator,<TAB> should show atom() as only possible completions, but shows a lot of other options as well.
  3. erlang:system_info(dist_<TAB> shows this:
    modules
    dist_ac:     dist_util:
    erlang:system_info(dist_buf_busy_limit)
    erlang:system_info(dist_ctrl)
    I wonder if we should not show modules when there are matching function arguments? Should function name completion have its own header?

External fun completion

If I type [fun ssl:connect/<TAB> the autocompletion is 2, 3, 4,. Why is there a comma at the end? I could want to use any of ,, ] or | at that position.

Fun completion

If I type fun(a) when<TAB> it completes as when, however it should not give any completions.
If I type fun(A) -<TAB> I get a bunch of incorrect completions.
If I type fun(A) -> <TAB> I get the same completions as above, should I really get any completions at all here?

lib/stdlib/test/edlin_expand_SUITE.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
%% Nested expression
{Bef1, KWE}=over_keyword_expression(Bef),
over_keyword_expression(Bef1, KWE++"end"++Expr);
over_keyword_expression("fi"++Bef, Expr) -> {Bef, "if" ++ Expr};
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we have any module with a list of all current keywords... we have erl_scan:reserved_word, but that is for checking if an atom is a reserved word and not a list of which reserved words there are.... @bjorng do you know of any place where this is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked with @bjorng and there is no such function.

We are only interested in keywords that end with an 'end' atom, so we would have to keep a list of those as well. Maybe it would be more concise if we tried to read a word and check if it is one of our special keywords, instead of pattern matching on the reversed string. Do you think its worth adding those two reserved words functions, and doing the refactoring?

lib/stdlib/src/shell_default.erl Outdated Show resolved Hide resolved
lib/stdlib/src/stdlib.app.src Outdated Show resolved Hide resolved
lib/stdlib/src/stdlib.app.src Outdated Show resolved Hide resolved
lib/stdlib/src/edlin_expand.erl Outdated Show resolved Hide resolved
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch from b713af7 to 30539eb Compare August 30, 2022 15:29
@frazze-jobb
Copy link
Contributor Author

Lots of great stuff in here now. Some details to polish though. For example:

No completions

If I type:

1> lists:ab<TAB>
functions

It prints an empty functions heading. I think it should not print anything as that is what it does when it cannot find a module that matches. Possibly the same thing applies when it does not find an argument type that matches. Right now it prints the slogan of the function, but should it print nothing?

I agree that it should not show the section title
if we would erase the last letter and get completions for that, then I think it could be easy to miss that you have typos on your prompt

Or maybe both of these should print the first suggestion that appears by erasing the characters before the cursor one by one? That is lists:ab would show the suggestions for lists:a and lists:all([{ would show the suggestions for lists:all(? Though I suppose that could potentially be very expensive to compute...

Multiple clauses

The completion for ssh:connect(<TAB> is not very intuitive:


16> ssh:connect(<TAB>
ssh:connect(Host, Port, Options, NegotiationTimeout)
ssh:connect(Var1, Var2, Var3)
ssh:host() :: {0..255, ...}      {0..65535, ...}    loopback           string()
ssh:connect(OpenTcpSocket, Options)
ssh:connect(Var1, Var2, Var3)
ssh:open_socket() :: {'$inet', ...}    port()

Where is the Var1 etc coming from? ssl:connect(<TAB> looks much better.


ssl:connect(Host, Port, TLSOptions)
ssl:connect(Host, Port, TLSOptions, Timeout)
ssh:host() :: {0..255, ...}      {0..65535, ...}    loopback           string()
ssl:connect(TCPSocket, TLSOptions)
ssl:connect(TCPSocket, TLSOptions, Timeout)
ssl:socket() :: {'$inet', ...}    port()

regarding multple clauses, the 'Var's appears if you have 'type()' in your function parameters in the spec, the reason I decided to it this way is because I expand the types, and putting the expanded types inside the function parameters would be nasty. But I could probably put the non expanded type in the function parameter somehow.

-spec connect(open_socket(), client_options(), timeout()) ->
                     {ok, connection_ref()} | {error, term()}
           ; (host(), inet:port_number(), client_options()) ->
                     {ok, connection_ref()} | {error, term()}.

so it would become:

ssh:connect(Host, Port, Options, NegotiationTimeout)
ssh:connect(host(), inet:port_number(), client_options())
ssh:host() :: {0..255, ...}      {0..65535, ...}    loopback           string()
ssh:connect(OpenTcpSocket, Options)
ssh:connect(open_socket(), client_options(), timeout())
ssh:open_socket() :: {'$inet', ...}    port()

system_info

erlang:system_info is one of the most complex functions we have to document, so I played around with it and found some bugs in the completion code.

1. `erlang:system_info(<TAB>` prints `erlang:system_info(info)`, i.e. the wrong `info` is highlighted.

I will have a look if I can make a smarter highlighting algorithm

2. `erlang:system_info({allocator,<TAB>` should show `atom()` as only possible completions, but shows a lot of other options as well.

This is the purpose of edlin_expand:match_argument function, however, when the term is not completed (in this case an uncompleted tuple), then everything can match, but I agree it should filter the others out.

3. `erlang:system_info(dist_<TAB>` shows this:
    ```
   modules
   dist_ac:     dist_util:
   erlang:system_info(dist_buf_busy_limit)
   erlang:system_info(dist_ctrl)
   ```
     
   I wonder if we should not show modules when there are matching function arguments? Should function name completion have its own header?

I can make a section title for that.
I don't know about your first idea, sometimes you want to have the possibility to get modules. I thought about if you could have a special keybind for module expansion vs the rest expansion. But not sure if its worth it. Often times module names do 'collide' with atoms and keywords.

External fun completion

If I type [fun ssl:connect/<TAB> the autocompletion is 2, 3, 4,. Why is there a comma at the end? I could want to use any of ,, ] or | at that position.

You could, my thought was that most of the time I want a comma when I specify my list and if I would want something else I would just erase and put the correct char. With the keybind for closing we could maybe erase the last comma and put the ]. But I don't have a good idea regarding '|'.

Fun completion

If I type fun(a) when<TAB> it completes as when, however it should not give any completions. If I type fun(A) -<TAB> I get a bunch of incorrect completions. If I type fun(A) -> <TAB> I get the same completions as above, should I really get any completions at all here?

I agree, no completions

@frazze-jobb
Copy link
Contributor Author

After discussion offline with @garazdawi , we concluded that list elements should never autocomplete with , | or ] and maps should not complete with , or }. With trailing comma support there might be a possibility that we revisit this.
For now it's easier to just type , instead of tab.

@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch 4 times, most recently from af36547 to 0394441 Compare September 15, 2022 07:58
lib/kernel/src/group.erl Outdated Show resolved Hide resolved
lib/kernel/test/interactive_shell_SUITE.erl Outdated Show resolved Hide resolved
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch 4 times, most recently from 2e14d16 to ee90cba Compare September 20, 2022 10:40
@frazze-jobb frazze-jobb force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch 3 times, most recently from 212cb7c to 27964cc Compare September 21, 2022 11:36
@garazdawi garazdawi force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch 2 times, most recently from 22b476e to 542d97c Compare September 26, 2022 20:00
@garazdawi garazdawi force-pushed the frazze/stdlib/shell_completion/OTP-14835 branch from 542d97c to 14387ab Compare September 27, 2022 07:37
@garazdawi garazdawi merged commit 807ee96 into erlang:master Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants