Parentheses in doc comments for ValueEnum break generated fish completions

[cstyles]

Please complete the following tasks

• I have searched the discussions
• I have searched the open and rejected issues

Rust Version

rustc 1.72.0 (5680fa18f 2023-08-23)

Clap Version

4.4.1

Minimal reproducible code

use clap::CommandFactory;

#[derive(clap::Parser)]
struct MyCommand {
    #[arg(long)]
    color: Color,
}

#[derive(Clone, clap::ValueEnum)]
enum Color {
    /// do something (default)
    Auto,
    /// do not use colorized output
    Never,
}

fn main() {
    clap_complete::generate(
        clap_complete::shells::Fish,
        &mut MyCommand::command(),
        "ls",
        &mut std::io::stdout(),
    );
}

Steps to reproduce the bug with the above code

$ cargo run | source
$ ls --color=<TAB>

Actual Behaviour

$ ls --color=<TAB>fish: Unknown command: default
fish: 
default
^~~~~~^
in command substitution

Expected Behaviour

$ ls --color=<TAB>
--color=auto  (show colors if the output goes to an interactive console (default))
--color=never                                        (do not use colorized output)

Additional Context

The solution might just be to escape parentheses in clap_complete::shells::Fish::escape_string. But A) I'm not sure if there are unintended consequences of doing that and B) the docs for that function mention that it's for escaping single quoted strings which the completions don't use. But it's also used in value_completion to escape a string inside double quotes so maybe that comment is just out-of-date? I didn't feel confident making the change so I'll defer to someone more knowledgeable.

Debug Output

No response

[jporwal05]

I would like to work on this @epage. Please assign this to me.

[jporwal05]

Below is my analysis so far:
The complete command that clap_complete generates for @cstyles's example, for fish shell is this:

complete -c ls -l color -r -f -a "{auto	do something (default)}"

If I go through fish shell's documentation, I don't see any mention of adding description/hint in the -a flag itself. The flag -d suits the purpose. So, if I compose the following command:

complete -c ls -l color -r -f -a auto -d "do something (default)"

And then try the auto-completion:

$ ls --color=<TAB>

I get the expected output:

--color=always  (Use colors)  --color=auto  (do something (default))  --color=never  (Use colors)

without any errors. I will do some more analysis with multiple parameters and their description. From the face value, it seems like separate complete command will be required for parameters that have a description/hint with them. If that is the case, this might be a bigger change/fix than anticipated.

[cstyles]

I don't think it's required that we use separate complete commands, though that might be a good idea. The relevant bit from that documentation page is this:

Each line of output is used as a separate candidate, and anything after a tab is taken as the description.

Which is what clap_complete is using:

clap/clap_complete/src/shells/fish.rs

Lines 171 to 175 in dc63cba

	Some(format!(
	"{}\t{}",
	escape_string(value.get_name(), true).as_str(),
	escape_string(&value.get_help().unwrap_or_default().to_string(), true)
	))

The problem is this, emphasis mine:

The argument to the -a switch is always a single string. At completion time, it will be tokenized on spaces and tabs, and variable expansion, command substitution and other forms of parameter expansion will take place.

I tried a couple different approaches but I think the simplest solution is to wrap the description in single quotes and ditch the comma-escaping. For me, that prevents any expansion inside the description and doesn't require any complicated escaping.

diff --git a/clap_complete/src/shells/fish.rs b/clap_complete/src/shells/fish.rs
index 5a069d35..175ae6aa 100644
--- a/clap_complete/src/shells/fish.rs
+++ b/clap_complete/src/shells/fish.rs
@@ -169,9 +169,9 @@ fn value_completion(option: &Arg) -> String {
                     None
                 } else {
                     Some(format!(
-                        "{}\t{}",
+                        "{}\t'{}'",
                         escape_string(value.get_name(), true).as_str(),
-                        escape_string(&value.get_help().unwrap_or_default().to_string(), true)
+                        escape_string(&value.get_help().unwrap_or_default().to_string(), false)
                     ))
                 })
                 .collect::<Vec<_>>()

[jporwal05]

Valid point @cstyles. I tried few more variations with your solution. Here are my observations:
✓

$ complete -c ls -l color -r -f -a "{auto\t'do something (default)',never\t'no color',always\t'always color'}"
$ ls --color=<TAB>
--color=always  (always color)  --color=auto  (do something (default))  --color=never  (no color)

✕ - I believe this is expected, so this case is fine?

$ complete -c ls -l color -r -f -a "{auto\t'do something %$# (default)',never\t'no &* color',always\t'always!! color'}"
$ ls --color=<TAB>
fish: $# is not supported. In fish, please use 'count $argv'.
complete -c ls -l color -r -f -a "{auto\t'do something %$# (default)',never\t'no &* color',always\t'always!! color'}"

✕ - Same as the previous one.

$ complete -c ls -l color -r -f -a "{auto\t'do something %$ (default)',never\t'no &* color',always\t'always!! color'}"
$ ls --color=<TAB>
fish: $  is not a valid variable in fish.
complete -c ls -l color -r -f -a "{auto\t'do something %$ (default)',never\t'no &* color',always\t'always!! color'}"

✓

$ complete -c ls -l color -r -f -a "{auto\t'do something % (default)',never\t'no &* color',always\t'always!! color'}"
$ ls --color=<TAB>
--color=always  (always!! color)  --color=auto  (do something % (default))  --color=never  (no &* color)

So, in summary, the solution to enclose the help text in '' , without , escaping, should work! I will fix.