You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
booleansInConditions is very strict, which is great and all (kind of the point of this project), but makes it a bit of a pain to enable in a large legacy codebase -- I believe (and may well be wrong) that should be possible to accept a significantly wider variety of inputs and still be equally correct - specifically: allowing non-booleans so long as only one of the types in the union can be false-y, eg
MyClass|null - if($var) is checking for object vs null, that's ok
bool|null - if($var) is ambiguous if we're checking for false or null, reject
array - if($var) can only be checking for empty-array vs non-empty-array, allow
array|null - is ambiguous, reject
non-empty-array|null - if($var) can only mean one thing, allow
0|1 - allowed
0|1|false - 0 and false are meaningfully different, we should reject and force the dev to handle it
etc
(I currently have booleansInConditions disabled because it makes a lot of noise in my large legacy project, and nearly all the instances that it flags are cases that I would personally consider to be false-positives -- but I just got bitten by a bug where I was handling a bool|null and forgot to handle the false and null cases separately... FWIW my gut says it's weird that I'm willing to accept multiple-potential-truths but want my linter to reject multiple-potential-falseys - but scanning my codebases, "multiple potential truths" is always fine in practice and "multiple false-y types all being treated the same" is nearly-always a bug, so maybe a rule like this would be broadly valuable?)
The text was updated successfully, but these errors were encountered:
Basically you want something different than the current booleansInConditions ruleset here offers 😊 Nothing wrong with that, you can reimplement it in your project or open-source package with whatever behaviour you'd want.
Why I'm against it: this is very similar to discussion about empty which also notoriously uses loose comparison with == / != (and ignores undefined variables on top). In theory it's safe to use empty on always-defined variables and types such as array or MyClass|null. But if you know empty and all its pitfalls, every time you see it in code, it should be a huge red flag. Allowing empty (and non-booleans in conditions) increases the burden on everyone that reads the code because they have to stop and think "I know that empty() is shitty but it might be okay in this situation", and then re-check the types passed there.
To make code review (and reading and writing) easier, it's better to never allow empty and non-booleans in conditions. In my opinion if ($var !== null) is always better than if ($var).
booleansInConditions
is very strict, which is great and all (kind of the point of this project), but makes it a bit of a pain to enable in a large legacy codebase -- I believe (and may well be wrong) that should be possible to accept a significantly wider variety of inputs and still be equally correct - specifically: allowing non-booleans so long as only one of the types in the union can be false-y, egMyClass|null
-if($var)
is checking for object vs null, that's okbool|null
-if($var)
is ambiguous if we're checking forfalse
ornull
, rejectarray
-if($var)
can only be checking for empty-array vs non-empty-array, allowarray|null
- is ambiguous, rejectnon-empty-array|null
-if($var)
can only mean one thing, allow0|1
- allowed0|1|false
- 0 and false are meaningfully different, we should reject and force the dev to handle it(I currently have
booleansInConditions
disabled because it makes a lot of noise in my large legacy project, and nearly all the instances that it flags are cases that I would personally consider to be false-positives -- but I just got bitten by a bug where I was handling abool|null
and forgot to handle thefalse
andnull
cases separately... FWIW my gut says it's weird that I'm willing to accept multiple-potential-truths but want my linter to reject multiple-potential-falseys - but scanning my codebases, "multiple potential truths" is always fine in practice and "multiple false-y types all being treated the same" is nearly-always a bug, so maybe a rule like this would be broadly valuable?)The text was updated successfully, but these errors were encountered: