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

clap_generate: zsh broken with two multi length arguments #3022

Closed
2 tasks done
Morganamilo opened this issue Nov 13, 2021 · 8 comments · Fixed by #4612
Closed
2 tasks done

clap_generate: zsh broken with two multi length arguments #3022

Morganamilo opened this issue Nov 13, 2021 · 8 comments · Fixed by #4612
Labels
A-completion Area: completion generator C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@Morganamilo
Copy link

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Rust Version

rustc 1.58.0-nightly (8b09ba6a5 2021-11-09)

Clap Version

3.0.0-beta.5

Minimal reproducible code

use clap::{App, IntoApp, Parser};
use clap_generate::{generate, Shell};
use std::io::stdout;

#[derive(Parser)]
pub struct Args {
    pub targets: Vec<String>,
    #[clap(required = true, raw = true)]
    pub files: Vec<String>,
}

fn main() {
    let mut app: App = Args::into_app();
    generate(Shell::Zsh, &mut app, "bug", &mut stdout());
}

Steps to reproduce the bug with the above code

With the completion installed:

% command <tab>
_arguments:comparguments:325: doubled rest argument definition: *::file -- Files to search for:

Actual Behaviour

So I have a program that has a usage like this: program: <targets>... -- <files>...

The zsh completion seems to really not like this and spits out an error when tab is hit:

_arguments:comparguments:325: doubled rest argument definition: *::file -- Files to search for:

The completion generated is:

#compdef bug

autoload -U is-at-least

_bug() {
    typeset -A opt_args
    typeset -a _arguments_options
    local ret=1

    if is-at-least 5.2; then
        _arguments_options=(-s -S -C)
    else
        _arguments_options=(-s -C)
    fi

    local context curcontext="$curcontext" state line
    _arguments "${_arguments_options[@]}" \
'-h[Print help information]' \
'--help[Print help information]' \
'*::targets:' \
'*::files:' \
&& ret=0
}

(( $+functions[_bug_commands] )) ||
_bug_commands() {
    local commands; commands=()
    _describe -t commands 'bug commands' commands "$@"
}

_bug "$@"%

Expected Behaviour

Don't be broken

Additional Context

No response

Debug Output

No response

@Morganamilo Morganamilo added the C-bug Category: Updating dependencies label Nov 13, 2021
@pksunkara pksunkara added the A-completion Area: completion generator label Nov 14, 2021
@epage
Copy link
Member

epage commented Nov 14, 2021

Thanks for reporting this!

One challenge with completions is we have to effectively re-implement clap's argument parsing for each shell we support. Example of other issues that look like they stem from this:

This is making me think that #1232 is even more important so we can share parsing logic between different shells and clap and more easily test it.

@pksunkara
Copy link
Member

To be more explicitly clear, it's not the parsing logic we need to share but rather the parsing requirements.

@drahnr
Copy link

drahnr commented Apr 24, 2022

The same issue arises with a more common pattern too (see https://github.com/drahnr/clap-completion-zoink/blob/84a3eb106309d066e80645f60a3065525d18c5a4 for a full working example):

#[derive(Debug, PartialEq, Eq, clap::Parser)]
#[clap(rename_all = "kebab-case")]
struct Common {
    pub paths: Vec<PathBuf>,
}

#[derive(Debug, PartialEq, Eq, clap::Subcommand)]
#[clap(rename_all = "kebab-case")]
enum Sub {
    Comp {
        #[clap(long)]
        shell: Shell,
    },
// imagine more subcommands here
}

#[derive(clap::Parser, Debug)]
#[clap(author, version, about, long_about = None)]
#[clap(rename_all = "kebab-case")]
struct Args {
    #[clap(flatten)]
    pub common: Common,

    #[clap(subcommand)]
    pub command: Sub,
}

On tab completion results in

./target/debug/clap-completion-zoink 
_arguments:comparguments:325: doubled rest argument definition: *::: :->clap-completion-zoink
#compdef clap-completion-zoink

autoload -U is-at-least

_clap-completion-zoink() {
    typeset -A opt_args
    typeset -a _arguments_options
    local ret=1

    if is-at-least 5.2; then
        _arguments_options=(-s -S -C)
    else
        _arguments_options=(-s -C)
    fi

    local context curcontext="$curcontext" state line
    _arguments "${_arguments_options[@]}" \
'-h[Print help information]' \
'--help[Print help information]' \
'-V[Print version information]' \
'--version[Print version information]' \
'*::paths:' \
":: :_clap-completion-zoink_commands" \
"*::: :->clap-completion-zoink" \
&& ret=0
    case $state in
    (clap-completion-zoink)
        words=($line[2] "${words[@]}")
        (( CURRENT += 1 ))
        curcontext="${curcontext%:*:*}:clap-completion-zoink-command-$line[2]:"
        case $line[2] in
            (comp)
_arguments "${_arguments_options[@]}" \
'--shell=[]:SHELL: ' \
'-h[Print help information]' \
'--help[Print help information]' \
&& ret=0
;;
(help)
_arguments "${_arguments_options[@]}" \
'*::subcommand -- The subcommand whose help message to display:' \
&& ret=0
;;
        esac
    ;;
esac
}

(( $+functions[_clap-completion-zoink_commands] )) ||
_clap-completion-zoink_commands() {
    local commands; commands=(
'comp:' \
'help:Print this message or the help of the given subcommand(s)' \
    )
    _describe -t commands 'clap-completion-zoink commands' commands "$@"
}
(( $+functions[_clap-completion-zoink__comp_commands] )) ||
_clap-completion-zoink__comp_commands() {
    local commands; commands=()
    _describe -t commands 'clap-completion-zoink comp commands' commands "$@"
}
(( $+functions[_clap-completion-zoink__help_commands] )) ||
_clap-completion-zoink__help_commands() {
    local commands; commands=()
    _describe -t commands 'clap-completion-zoink help commands' commands "$@"
}

_clap-completion-zoink "$@"

@TD-Sky
Copy link

TD-Sky commented Nov 10, 2022

I meet the same bug.

clap version:

  • clap-4.0.22
  • clap_complete-4.0.5

The Cli: cli.rs

The code to generate completions: build.rs

The zsh completion script: _cnc.rs

Error

_arguments:comparguments:327: doubled rest argument definition: *::: :->cnc

davvid added a commit to garden-rs/garden that referenced this issue Jan 8, 2023
`garden cmd <TAB>` generates an error on zsh:

    _arguments:comparguments:325: doubled rest argument definition:
    *::arguments -- Arguments to forward to custom commands:

The root cause of the issue is that the generated completion script
contains two `*::` entries in the `garden cmd` arguments:

    '*::commands -- Custom commands to run over the resolved trees:' \
    '*::arguments -- Arguments to forward to custom commands:' \

A simple workaround is to drop the 2nd `*::arguments` entry.
Use a `grep` filter to workaround the zsh completion bug from
clap-rs/clap#3022 for now.

Document the workaround and add link to the issue.

Closes #10
Related-to: clap-rs/clap#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Related-to: clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

	$ my-app <TAB>
	_arguments:comparguments:325: doubled rest argument definition:
	*::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid
Copy link
Contributor

davvid commented Jan 9, 2023

I believe #4612 may resolve this issue. I ran into this while creating https://github.com/davvid/garden so I figured we might as well fix it (rather than workaround it).

Kindly test it out and let me know if it works for y'all as well.

davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

	$ my-app <TAB>
	_arguments:comparguments:325: doubled rest argument definition:
	*::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

	$ my-app <TAB>
	_arguments:comparguments:325: doubled rest argument definition:
	*::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

	$ my-app <TAB>
	_arguments:comparguments:325: doubled rest argument definition:
	*::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Expose get_value_terminator() for use by clap_complete.

Related-to: clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

	$ my-app <TAB>
	_arguments:comparguments:325: doubled rest argument definition:
	*::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Expose get_value_terminator() for use by clap_complete.

Related-to: clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

	$ my-app <TAB>
	_arguments:comparguments:325: doubled rest argument definition:
	*::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

    $ my-app <TAB>
    _arguments:comparguments:325: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Expose get_value_terminator() for use by clap_complete.

Related-to: clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 9, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

    $ my-app <TAB>
    _arguments:comparguments:325: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

    $ my-app <TAB>
    _arguments:comparguments:325: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

    $ my-app <TAB>
    _arguments:comparguments:325: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

    $ my-app <TAB>
    _arguments:comparguments:325: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

    $ my-app <TAB>
    _arguments:comparguments:325: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
Emit the value_terminator into the zsh completion pattern when
completing multi-length arguments on zsh to avoid doubled
rest-arguments definitions:

    $ my-app <TAB>
    _arguments:comparguments:325: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

Follow the suggestion from
clap-rs#3266 (comment)
by including the value terminator in order to resolve the ambiguity.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
Expose get_value_terminator() for use by clap_complete.

Related-to: clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
Emit the user-defined value terminator into the zsh completion pattern
to avoid doubled rest-arguments definitions:

    $ my-app <TAB>
    _arguments:comparguments:325: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

clap-rs#3266 (comment)
noted that including the value terminator is one step towards a
robust solution for handling multiple multi-valued arguments.

This change does not yet handle automatically detecting when a value
terminator is needed, but it does add tests to ensure that
user-specified value terminators are used on zsh.

Related-to: clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
zsh completions for commands that end with two Vec arguments require
special care to handle the transition from the penultimate multi-valued
argument and the final multi-valued argument.

We can have two Vec args separated with a value terminator.
We can also have two Vec args with no value terminators specified.
Both of these scenarios require a terminator in the zsh completions.

Special-case the penultimate multi-valued argument and force it to emit
a value terminator when the final argument also takes multiple values
or has the `last` option set.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
zsh completions for commands that end with two Vec arguments require
special care to handle the transition from the penultimate multi-valued
argument and the final multi-valued argument.

We can have two Vec args separated with a value terminator.
We can also have two Vec args with no value terminators specified.
Both of these scenarios require a terminator in the zsh completions.

Special-case the penultimate multi-valued argument and force it to emit
a value terminator when the final argument also takes multiple values
or has the `last` option set.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
zsh completions for commands that have multiple Vec arguments require
special care.

We can have two Vec args separated with a value terminator.
We can also have two Vec args with no value terminators specified
where the final arg uses 'raw' and thus requires '--' to be used.

The 2nd of these scenarios requires special handling to avoid
emitting a duplicate '*:arguments' completion entry.

We already use the '-S' option when calling _arguments.
This option makes it so that completion stops after '--' is encountered.
This means that the handling for trailing 'raw' arguments does not need
to specified.

Special-case multi-valued arguments so that we can skip emitting
the final multi-valued argument if a previous multi-valued argument
has already been emitted.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/clap that referenced this issue Jan 10, 2023
zsh completions for commands that have multiple Vec arguments require
special care.

We can have two Vec args separated with a value terminator.
We can also have two Vec args with no value terminators specified
where the final arg uses 'raw' and thus requires '--' to be used.

The 2nd of these scenarios requires special handling to avoid
emitting a duplicate '*:arguments' completion entry.

Currently, the zsh completions generate an error in this scenario:

    $ my-app <TAB>
    _arguments:...: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

We already use the '-S' option when calling _arguments.
This option makes it so that completion stops after '--' is encountered.
This means that the handling for trailing 'raw' arguments does not need
to specified.

Special-case multi-valued arguments so that we can skip emitting
the final multi-valued argument if a previous multi-valued argument
has already been emitted.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
@TD-Sky
Copy link

TD-Sky commented Jan 11, 2023

I believe #4612 may resolve this issue. I ran into this while creating https://github.com/davvid/garden so I figured we might as well fix it (rather than workaround it).

Kindly test it out and let me know if it works for y'all as well.

Have you not fully fixed this feature yet? The completion doesn't break now, but it also does nothing.

Here are the Cargo.toml and the completion script: https://gist.github.com/TD-Sky/59eb28d92f8dae38129182e66da6b00c

@davvid
Copy link
Contributor

davvid commented Jan 13, 2023

@TD-Sky I spent some minutes trying to compile your project and looking into what might be u. It doesn't look like the project you shared (conceal) compiles in the two branches I tried -- main and zsh-complete.

If you have a working branch that that is made worse by these changes then please share the details.

Feel free to take a look at how I'm setting up the subcommands but we probably shouldn't clutter this issue with too many project-specific details. Anyways, here's some working examples if you want to see them:

This is the top-level parser:
https://github.com/davvid/garden/blob/main/src/cli.rs#L50

And this is where the sub-commands are defined:
https://github.com/davvid/garden/blob/main/src/cli.rs#L94

@TD-Sky
Copy link

TD-Sky commented Jan 16, 2023

@TD-Sky I spent some minutes trying to compile your project and looking into what might be u. It doesn't look like the project you shared (conceal) compiles in the two branches I tried -- main and zsh-complete.

If you have a working branch that that is made worse by these changes then please share the details.

Feel free to take a look at how I'm setting up the subcommands but we probably shouldn't clutter this issue with too many project-specific details. Anyways, here's some working examples if you want to see them:

This is the top-level parser: https://github.com/davvid/garden/blob/main/src/cli.rs#L50

And this is where the sub-commands are defined: https://github.com/davvid/garden/blob/main/src/cli.rs#L94

I have tried to compile with your patches in the local part of conceal branch zsh-complete and shared the Cargo.toml and completion script on gist. Should I push the patched version so you can try again?

I have read the code your arguments parser. But you don't use flatten as I did. Is it possible there are two different bugs cause your patches don't work for me?

davvid added a commit to garden-rs/garden that referenced this issue Jan 21, 2023
Related-to: #10
Related-to: clap-rs/clap#3022
Related-to: clap-rs/clap#4624
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to garden-rs/garden that referenced this issue Jan 21, 2023
Now that clap-rs/clap#4624 was merged we can use value_terminator to fix the
zsh completions for commands that use two consecutive multi-valued arguments.

A more complete solution may arrive in clap-rs/clap#4612 but until that is
merged we can use this solution for now. If that change is ever merged then
we can bump our clap_complete version and remove our use of value_terminator.

Related-to: #10
Related-to: clap-rs/clap#3022
Related-to: clap-rs/clap#4624
Signed-off-by: David Aguilar <davvid@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-completion Area: completion generator C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants