Skip to content

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Sep 1, 2022

Deprecates TypeUtils::getArrays() and TypeUtils::getAnyArrays() in favor of Type::getArrays()

I might be getting a bit greedy now, but this is the first step towards reducing usage of ConstantArrayType::getAllArrays()

@ondrejmirtes
Copy link
Member

getAnyArrays needs to die too in favour of a new method on Type :)

@herndlm
Copy link
Contributor Author

herndlm commented Sep 1, 2022

getAnyArrays needs to die too in favour of a new method on Type :)

sounds like a good follow-up :) or also here if you prefer that. I'll continue on Saturday or so I think

@herndlm
Copy link
Contributor Author

herndlm commented Sep 1, 2022

the one failure is the unrelated composer fork integration test again

@herndlm herndlm marked this pull request as ready for review September 1, 2022 19:27
@ondrejmirtes
Copy link
Member

My problem here is that Use PHPStan\Type\Type::getAnyArrays() instead is gonna be true for like 5 minutes and then we'll want people to rewrite it again :)

@herndlm
Copy link
Contributor Author

herndlm commented Sep 1, 2022

OK, sure, I'll do the new type method introduction here then soon

@herndlm herndlm marked this pull request as draft September 1, 2022 21:57
@herndlm herndlm changed the base branch from 1.8.x to 1.9.x October 5, 2022 15:41
@herndlm herndlm force-pushed the deprecate-get-arrays branch from 2e5195c to 4a182f7 Compare October 5, 2022 15:41
@herndlm herndlm force-pushed the deprecate-get-arrays branch from 4a182f7 to 1418606 Compare October 5, 2022 18:38
@herndlm
Copy link
Contributor Author

herndlm commented Oct 5, 2022

This should be ready too now. Next on my list would be to reduce usage of TypeUtils::flattenTypes() or ConstantArray::getAllArrays(). I'll continue on the weekend most likely

@herndlm herndlm marked this pull request as ready for review October 5, 2022 18:50
@ondrejmirtes ondrejmirtes merged commit 774f54e into phpstan:1.9.x Oct 6, 2022
@ondrejmirtes
Copy link
Member

Thank you! FlattenTypes is a weird one, I'm not sure what that should become on Type...

@herndlm herndlm deleted the deprecate-get-arrays branch October 6, 2022 05:10
@herndlm
Copy link
Contributor Author

herndlm commented Oct 6, 2022

Hmm after some thinking I'm not so fond of the two new type methods anymore. What would you think of a Type::getTypes(?string $className):array that returns types that are an instance of className instead? That would only really have a single implementation then and could be the successor of most of the TypeUtils methods

@ondrejmirtes
Copy link
Member

I'm pretty sure I want those dozens of type methods :) Why would you think otherwise?

@herndlm
Copy link
Contributor Author

herndlm commented Oct 6, 2022

Not entirely sure, it could help with with "unpacking" unions and intersections which is close to what also flattenTypes does. But it's hard. I guess in general I'd like to not see/use any of those type specific things and not have to e.g. handle constant arrays in a special way, but there's currently no way around that I guess

@herndlm
Copy link
Contributor Author

herndlm commented Oct 6, 2022

On the other hand flattenTypes is used very rarely and it essentially just unpacks a union

@ondrejmirtes
Copy link
Member

I think that we shouldn't replace flattenTypes per se, but always ask - "what are we trying to find out here?" It's possible that the answer is something like "we're looking for constant arrays", and there's already a special method for that. I don't think that we'll need Type::flatten at all...

@herndlm
Copy link
Contributor Author

herndlm commented Oct 6, 2022

Yeah, right. I guess what bothers me a bit is that we need to have special code so often for specific types which is why we need those methods. E.g. handle constant array optional keys. But I know that there is currently no alternative. At least none without unnecessary overhead

@herndlm
Copy link
Contributor Author

herndlm commented Oct 6, 2022

But I guess that's actually a good argument against the generic method which could promote bad practices :)

@herndlm
Copy link
Contributor Author

herndlm commented Oct 7, 2022

it looks like in all except one internal occurrences TypeUtils::flattenTypes($foo) can simply be replaced with $foo instanceof UnionType ? $foo->getTypes() : [$foo]. I somehow declared war on ConstantArrayType::getAllArrays() and to get rid of it I kind of need to get rid of flattenTypes first. I'm not sure though how to deprecate/replace it, e.g. a new helper that flattens only (potential) unions or the mentioned instanceof :/

@ondrejmirtes
Copy link
Member

Yeah, but I want to get rid of all instanceof *Type eventually 😊 Maybe with the exception of the TypeTraverser usages.

@herndlm
Copy link
Contributor Author

herndlm commented Oct 7, 2022

Yeah, but I want to get rid of all instanceof *Type eventually blush Maybe with the exception of the TypeTraverser usages.

exactly! which got me stuck a bit :)

@herndlm
Copy link
Contributor Author

herndlm commented Oct 18, 2022

I think I can get rid of both Type::getArrays() and Type::getConstantArrays() again. But I need a bit more time still. In case 1.9.x release is imminent - I'll try to reduce their usage and maybe it makes sense to migrate the last couple usages back to TypeUtils::*. I'd rather not see people using them tbh.. The latest cleanups show where we should be headed I think. And there we don't need them 😊

@ondrejmirtes
Copy link
Member

I have the same opinion, they're really not that specific.

The only thing I can think of is that if someone wants to do something custom with constant arrays, then they make sense for them.

@herndlm
Copy link
Contributor Author

herndlm commented Oct 18, 2022

good, let me know when you want to release 1.9.0, then I'll get rid of them again. in the meantime I'll try to clean up a bit more

@ondrejmirtes
Copy link
Member

I don't know yet, we're not in a hurry 😊 There's still some list and assert todos, and I want to see if @param-out makes it too.

Native vs. complete types also look really yummy but that might be too ambitious 😊

@rvanvelzen
Copy link
Contributor

There's still some list and assert todos

Do you happen to have a public list of those todos? 😁

@ondrejmirtes
Copy link
Member

@rvanvelzen I'm sorry, I promised to share that but forgot about that. Here you go:

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.

3 participants