psalm-strict-equality icon indicating copy to clipboard operation
psalm-strict-equality copied to clipboard

Many auto-fixable things do not get auto-fixed?

Open kkmuffme opened this issue 2 years ago • 5 comments

All these should be auto-fixable without side-effects?

$_SERVER['HTTP_HOST'] == 'my-domain.com'
$_GET['foo'] == 'bar'
substr( $range, 0, 1 ) == '-'
$file_extension == 'mp4' // if this were a number or '0' it would have side-effects, but if it's a multi-letter string there cannot be any side-effects

kkmuffme avatar Jul 27 '22 11:07 kkmuffme

Yeah, there's currently a few limitations that would need to be improved. Mainly:

  • I don't risk touching types known through docblock types: https://github.com/orklah/psalm-strict-equality/blob/main/src/Hooks/StrictEqualityHooks.php#L55. I made this plugin to refactor old legacy code with a lot of imprecise docblocks so handling this was not my primary goal. Ideally this should become a config
  • I don't risk touching unions: https://github.com/orklah/psalm-strict-equality/blob/main/src/Hooks/StrictEqualityHooks.php#L59. Mainly to avoid a lot of complexity. My goal was to auto fix a lot of no-brainer cases, I didn't go further. Depending on the php version, this may mean substr cases are not fixed (it used to return string|false)

Apart from those two limitations, if you have a string on both sides, it should get fixed. However, the string == string is not absolutely true, as underlined by this comment: https://github.com/orklah/psalm-strict-equality/blob/main/src/Hooks/StrictEqualityHooks.php#L96

This case is ignored because I considered it's a very rare occurence (so it will get fixed), but it should be documented somewhere. Those plugins were kinda published as a second thought in case it may be useful, I know the documentation and features need polishing :(

orklah avatar Jul 27 '22 16:07 orklah

I don't risk touching types known through docblock types: https://github.com/orklah/psalm-strict-equality/blob/main/src/Hooks/StrictEqualityHooks.php#L55. I made this plugin to refactor old legacy code with a lot of imprecise docblocks so handling this was not my primary goal. Ideally this should become a config

Makes sense, that's what I'm using it for :-)

Does this only mean docblocks in the code or also for docblocks in the stubs?

I don't risk touching unions: https://github.com/orklah/psalm-strict-equality/blob/main/src/Hooks/StrictEqualityHooks.php#L59. Mainly to avoid a lot of complexity. My goal was to auto fix a lot of no-brainer cases, I didn't go further. Depending on the php version, this may mean substr cases are not fixed (it used to return string|false)

Makes sense Correct me if I'm wrong, but one extremely common case that is a low hanging fruit to be auto-fixed is if 1 side is union and the other side is single of type string or int or float and not 0/1/'0'/'1'/''? e.g. $_GET['foo'] == 'bar' for example This would fix 95% of conditions for most codebases I guess, as it's what I am seeing mostly in the codebases I tested the plugin on.

kkmuffme avatar Jul 28 '22 08:07 kkmuffme

Correct me if I'm wrong, but one extremely common case that is a low hanging fruit to be auto-fixed is if 1 side is union and the other side is single of type string or int or float and not 0/1/'0'/'1'/''?

It's trickier than that. For example if you have bool|string and the other side is the int 5. Here's a short list of values for bool|string that would result in a difference between == and === true: https://3v4l.org/0krmr '5': https://3v4l.org/fXONg '5banana' in PHP7: https://3v4l.org/WrEvo '5.0': https://3v4l.org/Nv3iG

It would be very cool to be able to handle unions (even only on one side), but we should design a test that checks every single weird value agains others and make sure the plugin doesn't make mistakes before trying because rules are twisted

orklah avatar Jul 28 '22 17:07 orklah

Yeah, that's going to be impossible to find a universal solution. I'd rather go by what is "commonly" found in legacy code, bc from what I can see in most code bases, it's a 2-5 issues that make up 90% of the non-auto-fixable code.

Would you accept a PR for the most common, minimal cases to be auto-fixed: array|list|object|string == string

kkmuffme avatar Jul 29 '22 23:07 kkmuffme

I'd accept any improvement that is proved safe (like mentionned before, the goal was to make a first sweep without worrying, even if the codebase don't have tests, so I don't want to risk introducing bugs). So as long as we can check that weird cases are excluded, I'm in!

orklah avatar Jul 30 '22 07:07 orklah