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

Mutating scope refactor and regression test #1934

Merged

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented Oct 28, 2022

Added some regression test for the logic of keeping var related expression types.
Fixed the last test case to work correctly.

Comment on lines +66 to +67
assertType('array', $arr);
assertType("mixed", $arr[$key]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$arr[$key] was mixed~null before this PR

@rajyan rajyan changed the title Mutating scope refactor regression test Mutating scope refactor and regression test Oct 28, 2022
@rajyan rajyan marked this pull request as ready for review October 28, 2022 12:26
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@rajyan rajyan marked this pull request as draft October 28, 2022 23:28
@rajyan rajyan force-pushed the mutating-scope-refactor-regression-test branch from a91acd5 to c32ad74 Compare October 28, 2022 23:56
@rajyan rajyan marked this pull request as ready for review October 29, 2022 00:09
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@rajyan
Copy link
Contributor Author

rajyan commented Oct 29, 2022

@rvanvelzen
Cherry picked your tests from #1929 because the refactoring now includes your fix too.

@ondrejmirtes
Copy link
Member

I tried to break the logic in #1929 and #1934 and I did it. This example breaks in these PRs: https://phpstan.org/r/704ec47a-e5b0-4d5b-9bf6-037c4b563278

Relying on Variable being in there and invalidating those expressions is pretty smart, but there are expressions that don't contain a Variable and need to be invalidated too.

I guess that's what those failures in phpstan-webmozart-assert are about too.

@rajyan
Copy link
Contributor Author

rajyan commented Oct 29, 2022

Thank you for the example. I see, this logic isn't correct...
I'll have to investigate them further.

For the phpstan-webmozart-assert failure, I think rvanvelzen's explanation #1929 (comment) is right. (so it should fail as same as arrow function fails)

@rajyan
Copy link
Contributor Author

rajyan commented Oct 29, 2022

@ondrejmirtes
Do you think the logic of 93ef0b8 is correct enough?

@ondrejmirtes
Copy link
Member

I don't think that any changes around the Variables are correct. There's only one bug reported, and that's about function_exists(). So we need to retain this expression, and everything else needs to stay the same.

@ondrejmirtes
Copy link
Member

Also class_exists maybe.

@rajyan
Copy link
Contributor Author

rajyan commented Oct 29, 2022

I added the variable related specification logic in the migration of variableTypes and moreSpecificTypes
rajyan@3e20db3

to keep the logic before
https://github.com/rajyan/phpstan-src/blob/76be530e341d6452fbd4a910b8302438904caa55/src/Analyser/MutatingScope.php#L2910-L2922

but I think it's not correct enough
because of the #1934 (comment) case.

This pull request was first intended to fix that issue.

@rajyan
Copy link
Contributor Author

rajyan commented Oct 29, 2022

so, I think I can reset this pull request to 93ef0b8
and leave the phpstan/phpstan#8205 open

@rajyan
Copy link
Contributor Author

rajyan commented Oct 29, 2022

Made your example explicit that there are non variable expressions that should be invalidated.
https://3v4l.org/9bXe9

@ondrejmirtes
Copy link
Member

so, I think I can reset this pull request to 93ef0b8

Yeah, sure :)

@rajyan rajyan force-pushed the mutating-scope-refactor-regression-test branch from f703817 to 2780476 Compare October 29, 2022 11:59
@rajyan
Copy link
Contributor Author

rajyan commented Oct 29, 2022

@ondrejmirtes
Reset and added your comment as a test case 😄

@ondrejmirtes ondrejmirtes merged commit d0f49cf into phpstan:1.9.x Oct 29, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@rajyan rajyan deleted the mutating-scope-refactor-regression-test branch October 29, 2022 12:12
@rajyan rajyan mentioned this pull request Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants