Added rule to disallow weak comparisons #177
Conversation
| Dedicated tests for that rule would be good. Even if it is that simple :) |
| You mean two rule classes: one for equality and one for inequality? |
| No, I mean basically that I would expect this PR also to add a file |
| I have added a test class. |
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Testing\RuleTestCase; | ||
| | ||
| class DisallowedWeakComparisonRuleTest extends RuleTestCase |
There was a problem hiding this comment.
I think you can get rid of the new phpstan errors added to the baseline if you specify the generic type in the same fashion as it is done in e.g. https://github.com/phpstan/phpstan-strict-rules/blob/1.2.3/tests/Rules/DisallowedConstructs/DisallowedShortTernaryRuleTest.php#L9
There was a problem hiding this comment.
OK, I don't mind which variant I use. I just took an example from the existing test classes.
| { | ||
| if ($node instanceof Equal || $node instanceof NotEqual) { | ||
| return [ | ||
| 'Weak comparison via "' . $node->getOperatorSigil() . '" is not allowed. Use strong comparison instead.', |
There was a problem hiding this comment.
The correct nomenclature isn't weak/strong, but loose/strict.
Also please use the correct alternative in the error message. == should have === in the second part of the message, != should have !== in the second part of the message.
There was a problem hiding this comment.
Also: Please use RuleErrorBuilder here.
There was a problem hiding this comment.
Also: Please use sprintf instead of string concatenation.
There was a problem hiding this comment.
Should be fixed by 56a7475. I only took the others rules in the directory as examples which don't use RuleErrorBuilder either.
There was a problem hiding this comment.
sorry, I don't want to hijack this too much, but I noticed that the class / rule name is still using the other terms. does it make sense to adapt them too for consistency?
There was a problem hiding this comment.
Nicely spotted. Fixed that, too.
rules.neon Outdated
| - PHPStan\Rules\DisallowedConstructs\DisallowedEmptyRule | ||
| - PHPStan\Rules\DisallowedConstructs\DisallowedImplicitArrayCreationRule | ||
| - PHPStan\Rules\DisallowedConstructs\DisallowedShortTernaryRule | ||
| - PHPStan\Rules\DisallowedConstructs\DisallowedWeakComparisonRule |
There was a problem hiding this comment.
This rule has to be enabled only with bleedingEdge right now. Do a similar thing that was done here: phpstan/phpstan-doctrine@00e7f8f
There was a problem hiding this comment.
Should also be fixed by 56a7475. But honestly, I don't know if I got it right. I am still lacking understanding of which configuration options are available, what they mean and how the interact with each other.
| | ||
| conditionalTags: | ||
| PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule: | ||
| phpstan.rules.rule: %featureToggles.bleedingEdge% |
There was a problem hiding this comment.
@ondrejmirtes We are using bleeding edge to test @readonly and stuff in phpstan, but this rule would create thousands of errors for us in legacy apps.
wouldn‘t it make sense to have a additional feature flag, so one can enable bleeding edge but don‘t enable this rule?
There was a problem hiding this comment.
Either we want to have this enabled for everyone when PHPStan 2.0 + phpstan-strict-rules 2.0 are released, or we don't.
In the first case (enable for everyone), the additional feature toggle doesn't make sense. Bleeding edge is a preview of what's to come in the next major version.
I made a Twitter poll about this: https://twitter.com/OndrejMirtes/status/1537418180888514560
There was a problem hiding this comment.
BTW - instead of setting a feature toggle to false or whatever, you can always write a regex to ignore these problems.
| Thank you! |
4 participants
This rule disallows using weak comparison via
==or!=. Instead strong comparison via===or!==should be used. Also see phpstan/phpstan#7459.