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

[PHP^8.2] Access to an undefined property when type hinting interface with @property #10302

Closed
samlev opened this issue Dec 14, 2023 · 17 comments

Comments

@samlev
Copy link

samlev commented Dec 14, 2023

Bug report

Similar to #8537 there is still an issue when a variable is type-hinted with an interface that has a @property, or @property-read declaration. From PHP8.2 onwards PHPStan is unable to recognise these declarations on interfaces.

/** 
 * @property int $x
 * @property-read int $y
 * @property-write int $z
 */
interface SampleInterface
{
}

function test(SampleInterface $test): void
{
    echo $test->x;
    echo $test->y;
    $test->z = 5;
}

Code snippet that reproduces the problem

https://phpstan.org/r/a199e998-8445-4590-9942-fe4b04a704bd

Expected output

No errors!

Did PHPStan help you today? Did it make you happy in any way?

No response

@ondrejmirtes
Copy link
Member

Hi, @property above an interface does not mean and does not enforce anything. It's a hack because PHP interfaces do not allow to declare properties.

Why are you putting @property above an interface?

@samlev
Copy link
Author

samlev commented Dec 14, 2023

I'm using the interface for a type hint because you can't type-hint on traits. This is a Laravel project where I have an interface like this:

/**
 * @property-read Collection<JobBatch>|null $batches
 * @property-read int|null $batches_count
 * @property-read Collection<JobBatch>|null $running
 * @property-read int|null $running_count
 * @property-read bool $busy
 * @mixin Model
 */
interface BatchAware
{
    /**
     * @return MorphToMany<JobBatch>
     */
    public function batches(): MorphToMany;
    /**
     * @return MorphToMany<JobBatch>
     */
    public function running(): MorphToMany;
    /**
     * @return Attribute<bool, void>
     */
    public function busy(): Attribute;
}

Which is implemented through a trait:

/**
 * @mixin BatchAware
 */
trait HasJobBatches
{
    /**
     * @return MorphToMany<JobBatch>
     */
    public function batches(): MorphToMany
    {
        return $this->morphToMany(JobBatch::class, 'batchable');
    }

    /**
     * @return MorphToMany<JobBatch>
     */
    public function running(): MorphToMany
    {
        return $this->batches()
            ->whereNull('finished_at')
            ->whereNull('cancelled_at');
    }

    /**
     * @return Attribute<bool, void>
     */
    public function busy(): Attribute
    {
        return Attribute::make(
            fn () => ($this->running_count ?? $this->loadCount('running')->running_count) > 0
        );
    }
}

That marries up like this:

class SomeModel extends Model implements BatchAware
{
    use HasJobBatches;
    // ...
}

Then elsewhere I'm trying to add code that will handle any BatchAware model:

   /**
     * @param Collection<BatchAware>|array<BatchAware> $items
     * @return string[]
     */
    protected function batchIds(Collection|array $items): array
    {
        return collect($items)
            ->flatMap(fn (BatchAware $item) => $item->running->pluck('id')->all())
            ->unique()
            ->all();
    }

@ondrejmirtes
Copy link
Member

If you type SomeModel instead of BatchAware then this should work okay?

@samlev
Copy link
Author

samlev commented Dec 14, 2023

Yes, it would, but the point is that I want to add this functionality to multiple models, then elsewhere I want to be able to run code on any model that happens to have this interface. I don't want to have to do something like this:

   /**
     * @param Collection<SomeModel|AnotherModel|MoreModels|EvenMoreModels|AdditionalModels>|array<SomeModel|AnotherModel|MoreModels|EvenMoreModels|AdditionalModels> $items
     * @return string[]
     */
    protected function batchIds(Collection|array $items): array
    {
        return collect($items)
            ->flatMap(fn (SomeModel|AnotherModel|MoreModels|EvenMoreModels|AdditionalModels $item) => $item->running->pluck('id')->all())
            ->unique()
            ->all();
    }

The interface is used to indicate to the application/IDE that the specific properties will be available on any BatchAware model by ensuring that the methods that provide them are set.

@ondrejmirtes
Copy link
Member

The problem is that a model can implement BatchAware but without providing these dynamic properties. So PHPStan cannot trust the interface to actually have those properties. There's no such concept of "abstract properties" in PHP.

@ondrejmirtes
Copy link
Member

A question: if this class actually has the properties like int|null $batches_count, where are they defined?

class SomeModel extends Model implements BatchAware
{
    use HasJobBatches;
    // ...
}

@samlev
Copy link
Author

samlev commented Dec 14, 2023

Yeah, I fully understand that, but the interface itself still makes some promises (namely it enforces methods that happen to return specific relations). Because we lack "true" generics obviously we can't guarantee that I haven't added BatchAware to a non-model (although I could hint Model&BatchAware), and while I can guarantee that the batches() method returns a MorphToMany relation, I can't guarantee that it is a relation to JobBatch models... But that's the point of documenting the types in doc-blocks.

PHP Can't guarantee these things to be 100% true, but within the domain of my application I'm confident enough that they are true, so it's a frustration that PHPStan won't trust my documentation.

@samlev
Copy link
Author

samlev commented Dec 14, 2023

A question: if this class actually has the properties like int|null $batches_count, where are they defined?

Through the "Magic" of Laravel (I am actually running Larastan for the added rules/coverage, but this is not a Laravel specific use case).

In short, within Laravel, if you have a method that returns a Relation (such as HasOne or MorphToMany) then a property corresponding to the name of that function is available on the model... or more to the point the __get() method will first look for anything in the $attributes array, then it will look for methods with a similar name. Simpified, it looks something like this:

class Model
{
    protected array $attributes = [];

    public function __get(string $name): mixed
    {
        if (isset($this->attributes[$name]) {
             return $this->attributes[$name];
        }

        if (method_exists($this, $name)) {
            $this->attributes[$name] = $this->$name()->get();
            return $this->$name;
        }


        if (str_ends_with($name, '_count')) {
            $relation = substr($name, 0, -6);
            if (method_exists($this, relation)) {
                $this->attributes[$name] = $this->$relation()->count();
                return $this->$name;
            }
        }
    }
}

Obviously there's a lot more to it - e.g. reflection to ensure that the method actually returns a relation, etc. etc. etc., but ensuring that a methtod exists which returns a relation is enough vertainty needed to ensure that there are a number of properties "available".

In this instance, the HasJobBatches trait fulfulls the requirements of the BatchAware interface by providing a method batches(): MorphMany, which will provide a Collection of something at $model->batches and will provide a count of the related models at $model->batches_count.

@mattvb91
Copy link

Having the exact same issue with larastan. Have you managed to make it work yet?

@samlev
Copy link
Author

samlev commented Dec 22, 2023

Having the exact same issue with larastan. Have you managed to make it work yet?

Only with this :(

// @phpstan-ignore-next-line see: https://github.com/phpstan/phpstan/issues/10302

@mattvb91
Copy link

Ah ok thats unfortunately not an option for me due to thousands of occurances. I was hopeing we could solve it more in line without ignoring with a @mixin label of some kind as a workaround but i havent had pc access to test yet

@ondrejmirtes
Copy link
Member

Yes, I'm thinking I'd allow this for interfaces with @mixin but I'd need to look into if it really solves all use cases.

@ondrejmirtes
Copy link
Member

In the future this will be possible to solve with @phpstan-require-extends above an interface or a trait.

@mattvb91
Copy link

mattvb91 commented Jan 3, 2024

@ondrejmirtes thank you, would this also work with included interfaces or traits from third party libs inside vendor?

@ondrejmirtes
Copy link
Member

This is now possible to solve with @phpstan-require-extends and @phpstan-require-implements.

Here's an example: https://phpstan.org/r/2dec4d59-482d-4894-ba05-0c2453e28f2b

@mattvb91 If you have any questions please first try it out and then report if it doesn't work as expected.

@staabm
Copy link
Contributor

staabm commented Jan 11, 2024

since psalm does not allow access to properties/methods based on the interface type, I just opened a feature request on psalm so we can discuss getting feature parity in this regard

vimeo/psalm#10538

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants