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

implement isNull() on Type #1978

Merged
merged 2 commits into from Nov 7, 2022
Merged

implement isNull() on Type #1978

merged 2 commits into from Nov 7, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Nov 7, 2022

@staabm
Copy link
Contributor Author

staabm commented Nov 7, 2022

BTW I have a small thing to ask - please use first letter upper-case in your PR titles and commit messages.

starting with my next PR, I will take this into account.

@staabm staabm marked this pull request as ready for review November 7, 2022 13:01
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@VincentLanglet
Copy link
Contributor

Small thought for ConstantInteger, ConstantString, ConstantBoolean, ...

Rather than methods isConstantInteger on so on, did you think/do you plan to create isConstant since you'll already have isInteger, isBoolean, isString, ... , :)

@staabm
Copy link
Contributor Author

staabm commented Nov 7, 2022

Rather than methods isConstantInteger on so on, did you think/do you plan to create isConstant since you'll already have isInteger, isBoolean, isString, ... , :)

these types can already be checked with the ConstantScalarType interface, so I think there is currently no need to have them on Type again

@ondrejmirtes
Copy link
Member

@VincentLanglet I think a method like Type::getConstantStrings(): list<ConstantStringType> makes the most sense.

But integers complicate it a bit. I don't know how getConstantIntegers should be implemented in IntegerRangeType, I don't want huge arrays. Maybe we'll have to treat integers in a different way.

@VincentLanglet
Copy link
Contributor

Rather than methods isConstantInteger on so on, did you think/do you plan to create isConstant since you'll already have isInteger, isBoolean, isString, ... , :)

these types can already be checked with the ConstantScalarType interface, so I think there is currently no need to have them on Type again

I thought the purpose of all those methods were to get rid of all the instanceof checks.
So how do you get rid of the instanceof check in (one of you current diff)

($returnType->isNull()->yes() || $returnType instanceof ConstantBooleanType)

?

@staabm
Copy link
Contributor Author

staabm commented Nov 7, 2022

ondrej has a better idea of it, I am pretty sure.

I thought the purpose of all those methods were to get rid of all the instanceof checks.

I think we are getting rid of instanceof X while X is a concrete type class. I am not sure we need to get rid of it when X is an interface.

@ondrejmirtes
Copy link
Member

@VincentLanglet There's already Type::isTrue(): TrinaryLogic and Type::isFalse(): TrinaryLogic.

@staabm We want to get rid of instanceof Constant*Type too because very often people just do $type instanceof ConstantStringType and it doesn't occur to them they should handle 'foo'|'bar' as well.

@ondrejmirtes ondrejmirtes merged commit d9d1263 into phpstan:1.9.x Nov 7, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@herndlm
Copy link
Contributor

herndlm commented Nov 7, 2022

good, then there's still work left for me 😅

@staabm staabm deleted the is-null branch November 7, 2022 13:33
@ondrejmirtes
Copy link
Member

@herndlm There's ton of work to be done, I'm looking forward to get rid of instanceof TypeWithClassName 😊 And there's a lot more.

@herndlm
Copy link
Contributor

herndlm commented Nov 7, 2022

I know that there's no need to be worried hehe. great that multiple people are now actively cleaning up!

@ondrejmirtes
Copy link
Member

Also SubtractableType, that's gonna be a lot of fun 😂

@mad-briller
Copy link
Contributor

We want to get rid of instanceof Constant*Type too because very often people just do $type instanceof ConstantStringType and it doesn't occur to them they should handle 'foo'|'bar' as well.

i ran into this exact case last week in a custom rule 😅

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