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

"no-return" phpdoc not removed because superflous #7914

Open
staabm opened this issue Mar 26, 2024 · 9 comments
Open

"no-return" phpdoc not removed because superflous #7914

staabm opened this issue Mar 26, 2024 · 9 comments
Labels

Comments

@staabm
Copy link
Contributor

staabm commented Mar 26, 2024

Bug report

Description

cs-fixer is removing a @return never phpdoc above a method, as soon as a :never native type is added.
it is not removing a @return no-return phpdoc though, which is a alias of @return never in at least PHPStan and psalm.
(there are even more "aliases", see above links)

put test.php into the php-cs-fixer repo root:

// test.php
<?php
class X {
    /** @return never */
    private function doFoo(): never
    {
    }
    /** @return no-return */
    private function doFoo1(): never
    {
    }
}

expected result

class test
{
    private function doFoo(): never {}

    private function doFoo1(): never {}
}

actual result

class test
{
    private function doFoo(): never {}

    /** @return no-return */
    private function doFoo1(): never {}
}

Runtime version

$ php -v
PHP 8.2.12 (cli) (built: Oct 24 2023 21:15:35) (NTS Visual C++ 2019 x64)
Copyright (c) The PHP Group
Zend Engine v4.2.12, Copyright (c) Zend Technologies

$ php php-cs-fixer --version
PHP CS Fixer 3.52.2-DEV 15 Keys by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.2.12

Used command

php php-cs-fixer fix test.php within PHP-CS-Fixer repo root

Configuration file

PHP-CS-Fixer repo default config

Code snippet that reproduces the problem

see above

@keradus
Copy link
Member

keradus commented Mar 27, 2024

-1 for removing the phpstan/psalm's alias types.

+0 for renaming them to proper php-like @return never (and run superfluous annotation removal afterward)

@Wirone
Copy link
Member

Wirone commented Mar 27, 2024

-1 for removing the phpstan/psalm's alias types.

Why? These are equivalents of never, probably for historical reasons (introduced before native return type), so IMHO these should be removed if superfluous.

@keradus
Copy link
Member

keradus commented Mar 28, 2024

in my proposal they ultimately would. just with 2 atomic rules, and one can decide what they want. if they want "all", they enable both rules.

I do not want to clutter rule operating on proper types with "historical types", and i want explicit rule to clean up "historical types"

@Wirone
Copy link
Member

Wirone commented Mar 28, 2024

OK, now it's more clear how you see it, it totally wasn't clear from the first comment. I am not sure if I agree with this approach, though. Personally I would do this in the same fixer, maybe behind opt-in flag.

@julienfalque
Copy link
Member

I think I prefer a dedicated rule for replacing e.g. no-return with never because there might be other aliases (now or in the future) we want to replace that are not related to removing superfluous @return tags.

@Wirone
Copy link
Member

Wirone commented Apr 2, 2024

If it was general purpose fixer for unifying phpDoc, then OK. Making a rule just for @return and aliases of never seems like an overkill.

@keradus
Copy link
Member

keradus commented Apr 4, 2024

I think it makes sense to cover more aliases.
any more aliases you have in mind ?

@staabm
Copy link
Contributor Author

staabm commented Apr 4, 2024

phpstan and psalm support

    never
    never-return
    never-returns
    no-return

@Wirone
Copy link
Member

Wirone commented Apr 4, 2024

If there are not any more examples of annotations that could be standardised (alias → basic name), then I don't see a point in investing time in this, as it can be easily achieved with regex-based replace using @return (never\-returns?|no\-return)+ as a match pattern and @return never as an expected outcome.

image

Then it can be removed by no_superfluous_phpdoc_tags. I know that it would be better to have it automated, but I believe this is very edge case that not necessarily is worth being included in the core of Fixer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants