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

Support array-shapes in "array_merge" #641

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

voku
Copy link
Contributor

@voku voku commented Aug 25, 2021

No description provided.

@ondrejmirtes ondrejmirtes merged commit b4b44cf into phpstan:master Aug 25, 2021
@ondrejmirtes
Copy link
Member

Thank you!

@ondrejmirtes
Copy link
Member

Unfortunately I had to revert this extension: 53817aa

It's because of performance impact on real-world projects. The extension itself may not be the fault but it proves that having more ConstantArrayType instances during analysis leads to a slowdown.

The only way I see is to build a benchmarking mode into PHPStan itself that would compare the analysis times of project files and point out which file slowed down after this commit. Then isolate what exactly in that file causes it and try to optimize that. Unfortunately it's not my priority right now.

@voku
Copy link
Contributor Author

voku commented Aug 30, 2021

Understand. Is there a way we can overwrite the extension? Currently, the first matched extension always wins.

@ondrejmirtes
Copy link
Member

It's not possible. Everybody has to use the same array_merge extension that's in the core. I'd rather solve the problem than provide workarounds around it.

@voku
Copy link
Contributor Author

voku commented Aug 30, 2021

It's not possible. Everybody has to use the same array_merge extension that's in the core. I'd rather solve the problem than provide workarounds around it.

Maybe it's the "UnionType" change that has a bad performance impact? https://github.com/phpstan/phpstan-src/pull/641/files#diff-235402991c12d4f1091a88a9c19ed82c38749ed4e4dce1322474ed6c6d7fdcc8L40 Do you have a public repo to test the performance?

@ondrejmirtes
Copy link
Member

I don't. Right now you'd have to try the commit where the extension was still in (before the reverts) and try observing the performance impact on some of your projects.

@ondrejmirtes
Copy link
Member

Giving this another try :) #1653

@ondrejmirtes
Copy link
Member

Oh, ArrayMergeFunctionDynamicReturnTypeExtension already supports constant arrays...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants