diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 0349d7c984..b5492da6f2 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -34,6 +34,12 @@ jobs: - name: Set PATH run: echo "${GITHUB_WORKSPACE}/.local/bin" >>"${GITHUB_PATH}" + # The completion scripts are run in these to check what they send. Without + # them those tests skip, which is silent: CLI_SHELL_TESTS_REQUIRED below names + # them so that a skip is a failure and the coverage cannot go away unnoticed. + - if: matrix.os == 'ubuntu-24.04' + run: sudo apt-get update && sudo apt-get install -y bash-completion zsh fish + - if: matrix.go == 'stable' && matrix.os == 'ubuntu-24.04' run: make ensure-goimports @@ -42,6 +48,11 @@ jobs: - run: make vet - run: make test + env: + # The shells installed above, and only those: pwsh comes with the + # runner image, so requiring it would turn a change of image into a + # failure here. + CLI_SHELL_TESTS_REQUIRED: ${{ matrix.os == 'ubuntu-24.04' && 'bash,zsh,fish' || '' }} - run: make check-binary-size - if: matrix.go == 'stable' && matrix.os == 'ubuntu-24.04' diff --git a/autocomplete/bash_autocomplete b/autocomplete/bash_autocomplete index 42eb17b8b2..855c59a69b 100755 --- a/autocomplete/bash_autocomplete +++ b/autocomplete/bash_autocomplete @@ -11,15 +11,84 @@ __%[1]s_init_completion() { fi } -__%[1]s_build_completion_request() { - local -a words_before_cursor=("${COMP_WORDS[@]:0:${COMP_CWORD}}") - local current_word="${COMP_WORDS[COMP_CWORD]}" +# Remove one level of shell quoting from a word, the way the shell does before it +# hands a word to a command, and leave the result in __cli_completion_dequoted. +# +# The variables here hold no application name, unlike the functions: a name is free to +# hold a "-" or a ".", which a function name may and a variable name may not. They are +# declared local by the entry point below, so two applications never share one. +# +# Doing it here rather than with eval is what keeps a command line holding $(...) or +# `...` from being executed by pressing the tab key. +__%[1]s_dequote() { + local s="$1" out="" c n quote="" + local i=0 len=${#1} + + while (( i < len )); do + c="${s:i:1}" + if [[ "${quote}" == "'" ]]; then + if [[ "${c}" == "'" ]]; then quote=""; else out="${out}${c}"; fi + elif [[ "${quote}" == '"' ]]; then + if [[ "${c}" == '"' ]]; then + quote="" + elif [[ "${c}" == "\\" ]]; then + i=$(( i + 1 )) + n="${s:i:1}" + # Inside double quotes a backslash only escapes these. + case "${n}" in + '"' | "\\" | '$' | '`') out="${out}${n}" ;; + *) out="${out}\\${n}" ;; + esac + else + out="${out}${c}" + fi + else + case "${c}" in + "'" | '"') quote="${c}" ;; + "\\") i=$(( i + 1 )); out="${out}${s:i:1}" ;; + *) out="${out}${c}" ;; + esac + fi + i=$(( i + 1 )) + done - if [[ "${current_word}" == "-"* ]]; then - printf '%%s %%s --generate-shell-completion' "${words_before_cursor[*]}" "${current_word}" - else - printf '%%s --generate-shell-completion' "${words_before_cursor[*]}" + __cli_completion_dequoted="${out}" +} + +# The request names the completion in its first argument, where a "--" typed on the +# command line cannot turn it into a positional argument of whatever the command runs. +# The word under the cursor is sent as the last argument, empty or not, so that +# "cmd --" and "cmd -- " can be told apart. +# +# It is built as an array rather than as a string to eval, so that a word holding a +# space reaches the command as the single word it is. +# +# The words come from words/cword rather than from COMP_WORDS/COMP_CWORD: bash splits +# the line on COMP_WORDBREAKS, so "--opt=value" is three words in COMP_WORDS, while +# __%[1]s_init_completion puts it back together. The candidates are filtered against +# cur, which comes from there too, so a request built from anything else would ask the +# command about a different word than the one being completed. +__%[1]s_build_completion_request() { + local i cmd + + __%[1]s_dequote "${words[0]}" + cmd="${__cli_completion_dequoted}" + # A command typed as "~/bin/app" has to be run as the path it stands for. eval used + # to do that as a side effect of re-parsing the line, along with everything else on + # it; this is the one expansion worth keeping, and it needs nothing evaluated. The + # raw word decides, because a quoted "~" is not a home directory to the shell either. + if [[ "${words[0]}" == "~" || "${words[0]}" == "~/"* ]]; then + cmd="${HOME}${cmd:1}" fi + __cli_completion_request=("${cmd}" "__complete") + + for (( i = 1; i < cword; i++ )); do + __%[1]s_dequote "${words[i]}" + __cli_completion_request+=("${__cli_completion_dequoted}") + done + + __%[1]s_dequote "${words[cword]-}" + __cli_completion_request+=("${__cli_completion_dequoted}") } # Keep Bash 3 compatibility: associative arrays require Bash 4+, so @@ -42,23 +111,23 @@ __%[1]s_bash_autocomplete() { local words=("${COMP_WORDS[@]}") if [[ "${words[0]}" != "source" ]]; then - local cur opts + local cur prev opts local cword="${COMP_CWORD}" - local request_comp + local __cli_completion_dequoted="" + local -a __cli_completion_request=() + local -a __cli_completion_tokens=() + local -a __cli_completion_descriptions=() COMPREPLY=() cur="${words[$cword]}" __%[1]s_init_completion -n "=:" || return - request_comp="$(__%[1]s_build_completion_request)" - opts=$(eval "${request_comp}" 2>/dev/null) + __%[1]s_build_completion_request + opts=$("${__cli_completion_request[@]}" 2>/dev/null) # Completion output lines use "token:description" format. # Keep token/description in parallel arrays for Bash 3 compatibility. - __cli_completion_tokens=() - __cli_completion_descriptions=() - local line local longest=0 while IFS=$'\n' read -r line; do diff --git a/autocomplete/fish_autocomplete b/autocomplete/fish_autocomplete index 5f2fcd7f6b..3623e96eaf 100644 --- a/autocomplete/fish_autocomplete +++ b/autocomplete/fish_autocomplete @@ -3,15 +3,31 @@ function __%[1]s_perform_completion # Extract all args except the last one set -l args (commandline -opc) - # Extract the last arg (partial input) - set -l lastArg (commandline -ct) - - if string match -q -- "-*" $lastArg - set results ($args[1] $args[2..-1] $lastArg --generate-shell-completion 2> /dev/null) - else - set results ($args[1] $args[2..-1] --generate-shell-completion 2> /dev/null) + # Extract the last arg (partial input), with one level of quoting taken off the + # way the shell would take it off before handing a word to a command. The words + # before it come tokenized, which does that already. + set -l rawArg (commandline -ct) + # string unescape answers with nothing for a word it cannot read, such as one + # ending in a lone backslash. The word as typed is a better answer than no word, + # which would be read as a fresh one. + set -l lastArg (string unescape -- $rawArg; or printf '%%s' $rawArg) + + # The request names the completion in its first argument, where a "--" typed on + # the command line cannot turn it into a positional argument of whatever the + # command runs. The word under the cursor is sent as the last argument, quoted so + # that an empty one is still an argument, which tells "cmd --" from + # "cmd -- ". + # A command typed as "~/bin/app" has to be run as the path it stands for, which + # nothing else here does: the words are taken apart, not evaluated. The word has + # been through the tokenizer by now, so a quoted "~" is expanded too, where the + # shell would leave it alone. + set -l cmd $args[1] + if string match -q -- '~' $cmd; or string match -q -- '~/*' $cmd + set cmd $HOME(string sub -s 2 -- $cmd) end + set results ($cmd __complete $args[2..-1] "$lastArg" 2> /dev/null) + # Remove trailing empty lines for line in $results[-1..1] if test (string trim -- $line) = "" diff --git a/autocomplete/powershell_autocomplete.ps1 b/autocomplete/powershell_autocomplete.ps1 index fee6d0c7d2..6242d81a3d 100644 --- a/autocomplete/powershell_autocomplete.ps1 +++ b/autocomplete/powershell_autocomplete.ps1 @@ -1,9 +1,66 @@ $fn = $($MyInvocation.MyCommand.Name) $name = $fn -replace "(.*)\.ps1$", '$1' Register-ArgumentCompleter -Native -CommandName $name -ScriptBlock { - param($commandName, $wordToComplete, $cursorPosition) - $other = "$wordToComplete --generate-shell-completion" - Invoke-Expression $other | ForEach-Object { + param($wordToComplete, $commandAst, $cursorPosition) + + # One level of quoting is taken off each word, the way the shell would before + # handing it to a command. The value is not expanded: nothing on the command line + # is evaluated to answer a completion, so a "$(...)" reaches the command as the + # text it is rather than being run by pressing the tab key. + function __cliCompletionText($element) { + if ($element -is [System.Management.Automation.Language.StringConstantExpressionAst] -or + $element -is [System.Management.Automation.Language.ExpandableStringExpressionAst]) { + return $element.Value + } + return $element.Extent.Text + } + + $elements = $commandAst.CommandElements + if ($elements.Count -eq 0) { + return + } + + # The command name itself is the shell's to complete, not the command's. + if ($cursorPosition -le $elements[0].Extent.EndOffset) { + return + } + + # The request names the completion in its first argument, where a "--" typed on + # the command line cannot turn it into a positional argument of whatever the + # command runs. The word under the cursor is sent as the last argument, empty or + # not, so that "cmd --" and "cmd -- " can be told apart. + # + # Which word that is comes from the cursor rather than from a comparison with + # $wordToComplete, which PowerShell hands over normalized: an unfinished "hello + # arrives here as "hello", matches no element as written, and would be sent both + # as a word of its own and as the word being completed. Reading the cursor also + # leaves out what follows it, so completing in the middle of a line asks about + # the line up to that point. + # A command typed as "~/bin/app" needs nothing done to it here: PowerShell resolves + # the tilde when it looks the command up, where the three other shells pass the + # word on as written and never find the command. + $command = __cliCompletionText $elements[0] + $words = @() + $word = '' + for ($i = 1; $i -lt $elements.Count; $i++) { + $extent = $elements[$i].Extent + if ($cursorPosition -gt $extent.StartOffset -and $cursorPosition -le $extent.EndOffset) { + $word = __cliCompletionText $elements[$i] + } elseif ($extent.EndOffset -lt $cursorPosition) { + $words += __cliCompletionText $elements[$i] + } + } + + # The word being completed is the last argument, empty or not, which needs the + # argument passing mode PowerShell 7.3 made the default: before it, on Windows, an + # empty argument is dropped on the way to a native command and the request arrives + # a word short, which reads as a different command line rather than as an error. + # Windows PowerShell 5.1 has no such mode and cannot be helped from here. + if (Get-Variable -Name PSNativeCommandArgumentPassing -ErrorAction Ignore) { + $PSNativeCommandArgumentPassing = 'Standard' + } + + & $command __complete @words $word 2>$null | ForEach-Object { $parts = $_.Split(':', 2) if ($parts.Count -eq 2) { $completion = $parts[0].Trim() @@ -13,4 +70,4 @@ Register-ArgumentCompleter -Native -CommandName $name -ScriptBlock { [System.Management.Automation.CompletionResult]::new($_, $_, 'ParameterValue', $_) } } -} \ No newline at end of file +} diff --git a/autocomplete/zsh_autocomplete b/autocomplete/zsh_autocomplete index d24049a72f..f4c4ba4ca9 100644 --- a/autocomplete/zsh_autocomplete +++ b/autocomplete/zsh_autocomplete @@ -5,15 +5,32 @@ compdef _%[1]s %[1]s _%[1]s() { local -a opts # Declare a local array - local current - current=${words[-1]} # -1 means "the last element" - if [[ "$current" == "-"* ]]; then - # Current word starts with a hyphen, so complete flags/options - opts=("${(@f)$(${words[@]:0:#words[@]-1} ${current} --generate-shell-completion)}") - else - # Current word does not start with a hyphen, so complete subcommands - opts=("${(@f)$(${words[@]:0:#words[@]-1} --generate-shell-completion)}") + local -a request + # The request names the completion in its first argument, where a "--" typed on + # the command line cannot turn it into a positional argument of whatever the + # command runs. The word under the cursor is sent as the last argument, empty or + # not, so that "cmd --" and "cmd -- " can be told apart. + # (Q) takes one level of quoting off each word, the way the shell would before + # handing it to a command, so that a quoted word reaches the command as the word + # it is rather than with its quotes. + local raw="${words[CURRENT]}" + local current="${(Q)words[CURRENT]}" + # A word whose quote is still open has no closing quote to take off with it, so + # (Q) leaves it alone. Dropping the opening quote asks the command about the word + # being typed rather than about one starting with a quote character, which is what + # bash and PowerShell send for the same line. + if [[ "$current" == "$raw" && "$raw" == [\"\']* ]]; then + current="${current#[\"\']}" + fi + # A command typed as "~/bin/app" has to be run as the path it stands for, which + # nothing else here does: the words are taken apart, not evaluated. The raw word + # decides, because a quoted "~" is not a home directory to the shell either. + local cmd="${(Q)words[1]}" + if [[ "${words[1]}" == "~" || "${words[1]}" == "~/"* ]]; then + cmd="${HOME}${cmd#\~}" fi + request=("$cmd" "__complete" "${(@Q)words[2,CURRENT-1]}" "$current") + opts=("${(@f)$("${request[@]}" 2>/dev/null)}") if [[ "${opts[1]}" != "" ]]; then _describe 'values' opts diff --git a/command.go b/command.go index 4cd907a558..b17eadbaf2 100644 --- a/command.go +++ b/command.go @@ -157,6 +157,11 @@ type Command struct { didSetupDefaults bool // whether in shell completion mode shellCompletion bool + // what the shell completion request being answered says about the word being + // completed, or nil when this run is answering none. Only the root command holds + // it, and every run replaces it, so a Command answering several requests never + // carries one request's state into the next. + completion *completionRequest // whether global help flag was added globaHelpFlagAdded bool // whether global version flag was added diff --git a/command_run.go b/command_run.go index 8d5907151e..2c706b5ec7 100644 --- a/command_run.go +++ b/command_run.go @@ -118,20 +118,21 @@ func (cmd *Command) run(ctx context.Context, osArgs []string) (_ context.Context osArgs = append(osArgs, args...) } } - // handle the completion flag separately from the flagset since + // handle the completion request separately from the flagset since // completion could be attempted after a flag, but before its value was put // on the command line. this causes the flagset to interpret the completion - // flag name as the value of the flag before it which is undesirable + // request as the value of the flag before it which is undesirable // note that we can only do this because the shell autocomplete function - // always appends the completion flag at the end of the command + // sends the request in a place the flagset never reaches: the first argument, + // or, for a script generated before that change, the last one tracef("checking osArgs %v (cmd=%[2]q)", osArgs, cmd.Name) - cmd.shellCompletion, osArgs = checkShellCompleteFlag(cmd, osArgs) + cmd.shellCompletion, osArgs = parseShellCompleteRequest(cmd, osArgs) - tracef("setting cmd.shellCompletion=%[1]v from checkShellCompleteFlag (cmd=%[2]q)", cmd.shellCompletion && cmd.EnableShellCompletion, cmd.Name) + tracef("setting cmd.shellCompletion=%[1]v from parseShellCompleteRequest (cmd=%[2]q)", cmd.shellCompletion && cmd.EnableShellCompletion, cmd.Name) cmd.shellCompletion = cmd.EnableShellCompletion && cmd.shellCompletion } - tracef("using post-checkShellCompleteFlag arguments %[1]q (cmd=%[2]q)", osArgs, cmd.Name) + tracef("using post-parseShellCompleteRequest arguments %[1]q (cmd=%[2]q)", osArgs, cmd.Name) tracef("setting self as cmd in context (cmd=%[1]q)", cmd.Name) ctx = context.WithValue(ctx, commandContextKey, cmd) @@ -164,11 +165,17 @@ func (cmd *Command) run(ctx context.Context, osArgs []string) (_ context.Context tracef("using post-parse arguments %[1]q (cmd=%[2]q)", args, cmd.Name) if shouldRunCompletion(cmd) { - var beforeErr error - if ctx, beforeErr = runBefore(ctx, commandChain(cmd)); beforeErr != nil { - return ctx, beforeErr + // Everything after "--" is a positional argument of whatever the command runs, + // so there is no completion to run and nothing to prepare for one: a Before + // with a side effect would otherwise fire on every tab key past the + // terminator, for an answer that is always empty. + if !cmd.Root().completionTerminated() { + var beforeErr error + if ctx, beforeErr = runBefore(ctx, commandChain(cmd)); beforeErr != nil { + return ctx, beforeErr + } + runCompletion(ctx, cmd) } - runCompletion(ctx, cmd) return ctx, nil } diff --git a/completion.go b/completion.go index b78dc385fd..ea959cc27c 100644 --- a/completion.go +++ b/completion.go @@ -11,7 +11,18 @@ const ( completionCommandName = "completion" // This flag is supposed to only be used by the completion script itself to generate completions on the fly. + // + // What is deprecated is the request form, not this constant: a request appended to + // the end of the command line is indistinguishable from a positional argument + // after "--", so the scripts name it with completionCommandRequest instead. The + // flag is still understood, so that scripts generated before that change keep + // working, and this stays as the name they send. completionFlag = "--generate-shell-completion" + + // This argument is supposed to only be used by the completion script itself to + // generate completions on the fly. It is the first argument of the request, where + // "--" cannot turn it into a positional argument. + completionCommandRequest = "__complete" ) type renderCompletion func(cmd *Command, appName string) (string, error) diff --git a/completion_shell_test.go b/completion_shell_test.go new file mode 100644 index 0000000000..8199c39aab --- /dev/null +++ b/completion_shell_test.go @@ -0,0 +1,489 @@ +//go:build !windows + +package cli + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// This file checks what the generated completion scripts actually send, by running +// them in the shells they are written for and recording the arguments the command +// receives. Asserting on the text of a script only says that it was generated; a word +// the shell splits, quotes or normalizes differently is a difference the text cannot +// show. +// +// Every shell is skipped when it is not installed, so this adds nothing to a machine +// that has none of them. + +// completionCase is a command line, with the cursor at its end unless the line ends +// in a space, and the arguments the command is expected to receive for it. +type completionCase struct { + name string + line string + // bashWords is what bash puts in COMP_WORDS for line, with the cursor at its end. + // Bash splits on COMP_WORDBREAKS and keeps the quoting as typed, and a driver that + // works the words out for itself tests its own idea of that rather than the + // script's: these were measured in bash 5.3 with a completion function that dumps + // COMP_WORDS. + bashWords []string + // bashSkip says why bash is left out of this line, and is empty when it is not. + // Leaving bashWords out is not the signal: a case that simply forgot them would + // then quietly cover one shell fewer, which is what CLI_SHELL_TESTS_REQUIRED + // prevents a level up, for a shell rather than for a case. + bashSkip string + // pwshLine is line as it would be typed in PowerShell, where it differs. A quoted + // command name is only a command there when the call operator says so: "'app' su" + // is a string followed by a word, not a command being completed. + pwshLine string + want []string +} + +func completionCases() []completionCase { + return []completionCase{ + { + name: "a word being typed", + line: "app su", + bashWords: []string{"app", "su"}, + want: []string{"__complete", "su"}, + }, + { + name: "a fresh word", + line: "app sub ", + bashWords: []string{"app", "sub", ""}, + want: []string{"__complete", "sub", ""}, + }, + { + name: "a flag being typed", + line: "app sub --fl", + bashWords: []string{"app", "sub", "--fl"}, + want: []string{"__complete", "sub", "--fl"}, + }, + { + // COMP_WORDBREAKS holds "=", so bash splits this into three words and has + // to put them back together before asking. + name: "a flag holding its value", + line: "app --opt=va", + bashWords: []string{"app", "--opt", "=", "va"}, + want: []string{"__complete", "--opt=va"}, + }, + { + // One level of quoting comes off, the way the shell takes it off before + // handing a word to a command. + name: "a quoted word", + line: `app sub "hello world" `, + bashWords: []string{"app", "sub", `"hello world"`, ""}, + want: []string{"__complete", "sub", "hello world", ""}, + }, + { + // The quote is still open, so there is no closing quote to take off with + // it. The word is what is being typed, not one starting with a quote. + name: "a word whose quote is still open", + line: `app sub "hello wo`, + bashWords: []string{"app", "sub", `"hello wo`}, + want: []string{"__complete", "sub", "hello wo"}, + }, + { + // Everything after "--" is a positional argument of whatever the command + // runs, and the command is asked about it rather than running it. + name: "past a double dash", + line: "app exec -- git push ", + bashWords: []string{"app", "exec", "--", "git", "push", ""}, + want: []string{"__complete", "exec", "--", "git", "push", ""}, + }, + { + // The command word is a word like any other: one level of quoting comes + // off it too, or the request is sent to a command whose name holds the + // quotes. + name: "a quoted command word", + line: "'app' su", + pwshLine: "& 'app' su", + // Measured the way the words above were, with a completion function that + // records having been called: 'app' su and "app" su never reach it. + bashSkip: "a command word holding a quote matches no compspec, so bash completes it as a file name", + want: []string{"__complete", "su"}, + }, + { + // Answering a completion must not evaluate the command line. The old + // scripts re-parsed it, so a command substitution ran on the tab key. + name: "a command substitution", + line: "app sub $(touch NOPE) ", + bashWords: []string{"app", "sub", "$(touch NOPE)", ""}, + want: []string{"__complete", "sub", "$(touch NOPE)", ""}, + }, + } +} + +// pwshCommandLine is the command line to complete in PowerShell. +func (tc completionCase) pwshCommandLine() string { + if tc.pwshLine != "" { + return tc.pwshLine + } + return tc.line +} + +// TestCompletionScriptsRequest runs the generated scripts in the shells they are +// written for and checks the request each one builds. +func TestCompletionScriptsRequest(t *testing.T) { + // testing.Short is not read here: tests in this package add flags of their own to + // the standard flag set, which leaves testing.Short panicking on a flag set that + // has not been parsed. The shells run in parallel and each one skips when it is + // not installed, so the cost of leaving it in is a few seconds on a machine that + // has all four. + t.Parallel() + + checkRequiredShells(t) + + for _, shell := range []string{"bash", "zsh", "fish", "pwsh"} { + t.Run(shell, func(t *testing.T) { + t.Parallel() + + driver := shellDrivers[shell] + interpreter, err := exec.LookPath(driver.interpreter) + if err != nil { + skipMissingShell(t, shell, driver.interpreter+" is not installed") + } + + render := shellCompletions[shell] + require.NotNil(t, render) + script, err := render(&Command{Name: "app", EnableShellCompletion: true}, "app") + require.NoError(t, err) + + for _, tc := range completionCases() { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + if shell == "bash" && tc.bashSkip != "" { + t.Skip(tc.bashSkip) + } + + dir := t.TempDir() + writeCompletionTestApp(t, dir) + + got := completeInShell(t, shell, interpreter, script, tc, dir, []string{ + "PATH=" + dir + string(os.PathListSeparator) + os.Getenv("PATH"), + }) + assert.Equal(t, tc.want, got) + + assert.NoFileExists(t, filepath.Join(dir, "NOPE"), + "the command line must not be evaluated to answer a completion") + }) + } + }) + } +} + +// completeInShell drives one completion in one shell and returns the arguments the +// command received. dir is where the command line is completed, and env is added to +// the environment the shell runs in. +func completeInShell(t *testing.T, shell, interpreter, script string, tc completionCase, dir string, env []string) []string { + t.Helper() + + driver := shellDrivers[shell] + scriptPath := filepath.Join(dir, "completion."+shell) + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o644)) + argvPath := filepath.Join(dir, "argv") + + cmd := exec.Command(interpreter, driver.args(driver.prelude(t, interpreter)+driver.program(scriptPath, tc))...) + cmd.Dir = dir + cmd.Env = append(append(os.Environ(), "ARGV_LOG="+argvPath), env...) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "driving %s: %s", shell, out) + + got, err := os.ReadFile(argvPath) + require.NoError(t, err, "the completion did not run the command: %s", out) + return strings.Split(strings.TrimSuffix(string(got), "\n"), "\n") +} + +// TestCompletionScriptsTildeCommand checks that a command typed as "~/bin/app" is run +// as the path it stands for. Only PowerShell resolves a tilde while looking a command +// up; the other three pass the word on as written, and bash used to have it expanded +// by the eval this no longer does. +// +// The command is reachable through the tilde alone: it is not on PATH, so a shell that +// passes the word on unchanged finds nothing and the request never arrives. +func TestCompletionScriptsTildeCommand(t *testing.T) { + t.Parallel() + + tc := completionCase{ + line: "~/bin/app su", + // Written with a quoted tilde so that the driver does not expand it: what the + // script receives has to be the tilde bash puts in COMP_WORDS. + bashWords: []string{"~/bin/app", "su"}, + want: []string{"__complete", "su"}, + } + + for _, shell := range []string{"bash", "zsh", "fish", "pwsh"} { + t.Run(shell, func(t *testing.T) { + t.Parallel() + + interpreter, err := exec.LookPath(shellDrivers[shell].interpreter) + if err != nil { + skipMissingShell(t, shell, shellDrivers[shell].interpreter+" is not installed") + } + + render := shellCompletions[shell] + require.NotNil(t, render) + script, err := render(&Command{Name: "app", EnableShellCompletion: true}, "app") + require.NoError(t, err) + + home := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(home, "bin"), 0o755)) + writeCompletionTestApp(t, filepath.Join(home, "bin")) + + // A machine can get its shells from a tool manager that keeps what it + // needs under the real home, where moving HOME takes it away and the + // shell never starts. Nothing about the script can be learned then. + probe := exec.Command(interpreter, shellDrivers[shell].args("exit 0")...) + probe.Env = append(os.Environ(), "HOME="+home) + if out, err := probe.CombinedOutput(); err != nil { + skipMissingShell(t, shell, fmt.Sprintf("%s cannot run with HOME moved: %s", shell, out)) + } + + got := completeInShell(t, shell, interpreter, script, tc, t.TempDir(), []string{"HOME=" + home}) + assert.Equal(t, tc.want, got) + }) + } +} + +// checkRequiredShells fails when CLI_SHELL_TESTS_REQUIRED names a shell this file does +// not know. The variable is there so that coverage cannot go away quietly, which a +// typo in it would undo: a name matching nothing requires nothing. +func checkRequiredShells(t *testing.T) { + t.Helper() + for _, name := range strings.Split(os.Getenv("CLI_SHELL_TESTS_REQUIRED"), ",") { + name = strings.TrimSpace(name) + if name == "" { + continue + } + if _, ok := shellDrivers[name]; !ok { + t.Fatalf("CLI_SHELL_TESTS_REQUIRED names %q, which is not a shell driven here", name) + } + } +} + +// skipMissingShell skips a shell that is not installed, unless it is one the +// environment names as required. A skip is silent, and a machine that has none of the +// four reports the same green as one where every request is right, so a run that is +// meant to cover a shell says which ones and fails when it cannot. +// +// The shells are named rather than required as a group, so that a job requiring what +// it installs is not broken by a shell disappearing from a runner image. +func skipMissingShell(t *testing.T, shell, reason string) { + t.Helper() + for _, required := range strings.Split(os.Getenv("CLI_SHELL_TESTS_REQUIRED"), ",") { + if strings.TrimSpace(required) == shell { + t.Fatalf("%s is required by CLI_SHELL_TESTS_REQUIRED: %s", shell, reason) + } + } + t.Skip(reason) +} + +// writeCompletionTestApp writes the command the completion scripts ask, which records +// the arguments it receives and answers with one candidate. +func writeCompletionTestApp(t *testing.T, dir string) { + t.Helper() + app := "#!/bin/sh\n: > \"$ARGV_LOG\"\nfor a in \"$@\"; do printf '%s\\n' \"$a\" >> \"$ARGV_LOG\"; done\necho candidate\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, "app"), []byte(app), 0o755)) +} + +// shellDriver runs a completion the way its shell would, without a terminal. Each +// shell offers its own way in: what they have in common is that the script under test +// is sourced and the completion for a command line is asked for. +type shellDriver struct { + interpreter string + args func(program string) []string + prelude func(t *testing.T, interpreter string) string + program func(scriptPath string, tc completionCase) string +} + +var shellDrivers = map[string]shellDriver{ + "bash": { + interpreter: "bash", + args: func(p string) []string { return []string{"-c", p} }, + // The script calls the word-splitting helpers of bash-completion, so without + // it there is nothing to drive. + prelude: func(t *testing.T, interpreter string) string { + t.Helper() + var unusable []string + for _, p := range []string{ + "/usr/share/bash-completion/bash_completion", + "/etc/bash_completion", + "/opt/homebrew/share/bash-completion/bash_completion", + "/usr/local/share/bash-completion/bash_completion", + } { + if _, err := os.Stat(p); err != nil { + continue + } + // Finding the file is not the same as being able to use it: + // bash-completion 2.12 and later need bash 4.2, so sourcing it in the + // bash macOS ships leaves the helpers undefined and the script with + // nothing to call. Ask this bash what it ends up with rather than + // assuming that the file is enough, and go on looking when the answer + // is no: an older one further down the list may still work. + usable := exec.Command(interpreter, "-c", ". "+shQuote(p)+ + " >/dev/null 2>&1; declare -F _comp_initialize >/dev/null 2>&1 || declare -F _get_comp_words_by_ref >/dev/null 2>&1") + if err := usable.Run(); err != nil { + unusable = append(unusable, p) + continue + } + return ". " + p + "\n" + } + if len(unusable) > 0 { + skipMissingShell(t, "bash", interpreter+" cannot use the bash-completion in "+strings.Join(unusable, ", ")) + } + skipMissingShell(t, "bash", "bash-completion is not installed") + return "" + }, + program: func(scriptPath string, tc completionCase) string { + // COMP_WORDS and COMP_CWORD are what bash sets before it calls the + // completion function, which is what the script reads. They are written + // out as measured rather than worked out here: quoting them apart or + // letting eval build them would test this driver's idea of what bash does + // with a command line instead of the script's handling of what bash + // actually produces, and eval would run a command substitution on the way. + words := make([]string, 0, len(tc.bashWords)) + for _, w := range tc.bashWords { + words = append(words, shQuote(w)) + } + return fmt.Sprintf(` +. %s +COMP_WORDS=(%s) +COMP_CWORD=%d +COMP_LINE=%s +COMP_POINT=%d +__app_bash_autocomplete +`, shQuote(scriptPath), strings.Join(words, " "), len(tc.bashWords)-1, shQuote(tc.line), len(tc.line)) + }, + }, + "zsh": { + interpreter: "zsh", + args: func(p string) []string { return []string{"-f", "-c", p} }, + prelude: func(*testing.T, string) string { return "" }, + // The completion system is not started, since driving it needs a pseudo + // terminal, so the parts of it the script uses stand in for it and words and + // CURRENT are filled with zsh's own tokenizer. That last part is an assumption + // rather than something checked: unlike the bash words, which are written out + // as measured, these are what (z) makes of the line, which is close to what + // the completion system would pass but not known to be identical. + program: func(scriptPath string, tc completionCase) string { + return fmt.Sprintf(` +compdef() { : } +_describe() { : } +_files() { : } +. %s +line=%s +words=("${(z)line}") +[[ "$line" == *" " ]] && words+=("") +CURRENT=$#words +_app +`, shQuote(scriptPath), shQuote(tc.line)) + }, + }, + "fish": { + interpreter: "fish", + args: func(p string) []string { return []string{"-c", p} }, + prelude: func(*testing.T, string) string { return "" }, + // complete -C asks for the completions of a command line, which is the entry + // point fish itself uses. + program: func(scriptPath string, tc completionCase) string { + return fmt.Sprintf("source %s\ncomplete -C %s\n", fishQuote(scriptPath), fishQuote(tc.line)) + }, + }, + "pwsh": { + interpreter: "pwsh", + args: func(p string) []string { return []string{"-NoProfile", "-Command", p} }, + prelude: func(*testing.T, string) string { return "" }, + // The script registers its completer under the name of the file it is in, so + // its body is registered directly here. TabExpansion2 is what PowerShell calls + // on the tab key. + program: func(scriptPath string, tc completionCase) string { + return fmt.Sprintf(` +$script = Get-Content %s -Raw +$start = $script.IndexOf('-ScriptBlock {') + '-ScriptBlock {'.Length +$body = $script.Substring($start, $script.LastIndexOf('}') - $start) +Register-ArgumentCompleter -Native -CommandName app -ScriptBlock ([scriptblock]::Create($body)) +$line = %s +$null = TabExpansion2 -inputScript $line -cursorColumn $line.Length +`, pwshQuote(scriptPath), pwshQuote(tc.pwshCommandLine())) + }, + }, +} + +// shQuote quotes s for bash and zsh. +func shQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +// fishQuote quotes s for fish. +func fishQuote(s string) string { + return "'" + strings.NewReplacer(`\`, `\\`, "'", `\'`).Replace(s) + "'" +} + +// pwshQuote quotes s for PowerShell. +func pwshQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", "''") + "'" +} + +// TestCompletionScriptsSyntax checks that the generated scripts parse, for an app name +// holding characters a shell reads specially. A name is free to hold a "-" or a "." +// — docker-compose, golangci-lint — which a function name may hold and a variable name +// may not, so a script putting the name in a variable breaks for those apps only, and +// breaks the whole file: sourcing stops at the syntax error, before the completion is +// registered at all. +func TestCompletionScriptsSyntax(t *testing.T) { + t.Parallel() + + // The syntax check for each shell, given the file to read. + checks := map[string]func(string) []string{ + "bash": func(p string) []string { return []string{"-n", p} }, + "zsh": func(p string) []string { return []string{"-n", p} }, + "fish": func(p string) []string { return []string{"-n", p} }, + "pwsh": func(p string) []string { + return []string{ + "-NoProfile", "-Command", + "$errors = $null; $null = [System.Management.Automation.Language.Parser]::ParseFile(" + + pwshQuote(p) + ", [ref]$null, [ref]$errors); if ($errors) { $errors; exit 1 }", + } + }, + } + + // A space is left out: the function names have held the app name since long before + // this file, and a name holding a space breaks those in bash and zsh whatever the + // variables do. + for _, name := range []string{"app", "my-app", "my.app"} { + t.Run(name, func(t *testing.T) { + t.Parallel() + + for _, shell := range []string{"bash", "zsh", "fish", "pwsh"} { + t.Run(shell, func(t *testing.T) { + t.Parallel() + + interpreter, err := exec.LookPath(shellDrivers[shell].interpreter) + if err != nil { + skipMissingShell(t, shell, shellDrivers[shell].interpreter+" is not installed") + } + + render := shellCompletions[shell] + require.NotNil(t, render) + script, err := render(&Command{Name: name, EnableShellCompletion: true}, name) + require.NoError(t, err) + + dir := t.TempDir() + p := filepath.Join(dir, "completion."+shell) + require.NoError(t, os.WriteFile(p, []byte(script), 0o644)) + + out, err := exec.Command(interpreter, checks[shell](p)...).CombinedOutput() + require.NoError(t, err, "the %s script for %q does not parse: %s", shell, name, out) + }) + } + }) + } +} diff --git a/completion_test.go b/completion_test.go index 8550f6b41a..98f61f7216 100644 --- a/completion_test.go +++ b/completion_test.go @@ -242,7 +242,10 @@ func TestCompletionFishFormat(t *testing.T) { r.Contains(output, "(__myapp_perform_completion)", "completion function should be registered") } -func TestCompletionFishOmitsPositionalTokenFromDynamicCompletion(t *testing.T) { +func TestCompletionFishSendsTokenBeingCompleted(t *testing.T) { + // The word under the cursor is part of the request, quoted so that an empty one + // is still an argument: without it, "cmd --" and "cmd -- " would reach + // the command as the same request. cmd := &Command{ Name: "myapp", EnableShellCompletion: true, @@ -256,12 +259,16 @@ func TestCompletionFishOmitsPositionalTokenFromDynamicCompletion(t *testing.T) { output, err := fishRender(cmd, "myapp") r.NoError(err) - r.Contains(output, `if string match -q -- "-*" $lastArg`) - r.Contains(output, "set results ($args[1] $args[2..-1] $lastArg --generate-shell-completion 2> /dev/null)") - r.Contains(output, "set results ($args[1] $args[2..-1] --generate-shell-completion 2> /dev/null)") + r.Contains(output, `set -l lastArg (string unescape -- $rawArg; or printf '%s' $rawArg)`) + r.Contains(output, `set results ($cmd __complete $args[2..-1] "$lastArg" 2> /dev/null)`) + r.NotContains(output, completionFlag, "the deprecated request form must not be generated") } -func TestCompletionBashOmitsPositionalTokenFromDynamicCompletion(t *testing.T) { +func TestCompletionBashSendsTokenBeingCompleted(t *testing.T) { + // The word under the cursor is part of the request, empty or not: without it, + // "cmd --" and "cmd -- " would reach the command as the same request. + // The request is an array rather than a string to eval, so a word holding a space + // or a quote reaches the command as the single word it is. cmd := &Command{ Name: "myapp", EnableShellCompletion: true, @@ -275,9 +282,58 @@ func TestCompletionBashOmitsPositionalTokenFromDynamicCompletion(t *testing.T) { output, err := bashRender(cmd, "myapp") r.NoError(err) - r.Contains(output, `if [[ "${current_word}" == "-"* ]]; then`) - r.Contains(output, `printf '%s %s --generate-shell-completion' "${words_before_cursor[*]}" "${current_word}"`) - r.Contains(output, `printf '%s --generate-shell-completion' "${words_before_cursor[*]}"`) + r.Contains(output, `__myapp_dequote "${words[0]}"`) + r.Contains(output, `__cli_completion_request=("${cmd}" "__complete")`) + r.NotContains(output, "__myapp_completion_request", + "an app name belongs in a function name, which may hold a \"-\", not in a variable name") + r.Contains(output, `__myapp_dequote "${words[cword]-}"`) + r.Contains(output, `opts=$("${__cli_completion_request[@]}" 2>/dev/null)`) + r.Contains(output, `for (( i = 1; i < cword; i++ )); do`, + "the request must come from the words __myapp_init_completion reassembled, not from COMP_WORDS") + r.NotContains(output, `eval "`, "the request must not go through eval") + r.NotContains(output, completionFlag, "the deprecated request form must not be generated") +} + +func TestCompletionZshSendsTokenBeingCompleted(t *testing.T) { + cmd := &Command{ + Name: "myapp", + EnableShellCompletion: true, + } + + r := require.New(t) + + zshRender := shellCompletions["zsh"] + r.NotNil(zshRender, "zsh completion renderer should exist") + + output, err := zshRender(cmd, "myapp") + r.NoError(err) + + r.Contains(output, `request=("$cmd" "__complete" "${(@Q)words[2,CURRENT-1]}" "$current")`) + r.Contains(output, `opts=("${(@f)$("${request[@]}" 2>/dev/null)}")`, + "a command writing to stderr must not break the prompt") + r.NotContains(output, completionFlag, "the deprecated request form must not be generated") +} + +func TestCompletionPowershellSendsTokenBeingCompleted(t *testing.T) { + cmd := &Command{ + Name: "myapp", + EnableShellCompletion: true, + } + + r := require.New(t) + + pwshRender := shellCompletions["pwsh"] + r.NotNil(pwshRender, "pwsh completion renderer should exist") + + output, err := pwshRender(cmd, "myapp") + r.NoError(err) + + r.Contains(output, `& $command __complete @words $word 2>$null`) + r.Contains(output, `$PSNativeCommandArgumentPassing = 'Standard'`, + "an empty word has to survive the way to the command") + r.Contains(output, `if ($cursorPosition -gt $extent.StartOffset -and $cursorPosition -le $extent.EndOffset) {`, + "the word being completed must come from the cursor, not from a comparison with $wordToComplete") + r.NotContains(output, completionFlag, "the deprecated request form must not be generated") } func TestCompletionSubcommand(t *testing.T) { @@ -569,3 +625,348 @@ func TestCompletionShellWriteError(t *testing.T) { err := cmd.Run(buildTestContext(t), []string{"foo", completionCommandName, shellName}) assert.ErrorContains(t, err, "writer error") } + +// TestCompletionRequestNeverRunsAction is the regression test for +// https://github.com/urfave/cli/issues/1993: a shell asking for completions must +// never run the command, whatever the command line holds. A request appended to the +// end of the command line cannot promise that, because "--" turns it into a +// positional argument that a wrapper command is entitled to pass on, which is what +// https://github.com/urfave/cli/issues/1932 asked for. +func TestCompletionRequestNeverRunsAction(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + { + name: "plain", + args: []string{"foo", completionCommandRequest, "exec", ""}, + }, + { + name: "after a double dash", + args: []string{"foo", completionCommandRequest, "exec", "--", "rm", "-rf"}, + }, + { + name: "completing the double dash", + args: []string{"foo", completionCommandRequest, "exec", "--"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ran := false + out := &bytes.Buffer{} + cmd := &Command{ + EnableShellCompletion: true, + Writer: out, + Commands: []*Command{ + { + Name: "exec", + SkipFlagParsing: true, + Action: func(context.Context, *Command) error { + ran = true + return nil + }, + }, + }, + } + + r := require.New(t) + r.NoError(cmd.Run(buildTestContext(t), tc.args)) + r.False(ran, "the action must not run for a completion request") + }) + } +} + +// TestCompletionRequestAfterDoubleDash checks that the words after a "--" get no +// suggestion: they are positional arguments of whatever the command runs, so this +// command's flags and subcommands are no answer to them. The "--" being completed is +// not one of them. +func TestCompletionRequestAfterDoubleDash(t *testing.T) { + for _, tc := range []struct { + name string + args []string + expected string + }{ + { + // The completion is for the word after "exec", which has no subcommand of + // its own to offer beyond the built-in help. + name: "before the double dash", + args: []string{"foo", completionCommandRequest, "exec", ""}, + expected: "help:Shows a list of commands or help for one command\n", + }, + { + name: "the double dash itself", + args: []string{"foo", completionCommandRequest, "exec", "--"}, + expected: "--excitement\n--help:show help\n", + }, + { + name: "after the double dash", + args: []string{"foo", completionCommandRequest, "exec", "--", "git", "pu"}, + expected: "", + }, + { + name: "a flag after the double dash", + args: []string{"foo", completionCommandRequest, "exec", "--", "git", "--ver"}, + expected: "", + }, + } { + t.Run(tc.name, func(t *testing.T) { + out := &bytes.Buffer{} + cmd := &Command{ + EnableShellCompletion: true, + Writer: out, + Commands: []*Command{ + { + Name: "exec", + Flags: []Flag{&BoolFlag{Name: "excitement"}}, + Action: func(context.Context, *Command) error { return nil }, + }, + }, + } + + r := require.New(t) + r.NoError(cmd.Run(buildTestContext(t), tc.args)) + r.Equal(tc.expected, out.String()) + }) + } +} + +// TestCompletionDeprecatedRequestPassedOnAfterDoubleDash is the regression test for +// https://github.com/urfave/cli/issues/1932: after a "--", the deprecated request +// form is a positional argument, so a wrapper command passes it on to whatever it +// runs instead of answering it. That command, run by the wrapper, is the one the +// shell was asking about. +func TestCompletionDeprecatedRequestPassedOnAfterDoubleDash(t *testing.T) { + var got []string + out := &bytes.Buffer{} + cmd := &Command{ + EnableShellCompletion: true, + Writer: out, + Commands: []*Command{ + { + Name: "exec", + SkipFlagParsing: true, + Action: func(_ context.Context, cmd *Command) error { + got = cmd.Args().Slice() + return nil + }, + }, + }, + } + + r := require.New(t) + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", "exec", "--", "child", completionFlag})) + r.Equal([]string{"--", "child", completionFlag}, got) + r.Empty(out.String(), "the wrapper must not answer a request meant for what it runs") +} + +// TestCompletionRequestKeepsArgsShape checks that a ShellComplete function sees the +// same cmd.Args() under both request forms: the word being completed is part of them +// when it starts with "-", and left out otherwise. +func TestCompletionRequestKeepsArgsShape(t *testing.T) { + for _, tc := range []struct { + name string + args []string + expected string + }{ + { + name: "deprecated form completing a flag", + args: []string{"foo", "sub", "arg", "-", completionFlag}, + expected: "[arg -]\n", + }, + { + name: "request completing a flag", + args: []string{"foo", completionCommandRequest, "sub", "arg", "-"}, + expected: "[arg -]\n", + }, + { + name: "deprecated form completing a word", + args: []string{"foo", "sub", "arg", completionFlag}, + expected: "[arg]\n", + }, + { + name: "request completing a word", + args: []string{"foo", completionCommandRequest, "sub", "arg", "wor"}, + expected: "[arg]\n", + }, + } { + t.Run(tc.name, func(t *testing.T) { + out := &bytes.Buffer{} + cmd := &Command{ + EnableShellCompletion: true, + Writer: out, + Commands: []*Command{ + { + Name: "sub", + ShellComplete: func(_ context.Context, cmd *Command) { + fmt.Fprintf(cmd.Root().Writer, "%v\n", cmd.Args().Slice()) + }, + Action: func(context.Context, *Command) error { return nil }, + }, + }, + } + + r := require.New(t) + r.NoError(cmd.Run(buildTestContext(t), tc.args)) + r.Equal(tc.expected, out.String()) + }) + } +} + +// TestCompletionRequestStateIsPerRun checks that a Command answering several requests +// carries no state from one into the next. A shell runs one request per process, but +// a test, a REPL or an embedded use answers several through the same Command. +func TestCompletionRequestStateIsPerRun(t *testing.T) { + out := &bytes.Buffer{} + cmd := &Command{ + EnableShellCompletion: true, + Writer: out, + Commands: []*Command{ + { + Name: "exec", + Flags: []Flag{&BoolFlag{Name: "excitement"}}, + Action: func(context.Context, *Command) error { return nil }, + }, + }, + } + + r := require.New(t) + + // A request past a "--" gets no suggestion, and records that. + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "exec", "--", "git", "pu"})) + r.Empty(out.String()) + + // The next request is a different one, and is answered on its own terms. + out.Reset() + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "exec", "-"})) + r.Equal("--excitement\n--help:show help\n", out.String()) + + // The same holds for a request in the deprecated form, which says nothing about + // the word being completed and so must not read what an earlier one said. + out.Reset() + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "exec", "--", "git", "pu"})) + r.Empty(out.String()) + out.Reset() + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", "exec", "-", completionFlag})) + r.Equal("--excitement\n--help:show help\n", out.String()) +} + +// TestCompletionCustomShellCompleteNotRunPastDoubleDash checks that a command +// carrying a ShellComplete of its own suggests nothing past a "--" without having to +// know about "--": the words there are positional arguments of whatever it runs. +func TestCompletionCustomShellCompleteNotRunPastDoubleDash(t *testing.T) { + ran := false + out := &bytes.Buffer{} + cmd := &Command{ + EnableShellCompletion: true, + Writer: out, + Commands: []*Command{ + { + Name: "exec", + ShellComplete: func(_ context.Context, cmd *Command) { + ran = true + fmt.Fprintln(cmd.Root().Writer, "custom") + }, + Action: func(context.Context, *Command) error { return nil }, + }, + }, + } + + r := require.New(t) + + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "exec", "--", "git", "pu"})) + r.False(ran, "the completion func must not run past a double dash") + r.Empty(out.String()) + + // It is the "--" that stops it, not the command. + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "exec", "pu"})) + r.True(ran) + r.Equal("custom\n", out.String()) +} + +// TestCompletionRequestIgnoredWhenDisabled checks that the request form means nothing +// to an app that has not enabled shell completion: the first argument reaches it as +// the positional argument it wrote. +func TestCompletionRequestIgnoredWhenDisabled(t *testing.T) { + var got []string + out := &bytes.Buffer{} + cmd := &Command{ + Writer: out, + Action: func(_ context.Context, cmd *Command) error { + got = cmd.Args().Slice() + return nil + }, + } + + r := require.New(t) + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "bar"})) + r.Equal([]string{completionCommandRequest, "bar"}, got) + r.Empty(out.String()) +} + +// TestCompletionRequestNestedSubcommand checks that a request is answered by the +// command it names however deep that is, rather than by the one above it. +func TestCompletionRequestNestedSubcommand(t *testing.T) { + out := &bytes.Buffer{} + cmd := &Command{ + EnableShellCompletion: true, + Writer: out, + Commands: []*Command{ + { + Name: "one", + Commands: []*Command{ + { + Name: "two", + Flags: []Flag{&BoolFlag{Name: "deep"}}, + Action: func(context.Context, *Command) error { return nil }, + }, + }, + }, + }, + } + + r := require.New(t) + + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "one", ""})) + r.Equal("two\nhelp:Shows a list of commands or help for one command\n", out.String()) + + out.Reset() + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "one", "two", "-"})) + r.Equal("--deep\n--help:show help\n", out.String()) + + // The word being completed is a flag of the command it follows, even with a + // positional argument in between, which the arguments alone could not say. + out.Reset() + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "one", "two", "arg", "--de"})) + r.Equal("--deep\n", out.String()) +} + +// TestCompletionBeforeNotRunPastDoubleDash checks that a Before is not run for a +// request past a "--": there is no completion to prepare for, since the words there +// belong to whatever the command runs, and a Before with a side effect would fire on +// every tab key for an answer that is always empty. +func TestCompletionBeforeNotRunPastDoubleDash(t *testing.T) { + ran := 0 + out := &bytes.Buffer{} + cmd := &Command{ + EnableShellCompletion: true, + Writer: out, + Before: func(ctx context.Context, _ *Command) (context.Context, error) { + ran++ + return ctx, nil + }, + Commands: []*Command{ + { + Name: "exec", + Action: func(context.Context, *Command) error { return nil }, + }, + }, + } + + r := require.New(t) + + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "exec", "--", "git", "pu"})) + r.Zero(ran, "Before must not run for a request past a double dash") + r.Empty(out.String()) + + r.NoError(cmd.Run(buildTestContext(t), []string{"foo", completionCommandRequest, "exec", "pu"})) + r.Equal(1, ran, "Before still runs for a request the command answers") +} diff --git a/docs/v3/examples/completions/customizations.md b/docs/v3/examples/completions/customizations.md index 96f3565359..522cec4461 100644 --- a/docs/v3/examples/completions/customizations.md +++ b/docs/v3/examples/completions/customizations.md @@ -105,13 +105,15 @@ func main() { } ``` -#### Customization +#### The completion request -The default shell completion flag (`--generate-shell-completion`) is defined as -`cli.EnableShellCompletion`, and may be redefined if desired, e.g.: +Setting `cli.EnableShellCompletion` makes the app answer a completion request, which the +generated scripts send as a `__complete` first argument followed by the words typed so far and +the word being completed. That name is fixed: an app cannot rename it, since the completion script +and the app have to agree on it. ```go diff --git a/docs/v3/examples/completions/shell-completions.md b/docs/v3/examples/completions/shell-completions.md index 8c4137e598..217e33d3c9 100644 --- a/docs/v3/examples/completions/shell-completions.md +++ b/docs/v3/examples/completions/shell-completions.md @@ -7,7 +7,9 @@ search: The urfave/cli v3 library supports programmable completion for apps utilizing its framework. This means that the completion is generated dynamically at runtime by invoking the app itself with a special hidden -flag. The urfave/cli searches for this flag and activates a different flow for command paths than regular flow +first argument, `__complete`, followed by the words typed so far and, as the last argument, the word being +completed. The urfave/cli searches for that argument and activates a different flow for command paths than +regular flow. The following shells are supported - bash @@ -115,6 +117,56 @@ The procedure for other shells is similar to bash though the specific paths for shells may vary. Some of the sections below detail the setup need for other shells as well as examples in those shells. +#### `__complete` is reserved + +Setting `EnableShellCompletion` reserves `__complete` as the first argument of your app: a run +starting with it is answered as a completion request rather than passed on, and the words after it +are read as the command line being completed. An app that takes free-form positional arguments +therefore cannot receive `__complete` as its first one. An app that declares a command of that name +keeps it, and stops being completable in exchange. + +#### Nothing is completed after a `--` + +The words after a `--` are positional arguments of whatever your app runs with them, so urfave/cli +answers a request for one with no candidates and does not run your `ShellComplete` at all. A command +that wraps another one therefore cannot hand its completions on: `myapp exec -- git pu` offers +nothing rather than what `git` would offer. What it does do is leave your app's action alone, which +is what a shell asking for completions needs. + +#### What the shells cannot answer + +Two command lines have no answer, whichever shell you use. + +A word after a `--` that a flag took as its value is read as a word after a terminator, so +`myapp --separator -- ` offers nothing. Which flags take a value is known once the flags are +parsed, and the request is read before that. + +Windows PowerShell 5.1 drops an empty argument on the way to a native command, and PowerShell 7.0 +to 7.2 does the same on Windows unless `$PSNativeCommandArgumentPassing` is `Standard`, which the +generated script sets where it can. Without it the word being completed goes missing from the +request and the command answers as though the line ended a word earlier. PowerShell 7.3 and later +need nothing. + +#### Regenerate the script after upgrading + +Completion scripts generated before urfave/cli asked for completions with `__complete` end their request +with a `--generate-shell-completion` flag instead. Those scripts keep working, but a command line holding +a `--` cannot be answered through them: after `--` only positional arguments are accepted, so the flag +belongs to whatever the app runs rather than to the app itself, and the app runs instead of completing +(see [#1932](https://github.com/urfave/cli/issues/1932) and +[#1993](https://github.com/urfave/cli/issues/1993)). Regenerating the script and sourcing it again is what +resolves that: `__complete` is the first argument, where a `--` typed later on the command line can no +longer turn it into a positional argument. + +Until the script is regenerated, pressing tab on such a line runs your app. The flag is not what +stops it: after a `--` it is a positional argument, so it reaches your action along with everything +else on the line. Whether the run gets that far is up to your app. Declaring how many arguments a +command takes does not decide it, since an argument too many is not an error here; a typed argument +list rejects the flag as the number it is not, and an action that looks at what it was given can +turn it down. None of that is something to rely on to catch a tab key. A command that +passes its arguments on hands the flag to what it runs, which is what makes it the right reading for +a wrapper, and the wrong one for a tab key. Regenerating the script is what separates the two. + #### Default auto-completion ```go diff --git a/help.go b/help.go index 4bedf87d5d..8100bd7651 100644 --- a/help.go +++ b/help.go @@ -255,11 +255,20 @@ func DefaultCompleteWithFlags(ctx context.Context, cmd *Command) { } else { tracef("running default complete with os.Args flags[%v]", args) } - argsLen := len(args) + + if cmd == nil { + return + } + lastArg := "" - // parent command will have --generate-shell-completion so we need - // to account for that - if argsLen > 1 { + if req := cmd.Root().completion; req != nil && req.wordKnown { + // The request says which word is being completed, so there is nothing to work + // out from the position of the arguments. + lastArg = req.word + } else if argsLen := len(args); argsLen > 1 { + // A request in the deprecated form leaves the word out unless it starts with + // "-", and the parent command still has completionFlag on it, so the word is + // looked for one before the end. lastArg = args[argsLen-2] } else if argsLen > 0 { lastArg = args[argsLen-1] @@ -275,11 +284,8 @@ func DefaultCompleteWithFlags(ctx context.Context, cmd *Command) { return } - if cmd != nil { - tracef("printing command suggestions on command %[1]q", cmd.Name) - printCommandSuggestions(cmd.Commands, cmd.Root().Writer) - return - } + tracef("printing command suggestions on command %[1]q", cmd.Name) + printCommandSuggestions(cmd.Commands, cmd.Root().Writer) } // ShowCommandHelpAndExit exits with code after showing help via ShowCommandHelp. @@ -471,11 +477,64 @@ func checkVersion(cmd *Command) bool { return cmd.versionFlag != nil && cmd.versionFlag.IsSet() } -func checkShellCompleteFlag(c *Command, arguments []string) (bool, []string) { +// completionRequest is what a shell completion request says about the word being +// completed. +type completionRequest struct { + // word is the word the shell is completing. wordKnown says whether the request + // carried it: the deprecated request form does not. + word string + wordKnown bool + // terminated says whether a "--" precedes the word, which makes that word a + // positional argument of whatever the command runs rather than one this command + // has any suggestion for. + // + // A "--" a flag takes as its value counts here too, and is not one: telling them + // apart needs to know which flags take a value, which is known after the flags are + // parsed, and this is read before. Completing "app --sep -- s" therefore offers + // nothing. Erring this way costs a suggestion; erring the other way runs the + // command on the tab key. + terminated bool +} + +// parseShellCompleteRequest reports whether arguments are a shell completion request +// and returns the arguments to parse. What the request says about the word being +// completed is recorded on c, which is the root command. +// +// Two request forms are understood. The current one names the request up front: +// +// __complete ... +// +// The completion scripts send every word before the cursor, then the word under the +// cursor, which is the empty string when the cursor sits on a fresh word. Naming the +// request in the first argument is what keeps it out of reach of "--": everything +// after that terminator is a positional argument, so a request appended at the end of +// the command line cannot be told apart from a positional argument that happens to +// look like one. See the deprecated form below for what that ambiguity costs. +// +// The deprecated form appends completionFlag to the command line. Scripts generated +// before this change still use it, so it keeps working, with one caveat it cannot +// escape: a command line holding "--" is answered as an ordinary run rather than as a +// completion, because after "--" the flag is a positional argument that belongs to +// whatever the command runs. That is what a wrapper command needs (see +// https://github.com/urfave/cli/issues/1932), and it is why a shell that appends the +// flag after a "--" runs the command instead of completing it (see +// https://github.com/urfave/cli/issues/1993). Regenerating the completion script and +// sourcing it again resolves that in favor of completing, since the request is then +// no longer something a command line can imitate. +func parseShellCompleteRequest(c *Command, arguments []string) (bool, []string) { + // Whatever the previous run of this Command recorded says nothing about this one. + c.completion = nil + if (c.parent == nil && !c.EnableShellCompletion) || (c.parent != nil && !c.Root().shellCompletion) { return false, arguments } + // A command of that name, if the app happens to have one, is what was asked for: + // the request form is understood only where it shadows nothing. + if len(arguments) > 1 && arguments[1] == completionCommandRequest && c.Command(completionCommandRequest) == nil { + return true, c.parseCompletionRequest(arguments) + } + pos := len(arguments) - 1 lastArg := arguments[pos] @@ -483,18 +542,56 @@ func checkShellCompleteFlag(c *Command, arguments []string) (bool, []string) { return false, arguments } - // If arguments include "--" before the token being completed, shell completion - // is disabled because after "--" only positional arguments are accepted. + // The word being completed is at position pos-1, immediately before + // completionFlag, so only the arguments before that position are checked and + // completing "--" itself still works. // https://unix.stackexchange.com/a/11382 - // Note: The token being completed is at position pos-1 (immediately before completionFlag). - // We only check arguments before that position, so completing "--" itself still works. if pos >= 1 && slices.Contains(arguments[:pos-1], "--") { - return false, arguments[:pos] + // The flag is a positional argument here, so it is left in place for the + // command to pass on, and the command runs. + return false, arguments } + // This request form does not say which word is being completed, so + // DefaultCompleteWithFlags works it out from the arguments. + c.completion = &completionRequest{} return true, arguments[:pos] } +// parseCompletionRequest records what a request naming completionCommandRequest says +// about the word being completed, and returns the arguments to parse. +// +// The word is kept in those arguments when it starts with "-", and dropped from them +// otherwise, which is the shape the deprecated request form produced. A ShellComplete +// function reading cmd.Args() therefore sees the same thing under both forms. +func (cmd *Command) parseCompletionRequest(arguments []string) []string { + // arguments[0] is the program, arguments[1] is completionCommandRequest, and the + // word being completed is last. A request holding neither, which no script sends, + // is read as an empty word on an empty command line. + var words []string + word := "" + if len(arguments) > 2 { + words = arguments[2 : len(arguments)-1] + word = arguments[len(arguments)-1] + } + cmd.completion = &completionRequest{ + word: word, + wordKnown: true, + // Everything after a "--" is a positional argument of whatever the command + // runs. A "--" being completed is not one: it is the word itself, and flags + // still answer it. + terminated: slices.Contains(words, "--"), + } + + args := make([]string, 0, len(arguments)-1) + args = append(args, arguments[0]) + args = append(args, words...) + if strings.HasPrefix(word, "-") { + args = append(args, word) + } + return args +} + func shouldRunCompletion(cmd *Command) bool { tracef("checking completions on command %[1]q", cmd.Name) @@ -515,6 +612,18 @@ func shouldRunCompletion(cmd *Command) bool { return true } +// completionTerminated reports whether the completion request being answered has a +// "--" before the word being completed. The words there are positional arguments of +// whatever the command runs, so this command has no suggestion for them, and its +// completion func is not run at all: keeping that here rather than in the func +// applies it to every command, including one carrying a ShellComplete of its own, +// which would otherwise have to know about a "--" the parsed arguments no longer show. +// https://unix.stackexchange.com/a/11382 +func (cmd *Command) completionTerminated() bool { + req := cmd.Root().completion + return req != nil && req.terminated +} + func runCompletion(ctx context.Context, cmd *Command) { if cmd.ShellComplete != nil { tracef("running shell completion func for command %[1]q", cmd.Name) diff --git a/help_test.go b/help_test.go index d3c831371d..ed06c582f7 100644 --- a/help_test.go +++ b/help_test.go @@ -1884,7 +1884,7 @@ GLOBAL OPTIONS: `, output.String()) } -func Test_checkShellCompleteFlag(t *testing.T) { +func Test_parseShellCompleteRequest(t *testing.T) { t.Parallel() tests := []struct { name string @@ -1892,6 +1892,9 @@ func Test_checkShellCompleteFlag(t *testing.T) { arguments []string wantShellCompletion bool wantArgs []string + wantWord string + wantWordSet bool + wantTerminated bool }{ { name: "disable-shell-completion", @@ -1919,13 +1922,15 @@ func Test_checkShellCompleteFlag(t *testing.T) { wantArgs: []string{"foo"}, }, { + // The flag is a positional argument of whatever the command runs, so it + // stays in place and the command runs. name: "arguments include double dash", arguments: []string{"--", "foo", completionFlag}, cmd: &Command{ EnableShellCompletion: true, }, wantShellCompletion: false, - wantArgs: []string{"--", "foo"}, + wantArgs: []string{"--", "foo", completionFlag}, }, { name: "shell completion", @@ -1945,15 +1950,105 @@ func Test_checkShellCompleteFlag(t *testing.T) { wantShellCompletion: true, wantArgs: []string{"foo", "--"}, }, + { + // The deprecated request form says nothing about the word being completed, + // which DefaultCompleteWithFlags then works out from the arguments. + name: "deprecated form records no word", + arguments: []string{"prog", "sub", "-", completionFlag}, + cmd: &Command{ + EnableShellCompletion: true, + }, + wantShellCompletion: true, + wantArgs: []string{"prog", "sub", "-"}, + }, + { + name: "request names the completion", + arguments: []string{"prog", completionCommandRequest, "sub", ""}, + cmd: &Command{ + EnableShellCompletion: true, + }, + wantShellCompletion: true, + wantArgs: []string{"prog", "sub"}, + wantWordSet: true, + }, + { + // A word starting with "-" stays in the arguments, which is the shape the + // deprecated form produced, so a ShellComplete function sees no difference. + name: "request names the completion of a flag", + arguments: []string{"prog", completionCommandRequest, "sub", "--fl"}, + cmd: &Command{ + EnableShellCompletion: true, + }, + wantShellCompletion: true, + wantArgs: []string{"prog", "sub", "--fl"}, + wantWord: "--fl", + wantWordSet: true, + }, + { + // The word being completed is a positional argument of whatever the + // command runs, which this command has no suggestion for. It must still be + // a completion, or the command would run. + name: "request names the completion after a double dash", + arguments: []string{"prog", completionCommandRequest, "exec", "--", "git", "pu"}, + cmd: &Command{ + EnableShellCompletion: true, + }, + wantShellCompletion: true, + wantArgs: []string{"prog", "exec", "--", "git"}, + wantWord: "pu", + wantWordSet: true, + wantTerminated: true, + }, + { + // The "--" is the word being completed here, not a terminator, so flags + // still answer it. + name: "request names the completion of a double dash", + arguments: []string{"prog", completionCommandRequest, "exec", "--"}, + cmd: &Command{ + EnableShellCompletion: true, + }, + wantShellCompletion: true, + wantArgs: []string{"prog", "exec", "--"}, + wantWord: "--", + wantWordSet: true, + }, + { + name: "request without a word being completed", + arguments: []string{"prog", completionCommandRequest}, + cmd: &Command{ + EnableShellCompletion: true, + }, + wantShellCompletion: true, + wantArgs: []string{"prog"}, + wantWordSet: true, + }, + { + // The request form shadows nothing: a command of that name is what was + // asked for. + name: "a command of the same name wins", + arguments: []string{"prog", completionCommandRequest, "sub", ""}, + cmd: &Command{ + EnableShellCompletion: true, + Commands: []*Command{{Name: completionCommandRequest}}, + }, + wantShellCompletion: false, + wantArgs: []string{"prog", completionCommandRequest, "sub", ""}, + }, } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - shellCompletion, args := checkShellCompleteFlag(tt.cmd, tt.arguments) + shellCompletion, args := parseShellCompleteRequest(tt.cmd, tt.arguments) assert.Equal(t, tt.wantShellCompletion, shellCompletion) assert.Equal(t, tt.wantArgs, args) + gotWord, gotWordSet, gotTerminated := "", false, false + if req := tt.cmd.completion; req != nil { + gotWord, gotWordSet, gotTerminated = req.word, req.wordKnown, req.terminated + } + assert.Equal(t, tt.wantWordSet, gotWordSet) + assert.Equal(t, tt.wantWord, gotWord) + assert.Equal(t, tt.wantTerminated, gotTerminated) }) } }