Skip to content

Mutating scope refactor and regression test#1934

Merged
ondrejmirtes merged 3 commits into
phpstan:1.9.xfrom
rajyan:mutating-scope-refactor-regression-test
Oct 29, 2022
Merged

Mutating scope refactor and regression test#1934
ondrejmirtes merged 3 commits into
phpstan:1.9.xfrom
rajyan:mutating-scope-refactor-regression-test

Conversation

@rajyan

@rajyan rajyan commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

Comment thread src/Analyser/MutatingScope.php
@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
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

@rajyan

rajyan commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

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

@ondrejmirtes

Copy link
Copy Markdown
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

rajyan commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

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

rajyan commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

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

@ondrejmirtes

Copy link
Copy Markdown
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
Copy Markdown
Member

Also class_exists maybe.

@rajyan

rajyan commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

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

rajyan commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

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

@rajyan

rajyan commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

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

@ondrejmirtes

Copy link
Copy Markdown
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

rajyan commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

3 participants