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

Improve impossible type checker for void-returning functions #1857

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

rvanvelzen
Copy link
Contributor

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.

Super-interesting! I'd probably put this behind a bleedingEdge toggle.

The @phpstan-assert functionality is enabled for everyone because it can be considered just a smarter type inference, but this bit feels a little bit extra and can result in some extra errors, so bleedingEdge would feel safer for me.

@herndlm
Copy link
Contributor

herndlm commented Oct 17, 2022

wow you were fast 🎉
but is this safe? e.g. can it be assumed that @phpstan-assert string $var internally basically does is_string()? I'm just afraid that it could cause false-positives when people use such an assert for something like assertEmail() basically.

@ondrejmirtes
Copy link
Member

@herndlm Yes, Richard expressed this concern earlier but I said to try it. That's why it's in bleedingEdge now only.

@ondrejmirtes
Copy link
Member

@rvanvelzen I think this one is ready to merge. I really appreciate all the work you do, but I feel like you're overusing the Draft feature a bit :) I respect it and I wouldn't merge a PR in draft state, but sometimes I'm eager to merge something because to my eyes it seems totally fine, so don't hesitate to mark things as ready sooner. We can always fix this in the future.

Another problem is that GitHub doesn't notify me when something's marked as ready so I added a workflow that adds a bot comment to such PRs: e3b9771

@rvanvelzen rvanvelzen marked this pull request as ready for review October 17, 2022 12:05
@ondrejmirtes ondrejmirtes merged commit cd62b6c into phpstan:1.9.x Oct 17, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@rvanvelzen rvanvelzen deleted the bug-8169 branch October 17, 2022 12:33
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.

PHPDoc-based type narrowing (@phpstan-assert) is not considered via Impossible* checks
3 participants