fix(complete): fix bash completion for --option=value syntax#6364
fix(complete): fix bash completion for --option=value syntax#6364douzebis wants to merge 1 commit into
Conversation
e19b5c2 to
c8b7bbc
Compare
c8b7bbc to
2fa4e6d
Compare
2fa4e6d to
37a6cab
Compare
d64d742 to
44cd83e
Compare
|
Hi @epage — CI is green. Would you mind taking a look when you get a chance? |
|
Please do not use AI PR descriptions. They focus on the wrong things, creating a lot of unnecessary noise. For example, a testing section shouldn't say what you did that will be covered by CI but what you did beyond CI and whether the current coverage is sufficient and what was done to address that. |
44cd83e to
7ab2259
Compare
|
@epage better this way? |
| # last break character. We can't use $2 directly; instead we reassemble | ||
| # the word list by dropping single-character COMP_WORDBREAKS tokens and | ||
| # adjusting the index. Approach adapted from _comp__reassemble_words in |
There was a problem hiding this comment.
Dropping tokens seems inappropriate for dealing COMP_WORDBREAKS, particularly : but even =.
There was a problem hiding this comment.
@epage I believe dropping these tokens is appropriate. COMP_WORDBREAKS is purely a readline completion artefact — it has no effect on how bash parses the command line when the user hits ENTER. At execution time, bash uses IFS for word splitting, so --choice=first is passed to the program as a single argv[1] regardless of whether = is in COMP_WORDBREAKS. The split only exists during completion.
For example: if the user types exhaustive action --choice=f<TAB>, bash presents COMP_WORDS as ["exhaustive", "action", "--choice", "=", "f"]. By dropping the = separator token and adjusting the index, the clap completer receives ["exhaustive", "action", "--choice", "f"] with the cursor on "f" — enough to identify that "f" is a value candidate for --choice and complete it to "first". The bash completion script then feeds first back as the completion of the f fragment, resulting in exhaustive action --choice=first on the command line.
Is there a specific case where you think dropping a token produces wrong results?
There was a problem hiding this comment.
Unsure if the parser is sensitive to requires_equals yet but that would likely cause a value to be treated as a positional rather than attached to the option.
Speaking of, I'm unsure when we offer positionals with attached values but say you have num_args(0..=1), we should only offer attached values with = and not positionals. Similar for attached values starting with -- and flags.
There was a problem hiding this comment.
@epage , I looked at the current clap implementation and I believe dropping the tokens is correct:
-
It aligns with how bash already sees the command line —
=and:are separators from bash's perspective (assuming they would be in COMP_BREAKWORDS), so treating them as such before querying the clap completer is consistent with that view. -
It requires less toil: the clap completer always returns bare candidates (
first,second), and bash splices them back in place of just the fragment after the separator, naturally reconstructing--choice=first. The alternative — reassembling--choice=finto a single token before querying — would require the completion engine to remove every--choice=prefix from the COMPREPLY returned by the completer. -
Regarding
require_equals: support for it in the completion engine seems not to be implemented yet, so the concern would be theoretical at this point. -
Finally, this approach is consistent with how
_comp__reassemble_wordsworks in bash-completion, which is the established reference for this problem.
What do you think?
There was a problem hiding this comment.
Regarding require_equals: support for it in the completion engine seems not to be implemented yet, so the concern would be theoretical at this point.
I'm not too thrilled about concrete future use cases being dismissed as theoretical. This creates a ticking time bomb for how cargo will behave.
That was also just one example and likely nothing with : will work properly with this scheme.
There was a problem hiding this comment.
@epage , what would you like the bash glue script to do then?
- assuming COMP_WORDBREAKS is
"'><=;|&(: - assuming the command line is
foo --url=http://example.co<TAB>
My guess is that you want the bash glue to:
- undo the work of COMP_WORDBREAKS, i.e. re-assemble,
foo --url = http : //example.cointofoo --url=http://example.co - call the clap completer -- say it returns
( "http://example.co" "http://example.com" ) - break the items in the returned array according to COMP_WORDBREAKS, and keep only the last leg
- return the following to the bash completion engine:
( "//example.co" "//example.com" )
Is the above what you expect?
If not, do you have a specification of what you expect?
There was a problem hiding this comment.
My ideal scenario is we can act as if word breaks does not exist.
Approaches for doing that is likely best discussed in the issue rather than splitting it among all of the different PRs.
When characters in COMP_WORDBREAKS (e.g. '=', ':') appear in an argument, bash splits it into multiple tokens in COMP_WORDS. For example, '--option=value' becomes ["--option", "=", "value"], and $2 (the word-to-complete) only contains the fragment after the last break character. The Rust completer expects the unsplit form, so it received '=' as a positional argument and returned no completions. Strip single-character COMP_WORDBREAKS tokens from COMP_WORDS before calling the completer, adjusting _CLAP_COMPLETE_INDEX accordingly. When the cursor is on the break character itself (e.g. "--opt=<TAB>" with no value yet), append an empty string so the completer sees ["--opt", ""] at the right index. Restore words[$_cword]="$2" afterwards so that cursor-in-middle and quoted-word completion still work correctly. Approach adapted from _comp__reassemble_words in bash-completion: https://github.com/scop/bash-completion Update the test expectation for --choice=f<TAB>: with the fix the engine now correctly completes f -> first, so the line becomes --choice=first rather than staying unchanged. The added word-reassembly loop runs inside bash on every tab completion and adds a small but measurable amount of processing time. The pty-based shell integration tests (unstable-shell-tests) use a 50 ms read timeout in completest-pty. On slower machines this can be tight enough that single-tab completions time out before bash echoes the result back. Fixes clap-rs#6280 Fixes clap-rs#3920
453517a to
422b935
Compare
When characters in `COMP_WORDBREAKS` (e.g. `=`, `:`) appear in an
argument, bash splits it into multiple tokens in `COMP_WORDS`. For
example, `--option=value` becomes `["--option", "=", "value"]`, and
`$2` (the word-to-complete) only contains the fragment after the last
break character. The Rust completer expects the unsplit form, so it
received `=` as a positional argument and returned no completions for
`--option=` syntax.
Fix: strip single-character `COMP_WORDBREAKS` tokens from `COMP_WORDS`
before calling the completer, adjusting `_CLAP_COMPLETE_INDEX`
accordingly. When the cursor is on the break character itself (e.g.
`--opt=` with no value yet), append an empty string so the
completer sees `["--opt", ""]` at the right index. Restore
`words[$_cword]="$2"` afterwards so that cursor-in-middle and
quoted-word completion continue to work.
Approach adapted from `_comp__reassemble_words` in bash-completion:
https://github.com/scop/bash-completion
Fixes #6280
Fixes #3920