Skip to content

implement isNull() on Type#1978

Merged
ondrejmirtes merged 2 commits intophpstan:1.9.xfrom
staabm:is-null
Nov 7, 2022
Merged

implement isNull() on Type#1978
ondrejmirtes merged 2 commits intophpstan:1.9.xfrom
staabm:is-null

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Nov 7, 2022

@staabm
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

@VincentLanglet
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Thank you!

@herndlm
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@mad-briller
Copy link
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

6 participants