Skip to content

Use TypeUtils::getOldConstantArrays in array_column extension #1683

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

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Sep 1, 2022

looks like it made it a tiny bit smarter. testImprecise4 was converted to testConstantArray12

@ondrejmirtes
Copy link
Member

BTW if you don't plan to make more changes and if you're not just experimenting, you don't need to open the PR as a draft. The problem is that I don't get an email notification when you mark it as ready for review.

if you're just waiting for CI results, you can open it as ready-to-merge from the start. If the CI looks green I can merge it sooner.

@herndlm
Copy link
Contributor Author

herndlm commented Sep 1, 2022

Good to know. Yeah, just always anxious that ci might be red. Like here :)

@herndlm herndlm force-pushed the array-column-get-old-constant-arrays branch from 9e47876 to dc4a54b Compare September 1, 2022 11:21
@herndlm herndlm marked this pull request as ready for review September 1, 2022 11:27
@herndlm
Copy link
Contributor Author

herndlm commented Sep 1, 2022

how do you wish to have the new Type::getConstantArrays() implemented? :)
return an empty array everywhere there are none directly or via trait? UPDATE: maybe I should study existing traits first..

@ondrejmirtes
Copy link
Member

Yeah, ConstantArrayType should return [$this], UnionType should merge the results from all the types.

IntersectionType I'm not sure about, you can have ConstantArrayType&NonEmptyArrayType...

@ondrejmirtes
Copy link
Member

And please search and replace the usages programatically, either with a regex in PhpStorm, or write a script that uses PhpParser format-preserving printer and just switch those AST nodes around in a node visitor :) Here's an example: https://github.com/nikic/PHP-Parser/blob/master/doc/component/Pretty_printing.markdown#formatting-preserving-pretty-printing

@herndlm
Copy link
Contributor Author

herndlm commented Sep 1, 2022

Yeah, ConstantArrayType should return [$this], UnionType should merge the results from all the types.

IntersectionType I'm not sure about, you can have ConstantArrayType&NonEmptyArrayType...

right, but I meant mostly how e.g. IntegerType should implement it. I wonder if I should create a new e.g. NotConstantArrayType trait or just implement it returning an empty array in most of the types

@ondrejmirtes
Copy link
Member

Both are valid to me :) I don't like the traits too much but we already accepted that direction so it's fine for me.

@herndlm
Copy link
Contributor Author

herndlm commented Sep 1, 2022

fyi I have the Type::getConstantArrays adaption ready and will open the PR after this one got merged :)

@ondrejmirtes ondrejmirtes merged commit 03a76b7 into phpstan:1.8.x Sep 1, 2022
@ondrejmirtes
Copy link
Member

Alright, thank you :)

@herndlm herndlm deleted the array-column-get-old-constant-arrays branch September 1, 2022 12:37
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.

2 participants