Skip to content

Keep non nullability if right expr is NeverType #1063

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.

Sign up for GitHub

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

Jump to bottom
Merged
ondrejmirtes merged 4 commits into from
Mar 18, 2022
Merged

Keep non nullability if right expr is NeverType #1063

ondrejmirtes merged 4 commits into from
Mar 18, 2022

Conversation

@rajyan
Copy link
Contributor

@rajyan rajyan commented Mar 10, 2022 •
edited
Loading

fixes phpstan/phpstan#5351

I haven't found yet why the second test case passes without this fix.

The test wasn't what I intended. fixed

@rajyan
Copy link
Contributor Author

rajyan commented Mar 10, 2022

ready for review!

@rajyan rajyan changed the title Fix/issue 5351 Keep non nullability if right expr is NeverType Mar 16, 2022
ondrejmirtes
ondrejmirtes requested changes Mar 17, 2022
View reviewed changes
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.

Please do it only for explicit NeverType. Otherwise it's perfect 😊

@herndlm
Copy link
Contributor

herndlm commented Mar 17, 2022 •
edited
Loading

@rajyan just wanted to say that I love the "small" fixes you're doing here and there. I was mostly too scared of NodeScopeResolver yet :)
Update: I could've written that on Twitter too, sorry for another notification Ondrej. Again 😅

@rajyan
Copy link
Contributor Author

rajyan commented Mar 17, 2022 •
edited
Loading

@ondrejmirtes
Thank you, fixed!

@herndlm @ondrejmirtes
Thanks for your words.
PHPStan helped me a lot and greatly helping the PHP community. I want to know more about this tool, and make it greater if I can!

@ondrejmirtes ondrejmirtes merged commit 0a96fca into phpstan:1.5.x Mar 18, 2022
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 18, 2022

Thank you!

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 18, 2022

@herndlm This is usually what I have in mind when I tag an issue with "Easy fixes" milestone, it shouldn't be more than a few lines of code (but of course I can be wrong).

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 18, 2022

@rajyan Can you please add regression tests for this issue and make sure it fails when your fix is reverted? Thank you. phpstan/phpstan#4747

Also, this example https://phpstan.org/r/9879d278-5c2d-42ca-81e8-06e87bef529c from phpstan/phpstan#4885 should work but doesn't. Can you please look into it? Thanks.

@rajyan
Copy link
Contributor Author

rajyan commented Mar 18, 2022

@ondrejmirtes
I will try to take some time today or tomorrow:+1:

@rajyan rajyan deleted the fix/issue-5351 branch March 18, 2022 12:28
@rajyan rajyan mentioned this pull request Mar 18, 2022
Merged
@herndlm
Copy link
Contributor

herndlm commented Mar 18, 2022 •
edited
Loading

@herndlm This is usually what I have in mind when I tag an issue with "Easy fixes" milestone, it shouldn't be more than a few lines of code (but of course I can be wrong).

small as in, a few lines of code, but timing-wise it might take hours to think and get into it (at least for outsiders) ;)
but makes total sense!

This was referenced Mar 20, 2022
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Still nullable variable after use throw expression

3 participants

@rajyan @herndlm @ondrejmirtes