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

checkImplicitMixed #1645

Merged
merged 2 commits into from Aug 24, 2022
Merged

Conversation

MartinMystikJonas
Copy link
Contributor

Draft of checkImplicitMixed option as discussed in phpstan/phpstan#7835

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to actually test this somewhere :) Pick a rule when there's already checkExplicitMixed tested, and add a case for the implicit mixed.

Both changes in RuleLevelHelper should actually have a test. Thanks!

@ondrejmirtes
Copy link
Member

Also, the test should show that false doesn't report an error, and true does. @dataProvider is ideal for that.

@MartinMystikJonas
Copy link
Contributor Author

I tried to find some test where checkExplicitMixed is tested but all I found is just tests for specific bugs so I was not sure how this test should look like. Should I add new testCheckImplicitMixed() with some minimal input? Or copy some basic method like

public function testCallMethods(): void
with same input and just modify expected errors?

@ondrejmirtes
Copy link
Member

Don't copy this huge test, that's a maintenance nightmare :) Just add a small one, to show the differences.

@MartinMystikJonas
Copy link
Contributor Author

OK I will do it tomorrow ;-)

@MartinMystikJonas
Copy link
Contributor Author

Test added. Based on explicit mixed test I found in CallMethodsRuleTest.

@MartinMystikJonas
Copy link
Contributor Author

MartinMystikJonas commented Aug 24, 2022

I have another suggestion. I would welcome option that would allow casting implicit/explicit mixed. It is currently not allowed when checkXMixed is used. Maybe add new option allowCastMixed or two options allowCastXMixed? I think it would be enough to check it in findTypeToCheck of RuleLevelHelper right?

@ondrejmirtes
Copy link
Member

I really like how you made these two options independent :) Thank you very much!

@ondrejmirtes
Copy link
Member

About your suggestion - I don't get it, can you give me a code sample?

@MartinMystikJonas
Copy link
Contributor Author

At level 9 casting explicit mixed to string is not allowed. With implicit mixed option it would not be allowed for implicit mixed too.

https://phpstan.org/r/33a99c16-a95b-41c9-94bf-30e8404dd8cf

@ondrejmirtes
Copy link
Member

If you want you can just ignore this with a regex. But IMHO it's a valid error.

@MartinMystikJonas
Copy link
Contributor Author

That is what we do now. I just think it would be nice to have built-in support for ignoring such errors because it is often used. BUt I can live with just ignoring it.

@MartinMystikJonas
Copy link
Contributor Author

I just found out problems with checkImplicitMixed option. See https://phpstan.org/r/fbdaf0db-cf77-4cce-827f-63ca9facfaee

It seems that access lelel-2 key access to array with undefined structure always raises error even ehoen used in context where it is valid like in isset or setting array value.

But I do not know how to fix that.

@MartinMystikJonas
Copy link
Contributor Author

@ondrejmirtes I am not sure what to do now. I can prepare failing test but I do not know where to even start with fixing it.

@ondrejmirtes
Copy link
Member

It's expected, and I think it's not specific to implicit mixed. You can see the same thing with explicit mixed: https://phpstan.org/r/50a04edc-93f4-4871-981a-8d1147500d02

I think this error is desirable. You can fatal-crash PHP with some types and this construct: https://3v4l.org/oPMeC

@MartinMystikJonas
Copy link
Contributor Author

But here it is known it is an array and it works without errors on level 9.

@ondrejmirtes
Copy link
Member

Because static $cache is implicit mixed on purpose. We'd have to have some analysis of the whole function body to figure out what types it can be. There's an open issue for it.

@MartinMystikJonas
Copy link
Contributor Author

Oh I see. Problem is that you do not know if value of static is not set to someting completely different later and picked up in next call.

@ondrejmirtes
Copy link
Member

@MartinMystikJonas
Copy link
Contributor Author

Ok I will write it

@MartinMystikJonas
Copy link
Contributor Author

@ondrejmirtes
Copy link
Member

Please open a PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants