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

Initial version of newshell autocompletion #75

Merged
merged 4 commits into from Nov 16, 2020
Merged

Conversation

ret2libc
Copy link
Member

@ret2libc ret2libc commented Nov 9, 2020

This (quite big) PR adds support for autocompletion based on all the info available in "newshell".

Limitations of existing autocompletion:

  • it is based on few assumptions about the structure of the input, which makes it very fragile and less useful (e.g. if you put a space in the first place, no autocompletion; if you use $( it won't detect that a new command can be suggested; etc.)
  • it assumes only one argument should be autocompleted. It has no info about other arguments
  • it requires manual changes every time a new command is added/removed and/or arguments accepted by a command are changed

Advantages of cfg.newshell.autocompletion:

  • whenever possible it is based on tree-sitter syntax tree, so it can easily autocomplete new commands/new arguments everywhere one is expected (e.g. after $(, @@c:, ., etc.)
  • it gets all the information about available commands/arguments from RzCmd, so whenever a new command/argument is added, it automatically supports it
  • it supports more than one argument

Current limitations/issues:

  • only file, env variables, zign spaces, and "choices" (see za <type> field) are autocompleted.
  • commands that are not converted to "newshell" style are not autocompleted.
  • as a command you are writing may not be valid yet while writing it, some sort of guessing is needed anyway to try to understand which token should be autocompleted (e.g. zs "hell<tab> is not valid according to the parser, because the " is not completed, so we need to try and see what is the proper thing to do).

@XVilka
Copy link
Member

XVilka commented Nov 9, 2020

  1. There are some conflicts to be resolved.
  2. Will it work with e scr.prompt.popup=true?

@ret2libc
Copy link
Member Author

ret2libc commented Nov 9, 2020

2. Will it work with `e scr.prompt.popup=true`?

Not yet. I would wait a bit for that and first provide basic autocompletion. I've tried it in radare2 and it doesnt' work that well...

librz/core/core.c Outdated Show resolved Hide resolved
librz/core/core.c Outdated Show resolved Hide resolved
@ret2libc
Copy link
Member Author

ret2libc commented Nov 9, 2020

Unfortunately, unit testing this seems quite hard as it would require to setup a full RzCore with flags, zignatures, files, etc. Also, doing a rz-test test is not easy either, because of the "interactive" requirement :(

I was only able to unit test the dietline.c part, that actually replaces text in the RzCons line. So that part should be well tested. However, the part in core.c:newshell_autocompletion needs to be tested manually.

@ret2libc ret2libc marked this pull request as draft November 9, 2020 13:38
@ret2libc
Copy link
Member Author

ret2libc commented Nov 9, 2020

I'm converting this to draft as I actually found many things to still improve for the initial review. The hardest thing, as you can see, it's identifying exactly what token the user is trying to write. The actual autocompletion and finding of the possible options is quite easy.

@thestr4ng3r
Copy link
Member

Unfortunately, unit testing this seems quite hard as it would require to setup a full RzCore with flags, zignatures, files, etc. Also, doing a rz-test test is not easy either, because of the "interactive" requirement :(

I was only able to unit test the dietline.c part, that actually replaces text in the RzCons line. So that part should be well tested. However, the part in core.c:newshell_autocompletion needs to be tested manually.

Theoretically you should be able to just create a local RzCore, just like there are many tests using local RzAnals for example. Since unit tests run sequential per test file, it might even work despite all the ugly globals. Did you encounter any specific issues trying to do this?

@ret2libc
Copy link
Member Author

ret2libc commented Nov 9, 2020

Unfortunately, unit testing this seems quite hard as it would require to setup a full RzCore with flags, zignatures, files, etc. Also, doing a rz-test test is not easy either, because of the "interactive" requirement :(
I was only able to unit test the dietline.c part, that actually replaces text in the RzCons line. So that part should be well tested. However, the part in core.c:newshell_autocompletion needs to be tested manually.

Theoretically you should be able to just create a local RzCore, just like there are many tests using local RzAnals for example. Since unit tests run sequential per test file, it might even work despite all the ugly globals. Did you encounter any specific issues trying to do this?

I did not try actually... I could though. I'll let you know.

@ret2libc ret2libc force-pushed the newshell-autocompletion branch 3 times, most recently from 0e50919 to 6f904ac Compare November 10, 2020 11:57
@ret2libc ret2libc marked this pull request as ready for review November 10, 2020 11:58
@ret2libc
Copy link
Member Author

Ok, I think it is ready for review now. I tried to add some comments wherever I thought there was some need. If something is hard to understand, tell me and I'll try to document/explain better.

@ret2libc ret2libc force-pushed the newshell-autocompletion branch 2 times, most recently from 98608f6 to e8ee621 Compare November 10, 2020 14:20
@karliss
Copy link
Member

karliss commented Nov 10, 2020

Any thoughts on supporting custom completion callback type? You already have union which could store the callback pointer for corrsponding RzCmdArgType. Could be used to reduce amount of direct dependencies for the command processing/completion mechanism and also allow custom completion for plugin commands. For example zignature completion is somewhat specific to zignature command family.

}

static void print_options(int argc, const char **argv) {
int cols = (int)(rz_cons_get_size (NULL) * 0.82);
Copy link
Member

Choose a reason for hiding this comment

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

What's with 0.82 ? Subjectively pleasing fraction of screen to fill?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea... I just copied this code from what was already there and extracted it in a function because I needed to do the same thing. It's probably a "random" number though, yeah.

librz/core/core.c Outdated Show resolved Hide resolved
Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

LGTM.

@ret2libc
Copy link
Member Author

Any thoughts on supporting custom completion callback type? You already have union which could store the callback pointer for corrsponding RzCmdArgType. Could be used to reduce amount of direct dependencies for the command processing/completion mechanism and also allow custom completion for plugin commands. For example zignature completion is somewhat specific to zignature command family.

What would be the advantage of completing zignatures arguments with a "custom" function instead of having a RZ_CMD_ARG_TYPE_ZIGN type? I think the custom function would have to be in RzCore anyway, so I don't see a clear advantage by doing that. However, I agree it can be useful when some autocompletion is just for one command and defining a new type just for that would be a bit too much or when you have external plugins.

@XVilka
Copy link
Member

XVilka commented Nov 12, 2020

Yes, having a custom autocompletion would be highly beneficial.
For example, take current windbg:// pipe support with various Windows-specific commands. Or Yara plugin that uses corresponding commands that are not in line with rizin.

@ret2libc ret2libc merged commit 5ba68e1 into dev Nov 16, 2020
@ret2libc ret2libc deleted the newshell-autocompletion branch December 21, 2020 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants