search

PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
856 stars 53 forks source link

Generic/PHPDocTypes and PSR5/PHPDocTypes: Adds sniffs #469

Closed james-cnz closed 3 months ago

james-cnz commented 4 months ago

Description

Adds sniffs for PHPDoc types, both a generic sniff, and one for conformance to the PHP-FIG PSR-5 rules relating to types (using the generic sniff, with different properties set).

PHPStan and Psalm are the two most commonly used PHP type checkers, and phpDocumentor is moving to base it's type handling off PHPStan, while PHPStorm uses Psalm for type checking. My feeling is that types that are accepted by both PHPStan and Psalm are essentially de facto valid PHPDoc types, so the generic checks aim to check that types meet these criteria.

The PSR-5 sniff only checks rules relating to types.

The checker handles all type-related tags (property-*, template, param, return, and var). I think this makes sense, because it's likely that it will be desired to check all types by the same standard.

Type comparisons are performed between PHPDoc types and PHP natives types, where present. It isn't perfect, because the checker is only aware of a single file at a time, and can't recognise global constants, but I think it's fairly good for what it is. It takes account of namespaces, use aliases, and class hierarchies in the current file. While PHPStan and Psalm will do much better checking, they are easiest to set up for code that adheres to OO principles, and may produce spurious errors for code that makes use of require/include(_once), so the type-checking feature of this sniff may be useful in these cases.

The sniff can check for misplaced type tags, except for misplaced var tags. My understanding is that var tags are appropriate where a variable is first declared, which can include an assignment, as a foreach loop variable, or being returned as a pass-by-reference parameter where one wasn't passed in. The sniff doesn't attempt to check this.

I originally wrote this for the Moodle Coding Style project, but I think it may be more broadly useful, so am submitting it here instead.

Suggested changelog entry

Types of changes

PR checklist

jrfnl commented 4 months ago

@james-cnz Thank you for your willingness to contribute to PHP_CodeSniffer and I can see you've put a huge amount of work into this.

However, this PR does a lot of things, including adding a new standard, new utility class and two new sniffs. The main sniff itself has the same problems as the pre-existing sniffs: it does too much. Aside from that, it is nearly certain that it will conflict with the existing Variable/Function comment sniffs and will make it impossible to use a combination.

All in all, I'm not keen on this PR as-is. All of this should have been discussed in an issue first as per the contributing guide on New features.

jrfnl commented 4 months ago

For reference a previous discussion on this work as it was first pulled to an external standard: https://github.com/moodlehq/moodle-cs/pull/123

james-cnz commented 4 months ago

I understand that it's generally desirable to divide the checking up into different sniffs, so users can select the ones they want. With the type tags, though, I think users are most likely to want to check them all in the same way, or check none of them. e.g. If users want to check the var type tag for conformance to PHP-FIG's PSR-5, then they're most likely to want to check all type tags for this conformance. Dividing the checking into different sniffs wouldn't be beneficial here, I think.

Yes, I think it would conflict with the existing sniffs, but I think they serve somewhat different use cases. The proposed sniffs aim to check either that types will be parsed by PHPStan and Psalm, or that types conform to PHP-FIG's PSR-5, while the existing sniffs check for conformance to their own standard, I think? As part of supporting PHPStan and Psalm types, the proposed checker supports types that span multiple lines, include the $this type, and include variables specified as part of a callable type, as well as functions that annotate only some parameters. I don't think the existing sniffs support these things, so they might not be used in the same cases. The PSR-5 standard and the existing sniffs flatly contradict, I think, PSR-5 requiring short type names, and the existing sniffs requiring long ones, so these couldn't be used together.

My apologies for not posting first. I assumed this would be to avoid me carrying out unnecessary work if the sniff wasn't wanted, and figured that since I'd done most of the work already, it probably didn't apply.

jrfnl commented 4 months ago

I assumed this would be to avoid me carrying out unnecessary work if the sniff wasn't wanted, and figured that since I'd done most of the work already, it probably didn't apply.

@james-cnz Those guidelines are there for a reason and yes, preventing unnecessary work is part of that reason, but not the only reason for that guideline. You are new to this repo and have no history of contributing to this project. Your PR demonstrates a lack of familiarity with the project, with the codebase, with established best practices, with the history and the envisioned future. Coaching this PR in the direction where it should be going in would take months and the chance of it ever getting to a merge-ready place are slim.

PHPCS allows for external standards. As things are, I suggest you'd be better off creating your own external standard where you can do things your way. As a side-note, I'd recommend not using PSR5 as a ruleset name in an non-sanctioned external standard as that is likely to come back and bite you in the future when there will be a sanctioned PSR5 standard.