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

[Console] Autocompletion problems #44729

Closed
ondrejmirtes opened this issue Dec 20, 2021 · 17 comments
Closed

[Console] Autocompletion problems #44729

ondrejmirtes opened this issue Dec 20, 2021 · 17 comments

Comments

@ondrejmirtes
Copy link
Contributor

Symfony version(s) affected

5.4.1

Description

I'm trying out the new autocompletion feature on a console app. I've put eval "$(/Users/ondrej/Development/phpstan/bin/phpstan completion bash)" into my ~/.bash_profile file.

I'm trying to autocomplete various things like option names and command names, but unsuccessfully. This is how the output looks like:

$ bin/phpstan a <TAB>
                                               The "--symfony" option requires a value.                                                _complete [-s|--shell SHELL] [-i|--input INPUT] [-c|--current CURRENT] [-S|--symfony SYMFONY]

                                               The "--symfony" option requires a value.                                                _complete [-s|--shell SHELL] [-i|--input INPUT] [-c|--current CURRENT] [-S|--symfony SYMFONY]

                                               The "--symfony" option requires a value.                                                _complete [-s|--shell SHELL] [-i|--input INPUT] [-c|--current CURRENT] [-S|--symfony SYMFONY]

And after pressing enter, the default command of the application runs.

How to reproduce

git clone git@github.com:phpstan/phpstan-src.git
cd phpstan-src
composer install
bin/phpstan a <TAB>

Possible Solution

No response

Additional Context

This is how the bash completion script looks like:

$ /Users/ondrej/Development/phpstan/bin/phpstan completion bash
# This file is part of the Symfony package.
#
# (c) Fabien Potencier <fabien@symfony.com>
#
# For the full copyright and license information, please view
# https://symfony.com/doc/current/contributing/code/license.html

_sf_phpstan() {
    # Use newline as only separator to allow space in completion values
    IFS=$'\n'
    local sf_cmd="${COMP_WORDS[0]}"

    # for an alias, get the real script behind it
    if [[ $(type -t $sf_cmd) == "alias" ]]; then
        sf_cmd=$(alias $sf_cmd | sed -E "s/alias $sf_cmd='(.*)'/\1/")
    fi

    if [ ! -f "$sf_cmd" ]; then
        return 1
    fi

    local cur prev words cword
    _get_comp_words_by_ref -n := cur prev words cword

    local completecmd=("$sf_cmd" "_complete" "-sbash" "-c$cword" "-S")
    for w in ${words[@]}; do
        w=$(printf -- '%b' "$w")
        # remove quotes from typed values
        quote="${w:0:1}"
        if [ "$quote" == \' ]; then
            w="${w%\'}"
            w="${w#\'}"
        elif [ "$quote" == \" ]; then
            w="${w%\"}"
            w="${w#\"}"
        fi
        # empty values are ignored
        if [ ! -z "$w" ]; then
            completecmd+=("-i$w")
        fi
    done

    local sfcomplete
    if sfcomplete=$(${completecmd[@]} 2>&1); then
        local quote suggestions
        quote=${cur:0:1}

        # Use single quotes by default if suggestions contains backslash (FQCN)
        if [ "$quote" == '' ] && [[ "$sfcomplete" =~ \\ ]]; then
            quote=\'
        fi

        if [ "$quote" == \' ]; then
            # single quotes: no additional escaping (does not accept ' in values)
            suggestions=$(for s in $sfcomplete; do printf $'%q%q%q\n' "$quote" "$s" "$quote"; done)
        elif [ "$quote" == \" ]; then
            # double quotes: double escaping for \ $ ` "
            suggestions=$(for s in $sfcomplete; do
                s=${s//\\/\\\\}
                s=${s//\$/\\\$}
                s=${s//\`/\\\`}
                s=${s//\"/\\\"}
                printf $'%q%q%q\n' "$quote" "$s" "$quote";
            done)
        else
            # no quotes: double escaping
            suggestions=$(for s in $sfcomplete; do printf $'%q\n' $(printf '%q' "$s"); done)
        fi
        COMPREPLY=($(IFS=$'\n' compgen -W "$suggestions" -- $(printf -- "%q" "$cur")))
        __ltrim_colon_completions "$cur"
    else
        if [[ "$sfcomplete" != *"Command \"_complete\" is not defined."* ]]; then
            >&2 echo
            >&2 echo $sfcomplete
        fi

        return 1
    fi
}

complete -F _sf_phpstan phpstan
@wouterj
Copy link
Member

wouterj commented Dec 20, 2021

Thanks for the issue!

The actual issue raises from $application->getVersion() returning an empty version. This is because bin/phpstan doesn't work fully as expected when inside the phpstan-src repo. It can be "fixed" using this change:

diff --git a/bin/phpstan b/bin/phpstan
index b87104469..fd27ed0a6 100755
--- a/bin/phpstan
+++ b/bin/phpstan
@@ -88,7 +88,7 @@ use Symfony\Component\Console\Helper\ProgressBar;

        $version = 'Version unknown';
        try {
-               $version = \Jean85\PrettyVersions::getVersion('phpstan/phpstan')->getPrettyVersion();
+               $version = \Jean85\PrettyVersions::getVersion('phpstan/phpstan')->getPrettyVersion() ?: 'Version unknown';
        } catch (\OutOfBoundsException $e) {

        }

However, this issue made me realize that we probably forgot about the standalone app usage when implementing --symfony.

This option is used to determine if the installed shell completion script is still compatible with the complete command in Symfony (e.g. if we add a feature in 6.1 that requires a modification of the bash script). However, we currently set it using $application->getVersion(). This means that we're no longer checking Symfony versions when it's used by e.g. PHPstan. I have no idea how we can set this to the real symfony/console version here.
Maybe we should use a custom version scheme for completion? (e.g. one that we increase manually, independently from Symfony releases, whenever we break/change something?)

@GromNaN
Copy link
Member

GromNaN commented Dec 20, 2021

👍 for adding a completion version that is incremented only when there is a breaking change. Like composer does for plugins.

@stof
Copy link
Member

stof commented Dec 20, 2021

yeah. either that or we should use \Composer\InstalledVersions::getVersion('symfony/console') to get the version (but then, that will be a pain when using a dev version of symfony/console).

@ondrejmirtes
Copy link
Contributor Author

After patching bin/phpstan like this:

@@ -88,7 +88,7 @@ use Symfony\Component\Console\Helper\ProgressBar;

        $version = 'Version unknown';
        try {
-               $version = \Jean85\PrettyVersions::getVersion('phpstan/phpstan')->getPrettyVersion();
+               $version = \Jean85\PrettyVersions::getVersion('phpstan/phpstan')->getPrettyVersion() ?: $version;
        } catch (\OutOfBoundsException $e) {

        }

I'm getting this when I try to autocomplete:

$ bin/phpstan a<TAB>-bash: __ltrim_colon_completions: command not found
-bash: __ltrim_colon_completions: command not found
-bash: __ltrim_colon_completions: command not found
-bash: __ltrim_colon_completions: command not found

@stof
Copy link
Member

stof commented Dec 20, 2021

@ondrejmirtes this patch is not the solution, as the script still expects to have something matching the version of symfony/console

@ondrejmirtes
Copy link
Contributor Author

Maybe something under the completion command could be used to output the Symfony version, like bin/phpstan completion -v, instead of relying on the custom Console Application's version.

@dkarlovi
Copy link
Contributor

dkarlovi commented Dec 20, 2021

The suggestion by @stof was exactly the solution I had in mind when I suggested this flag: we need to know which version of Symfony Console created the completion script. This means the version of Symfony Console should always get hardcoded into the resulting (Bash, Zsh, Fish) script, if that currently doesn't happen in any scenario (framework, just the component, etc), it's a bug.

IMO if it ends up being dev-master or whatever, that can be treated like a version mismatch and nudge the user to update the completion script with a stable version.

@dkarlovi
Copy link
Contributor

dkarlovi commented Dec 20, 2021

@wouterj it should not pass PHPStan's version here because it will not make sense to the Symfony completion command (which will in the future notify the user the completion script needs to be updated), it should in all cases be symfony/console which produced the completion shell script.

Alternative to this is PHPStan also overriding the completion command and then making sense out of its own version to know if it needs to notify the user to update the completion script (basically replicating what Symfony is/will be doing, but with own versions used).

@wouterj
Copy link
Member

wouterj commented Dec 20, 2021

For clarity, let's keep discussions separate :)

I've moved the --symfony discussion to #44733


Unless that issue is fixed, we're now focusing on testing PHPstan completion with the application's version. For this, the patch used by @ondrejmirtes is correct.

The bash: __ltrim_colon_completions: command not found error seems to be from a missing bash-completion package (ref). Do you use bash completion without the completion package by any chance? If so, can you try with the completion package? If that's the fix, we can probably try to get a better error (i.e. by detecting if the function is available and suggesting installing the package if it's not).

Some background: we need this advanced completion function as Bash does not support commands with colons in it - which are very common in apps using Symfony Console.

@ondrejmirtes
Copy link
Contributor Author

Hi, I'd love the autocompletion to work with as little user intervention as possible. Otherwise I can't really sell this to PHPStan users if they need to figure out how to make it work on their machine in the first place :)

For example I've got this in my /opt/homebrew/etc/profile.d/bash_completion.sh file:

# Check for interactive bash and that we haven't already been sourced.
[ -z "$BASH_VERSION" -o -z "$PS1" -o -n "$BASH_COMPLETION" ] && return

# Check for recent enough version of bash.
bash=${BASH_VERSION%.*}; bmajor=${bash%.*}; bminor=${bash#*.}
if [ $bmajor -gt 3 ] || [ $bmajor -eq 3 -a $bminor -ge 2 ]; then
    if shopt -q progcomp && [ -r /opt/homebrew/Cellar/bash-completion/1.3_3/etc/bash_completion ]; then
        # Source completion code.
        . /opt/homebrew/Cellar/bash-completion/1.3_3/etc/bash_completion
    fi
fi
unset bash bmajor bminor

And apparently this is all I need to make git autocompletion work:

$ git clone --<TAB>
--bare                 --jobs=                --recurse-submodules   --shared
--branch=              --local                --reference-if-able=   --single-branch
--checkout             --mirror               --reference=           --sparse
--config=              --no-...               --reject-shallow       --tags
--depth=               --no-checkout          --remote-submodules    --template=
--dissociate           --no-hardlinks         --separate-git-dir=    --upload-pack=
--filter=              --no-tags              --server-option=       --verbose
--hardlinks            --origin=              --shallow-exclude=
--ipv4                 --progress             --shallow-since=
--ipv6                 --quiet                --shallow-submodules

Could Symfony do something similar? E.g. once autocompletion for other tools works on my system, Symfony would also work automatically? As you can see, I know nothing about bash, I just know what the end goal for the end user should be :)

Another thing that's not clear to me is what should be done when PHPStan is installed multiple times locally in different projects and maybe even in different versions. Do I need currently need to run eval "$(/Users/ondrej/Development/phpstan/bin/phpstan completion bash)" for every project, or is once instance sufficient?

@dkarlovi
Copy link
Contributor

@ondrejmirtes

  1. the only intervention needed would be to generate the completion script once in the correct place. Where that "correct place" is depends on the target system.

The reason git works by default is the package ships its completion script in the correct place by default for the Bash completion package to find it. Bash completion also ships with a bunch of completions for all sorts of programs already, they start working without even doing anything

$ rpm -ql git-core | grep comple
/usr/share/bash-completion
/usr/share/bash-completion/completions
/usr/share/bash-completion/completions/git
/usr/share/bash-completion/completions/gitk
/usr/share/git-core/contrib/completion
/usr/share/git-core/contrib/completion/git-completion.tcsh
/usr/share/git-core/contrib/completion/git-prompt.sh
  1. if PHPStan is installed multiple times, the common completion script should use current the version of PHPStan you're currently using to complete. It means PHPStan 1.0 in one folder should complete different options and commands compared to PHPStan 1.1 in another folder, even though the completions get invoked using the same exact completion script.

@ondrejmirtes
Copy link
Contributor Author

Is it possible to architect the solution in such a way that there would have to be only one bash completion script for all Symfony apps? It'd be nice to somehow get into those OS packages and have for example /usr/share/bash-completion/completions/symfony-console in the system.

BTW I made it work, it's really nice! :)

$ bin/phpstan --<TAB>
--ansi            --help            --no-interaction  --quiet           --verbose         --version
phpstan master $ bin/phpstan --<TAB>
--ansi            --help            --no-interaction  --quiet           --verbose         --version
phpstan master $ bin/phpstan analyse --<TAB>
--allow-empty-baseline  --error-format          --memory-limit          --verbose
--ansi                  --fix                   --no-interaction        --version
--autoload-file         --generate-baseline     --no-progress           --watch
--configuration         --help                  --pro                   --xdebug
--debug                 --level                 --quiet

I made it work by running brew install bash-completion@2 and putting [[ -r "/opt/homebrew/etc/profile.d/bash_completion.sh" ]] && . "/opt/homebrew/etc/profile.d/bash_completion.sh" into ~/.bash_profile.

@dkarlovi
Copy link
Contributor

dkarlovi commented Dec 21, 2021

Is it possible to architect the solution in such a way that there would have to be only one bash completion script for all Symfony apps?

No, the completion script's last line registers the completion function for a specific executable name, in your case

complete -F _sf_phpstan phpstan

It cannot be registered for all Symfony based apps because it doesn't know all of their (executable) names. Even if it's exactly the same script, it will need to be created/registered for each individual app. That's the reason why the completion command is kept, PHPStan still needs to generate its own completion script and put its own name in there. :)

That could be added as a feature: allow specifying additional names in the completion command, so you could do for example:

phpstan completion bash behat console somethingelse

And it would generate

complete -F _sf_phpstan phpstan
complete -F _sf_phpstan behat
complete -F _sf_phpstan console
complete -F _sf_phpstan somethingelse

But IMO, it would be very unwieldy and messy for the user. Maybe core team disagrees.

Great, looks good.

@dkarlovi
Copy link
Contributor

It'd be nice to somehow get into those OS packages and have for example /usr/share/bash-completion/completions/symfony-console in the system.

BTW this is exactly my expectation what will happen when composer/composer#10292 is merged, the package maintainers put the completion script(s) generated by the executable into the OS packages and it just works.

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Friendly reminder that this issue exists. If I don't hear anything I'll close this.

@wouterj
Copy link
Member

wouterj commented Jul 15, 2022

I believe this is now fixed in 6.2 by #46901

@wouterj wouterj closed this as completed Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants