Skip to content

NonAcceptingNeverType#2540

Merged
ondrejmirtes merged 8 commits intophpstan:1.10.xfrom
jiripudil:non-accepting-never-type
Aug 24, 2023
Merged

NonAcceptingNeverType#2540
ondrejmirtes merged 8 commits intophpstan:1.10.xfrom
jiripudil:non-accepting-never-type

Conversation

@jiripudil
Copy link
Copy Markdown
Contributor

split from #2485, closes phpstan/phpstan#9133

The idea is that never should accept no value at all, so that it can truly act as PHP's bottom type. In the light of that, the naming feels a bit strange: NeverType alone should already be non-accepting. But I know that's not possible right now due to how it is used especially with generics.

Perhaps we could rename NonAcceptingNeverType to NeverType and turn the current (implicit) NeverType into something like UnknownType? Is it something worth revisiting in PHPStan 2.0?

On a related note, are there other places where I should replace NeverType with NonAcceptingNeverType? Maybe everywhere NeverType is instantiated as explicit?

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost perfect, thank you :)

  1. Should we override and change isAcceptedWithReasonBy? Maybe not, just asking.
  2. We should clarify the relationship between NeverType and NonAcceptingNeverType. Ideally with tests in TypeCombinatorTest::dataUnion and dataIntersect, and by implementing it in isSuperTypeOf/isSubTypeOf.
  3. Another regression test can be written for phpstan/phpstan#6485.

@ondrejmirtes
Copy link
Copy Markdown
Member

As for your questions:

Perhaps we could rename NonAcceptingNeverType to NeverType and turn the current (implicit) NeverType into something like UnknownType? Is it something worth revisiting in PHPStan 2.0?

On a related note, are there other places where I should replace NeverType with NonAcceptingNeverType? Maybe everywhere NeverType is instantiated as explicit?

I think the PR changes in src/ look exactly as they should. UnknownType is more close to mixed or any which is the exact opposite :)

Maybe everywhere NeverType is instantiated as explicit?

That would actually defeat the purpose of NonAcceptingNeverType and it would start failing in more places than we want it to :)

As for any future plans - I think that we can unify the code to have a single NeverType again once this suggestion is implemented: phpstan/phpstan#6732 (comment)

@jiripudil
Copy link
Copy Markdown
Contributor Author

Should we override and change isAcceptedWithReasonBy? Maybe not, just asking.

I think not. From a theoretical standpoint, never is a subtype of everything, and should therefore be accepted everywhere. From a practical one, no actual value can be of type never, hence it usually indicates unreachable code where any further analysis is pointless anyway.

Another regression test can be written for phpstan/phpstan#6485.

👍

We should clarify the relationship between NeverType and NonAcceptingNeverType. Ideally with tests in TypeCombinatorTest::dataUnion and dataIntersect, and by implementing it in isSuperTypeOf/isSubTypeOf.

That's what I'm struggling with the most. In the ideal world in my head, they should be one, and I find it difficult to find where to draw the line between them. For example, with NonAcceptingNeverType extending NeverType, many methods in NonAcceptingNeverType currently return (implicit) NeverType, and I don't know if that's correct or not.

Maybe it would be wiser if NonAcceptingNeverType did not extend NeverType and rather was a separate Type implementation instead? But then there's a ton of instanceof NeverType in the codebase that would probably need to be updated...

UnknownType is more close to mixed or any which is the exact opposite :)

That's right. For instance, TypeScript's unknown is essentially equivalent to PHPStan's StrictMixedType: it can only be assigned to itself, but it accepts everything – a quality it shares with current NeverType! So I'd say NeverType is, in fact, already halfway there :)

I think that we can unify the code to have a single NeverType again once this suggestion is implemented: phpstan/phpstan#6732 (comment)

Yes, I was thinking the proposed UnknownType could be used to represent this unresolved state, it looks like a good fit for the use case.

Maybe everywhere NeverType is instantiated as explicit?

That would actually defeat the purpose of NonAcceptingNeverType and it would start failing in more places than we want it to :)

Oh yes, right, I remember, that's exactly what happened in #2110 😅 what if we tread more carefully this time? It seems to me that most of the places that currently instantiate an explicit NeverType do represent an unreachable code situation, and some places with an implicit NeverType do as well (e.g. ObjectType subtraction). My gut feeling says it should be safe to switch to a true (non-accepting) never in these cases 🤔

Also, what about here?

@ondrejmirtes ondrejmirtes force-pushed the non-accepting-never-type branch from 1c2cfd9 to 9ac9ba3 Compare August 24, 2023 13:47
@ondrejmirtes
Copy link
Copy Markdown
Member

Hi, I really appreciate your work and I brought it over the finish line :) If you feel like NonAcceptingNeverType should be used in more places instead of NeverType, feel free to do so.

I kept NonAcceptingType extending NeverType, because why not. Let's not break existing instanceof NeverType code.

The class inheritance relationship can be different to what isSuperTypeOf says. I'm not even sure what the correct answer is - I just wanted TypeCombinator to behave always the same way, no matter the order of types :)

@ondrejmirtes ondrejmirtes merged commit 4450cf1 into phpstan:1.10.x Aug 24, 2023
@jiripudil jiripudil deleted the non-accepting-never-type branch August 24, 2023 16:11
@jiripudil
Copy link
Copy Markdown
Contributor Author

Hi, thanks!

If you feel like NonAcceptingNeverType should be used in more places instead of NeverType, feel free to do so.

Sure, I'll try and follow up on this to see if it's feasible.

I'll go and rebase #2485 as soon as I have a minute :)

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