Skip to content

PHP 8.2: Issue with type declaration for iterable|object #11797

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

Closed
mpyw opened this issue Jul 26, 2023 · 8 comments
Closed

PHP 8.2: Issue with type declaration for iterable|object #11797

mpyw opened this issue Jul 26, 2023 · 8 comments

Comments

@mpyw
Copy link
Contributor

mpyw commented Jul 26, 2023

Description

With PHP 8.2, the following changes were applied:

As a result, iterable became an alias for Traversable|array, changing the result of reflection acquisition as mentioned here.

However, there's a problem. While Traversable|object becomes an error because it's redundant, iterable|object compiles without any issue. But if we resolve the alias of iterable|object, it becomes Traversable|object|array, which is an invalid expression for PHP's type declaration.

In fact, we encountered a bug where creating a mock of a class with a method that takes iterable|object as an argument in the mockery/mockery causes a compile error only in PHP 8.2 or later. Should this be fixed by prohibiting the declaration of iterable|object itself? Or is this intended behavior, and should be addressed on the library side?

class ExampleClass
{
    public function __invoke(iterable|object $arg): void
    {
    }
}
// Type Traversable|object|array contains both object and a class type, which is redundant
$mock = \Mockery\Mockery::mock(Example::class);

PHP Version

PHP 8.2

Operating System

No response

Related Issues

@iluuu1994
Copy link
Member

This change was made in #7309. /cc @Girgias Should iterable|object be prohibited? iterable|array already is.

@Girgias
Copy link
Member

Girgias commented Jul 26, 2023

This took me a while to dig up again but here is the rationale:

#9558 (comment)

@iluuu1994
Copy link
Member

@Girgias Thanks for the comment. IMO we should either report both or neither. If iterable is considered a form of internal type alias, we should allow iterable|array.

@Girgias
Copy link
Member

Girgias commented Jul 26, 2023

@Girgias Thanks for the comment. IMO we should either report both or neither. If iterable is considered a form of internal type alias, we should allow iterable|array.

The thing is that array <: iterable but iterable </: object (i.e. array is a proper subtype of iterable whereas iterable is not a proper subtype of object) so adding array to something that declares iterable is necessarily redundant (and has always been marked that way).

The other case is not true, and this has also always been the case to allow object|iterable, so changing this is a BC break. Moreover, if we add support for userland type aliasing the semantics should be the same, and I don't think it's wise to need to resolve a known alias and throw redundancies when the alias is out of the user's control.

@iluuu1994
Copy link
Member

@Girgias If we had type aliases, with A1 = X|Y and A2 = Y would we error for A1|A2? Or just for A1|Y? I don't really care too much, it just seems the distinction is somewhat arbitrary, especially if we consider type aliases as "copy paste" types.

@Girgias
Copy link
Member

Girgias commented Jul 26, 2023

I'm not sure that example is actually useful, as we currently allow redundant class types (e.g. a child class and a parent class when the child class is "obviously" redundant) when this requires resolving the type.

A better example would be, what do we do with NumberLike = int|float|GMP and have somewhere else a declaration of object|NumberLike, should that type declaration fail? That would probably seem odd because the declaration of NumberLike may have been int|float in a prior version of a library and it just starting to throw would be weird.

@iluuu1994
Copy link
Member

iluuu1994 commented Jul 27, 2023

@Girgias Right, thanks for the explanation. I agree. We should only fail for A1|A2 if A2 is fully redundant, i.e. A2 <: A1. So feel free to close this issue. It gets more complex, e.g. A1|A2|A3 where A3 <: A1|A2, but I don't think that's worth implementing. I'd also be happy with no redundancy checks, but we already have them.

@Girgias
Copy link
Member

Girgias commented Jul 27, 2023

Going to close then.

@mpyw this should be fixed on the library side, ideally iterable might become reflectable as something like ReflectionTypeAlias when/if PHP supports type defs/alias natively and would have more consistent behaviour with userland.
But for the time being it might be a bit scuffed.

@Girgias Girgias closed this as not planned Won't fix, can't repro, duplicate, stale Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants