Skip to content

Fix issue 8467 intersect resolved conditional types #2059

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

Merged
merged 3 commits into from
Dec 7, 2022

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented Dec 7, 2022

I thought about this once in 42bddf0 but wasn't sure when this is needed.

The composer integration test failure was a very good example.
The conditional type knows $initialClone=true => $origCwd=string|null, but isset($originCwd) has already specified $originCwd=>'string' in the if ($initialClone && isset($origCwd)) truthy scope. Conditional type can be resolved in the scope too, so it overwrites the $originCwd's type as $origCwd=string|null.

Before my changes (in 1.9.2) resolving conditional types for specified types were skipped, but I think it is better to intersect, because the conditional type can be narrower than the specified type.


if ($initialClone && isset($origCwd)) {
assertType('string', $origCwd);
assertType('null', $cwd);
Copy link
Contributor Author

@rajyan rajyan Dec 7, 2022

Choose a reason for hiding this comment

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

I believe this one is not covered in createConditionalExpressions yet and not working before 1.9.2 too.
I think it might be better to solve it in a different PR

@rajyan
Copy link
Contributor Author

rajyan commented Dec 7, 2022

The first error of larastan test failure looks like an improvement, but not sure with the second one.

@rajyan rajyan marked this pull request as draft December 7, 2022 02:16
@rajyan
Copy link
Contributor Author

rajyan commented Dec 7, 2022

https://github.com/phpstan/phpstan-src/actions/runs/3635043616/jobs/6133787739

Tested in the playground and I believe both larastan test failure are improvements.
https://laravelplayground.com/#/snippets/45e88341-c412-4c26-b3eb-5956890561aa

First one
https://github.com/nunomaduro/larastan/blob/325d2f377f17d99529ea76833479e6ab6d6ff64c/tests/Type/data/collection-stubs.php#L33
we know that the items is <int, App\User>

second one
https://github.com/nunomaduro/larastan/blob/325d2f377f17d99529ea76833479e6ab6d6ff64c/tests/Type/data/collection-generic-static-methods.php#L171
the result is always an empty items because the callback is invalid, that means TKey cannot be resolved, and should be array-key (int|string).

Not sure enough with the second test. Maybe @canvural knows something?

@rajyan rajyan marked this pull request as ready for review December 7, 2022 04:22
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@rajyan
Copy link
Contributor Author

rajyan commented Dec 7, 2022

The test is failing in #2058 too, it may have nothing to do with this change anyway.

@ondrejmirtes ondrejmirtes merged commit ee86dda into phpstan:1.9.x Dec 7, 2022
@ondrejmirtes
Copy link
Member

Thank you 😊

@rajyan rajyan deleted the fix-issue-8467 branch December 8, 2022 08:49
@canvural
Copy link
Contributor

canvural commented Dec 8, 2022

phpstan/phpstan-src/actions/runs/3635043616/jobs/6133787739

Tested in the playground and I believe both larastan test failure are improvements. laravelplayground.com/#/snippets/45e88341-c412-4c26-b3eb-5956890561aa

First one nunomaduro/larastan@325d2f3/tests/Type/data/collection-stubs.php#L33 we know that the items is <int, App\User>

second one nunomaduro/larastan@325d2f3/tests/Type/data/collection-generic-static-methods.php#L171 the result is always an empty items because the callback is invalid, that means TKey cannot be resolved, and should be array-key (int|string).

Not sure enough with the second test. Maybe @canvural knows something?

phpstan/phpstan-src/actions/runs/3635043616/jobs/6133787739

Tested in the playground and I believe both larastan test failure are improvements. laravelplayground.com/#/snippets/45e88341-c412-4c26-b3eb-5956890561aa

First one nunomaduro/larastan@325d2f3/tests/Type/data/collection-stubs.php#L33 we know that the items is <int, App\User>

second one nunomaduro/larastan@325d2f3/tests/Type/data/collection-generic-static-methods.php#L171 the result is always an empty items because the callback is invalid, that means TKey cannot be resolved, and should be array-key (int|string).

Not sure enough with the second test. Maybe @canvural knows something?

I think this was caused because of a change in Laravel, not by this change. Because Larastan's own CI is also failing. So I'll fix this there 👍🏽

@ondrejmirtes
Copy link
Member

Yeah, I'll update the commit on our side once you fix it, thank you :)

@rajyan
Copy link
Contributor Author

rajyan commented Dec 8, 2022

Thank you!

@canvural
Copy link
Contributor

canvural commented Dec 8, 2022

@ondrejmirtes larastan/larastan@1a11021 Larastan's CI is green now.

@ondrejmirtes
Copy link
Member

PHPStan is green too 🎉 https://github.com/phpstan/phpstan/actions/runs/3649224831/jobs/6163628972

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.

4 participants