feature: NoSuperfluousPhpdocTagsFixer - support untyped and empty annotations in phpdoc

[supersmile2009]

This PR fixes a bug/unsupported case where no_superfluous_phpdoc_tags filter won't remove useless @param and @var tags when there's no type and/or when the tag is empty. Examples:

class Foo {
    /**
     * @var
     */
    private $foo;

    /**
     * @param $foo
     */
    public function foo($foo) {}
}

[keradus]

I would count it as enhancement and not as a bug, to be honest. it's not that fixer was doing wrong changes, it's simply that now it learned new tricks

[coveralls]

Coverage decreased (-0.003%) to 92.913% when pulling 37a640e on supersmile2009:untyped-empty-annotations-in-phpdoc into aafad72 on FriendsOfPHP:master.

[keradus]

please rebase and solve issues detected by CI

[kubawerlos]

I would advocate this to be a bugfix, the description of the fixer is "Removes @param, @return and @var tags that don't provide any useful information." which now it still does, but better.

For me exactly the same reasoning as this being bugfix (don't get confused by a label, see the milestone).

[GrahamCampbell]

I think this new ability should be controlled via a configuration option, off by default.

[supersmile2009]

@GrahamCampbell
Here are 2 cases:

    /**
     * @param int $foo
     */
    public function foo(int $foo) {}

    /**
     * @param $foo
     */
    public function foo($foo) {}

They are conceptually the same.
The 1-st one is correctly handled by php-cs-fixer - param entry gets removed.
The 2-nd one isn't.
I don't quite get your point about a configuration option. I'm not targeting malformed or unsupported/unparsable docblock entries here. These indeed would need a dedicated config option (off by default) if anyone wants to be able to remove them. But I can't find a single reason why anyone would want to remove docblock in the 1-st case, but keep it in the 2-nd one (or any other similar case with an empty or untyped annotation). And this is exactly what I'm targeting in this PR.

[GrahamCampbell]

The second case is invalid syntax.

[julienfalque]

@supersmile2009 Looks good, thank! Please rebase your branche to solve conflicts.

IMO this can be merged as an enhancement, so enabled without option. @SpacePossum @keradus @kubawerlos What do you think?

[supersmile2009]

Hi @julienfalque. Ok, I'll have some spare time this week so I'll rebase it.

I'd also like to mention that I understand @GrahamCampbell's reasoning about the cases handled with this PR being technically invalid. However if we think of it from a practical standpoint, the are 2 options how you'd want to handle these cases:

• You want to remove the empty tags because you consider them superfluous as they don't bring any value to your code. Clearly a case for the no_superfluous_phpdoc_tags filter.
• You care about the types and you want to fix these invalid empty tags by adding the correct types. Notifying the developer about invalid/empty tags is clearly out of scope of the cs-fixer's responsibility and should be handled at code review and/or with the static analysis tools, which would raise a warning about being unable to infer a type. Having these tags removed by cs-fixer with the no_superfluous_phpdoc_tags filter doesn't really matter as you'll have to fix them anyway if you care about it.

Note that I'm targeting only clearly and reliably identifiable empty phpDoc tags with this PR. Any other invalid or unsupported cases are not affected by this change.

[supersmile2009]

Hi @julienfalque.
I've rebased the branch.
Here's a list of additional changes compared to the previous commit:

1. I added a few extra test cases - primarily testing the same invalid/empty annotations, but in single-line doc blocks.
2. I had to fix one expectation in the PHP8_0.test. Now that return is properly recognized as redundant and rightfully removed.
3. I also had to fix the numeric constants matching expression. Previously in some edge cases when used within the new super-flexible expressions in NoSuperfluousPhpdocTagsFixer the old type expression could match " string &..." in some test cases which had been added since I originally created this PR. So I replaced [\d.]+ with a more precise expression for numeric constants. I also added the tests to cover more numeric constants which can be matched with the new expression. I understand this is a bit of a scope creep, but it's essential to correctly match invalid/empty @param blocks. I could extract this change into a separate PR if you wish and then rebase current branch once that PR is merged.

[keradus]

@julienfalque , can you decide what next here?

[julienfalque]

Thank you @supersmile2009.