Reimplement cli hints based on command arg docs

[jhelbaum]

Now that the command argument specs are available at runtime (#9656), this PR addresses #8084 by implementing a complete solution for command-line hinting in redis-cli.

It correctly handles nearly every case in Redis's complex command argument definitions, including BLOCK and ONEOF arguments, reordering of optional arguments, and repeated arguments (even when followed by mandatory arguments). It also validates numerically-typed arguments. It may not correctly handle all possible combinations of those, but overall it is quite robust.

Arguments are only matched after the space bar is typed, so partial word matching is not supported - that proved to be more confusing than helpful. When the user's current input cannot be matched against the argument specs, hinting is disabled.

Partial support has been implemented for legacy (pre-7.0) servers that do not support COMMAND DOCS, by falling back to a statically-compiled command argument table. On startup, if the server does not support COMMAND DOCS, redis-cli will now issue an INFO SERVER command to retrieve the server version (unless HELLO has already been sent, in which case the server version will be extracted from the reply to HELLO). The server version will be used to filter the commands and arguments in the command table, removing those not supported by that version of the server. However, the static table only includes core Redis commands, so with a legacy server hinting will not be supported for module commands.

Command and argument tables for the server and CLI use different structs, due primarily to the need to support different runtime data. In order to generate code for both, macros have been added to commands.c to make it possible to configure the code generation differently for different use cases.

I have implemented a basic testing framework for the command hints based on new (undocumented) command line options to redis-cli: --test_hint 'INPUT' prints out the command-line hint for a given input string, and --test_hint_file <filename> runs a suite of test cases for the hinting mechanism. The test suite is in tests/assets/test_cli_hint_suite.txt, and it is run from tests/integration/redis-cli.tcl.

[jhelbaum]

Thanks for your comments. Sorry it's taken so long for me to get back to you. Responses inline:

i think this approach of first tagging the arguments that are matched and then generating the hint for the reminder of the line based on these booleans is too limited.

Regarding the general approach, I'm open to suggestions. I haven't been able to come up with anything dramatically better. We have to match up arguments with input words in order to know which words and which arguments have been consumed and shouldn't be hinted. And we have to generate a hint describing any arguments which might still be matched. Some of the edge cases are tricky, though, and you've found a few of them.

(Nitpick: The matching isn't boolean, it's numeric, since one argument entry can match more than one input word, and an argument entry can be partially matched, such as a token without its parameter value.)

for instance, if the last match is a token which should be followed by a mandatory argument, we're unable (and won't be able) to handle it properly. for instance typing SET key value EX should have hinted either only <seconds>. or adding [NX | XX] [GET], but currently it hints the seconds last and suggests you add NX after EX. i'd argue that this is worse than the previous one which was easy to see that got broken and stopped responding)

Actually, I thought I was handling this scenario correctly. You seem to have found a bug. I'll investigate.

Your first and third examples are cases I believe I can handle. I'll try to fix them.

The second example is trickier, but it might also be doable. It's a bit problematic, though.

Consider the following partial input:

ZADD k IN

This is presumably the start of the token INCR, but it hasn't been matched yet. Do we want to guess this and suggest the completion of the word INCR? We currently only match whole words, not prefixes.

But ZADD k 3 is presumably the start of a score, which we could know only by trying to validate the numeric data type - so far the code doesn't do any type checking.

And, of course, if the next mandatory argument was string-valued, we'd have no way to know whether IN was the start of INCR or the next mandatory string argument.

• what exactly do we do now for older servers (i didn't bother to check yet).

The code currently falls back to the previous hinting algorithm if no arg specs are available.

• do we wanna re-generate help.h so that we can do fancy things even with older servers?

Interesting. Is it that common that an upgraded CLI is run with an old server? If it's worth the effort, that can be left for a future task.

[oranagra]

regarding the mandatory arguments for tokens, i don't see how you could handle these, but please try to fix the bug and i'll review. maybe that would be acceptable.

regarding optional arguments coming before mandatory ones, maybe we could have some code to realize we've hit a mandatory argument, and go mark all the optional ones before it as irrelevant (or keep a pointer to the first argument that's relevant going forward).

[jhelbaum]

Okay, I think I've fixed the cases we discussed earlier. Let me know what you think of them, and the behavior in other situations.

I've also added a test suite for the hint mechanism.

(I also seem to have messed up my branch of the repo, but I'll fix that later... sorry...)

[oranagra]

haven't reviewed the code yet, but i did give it a quick test. so first off, it does work a lot better! thank you!

here are a few places were i see room for improvement:

• this works good (typing p doesn't yet do anything)

• but then, i add x and it hides everything (since it's matching px, but it could also be pxat, so i'd rather do the matching only after hitting space (i.e. exclude the last non-terminated word from the matching and add it only when the user presses space).

• for some reason, when i type an unrecognized keyword, it messes up the rest of the matching, maybe that's avoidable.

• if i don't "consume" the ANY arg, it keeps hunting me like a ghost, even if i already moved on and it's no longer valid.

• if i type an incomplete keyword here (i.e. the n), it assume it's a score and hides all the optional args.

• and then when i add the x of nx it recognizes it's not a score, and re-adds all the optional args.

• when i type the initial score, it suggests member next, but when when i type another score, it still suggests [score member] ideally, it would suggest member [score member]. but that's petty, so just a small room for improvement, certainly not a must.

[jhelbaum]

Thanks - those are some great examples. I'll go through them and see what can be improved. Will update you when there's news.

[jhelbaum]

i'd rather do the matching only after hitting space

Do you mean in general? Only match the last word after the user hits space? That's not the behavior of the old hints. They appear as soon as the command name is complete, and then they match each word as soon as its first character is typed.

We can change that behavior in general, and maybe we should. But it would be a change from the current behavior.

[oranagra]

for positional arguments, maybe we can mark them as matched as soon as one character is typed. but for optional args (specifically when there's more than one option), it could be a problem. what bothered me specifically was when you type PX, it immediately hides all the options and shows "milliseconds", but it could still be PXAT.

i'm not certain, but i feel it might be better.

[jhelbaum]

• but then, i add x and it hides everything (since it's matching px, but it could also be pxat, so i'd rather do the matching only after hitting space (i.e. exclude the last non-terminated word from the matching and add it only when the user presses space).

Most of the glitches you point out in this comment can be solved by deciding that we only match words after the space is typed. I've made that change and I prefer the behavior that way. I'm inclined to adopt that rule in general.

• for some reason, when i type an unrecognized keyword, it messes up the rest of the matching, maybe that's avoidable.

I'm not sure that's avoidable. The unrecognized keyword is not valid. I think the only solution would be to stop hinting if an unmatchable word is entered.

• if i don't "consume" the ANY arg, it keeps hunting me like a ghost, even if i already moved on and it's no longer valid.

This one is tricky. I need to think a bit more about how to fix it.

when i type the initial score, it suggests member next, but when when i type another score, it still suggests [score member] ideally, it would suggest member [score member]. but that's petty, so just a small room for improvement, certainly not a must.

I agree that it's not ideal, but I don't think it's easy to solve and I'm not inclined to spend time on it for now.

[oranagra]

@jhelbaum i took anther quick look (mostly at the tests, not the code). AFAICT the main remaining issue that's hard to overlook is the problem with the ANY after COUNT in GEORADIUS.

regarding the other issues.

• i think that after an unrecognized input, we can simply stop hinting (might be better than showing irrelevant options).
• the problem i mentioned about ZADD is arguably even more annoying in BITFIELD after repeating the first action

[jhelbaum]

@oranagra Okay, I believe I've fixed all the issues we've discussed.

The code now handles:

• Repeated arguments
• Unmatched optional arguments "orphaned" by a subsequent matched argument
• Disabling hinting when argument matching fails
• Type validation of integers and doubles

I can't rule out the possibility that I've overlooked some edge cases, but I think it's pretty solid in most circumstances.

Let me know what you think.

[jhelbaum]

(I should note that changes to the command arg specs will break the unit tests for the hints, since the tests use the current command docs. This just happened with BITFIELD_RO.)

[jhelbaum]

I'm not sure how soon I'll have the time to do more work on this. When I do, I guess I'll start with separating the commands structs.

[jhelbaum]

@oranagra I don't know when/if I'll have the time to do this refactoring properly. I'm not sure how best to proceed.

We ended up in this situation because of the desire to support pre-7.0 servers with the best possible hinting. That required static linking of the command argument specs in the CLI, which then either needs to duplicate the command tables for the client and server, or to refactor the command struct to separate the server-specific runtime parts from the shared syntax specs. That refactoring is nontrivial, since the command struct is used widely in the server code.

I started working on the refactoring but am now struggling with some memory bugs, and in any case it's going to be a big change. I can't promise to spend much more time on it.

So that's where this currently stands. I think the improved hinting would be a very nice feature, but the question is how much effort is it worth to support legacy servers, and how long it will take to get the rest of the work done.

[jhelbaum]

@oranagra I think I've made all the changes you asked for except for sharing the arg structs. Let me know what you think when you have the chance.

[guybe7]

@jhelbaum heads up: https://github.com/redis/redis/pull/11051

[jhelbaum]

@guybe7 So I assume I should wait until #11051 is merged before moving forward with this?

[guybe7]

@jhelbaum i guess so i'll try to get it merged tomorrow

[jhelbaum]

@oranagra

I removed key_specs_static etc. I hope I did it right. The tests pass.

[jhelbaum]

Okay, something is leaking from the changes I made to module.c... not sure what I did wrong.

[oranagra]

ok. so what's next? are we conceptually done?

[jhelbaum]

As far as I'm concerned, it's done. The code is feature complete, and I think includes all the appropriate refactorings. I assume it could use to be reviewed more thoroughly, and people might want to interact with it and try to find more buggy edge cases.

[guybe7]

@jhelbaum https://github.com/redis/redis/pull/11051 is merged

[jhelbaum]

@jhelbaum #11051 is merged

Thanks. I updated this PR.

[jhelbaum]

@oranagra Is there any plan for moving this change forward?

[oranagra]

@jhelbaum my apologies, i do wanna merge it but currently don't have the time to review it properly. i did review bits of it, and provided guidelines for the design decisions we made, so i'm quite confident these are ok. maybe if we can find a way to split it, find another person with more free time to review the redis-cli.c part, and i will make sure to allocate time to review the other files?

[oranagra]

Thanks for taking a look. At the time I did review all the parts outside redis-cli.c and did skim through redis-cli.c to provide guidance and advice. But I felt the code need an extra pair of eyes to review it aside from the author and was waiting for someone to step up. I agree the risk for issues is small but still don't like to merge a mass of unreviewed code. Maybe the level of review you did is enough? I suppose there's no need to explicitly look for bugs, but I think we do need to make sure it is constructed in a readable manner and includes enough comments to be maintainable.

[oranagra]

If we can pick this up and close it within a couple of weeks, I think it can fit 7.2

[zuiderkwast]

I agree the risk for issues is small but still don't like to merge a mass of unreviewed code. Maybe the level of review you did is enough? I suppose there's no need to explicitly look for bugs, but I think we do need to make sure it is constructed in a readable manner and includes enough comments to be maintainable.

Yeah, I didn't execute the code in my head, but the code looks nice, seems to have enough comments, fairly short functions, comprehensive variable names, etc. I would say it's probably maintainable.

[jhelbaum]

Yes, thanks for taking a look at this. I'm unlikely to have much time to devote to this at the moment, however.

Regarding maintenance, I would just note that the test suite has a minor maintenance burden in that it is dependent on the current command specs. If some of the command argument specs change, the test cases for those commands will have to be updated accordingly (since the hint strings will change).

It might be "better" in some ideal world for the tests to be based on some static set of command specs, but that's not realistic to implement IMHO.

In any case, it would be nice to see this code merged. I think it's a handy feature.

[oranagra]

@zuiderkwast @jhelbaum i gave it some thought and also consulted Yossi and Itamar, i decided to take Viktor's advise and merge it without another reviewer, considering this is an isolated area, and we can fix things or improve readability later. So now it'll just take someone to defrost this PR and bring it back to mergable state. @zuiderkwast maybe you wanna have a go at that? (since Jason said he doesn't currently have the time)