Reflection parameter type could be more precise about specific BackedEnum::from() and tryFrom() type

[ondrejmirtes]

Description

The following code:

<?php

declare(strict_types=1);

enum IntBacked: int {
    case One = 1;
}

$rm = new ReflectionMethod(IntBacked::class, 'tryFrom');
var_export((string) $rm->getParameters()[0]->getType());

Resulted in this output:

'string|int'

But I expected this output instead:

'int'

Because when a bad type is passed in there, it's rejected with TypeError:

Argument #1 ($value) must be of type int, string given

Related: #12863

PHP Version

PHP 8.1+

Operating System

No response

[iluuu1994]

I'm not sure how this can be achieved without splitting BackedEnum interface for int- and string-backed enums. A common interface cannot contain from(int|string $value) as it would not be with from(int $value) and from(string $value). Removing from from BackedEnum and moving them to more specialized interfaces would be a big BC break. I don't think that's something we should do for the sole purpose of improving reflection.

[ondrejmirtes]

I just want Reflection to tell the truth about actual behaviour of the methods.

If you're worried about parameter contravariance I don't think LSP applies to static methods.

[iluuu1994]

@ondrejmirtes Technically yes, the implementation here doesn't match the interface. There is intentional coercion to int/string depending on the enum type because the backing table is type-sensitive. This is not like normal array behavior in PHP where ''42'' is coerced to 42. We could've instead chosen to throw a different type of exception I suppose. But that doesn't solve your typing problem.

If you're worried about parameter contravariance I don't think LSP applies to static methods.

static functions aren't exempt from LSP checks. I may be misunderstanding your point.

[ondrejmirtes]

static functions aren't exempt from LSP checks. I may be misunderstanding your point.

Sorry, I was reacting to this and probably misunderstood what you were saying:

A common interface cannot contain from(int|string $value) as it would not be with from(int $value) and from(string $value)

So there isn't a way in Reflection to declare IntBacked from and tryFrom methods' parameter as int?

But ideally we would want that, right? What kind of refactoring would lead us there?

[iluuu1994]

Sorry, I was reacting to this and probably misunderstood what you were saying:

A common interface cannot contain from(int|string $value) as it would not be with from(int $value) and from(string $value)

Heh, my sentence is completely missing the word "compatible", maybe that contributed to the confusion ^^

So there isn't a way in Reflection to declare IntBacked from and tryFrom methods' parameter as int?

Not if we want to stay LSP-conformant with BackedEnum, or removing from/tryFrom from BackedEnum and moving them to IntBackedEnum/StringBackedEnum. Essentially, from(BackingType $value) would be correct, as an associated type. But PHP doesn't have that.

But ideally we would want that, right? What kind of refactoring would lead us there?

interface BackedEnum {}

interface IntBackedEnum extends BackedEnum {
    public static function from(int $value): static;
    public static function tryFrom(int $value): ?static;
}

interface StringBackedEnum extends BackedEnum {
    public static function from(string $value): static;
    public static function tryFrom(string $value): ?static;
}

This would work, but makes BackedEnum useless. Yes, you can rely on from/tryFrom being there, but it doesn't help you with static analysis.

[ondrejmirtes]

But:

1. These static methods violate LSP already.
2. I'm sure PHP internals have a way of sneaking the "right" parameter type into IntBacked::from even without properly introducing a new interface and without causing runtime errors/warnings 😊 Is it possible?

I personally don't need this for PHPStan as I can simply fix this in BetterReflection here https://github.com/Roave/BetterReflection/blob/e0f312cd09d09aa358e7b2a38bb17ed90b380d41/src/Reflection/ReflectionClass.php#L597 and create the type based on the backing scalarTypr, I'm just interested in the native reflection reporting this correctly for PHP users 😊

[iluuu1994]

These static methods violate LSP already.

Not sure. Essentially, all internal functions are typed as mixed (as in, the signature is completely irrelevant). They do parameter type verification manually, as part of the implementation. You can think of this as an additional restriction, similar to the following:

static function from(string|int $value) {
    if (!is_int($value)) {
        throw new TypeError();
    }
}

In other words, the function does have the given signature (which is why reflection reports it in this way), but there's a more specific runtime type enforcement. This is what I (probably confusingly) referenced above:

We could've instead chosen to throw a different type of exception I suppose. But that doesn't solve your typing problem.

[ondrejmirtes]

Yes, that is an LSP violation 😊 If you implement an interface but you decide to always throw an exception from some of the methods, even if you’re not changing the signatures, you’re essentially breaking the interface because the implementation cannot be used in its place.

[DanielEScherzer]

What if something other than a TypeError was thrown, e.g. EnumWrongBackingError (name can be changed)? At least then it would be a bit clearer that the reason for the error is because the parameter doesn't correspond to the backing type, rather than because the parameter is always the wrong type (e.g. null or an object as a parameter is always going to be wrong, regardless of the enum's backing).

We could make EnumWrongBackingError extend TypeError if we wanted to minimize the BC break

[iluuu1994]

I don't think the error is confusing. Ondřej was asking about more accurate reflection.

The most correct solution would be to use associated types, or even a bottom type (e.g. never) instead of int|string, so that it could correctly be widened in the enum implementing the interface. The engine asserts that LSP is not violated for internal classes, and I don't want to add an exception for this case.

[DanielEScherzer]

I don't think the error is confusing. Ondřej was asking about more accurate reflection.

The most correct solution would be to use associated types, or even a bottom type (e.g. never) instead of int|string, so that it could correctly be widened in the enum implementing the interface.

Ah, thanks for explaining, that makes more sense. But, we don't actually allow never has a parameter type, at least not in userland classes?

Fatal error: never cannot be used as a parameter type in /in/cNNrk on line 4

https://3v4l.org/cNNrk