Bash completion V2 with completion descriptions

[marckhouzam]

With Cobra supporting shell completion for Bash, Zsh, Fish and Powershell, having a uniform experience across all shells is a desirable feature. With Fish (#1048) , Zsh (#1070) and PowerShell (#1208) sharing the same Go completion logic, their behaviour is strongly aligned.

This PR uses the same Go completion code to power Bash completion.

However, it is important to keep support for legacy bash completion for backwards-compatibility of existing bash custom completions (cmd.BashCompletionFunction and BashCompCustom) which are being used by many programs out there. Therefore this PR also adds support for legacy bash custom completion through Go completions (see more below). Therefore this PR introduces a V2 version of bash completion while retaining the current V1 support.

Completion descriptions:
The simplicity of the proposed bash completion script allowed to also add descriptions to bash completions.

bash-5.0$ helm show [tab][tab]
all     (show all information of the chart)  readme  (show the chart's README)
chart   (show the chart's definition)        values  (show the chart's values)

Descriptions are not actually supported natively in Bash, but I fell upon a Stack Overflow answer that suggested how it could be implemented by the script.
With Fish, Zsh and PowerShell all supporting completion descriptions, I felt it would be a nice addition for Bash (can be disabled).

Maintenance:
Having a v2 version for bash completion while keeping v1 would impose that Cobra maintainers maintain both v1 and v2 versions.
To reduce this maintenance burden, the PR completely replaces bash completion v1 with this new approach.

The current bash completion logic has been extremely useful to many projects over the last 5 years and has been an amazing initiative by @eparis and all maintainers and contributors that have worked on it. It has been the inspiration behind this proposed evolution.

Backwards compatibility
The PR retains backwards compatibility not only in compilation but in also honouring legacy bash custom completion.

To do so, it introduces two new (internal) ShellCompDirective:shellCompDirectiveLegacyCustomComp and shellCompDirectiveLegacyCustomArgsComp which are used to tell the new bash completion script to perform legacy bash completion as was done before, when appropriate.

One complexity is that bash code inserted by the program using Cobra could technically reference any variable or function that was part of the previous bash completion script (e.g., two_word_flags, must_have_one_flag, etc). However, Iooking at kubectl, it seems that the $last_command and $nouns variables should be sufficient. Therefore, the new solution sets those two variables to allow the injected legacy custom bash code to use them.

I have tested this solution with kubectl and have confirmed that its legacy completion continues to work. However, there is still a risk of breaking some completions for some programs out there, if they use other variables or functions of the current bash script.

Here is what I feel are our options for backwards compatibility:

1. Ignore this PR and keep bash completion as is
2. Keep the current bash completion and introduce a V2 using custom Go completions
3. Keep but deprecate the current bash completion and introduce a V2 using custom Go completions which also support legacy custom completions. This would give an easy migration path to programs to move to the V2 solution, while not even having to get rid of their legacy custom completion code. The current bash completion could then be removed in a 2.0 version of Cobra. (commits 1 and 2 of this PR)
4. Replace the current bash completion with the new custom Go completions which also support legacy custom completions almost completely (commits 1, 2 and 3 of this PR)

This PR implements option 2.

Advantages of this new approach:

1. Aligned behaviour across bash, zsh and fish
2. Reduced maintenance
3. Fixes and improvements to completion support would automatically propagate to all four shells
4. New support for completion descriptions for bash
5. Fixed-size completion script of less than 300 lines (in comparaison the v1 script for kubectl is over 12,000 lines)

[Luap99]

Hi @marckhouzam, thank you for your efforts.

The simplicity of the proposed bash completion script allowed to also add descriptions to bash completions.

I like that feature.

I tested this branch and it works very good.
One think i noticed is that there is a problem with the bash completion if they contain a colon or a equal sign.
I defined the completion like this:

// If there is only a recommended value before we reach the equal sign or the colon, then the completion works as expected.
// If the values differ afterwards, completion fails in bash, but works in fish and zsh
rootCmd.Flags().String("equalSignWorks", "", "test custom comp with equal sign")
rootCmd.RegisterFlagCompletionFunc("equalSignWorks", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
	return []string{"first=Comp\tthe first value", "second=Comp\tthe second value", "forth=Comp\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
})

rootCmd.Flags().String("equalSignWorksNot", "", "test custom comp with equal sign")
rootCmd.RegisterFlagCompletionFunc("equalSignWorksNot", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
	return []string{"first=Comp1\tthe first value", "first=Comp2\tthe second value", "first=Comp3\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
})

rootCmd.Flags().String("colonWorks", "", "test custom comp with colon one value works")
rootCmd.RegisterFlagCompletionFunc("colonWorks", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
	return []string{"first:ONE\tthe first value", "second\tthe second value", "forth\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
})

rootCmd.Flags().String("colonWorksNot", "", "test custom comp with colon doesnt work")
rootCmd.RegisterFlagCompletionFunc("colonWorksNot", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
	return []string{"first:ONE\tthe first value", "first:SECOND\tthe second value", "second:Comp1\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
})

I can confirm that this already exists in the current version.

[marckhouzam]

One thing I noticed is that there is a problem with the bash completions if they contain a colon or an equal sign.

Thank you @Luap99 for trying this feature and reporting on it.
I tried your example code and reproduced the problem.

The reason : and = are causing problems is that they are part of the characters that bash considers as word separators. By default both are part of $COMP_WORDBREAKS:

bash-5.0$ echo $COMP_WORDBREAKS
"'><=;|&(:

You can remove them from $COMP_WORDBREAKS if you wish, but it could affect other completion scripts you use.

For example:

• to remove the colon: COMP_WORDBREAKS=${COMP_WORDBREAKS//:}
• to remove the equal: COMP_WORDBREAKS=${COMP_WORDBREAKS//=}
• to remove both: COMP_WORDBREAKS=${COMP_WORDBREAKS//[:=]}

Removing the : may or may not be a good idea depending on if the program uses : as a separator. For example git completion needs COMP_WORDBREAKS to contain the :, although git's completion script will add it back if you remove it (git does not use Cobra). See https://github.com/git/git/blob/a08a83db2bf27f015bec9a435f6d73e223c21c5e/contrib/completion/git-completion.bash#L45

Removing the = has the same risk, but will also prevent the --flag= completion form from working properly. If you don't use the --flag= form you may not care about this.

I believe it would be pretty complex to work around this transparently in Cobra; and causing more complexity is the fact that we don't know if the : should or should not be a delimiter for the program using Cobra.

Do you have an example of this causing your program issues, or were you just doing detailed testing?

We'll probably have to live with it, just like the current Cobra bash completion lives with it.

P.S. You are right that the : seems to work for zsh (I didn't try fish), but the = does cause problems with the --flag= form of completion even for zsh.

[Luap99]

Thank you for your information. I didn't knew $COMP_WORDBREAKS exists. I agree that we should properly not mess with it.

Do you have an example of this causing your program issues, or were you just doing detailed testing?

Unfortunately this a real problem. For example we have the following format for images repo/path/imagename:tag. You can have the same images with different tags.
Since this works currently in the handwritten script i looked it up and found that the shell builtin function __ltrim_colon_completions "$cur" is used there and there is no modification of $COMP_WORDBREAKS.

$ type __ltrim_colon_completions 
__ltrim_colon_completions ist eine Funktion.
__ltrim_colon_completions () 
{ 
    if [[ "$1" == *:* && "$COMP_WORDBREAKS" == *:* ]]; then
        local colon_word=${1%"${1##*:}"};
        local i=${#COMPREPLY[*]};
        while [[ $((--i)) -ge 0 ]]; do
            COMPREPLY[$i]=${COMPREPLY[$i]#"$colon_word"};
        done;
    fi
}

The same goes for the equal form. we have many flags which accept something like this:

• filter=all
• filter=stopped
• filter=running
  or
• label=role:
• label=type:
• label=user:

I found out that this is currently handeld by this function: https://github.com/containers/libpod/blob/master/completions/bash/podman#L301

# __podman_map_key_of_current_option returns `key` if we are currently completing the
# value of a map option (`key=value`) which matches the extglob given as an argument.
# This function is needed for key-specific completions.
__podman_map_key_of_current_option() {
	local glob="$1"

	local key glob_pos
	if [ "$cur" = "=" ] ; then        # key= case
		key="$prev"
		glob_pos=$((cword - 2))
	elif [[ $cur == *=* ]] ; then     # key=value case (OSX)
		key=${cur%=*}
		glob_pos=$((cword - 1))
	elif [ "$prev" = "=" ] ; then
		key=${words[$cword - 2]}  # key=value case
		glob_pos=$((cword - 3))
	else
		return
	fi

	[ "${words[$glob_pos]}" = "=" ] && ((glob_pos--))  # --option=key=value syntax

	[[ ${words[$glob_pos]} == @($glob) ]] && echo "$key"
}

It then gets called here: https://github.com/containers/libpod/blob/master/completions/bash/podman#L2183

I believe it would be pretty complex to work around this transparently in Cobra; and causing more complexity is the fact that we don't know if the : should or should not be a delimiter for the program using Cobra.

Yes you are right we don't know that so my only thought would be to add this as cobra.ShellCompDirective something like:

• ShellCompDirectiveNoEqualBreak
• ShellCompDirectiveNoColonBreak

I know that this would make your script a lot more complicated but i would really like if we could handle this.

[marckhouzam]

Thank you @Luap99, this information is very useful. Your idea of new ShellCompDirectives is interesting as it would give control to the program using Cobra. I will give it some thought and try out different things. If you would like to try out things on your end, don't hesitate.

[marckhouzam]

@Luap99 I pushed a new commit which I believe fixes the problem with the : and the = when they are part of a completion. It was not as difficult as I expected, thanks to your hints. Also, I believe this approach is correct for any program, as Cobra does not do special completion for : or =.

Could you try this new version and let me know if it fixes your problem?

P.S. I haven't yet looked at the escaping problem that you have mentioned. I will soon.

[Luap99]

Could you try this new version and let me know if it fixes your problem?

Yes that works perfectly. Thanks.

[Luap99]

@marckhouzam Thanks. This works perfectly now. No more issues from my side.

[marckhouzam]

This latest version completely replaces the current bash completion solution.
It should be (almost completely) backwards compatible as it support legacy custom completion.

To do so, it introduces two new (internal) ShellCompDirective:

1. shellCompDirectiveLegacyCustomComp
2. shellCompDirectiveLegacyCustomArgsComp

which are used to tell the new bash completion script to perform legacy bash completion as was done before, when appropriate.

One complexity is that bash code inserted by the program using Cobra could technically reference any variable or function that was part of the previous bash completion script (e.g., two_word_flags, must_have_one_flag, etc). However, Iooking at kubectl, it seems that the $last_command and $nouns variables should be sufficient. Therefore, the new solution sets those two variables to allow the injected legacy custom bash code to use them.

I have tested this solution with kubectl and have confirmed that its legacy completion continues to work. However, there is still a risk of breaking some completions for some programs out there, if they use other variables or functions of the current bash script. Here is what I feel are our options for backwards compatibility:

1. Ignore this PR and keep bash completion as is
2. Keep the current bash completion and introduce a V2 using custom Go completions (commit 1 of this PR)
3. Keep but deprecate the current bash completion and introduce a V2 using custom Go completions which also support legacy custom completions. This would give an easy migration path to programs to move to the V2 solution, while not even having to get rid of their legacy custom completion code. The current bash completion could then be removed in a 2.0 version of Cobra. (commits 1 and 2 of this PR)
4. Replace the current bash completion with the new custom Go completions which also support legacy custom completions almost completely (commits 1, 2 and 3 of this PR)

[marckhouzam]

@Luap99 if you are able to test this last version, it would help confirm it is still good.

I believe this is as far as I can go. I will wait for the maintainers to decide if they are interested in including this PR and if so, how they want to go about it. /cc @jharshman @jpmcb

I haven't updated the documentation yet, but that should not be very big.

[Luap99]

@marckhouzam I can confirm this still works for me with and without descriptions. But i do not use any legacy bash completion functions/annotations.

I have one concern though. The API for generating the shell completions scripts would differ even more:

	no desc	with desc
bash	GenBashCompletion	GenBashCompletionWithDesc
zsh	GenZshCompletionNoDesc	GenZshCompletion
fish	GenFishCompletion(...,false)	GenFishCompletion(...,true)
powershell	GenPowershellCompletion	---

I know this is done to keep it backwards compatible, but this is very unintuitive.
I would prefer the fish completion approach.

[marckhouzam]

I can confirm this still works for me with and without descriptions. But i do not use any legacy bash completion functions/annotations.

Thanks for check @Luap99. I just wanted to make sure it still behaved as it did for you before.

I have one concern though. The API for generating the shell completions scripts would differ even more:
I know this is done to keep it backwards compatible, but this is very unintuitive.
I would prefer the fish completion approach.

Yes, this is pretty bad. Three shells, three different API for the program to use. We couldn't have done much worse if we tried 😢 .

The functions GenBashCompletion() and GenZshCompletion() already exist in a Cobra release, so we need to keep them or else we would break compilation for programs. And in Go, as you probably know, we cannot use the same name for another function, even if it has more parameters. So the only improvement on this that I can see is to align bash v2 with zsh and have GenBashCompletion() and GenBashCompletionNoDesc().

However, I felt that automatically enabling descriptions for bash completions for existing programs, would be a little too harsh, so I choose to leave descriptions off like before and require the program to opt-in by explicitly calling GenBashCompletionWithDesc(). This decision may not be right. If this PR gets considered for inclusion, we can see what the maintainers recommend.

[marckhouzam]

Actually, another option is to create a set of new functions that duplicate some existing ones:

GenBashCompletionWithDesc(..., bool)
GenZshCompletionWithDesc(..., bool)
GenFshCompletionWithDesc(..., bool)

or some similar type of name. This would lead to this:

	no desc	with desc
bash	GenBashCompletion() and GenBashCompletionWithDesc(...,false)	GenBashCompletionWithDesc(...,true)
zsh	GenZshCompletionNoDesc() and GenZshCompletionWithDesc(...,false)	GenZshCompletion() and GenZshCompletionWithDesc(...,true)
fish	GenFishCompletion(...,false) and GenFishCompletionWithDesc(...,false)	GenFishCompletion(...,true) and GenFishCompletionWithDesc(...,true)

Not sure if that is better or not.

[Luap99]

What about adding a new function: GenShellCompletion(shellname,ioWriter,desc) and using an switch inside to generate the shell completions script for the passed shell name.
The advantage would be that someone could just call GenShellCompletion with args[0] in their Run function and doesn't have to use an switch like in your example in the shell_completions file.

[marckhouzam]

What about adding a new function: GenShellCompletion(shellname,ioWriter,desc) and using an switch inside to generate the shell completions script for the passed shell name.
The advantage would be that someone could just call GenShellCompletion with args[0] in their Run function and doesn't have to use an switch like in your example in the shell_completions file.

@Luap99 I like the idea, I'll try it out.
And you're making me wonder, since the completion command is kind of standard and becoming larger (with 4 shells supported), maybe it would be interesting for Cobra to provide the completion command automatically (if it is not already provided by the program)? It would lower the barrier to entry for completion, as any program using Cobra would immediately have the completion command available. Kind of like the help command. This would be for another PR though.

[marckhouzam]

maybe it would be interesting for Cobra to provide the completion command automatically (if it is not already provided by the program)?

I've posted a PR for this: #1192