NonAcceptingNeverType

[jiripudil]

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?

[ondrejmirtes]

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 Calling Closure(never): something not considered as an error phpstan#6485.

[ondrejmirtes]

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]

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]

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 :)

[jiripudil]

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 :)