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

shell completions do not expand ~ or environment variables #1577

Closed
twpayne opened this issue Jan 9, 2022 · 17 comments
Closed

shell completions do not expand ~ or environment variables #1577

twpayne opened this issue Jan 9, 2022 · 17 comments
Labels
area/shell-completion All shell completions kind/bug A bug in cobra; unintended behavior lifecycle/frozen Prevents GitHub actions from labeling issues / PRs with stale and rotten triage/needs-info Needs more investigation from maintainers or more info from the issue provider

Comments

@twpayne
Copy link
Contributor

twpayne commented Jan 9, 2022

When using fish completions with a cobra.Command.ValidArgsFunction to specify custom completions, leading ~s and environment variables are not expanded before the argument to complete is passed to the ValidArgsFunction.

For example, if the user types the following to trigger shell completion:

$ command arg ~/.z<tab>

ValidArgsFunction is called with the toComplete argument equal to ~/.z. The tilde expansion should be done before this, i.e. the toComplete argument should be /home/user/.z where /home/user is the user's home directory.

Similarly, if the user types:

$ command arg $SOME_VAR/.z

then ValidArgsFunction is called with the toComplete argument equal to $SOME_VAR/.z. Instead, toComplete should have the value with $SOME_VAR expanded.

This is different to other shell completions (e.g. zsh), where ~ and environment variables are expanded by the shell before being passed to ValidArgsFunction.

The expansion of ~ and environment variables must be done in the fish shell itself. It cannot be done by the ValidArgsFunction because:

  • The value of environment variables might not be known. For example, if toComplete contains an unexported environment variable then its value is not available to the process executing ValidArgsFunction.
  • The exact interpretation of ~ depends on the shell. Example common interpretations include ~ for the user's home directory, ~/file for a file in the user's home directory, ~username for a different user's home directory, and ~username/file for a file in a different user's home directory. It is unreasonable for the ValidArgsFunction to emulate fish's behavior exactly.
  • As the fish completions behave differently to other shell completions, implementing fish's logic in ValidArgsFunction would break other shell completions.

Refs twpayne/chezmoi#1796 (comment).

@marckhouzam
Copy link
Collaborator

Thanks for the report @twpayne. I am not able to reproduce the problem. Which version of fish are you using and on which platform? I'm using fish 3.3.1 on MacOS.

Here is a program to try to reproduce. I built it and then ran the following, which seems to expand properly:

$ fish
Welcome to fish, the friendly interactive shell
marckhouzam@fish ~/g/s/g/m/test (master)> ./prog __complete $HOME/bin ~/bin2
[Debug] args: [/Users/marckhouzam/bin], toComplete: /Users/marckhouzam/bin2
:0
Completion ended with directive: ShellCompDirectiveDefault
package main

import (
	"fmt"
	"github.com/spf13/cobra"
)

var rootCmd = &cobra.Command{
	Use: "prog",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		cobra.CompDebugln(fmt.Sprintf("args: %v, toComplete: %v", args, toComplete), true)
		return nil, cobra.ShellCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {},
}

func main() {
	rootCmd.Execute()
}

@twpayne
Copy link
Contributor Author

twpayne commented Jan 10, 2022

Which version of fish are you using and on which platform? I'm using fish 3.3.1 on MacOS.

I'm also using fish 3.3.1 on macOS installed with Homebrew.

Here is a program to try to reproduce. I built it and then ran the following, which seems to expand properly:

This does not reproduce the problem because it does not invoke the tab completion logic in fish.

To reproduce the problem, run the following with the modified Go code below. The modified Go code just adds a completion command to generate the fish completion file. Ensure that prog is in your $PATH.

$ fish
$ prog completion > prog.fish                           # generate fish completion for prog
$ . prog.fish                                           # source fish completion for prog
$ set -x BASH_COMP_DEBUG_FILE /tmp/bash-completion.log  # enable fish completion debugging
$ prog $HOME/<tab><Ctrl-C>                              # invoke tab completion logic
$ cat $BASH_COMP_DEBUG_FILE                             # print debug file

I see the following in $BASH_COMP_DEBUG_FILE:


========= starting completion logic ==========
Starting __prog_perform_completion
args: prog
last arg: '$HOME/'
Calling prog __completeNoDesc  '$HOME/'
[Debug] args: [], toComplete: $HOME/
Comps:
DirectiveLine: :0
flagPrefix:
Completion results: :0
Completions are:
Directive is: 0
nospace: 0, nofiles: 0
prefix: \$HOME/
Filtered completions are:
numComps: 0
Requesting file completion

Note that toComplete is $HOME/, i.e. the $HOME environment variable has not been expanded.

Modified Go code:

package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

var rootCmd = &cobra.Command{
	Use: "prog",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		cobra.CompDebugln(fmt.Sprintf("args: %v, toComplete: %v", args, toComplete), true)
		return nil, cobra.ShellCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {},
}

var completionCmd = &cobra.Command{
	Use: "completion",
	RunE: func(cmd *cobra.Command, args []string) error {
		return rootCmd.GenFishCompletion(os.Stdout, false)
	},
}

func main() {
	rootCmd.AddCommand(completionCmd)
	rootCmd.Execute()
}

@twpayne
Copy link
Contributor Author

twpayne commented Jan 10, 2022

For info, I've asked for help on fish's gitter.

@marckhouzam
Copy link
Collaborator

Sorry for getting back late. The problem is in the fact that cobra's fish script purposefully escapes the arguments, preventing them from being expanded by the shell. See here:

set -l lastArg (string escape -- (commandline -ct))

We made this choice willingly but I didn't realize the impact it would have. See this comment #1249 (comment)

I'll have to look into alternatives.

@twpayne could you point me to the chezmoi code that has problems with this? I'm asking because although variables and ~ are not expanded, file completion still works from the user's perspective. I'm assuming the problem is for the program receiving this unexpanded argument.

@twpayne
Copy link
Contributor Author

twpayne commented Jan 10, 2022

The relevant code is here: https://github.com/twpayne/chezmoi/blob/v2.9.5/internal/cmd/config.go#L1954-L2001, but before you click on the link, please read the following.

There's a bunch of project-specific code in here, but the short version is that chezmoi operates on a subset of files in the user's home directory and this code tries to only suggest autocompletion for files that chezmoi operates on, and not every possible file.

Specific code blocks:

@krobelus
Copy link
Contributor

fish doesn't expand variables because the final completions should still have the variables.
(It could potentially expand before computing completions, and later "unexpand", but that would be a heuristic.)
So user-defined completions will generally not work for tokens that contain $HOME or ~.

chezmoi operates on a subset of files

the usual hack to support that in fish is to invoke complete -C on a command without custom completions to get the file completions, and then filter the results

set -l token (commandline -t)
complete -C "__fish_command_without_completions $token" | grep chezmoi

not sure how to integrate this into Cobra.

The same hack can be used to implement other missing features: fish ships a helper __fish_complete_suffix (which uses that hack) for filtering by file extension, that can be used to implement shellCompDirectiveFilterFileExt
Similarly __fish_complete_directories can be used to implement shellCompDirectiveFilterDirs

@twpayne
Copy link
Contributor Author

twpayne commented Jan 11, 2022

Thank you for this insight @marckhouzam and @krobelus - it all makes sense now.

Re:

fish doesn't expand variables because the final completions should still have the variables. (It could potentially expand before computing completions, and later "unexpand", but that would be a heuristic.) So user-defined completions will generally not work for tokens that contain $HOME or ~.

I think that unexpand can be done precisely, without needing a heuristic. The rough plan would be something like:

  1. The user wants to autocomplete ~/$SOME_VAR/whatever.
  2. fish completions expand this into /home/user/contents_of_some_var/whatever.
  3. fish completions remember during the expansion that ~/ was expanded to /home/user/ and that $SOME_VAR was expanded to contents_of_some_var. This can be be done by remembering the position of the variable in the original string and the position of the expanded value.
  4. When fish needs to translate the autocompletion /home/user/contents_of_some_var/whatever/foo back to an actual completion to present to the user, it can observe that the /home/user/ is at the same position and unchanged and so replace it with ~/ and similarly with contents_of_some_var which can be replaced with $SOME_VAR. It's important that the fish completion logic only re-replaces a substitution if and only if the substitution is for the same exact substring at the same exact position in the original string.
  5. The net result is that the user gets accurate shell completion, with tilde and environment variables are preserved.

Thoughts?

@marckhouzam
Copy link
Collaborator

marckhouzam commented Jan 11, 2022

fish doesn't expand variables because the final completions should still have the variables.

From what I can see, this is fine. Remember that the Cobra completion script does not provide the completions itself but calls the program to get them. Therefore, when calling the program from the script, the variables could be expanded, while keeping them unexpanded for the script.

The issue is that we escape the variables.
To illustrate, let's take the test program and replace the string escape command with string unescape (this makes the sed simpler and string unescape is a no-op in this case):

$ ./prog completion fish | sed s/escape/unescape/ | source
$ ./prog $HOME/bin/<TAB>

In the debug logs I now see what we want:

========= starting completion logic ==========
Starting __prog_perform_completion
args: ./prog
last arg: $HOME/bin/
Calling ./prog __complete  $HOME/bin/
[Debug] args: [], toComplete: /Users/marckhouzam/bin/

But to remove the string escape we will need to explicitly check for an empty last argument. And I'll have to look back at our discussion in #1249 (comment) to see if anything else could be affected.

@krobelus does this make sense, or am I confused?

@marckhouzam
Copy link
Collaborator

I don't think I had fully understood the situation. And I now better understand @krobelus explanation. Before continuing with the fish shell discussion, let's figure out if we may not have a larger "problem" which affects all shells.

Let's take chezmoi as a concrete example, where I couldn't get things to work in any shell.

$ git clone git@github.com:twpayne/chezmoi.git
$ cd chezmoi
$ git checkout v2.9.5
$ make

$ ./chezmoi init
$ ./chezmoi add ~/.bashrc
$ bash
bash-5.1$ source <(./chezmoi completion bash)
bash-5.1$ ./chezmoi cat /U<TAB>
# this completes to
bash-5.1$ ./chezmoi cat /Users/marckhouzam/.bashrc

# But...
bash-5.1$ ./chezmoi cat ~/<TAB>
# does not complete at all

@twpayne I am missing something?

@twpayne
Copy link
Contributor Author

twpayne commented Jan 12, 2022

Let's take chezmoi as a concrete example, where I couldn't get things to work in any shell.
@twpayne I am missing something?

I wouldn't use chezmoi as an example here. chezmoi is a medium-sized CLI with a few custom completion rules, written by someone who is not particularly familiar with shell completions.

Instead, I would recommend using a minimal Go example, like the one that you wrote here. This will ensure that any problems are clearly attributable to cobra/shell completion code, instead of being attributable to the author of chezmoi :)

I'd be happy to extend the minimal Go example to cover a few other cases (e.g. relative paths, absolute paths, custom argument completions, etc.) if that would be helpful.

@marckhouzam
Copy link
Collaborator

I wouldn't use chezmoi as an example here...

Ok, here is a minimal test program (see below). It has a binOnly command which has shell completion that does file completion but where there is a file or directory with bin in the name, it only shows that file/directory.

zsh$ source <(./prog completion zsh)
zsh$ touch ~/mybin ~/my2bin ~/myfile
zsh$ ./prog binOnly /Users/marckhouzam/my<TAB>
/Users/marckhouzam/my2bin  /Users/marckhouzam/mybin

# Above we see that the program filters the completion to only files with 'bin'.
# Explicitly we have:
zsh$ ./prog __complete binOnly /Users/marckhouzam/my
[Debug] args: [], toComplete: /Users/marckhouzam/my
/Users/marckhouzam/my2bin
/Users/marckhouzam/mybin
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

# But let's try with $HOME
zsh$ ./prog __complete binOnly $HOME/my
[Debug] args: [], toComplete: /Users/marckhouzam/my
/Users/marckhouzam/my2bin
/Users/marckhouzam/mybin
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

# We get the same output as before, which seems right,
# BUT:
zsh$ ./prog binOnly $HOME/my<TAB>
# Nothing!
# The filtering didn't work.

The problem is that the shell has $HOME/my at the command-line but the __complete command returns choices that start with /Users/marckhouzam/. The shell then sees the completion choices don't match with $HOME/my so it ignores those choices.

The same thing happens in bash.

So first question @twpayne is that I don't understand why you see a problem only with fish...

Program:

package main

import (
	"fmt"
	"os"
	"path/filepath"
	"strings"

	"github.com/spf13/cobra"
)

var rootCmd = &cobra.Command{
	Use: "prog",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		cobra.CompDebugln(fmt.Sprintf("args: %v, toComplete: %v", args, toComplete), true)
		return nil, cobra.ShellCompDirectiveNoFileComp
	},
	Run: func(cmd *cobra.Command, args []string) {},
}

var binOnlyCmd = &cobra.Command{
	Use: "binOnly",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		cobra.CompDebugln(fmt.Sprintf("args: %v, toComplete: %v", args, toComplete), true)
		dir := filepath.Dir(toComplete)
		if len(dir) == 0 {
			dir = "."
		}
		var comps []string
		if files, err := os.ReadDir(dir); err == nil {
			for _, file := range files {
				if strings.Contains(file.Name(), "bin") {
					comps = append(comps, dir+string(os.PathSeparator)+file.Name())
				}
			}
		}
		return comps, cobra.ShellCompDirectiveNoFileComp
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("Shell completion for this command should only show files that have 'bin' in their name")
	},
}

func main() {
	// Add a sub command for the default completion command to also be added
	rootCmd.AddCommand(binOnlyCmd)
	rootCmd.Execute()
}

@twpayne
Copy link
Contributor Author

twpayne commented Jan 14, 2022

Thanks for the further investigation :)

So first question @twpayne is that I don't understand why you see a problem only with fish...

The user reporting the problem was using fish, I didn't investigate other shells. Interesting that this seems to be a more general problem. I'll update the issue title.

@twpayne twpayne changed the title fish completions do not expand ~ or environment variables shell completions do not expand ~ or environment variables Jan 14, 2022
@marckhouzam
Copy link
Collaborator

The user reporting the problem was using fish, I didn't investigate other shells. Interesting that this seems to be a more general problem. I'll update the issue title.

Makes sense 😀

This issue will require some thought.

@github-actions
Copy link

This issue is being marked as stale due to a long period of inactivity

@twpayne
Copy link
Contributor Author

twpayne commented Mar 16, 2022

I don't think this issue is stale. On the contrary, it communicates a genuine problem that still exists and should not be forgotten.

@jpmcb jpmcb added area/shell-completion All shell completions lifecycle/frozen Prevents GitHub actions from labeling issues / PRs with stale and rotten kind/bug A bug in cobra; unintended behavior triage/needs-info Needs more investigation from maintainers or more info from the issue provider and removed kind/stale labels Mar 16, 2022
@twpayne
Copy link
Contributor Author

twpayne commented Jan 22, 2023

Closing due to inactivity.

@krobelus
Copy link
Contributor

There's a proposed change to fish that will fix this issue.
It will make commandline -opc return readily-expanded arguments.
So given command arg $HOME/.z we'll call something like command __complete arg /home/user/.z.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/shell-completion All shell completions kind/bug A bug in cobra; unintended behavior lifecycle/frozen Prevents GitHub actions from labeling issues / PRs with stale and rotten triage/needs-info Needs more investigation from maintainers or more info from the issue provider
Projects
None yet
Development

No branches or pull requests

4 participants