Page MenuHomePhabricator

Sniff or phan rule against use `?:` and `if` on returns that may be a string
Open, Needs TriagePublic

Description

We've had numerous failures over the years with User:0, and [[0]] failing in myserious ways.

Recent example: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/610875/2/includes/skins/Skin.php

Now that we have Phan, perhaps we can automatically detect this common mistake?

Event Timeline

xSavitar renamed this task from Sniff or phan rule against use `?:` and `if` on retunrns that may be a string to Sniff or phan rule against use `?:` and `if` on returns that may be a string.Jul 16 2020, 10:13 PM

Basically, this would be a variant of the NonBoolBranchPlugin, but warning when encountering a string, rather than any non-boolean value, while also handling the ternary operator.

I've created such a plugin for phan. It only shows an error when the condition is either string or string|null. Is support for union types such as string|false also required? That gave more false positives.

Where can upload the plugin to? I'd rather not request a new repository if it is decided that it's better off in the code sniffer.

Where can upload the plugin to? I'd rather not request a new repository if it is decided that it's better off in the code sniffer.

mediawiki/tools/phan is the right place IMHO. We could create a dedicated folder for our plugins, especially if these plugins are short. I think this was already attempted in the past.

Also, phan includes tons of modes and plugins, way more than the ones we use. Perhaps there's already one for use cases like this one? Or maybe it could be part of a more generic mode about loose comparisons (I'm fairly sure there's an option for that). If not, it might be a good idea to propose the feature upstream as well.

mediawiki/tools/phan is the right place IMHO. We could create a dedicated folder for our plugins, especially if these plugins are short. I think this was already attempted in the past.

I'll create a patch there then. Thanks.

Also, phan includes tons of modes and plugins, way more than the ones we use. Perhaps there's already one for use cases like this one? Or maybe it could be part of a more generic mode about loose comparisons (I'm fairly sure there's an option for that). If not, it might be a good idea to propose the feature upstream as well.

The NonBoolBranchPlugin I mentioned is one of them, but a bit too specific and not broad enough. I feel having the code sniffer ban == and != would go a long way before needing something like that.

Change 614025 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/tools/phan@master] Add a plugin to identify string to bool coercions in if statements

https://gerrit.wikimedia.org/r/614025

Copying my comment from gerrit:

I am counting 346 issues for MW core, which is quite a lot.
Also, I'm wondering whether we should instead open an issue upstream and ask for NonBoolBranchPlugin to be configurable. That is, being able to specify which types are forbidden/allowed. I think it wouldn't be super-hard to implement, and at that point:

  • We wouldn't have to maintain this plugin
  • Each repo might change the setting to allow/disallow more types

Just so I can understand, what do we want exactly? Being able to specify types like I wrote above (and then forbid only strings in our config)? Or something more generic like "forbid scalars, allow objects/arrays"?

Copying my comment from gerrit:

I am counting 346 issues for MW core, which is quite a lot.
Also, I'm wondering whether we should instead open an issue upstream and ask for NonBoolBranchPlugin to be configurable. That is, being able to specify which types are forbidden/allowed. I think it wouldn't be super-hard to implement, and at that point:

  • We wouldn't have to maintain this plugin
  • Each repo might change the setting to allow/disallow more types

Just so I can understand, what do we want exactly? Being able to specify types like I wrote above (and then forbid only strings in our config)? Or something more generic like "forbid scalars, allow objects/arrays"?

The original intention of the task has to do with preventing a check against an empty string also accidentally identifying '0' as false. Other patterns I've seen are if ( $varThatMightBeNull ), (where varThatMightBeNull is an object) specifically for things ?? can't do, and checking if an array is empty. Those aren't scalars though - banning scalars (excluding booleans obviously) might be enough. At that point, the configuration option for the plugin boils down to "be strict (booleans only)" or "only allow non-scalar values (excluding booleans)".

Note that the upstream plugin doesn't handle the ternary operator - something that is also (perhaps even more so) susceptible to the issue described in this task.

The original intention of the task has to do with preventing a check against an empty string also accidentally identifying '0' as false.

Yup, I agree that this is a good use case. So it would mean "forbid 'string' type".

Other patterns I've seen are if ( $varThatMightBeNull ), (where varThatMightBeNull is an object) specifically for things ?? can't do,

This would mean "forbid null", but that would be too much, so we can ignore this for now (like your plugin does).

and checking if an array is empty.

I wouldn't forbid this, I personally prefer an implicit cast over $arr === [].

Those aren't scalars though - banning scalars (excluding booleans obviously) might be enough. At that point, the configuration option for the plugin boils down to "be strict (booleans only)" or "only allow non-scalar values (excluding booleans)".

Given the above, perhaps asking for the possibility to specify types and forbidding strings would be enough for now?

Note that the upstream plugin doesn't handle the ternary operator - something that is also (perhaps even more so) susceptible to the issue described in this task.

This should be filed as an issue upstream, I might send a PR.

Other patterns I've seen are if ( $varThatMightBeNull ), (where varThatMightBeNull is an object) specifically for things ?? can't do,

This would mean "forbid null", but that would be too much, so we can ignore this for now (like your plugin does).

and checking if an array is empty.

I wouldn't forbid this, I personally prefer an implicit cast over $arr === [].

Those aren't scalars though - banning scalars (excluding booleans obviously) might be enough. At that point, the configuration option for the plugin boils down to "be strict (booleans only)" or "only allow non-scalar values (excluding booleans)".

Given the above, perhaps asking for the possibility to specify types and forbidding strings would be enough for now?

I'm not sure what there is to ask - PHP doesn't consider null as scalar (at least not as far as is_scalar is concerned), so object, array and null fit in the same group, with scalars in the other. Unless you'd like the plugin to allow users to specify that only arrays and bools are allowed to be used in conditionals, or only objects and bools etc. I think that wouldn't be really worth it.

What I meant is adding a list of types that are forbidden in conditionals (can be e.g. [ 'string', 'null', 'object', 'array', ... ]). For our use case, we'd forbid just string for now.

I think this wouldn't be complicated to implement upstream, and I might send a PR for this as well. For sure, it would be easier to maintain than a custom plugin.