False positive for RedundantPropertyInitializationCheck

[ADmad]

I don't think the RedundantPropertyInitializationCheck error should be shown for this code:

https://psalm.dev/r/220b68f789

phpstan correctly doesn't show any error for it https://phpstan.org/r/016e0982-ef49-492b-944d-60faed480d4f

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/220b68f789

<?php declare(strict_types = 1);

class A 
{
    /** @psalm-suppress MissingConstructor */
    protected object $o;
    
    public function getter(): object
    {
        if (!isset($this->o)) {
            $this->o = new stdClass;
        }
        
        return $this->o;
    }
}

Psalm output (using commit 3a298d0):

ERROR: RedundantPropertyInitializationCheck - 10:14 - Property $this->o with type object should already be set in the constructor

ERROR: MissingConstructor - 6:22 - A has an uninitialized property A::$o, but no constructor

[orklah]

Psalm and PHPStan made different choices on that matter.

Based on your example, this is a horrible bug for which PHPStan is completely fine: https://phpstan.org/r/e6f0360d-2f60-4099-90f3-66d5f34489c5

What Psalm does is forcing you to initialize every property in the constructor. That's why you get a MissingConstructor and why you'd get a PropertyNotSetInConstructor if you did have a constructor but didn't initialize your property.

As a tradeoff, Psalm can know assume every property you have is initialized and will in turn tell you that it's pointless to check if they are.

So if you have a project that relies on having uninitialized properties, you should not only suppress MissingConstructor and PropertyNotSetInConstructor but also RedundantPropertyInitializationCheck and you'll be left with manual checking for property initialization (like PHPStan does)

[orklah]

Opened #7593 to improve documentation for these issues

[ADmad]

Based on your example, this is a horrible bug for which PHPStan is completely fine: https://phpstan.org/r/e6f0360d-2f60-4099-90f3-66d5f34489c5

Yes, for this one would expect an error but my code has an isset() check.

What Psalm does is forcing you to initialize every property in the constructor. That's why you get a MissingConstructor and why you'd get a PropertyNotSetInConstructor if you did have a constructor but didn't initialize your property.

I am fine with the MissingConstructor (and PropertyNotSetInConstructor) but RedundantPropertyInitializationCheck is incorrect IMO, since the property has not been initialized. The fact that it's not initialized through the constructor is why the isset() check is done. PHP (8+) is totally fine with typed properties being initialized outside of the constructor and there's no code path where the provided example would generate an error.

[ADmad]

BTW when you have code like if (!isset($this->o) || ...) { the error changes from RedundantPropertyInitializationCheck to TypeDoesNotContainType. I would expect at least the error type to stay consistent for easy suppression.

[orklah]

I would expect at least the error type to stay consistent for easy suppression.

This is probably a bug. Can you open an issue with a reproducer?

[ADmad]

https://psalm.dev/r/864c8178c5

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/864c8178c5

<?php declare(strict_types = 1);

class A 
{
    /** @psalm-suppress MissingConstructor */
    protected object $o;
    
    protected bool $x = false;
    
    public function getter(): object
    {
        if (!isset($this->o) || $this->x) {
            $this->o = new stdClass;
        }
        
        return $this->o;
    }
}

Psalm output (using commit 3a298d0):

ERROR: TypeDoesNotContainType - 12:13 - Type object for $this->o is always isset

ERROR: MissingConstructor - 6:22 - A has an uninitialized property A::$o, but no constructor

[AndrolGenhald]

I think the easiest solution here is to just default-initialize to null... Then there's no risk of child classes accidentally accessing it before it's initialized, and the child classes will warn when using it that it's nullable so you get a nice hint that you should be using getter() instead of direct access.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/eb8ff4f282

<?php declare(strict_types = 1);

class A 
{
    protected ?object $o = null;
    
    public function getter(): object
    {
        if (!isset($this->o)) {
            $this->o = new stdClass;
        }
        
        return $this->o;
    }
}

Psalm output (using commit 3a298d0):

No issues!