Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kkmuffme opened this issue Jul 27, 2022 · 5 comments · May be fixed by #5
Open

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

kkmuffme opened this issue Jul 27, 2022 · 5 comments · May be fixed by #5

Comments

@kkmuffme
Copy link
Contributor

kkmuffme commented Jul 27, 2022

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
@orklah
Copy link
Owner

orklah commented Jul 27, 2022

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

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 :(

@kkmuffme
Copy link
Contributor Author

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.

@orklah
Copy link
Owner

orklah commented Jul 28, 2022

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

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Jul 29, 2022

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

@orklah
Copy link
Owner

orklah commented Jul 30, 2022

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!

@kkmuffme kkmuffme linked a pull request Sep 12, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants