Skip to content
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

Do not check variance validity in private methods #2064

Merged
merged 1 commit into from Dec 8, 2022

Conversation

jiripudil
Copy link
Contributor

As part of my ongoing work on generics (and type projections, eventually 🀞), I'm investigating an idea to add an (optional) variance check for properties, and I thought it would be a nightmare – you couldn't even have a class Collection with a covariant template for the item type because you couldn't use the template type in the inherently invariant private array $items.

That's when it occurred to me that perhaps the variance check should ignore private members altogether – variance is all about subtyping, and private members do not really come into play in that context. PHP doesn't enforce variance rules for private members in normal subtyping, and I think neither should PHPStan in generic subtyping.

(Kotlin approves πŸ˜‰)

@ondrejmirtes
Copy link
Member

Perfect! :)

One more idea - these rules should be more loose for immutable classes. PHPStan supports @immutable above classes and also readonly class keyword for PHP 8.2. So I think it's fine to use covariant @param T in a class that's readonly/immutable.

@ondrejmirtes ondrejmirtes merged commit c9d76cb into phpstan:1.9.x Dec 8, 2022
376 of 377 checks passed
@ondrejmirtes
Copy link
Member

I think there's already a similar exception for constructor.

@jiripudil jiripudil deleted the no-private-variance-check branch December 8, 2022 11:16
@jiripudil
Copy link
Contributor Author

jiripudil commented Dec 8, 2022

So I think it's fine to use covariant @param T in a class that's readonly/immutable.

I can see where the idea is coming from, but I have to disagree. This is not about mutability, it's about substitutability.

When I think about generic variance, I do this mental exercise where I "compile" the generics in my head into monomorphized code: a separate class for each instance of the type argument, with the type argument inlined. It really helps me think about the problem in a more hands-on, focused way.

interface Animal {}
interface Dog extends Animal {}

/** @template T */
readonly class Collection
{
    /**
     * @param T $item
     * @return Collection<T>
     */
    public function add($item): self { /* ... */ }
}

I turn the code above into:

readonly class CollectionAnimal
{
    public function add(Animal $item): self { /* ... */ }
}

readonly class CollectionDog
{
    public function add(Dog $item): self { /* ... */ }
}

Now, when I make the template type variant, I'm essentially just adding a subtyping relation between the classes. For a covariant template, I'm saying that readonly class CollectionDog extends CollectionAnimal (and the other way around for a contravariant template):

readonly class CollectionAnimal
{
    public function add(Animal $item): self { /* ... */ }
}

readonly class CollectionDog extends CollectionAnimal
{
    public function add(Dog $item): self { /* ... */ }
}

This obviously violates LSP: wherever I accept CollectionAnimal, I should be able to substitute it with CollectionDog, but that's not possible because I wouldn't be able to add a Cat into it at runtime, readonly or not.


I think there's already a similar exception for constructor.

As I understand it, that's a slightly different case. Private members are ignored altogether while in the constructor, the check just omits the initial positional variance. For covariant T:

/**
 * @param T $a
 * @param callable(T): void $b
 */
public function __construct(
    mixed $a,
    callable $b,
) {}

Here, $a is allowed as a parameter, but $b is not, because it contains the template type in a callable parameter which is a contravariant position.

@ondrejmirtes
Copy link
Member

OK, great, makes sense, thank you :)

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.

None yet

2 participants