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 isArray instead of array supertype checks #1634

Merged
merged 2 commits into from Aug 25, 2022

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Aug 18, 2022

another small (got bigger over time..) refactor. I'll stop now.. :)

UPDATE: added another commit that does the same for isString

@herndlm
Copy link
Contributor Author

herndlm commented Aug 18, 2022

I was not expecting that one additional failure. weird. needs some debugging..

@staabm
Copy link
Contributor

staabm commented Aug 21, 2022

one more here

($stringType->isSuperTypeOf($leftType)->yes() && $stringType->isSuperTypeOf($rightType)->yes())

@herndlm herndlm force-pushed the is-array-cleanups branch 3 times, most recently from 1388797 to ded6e19 Compare August 22, 2022 15:43
@ondrejmirtes
Copy link
Member

Please ping me once you mark this as ready, I'm looking forward to merge this.

@herndlm
Copy link
Contributor Author

herndlm commented Aug 24, 2022

I think in order to be able to continue here I'd need #1644 to avoid weird behaviour or exceptions

@herndlm herndlm marked this pull request as ready for review August 25, 2022 15:09
@herndlm
Copy link
Contributor Author

herndlm commented Aug 25, 2022

@ondrejmirtes this is ready now. I could create isInteger next and do the same if you like that

@ondrejmirtes
Copy link
Member

Yeah, you could be the guy who spends the next 6 months painstakingly creating methods on Type interface for all the use cases where we currently use instanceof *Type and TypeUtils methods :) That would be really awesome if it sounds interesting to you :)

@ondrejmirtes ondrejmirtes merged commit b157177 into phpstan:1.8.x Aug 25, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@herndlm herndlm deleted the is-array-cleanups branch August 25, 2022 16:53
@herndlm
Copy link
Contributor Author

herndlm commented Aug 25, 2022

Yeah, you could be the guy who spends the next 6 months painstakingly creating methods on Type interface for all the use cases where we currently use instanceof *Type and TypeUtils methods :) That would be really awesome if it sounds interesting to you :)

you need to work on your sales pitch :D
I don't want to promise much, but I'll definitely continue with some primitive type methods. Feel free to mention any methods you're interested in

@ondrejmirtes
Copy link
Member

It's not my fault there's "pain" in "painstakingly" 🤣

@ondrejmirtes
Copy link
Member

Another easy one should be to replace stuff like instanceof TypeWithClassName, instanceof ObjectType, TypeUtils::getDirectClassNames().

@herndlm
Copy link
Contributor Author

herndlm commented Aug 26, 2022

AFAIK the problems with instanceof replacements are then that methods are not guaranteed to exist on the Type interface, right? E.g. it's not safe anymore to call getClassName() on the potential union :/

ah and I misread "painstakingly" with "painfully" yesterday I guess. Both are fitting terms I think :D

@ondrejmirtes
Copy link
Member

This is the problem I'm trying to solve - there needs to be methods for EVERYTHING directly on Type. It nicely solves issues when the type is UnionType etc.

@herndlm
Copy link
Contributor Author

herndlm commented Aug 26, 2022

definitely, yeah. it would also surely help with some constantarray operations I think and then greatly simplify the code that currently needs conditionals to deal with them with the downside of blowing up the Type interface I guess. but for the array/constantarray methods there could be a trait that implements the array-specific methods once for all non-array types or so and then maybe it's not a big deal after all

@ondrejmirtes
Copy link
Member

What we can do once the transformation is complete is to remove all specific Template*Type implementations and have just one TemplateType (class!) that supports any bound.

We can also stop inheriting type classes from each other.

And I guess we could also have a general SubtractableType that supports any type.

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