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

Use TypeUtils::getOldConstantArrays in array_pop and array_shift extensions #1668

Merged

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Aug 30, 2022

Since the logic for removing first and last elements was already there I wanted to re-use it. I'm not the biggest fan of passing this info by reference, but did not come up with something better yet.

I'd like to get rid of TypeUtils::getConstantArrays for performance reasons where possible.

@herndlm
Copy link
Contributor Author

herndlm commented Aug 31, 2022

I have a refactor idea and will try to get rid of the reference

@herndlm herndlm marked this pull request as draft August 31, 2022 04:06
@@ -220,6 +220,32 @@ public function getValueTypes(): array
return $this->valueTypes;
}

public function getFirstValueType(): Type
Copy link
Contributor Author

@herndlm herndlm Aug 31, 2022

Choose a reason for hiding this comment

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

there's a getValueTypes right above which simply returns all valueTypes while this one is "smart" and returns the type that you would expect when you access the first or last element in PHP => it deals with optional keys correctly.

I was just afraid that naming-wise one might expect that this should simply return $this->valueTypes[0]. is the name OK, or does maybe getFirstValue or getFirstEntry or so make more sense? hmm

Copy link
Contributor

@staabm staabm Aug 31, 2022

Choose a reason for hiding this comment

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

in case the method is not re-usable/usefull as is for other use-cases you might move them as a private method into the extensions instead. that way its not public api and doesn't need in detail perfection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also possible, I agree, I'm fine with anything here I guess

@herndlm
Copy link
Contributor Author

herndlm commented Aug 31, 2022

was able to simplify this by not forcing re-usage of existing code. that existing code would have been slower anyways and do things that are not needed here

@staabm
Copy link
Contributor

staabm commented Aug 31, 2022

I'd like to get rid of TypeUtils::getConstantArrays for performance reasons where possible.

do you have a benchmark somewhere?
(as it also improves correctness, speed is not the most important thing IMO)

@herndlm
Copy link
Contributor Author

herndlm commented Aug 31, 2022

do you have a benchmark somewhere? (as it also improves correctness, speed is not the most important thing IMO)

nope, I'd also like to avoid doing one just for this tbh. In general using getConstantArrays should be avoided I think because it uses ConstantArray::getAllArrays which calls powerSet to deal with optional keys and that potentially blows up the unions in extreme cases with many optional keys.

I'm just going with what Ondrej also was recently doing basically where he used getOldConstantArrays in e.g. a96cdf2 (UPDATE: that's actually a bad example 😅), 771b860 or a6cec39

But you're right, it improves correctness, so the potential performance improvements are coming with it for free :)

@herndlm herndlm marked this pull request as ready for review August 31, 2022 08:00
@staabm
Copy link
Contributor

staabm commented Aug 31, 2022

is this PR also expected to fix phpstan/phpstan#7804 ?

@herndlm
Copy link
Contributor Author

herndlm commented Aug 31, 2022

is this PR also expected to fix phpstan/phpstan#7804 ?

I would not expect that, no. Here only ConstantArray behaviour and the return type handling are adapted.

@ondrejmirtes ondrejmirtes merged commit 169af1d into phpstan:1.8.x Aug 31, 2022
@ondrejmirtes
Copy link
Member

Nice! Thank you.

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