InvalidDocblock when annotating promoted property with @var only

[vudaltsov]

https://psalm.dev/r/f99085fcfb

Also adding non-type property annotations does not have any effect: https://psalm.dev/r/c33004265b. I can add a separate issue if needed.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/f99085fcfb

<?php

final class A
{
	public function __construct(
        /**
         * @var iterable<string>
         */
        private iterable $strings,
    ) {
    }
}

Psalm output (using commit 7fc4ae3):

ERROR: InvalidDocblock - 9:9 - Param strings of A::__construct should be documented as a param or a property, not both

https://psalm.dev/r/c33004265b

<?php

final class A
{
	public function __construct(
        /**
         * @psalm-readonly
         */
        private string $string,
    ) {
    }
    
    private function mutateString(): void
    {
    	$this->string = '';
    }
}

Psalm output (using commit 7fc4ae3):

No issues!

[orklah]

Yeah, please open a new issue for the second example

[orklah]

The error is caused by #6764 , Psalm thinks iterable comes from param type and iterable<string> comes from property type. We should probably allow signature return type to be independant for that check

[orklah]

I'll think about it this afternoon. This issue dug up the exact problem we were trying to solve by forbidding to have more than one type.

At worse, we could have something like

/**
 * @param int $param
 */
public function __construct(
        /**
         * @var string
         */
        private float $param,
    ) {
    }

And we have to decide what to do with that

[orklah]

The most natural way to do that in Psalm would be:

1. The param type must be equal or more precise than the property type
2. The param type is defined by "the param docblock ?? the signature type ?? the property docblock"

Those are the rules when there are no promoted properties, but those two rules make the first example fail because the param type becomes iterable<mixed, mixed> and the property type is iterable<mixed, string>

To make the example pass, those rules should be applied:

1. the param and property types must be exactly the same
2. the param and property types are defined by "the param docblock ?? the property docblock ?? the signature type"

I think it's a little weird to do that because then, there is absolutely no difference between a param docblock and a property in terms of features so why bother using @var here?

The first set of rules would also allow this:
https://psalm.dev/r/3b7fd246d1
In this example, we restrict the type of the incoming parameter while still allowing the property to have a wider type than the param. With the second set of rules this becomes impossible (as both must be absolutely equal)

I'm really conflicted on what to do here. I'm not sure what would be best. @weirdan @ondrejmirtes WDYT?

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/3b7fd246d1

<?php
class A{
    /** @param non-empty-string $param */
    public function __construct(
 		/** @var string */
        private string $param,
    ) {
    }
    
    public function setEmpty() {
        $this->param = '';
    }   
}

Psalm output (using commit 0fbf515):

ERROR: InvalidDocblock - 6:9 - Param param of A::__construct should be documented as a param or a property, not both

ERROR: UndefinedThisPropertyAssignment - 11:9 - Instance property A::$param is not defined

INFO: MissingReturnType - 10:21 - Method A::setEmpty does not have a return type, expecting void

[ondrejmirtes]

Hi, when I was implementing promoted properties in PHPStan, I wasn't sure how people are going to use it. So PHPStan supports declaring promoted property type through both @param and the @var style here.

So in case of constructor @param tag, it uses the type for both the method parameter (for instantiation) and also for the property type.

In case of @var (https://phpstan.org/r/e79ca1d7-529f-49d6-abdb-55eb81d27138), it uses the type for the promoted property and it also uses it for the parameter type in this special case.

I don't know what would happen if someone uses both @var and @param for promoted property, I haven't defined that case, and I don't know why anyone would want to choose that.

I don't know if there's some unclear case to take care of, if it is, please ask about that :)

[weirdan]

I think as far as docblocks are concerned we should require @var and @param to be the same if both are present for some reason, otherwise use whatever is there for both property and parameter type. This gives us a single docblock type. And its relation with the native type should be the same as we have now for parameters - that's 'docblock type should be a subtype of native type', if I'm not mistaken.

[orklah]

Thanks for your answers! I'll read that calmly tomorrow and I'll fix that :)