-
Notifications
You must be signed in to change notification settings - Fork 483
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 expression resolving of superglobals #3762
base: 2.1.x
Are you sure you want to change the base?
Improve expression resolving of superglobals #3762
Conversation
5d687dc
to
95b8360
Compare
This pull request has been marked as ready for review. |
I'm not a huge fan of this approach. It's going to pollute the output of Could we add these to the scope only when they are narrowed by some assignments or conditions? I think we already do a similar thing for |
smart, why haven't I thought about this :) I pushed something, not sure if it's going in the right direction or not though. |
I have a feeling that the stuff accessing |
} | ||
|
||
/** @return array<string, ExpressionTypeHolder> */ | ||
private function getNativeSuperglobalTypes(): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could de-duplicate this method by passing in the expressionTypes via parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right. I stole this basically from getNativeConstantTypes()
vs getConstantTypes()
which could be adapted in a similar way then. maybe in a follow-up refactor :)
86901a7
to
44586ac
Compare
44586ac
to
990605a
Compare
UPDATE: perf. testing is less relevant, the solution was adapted to lazily add global expression types which should avoid performance issues