Report bool properties as too wide when only used with true/false

[staabm]

closes phpstan/phpstan#13384

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

1. This doesn't have to be limited to properties
2. This doesn't have to be limited to PHP versions with native standalone true/false types support. We have PHPDocs after all.

There are many TooWide* rules with duplicated logic and maybe some variances in the logic. First we should refactor and deduplicate the logic where possible.

Then we could introduce a new bleeding edge toggle to enable reporting "too wide bool".

[staabm]

this is a topic for a followup: NodeScopeResolver's ClassStatementsGatherer works on a per class level, which means access to private properties from a closure-scope (from within a different class) are not seen when analyzing the class which declares the property

[ondrejmirtes]

Yeah of course, static analysis is not fit for these shenanigans 😊 Especially for private properties.

[staabm]

I think we are good to go

[ondrejmirtes]

Hey, you're going to kill me 😅

I'm a bit worried about the pedantry of this rule. I realized I don't want it to force people to be adding @return false when they just have a : bool return type above their functions.

So I think it should work like this:

• On all PHP versions: When the function has @return bool, the rule can enforce to change it to @return true or @return false (same for properties with @var)
• On PHP 8.2+: When the function does not have a PHPDoc bool type, only a native bool type, the rule can enforce it being changed to true or false.

The places that call TooWideTypeCheck have easy access to getPhpDocReturnType / getNativeReturnType so it can be easily passed to check* methods as arguments.

Please let me know if you want to change the logic, else I can tackle it myself.

[ondrejmirtes]

Also: we should give these errors different error messages (that tell the user what they need to do) with differenr error identifiers.

[staabm]

Hey, you're going to kill me 😅

;)

So I think it should work like this:

should we keep the bleeding edge toggle then again?

[ondrejmirtes]

Yeah, all of this has to happen only with bleeding edge.

The only exception would be if we went back in time and managed to implement the part with native types before 8.2 was released. (New rules for PHP 8.5 today are always reported, even without bleeding edge)

[staabm]

adjusted per feedback

ohh I missed this one:

Also: we should give these errors different error messages (that tell the user what they need to do) with differenr error identifiers.

[staabm]

Also: we should give these errors different error messages (that tell the user what they need to do) with differenr error identifiers.

here we go. I guess next step is a auto-fixer?

[ondrejmirtes]

here we go. I guess next step is a auto-fixer?

I'm not sure about that. I'd like the auto-fixes to be a slam dunk, to be always correct. I'd say they are only suitable for opinionated "coding standard" rules. Like I'm doing for phpstan-src here: https://github.com/phpstan/phpstan-src/tree/2.1.x/build/PHPStan/Build

Let's take your original issue (https://phpstan.org/r/5bbec0ea-63ee-46e0-8584-2f232c522df3) - the solution isn't to change private static bool $registered = false; to private static false $registered = false; but different.

[staabm]

I see. Makes sense.

[ondrejmirtes]

Perfect, thank you!