False positive PropertyNotSetInConstructor

[klimick]

<?php

trait CreatedAt {
    protected DateTimeImmutable $createdAt;

    private function onCreated(): void {
        $this->createdAt = new DateTimeImmutable();
    }
}

class Foo {
    use CreatedAt;

    public function __construct() {
    	$this->onCreated();
    }
}

class Bar extends Foo {}

Property $createdAt was initialized it Foo constructor. But psalm knows nothing about it in Bar.

https://psalm.dev/r/106a0483f7

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/106a0483f7

<?php

trait CreatedAt {
    protected DateTimeImmutable $createdAt;

    private function onCreated(): void {
        $this->createdAt = new DateTimeImmutable();
    }
}

class Foo {
    use CreatedAt;

    public function __construct() {
    	$this->onCreated();
    }
}

class Bar extends Foo {}

Psalm output (using commit 93e9054):

ERROR: PropertyNotSetInConstructor - 19:7 - Property Bar::$createdAt is not defined in constructor of Bar and in any methods called in the constructor

[designermonkey]

I also have an example at: https://psalm.dev/r/9a28243243

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/9a28243243

<?php declare(strict_types=1);

namespace Jorpo\Concerto\Event
{
    abstract class Event
    {
        protected string $uniqueId;
        protected string $when;

        protected function generateUniqueId(): void
        {
            $this->uniqueId = 'test';
        }

        protected function now(): void
        {
            if (empty($this->when)) {
                $this->when = 'test';
            }
        }
    }

}

namespace Jorpo\Concerto\FieldTypes\EnableFieldType\Model
{
    use Jorpo\Concerto\Event\Event;

    class SomeEvent extends Event
    {
        protected string $someValue;

        public function __construct(string $someValue)
        {
            $this->someValue = $someValue;
            $this->now();
            $this->generateUniqueId();
        }

        public function fieldTypeId(): string
        {
            return $this->someValue;
        }
    }
}

Psalm output (using commit ff00255):

ERROR: PropertyNotSetInConstructor - 29:11 - Property Jorpo\Concerto\FieldTypes\EnableFieldType\Model\SomeEvent::$uniqueId is not defined in constructor of Jorpo\Concerto\FieldTypes\EnableFieldType\Model\SomeEvent and in any methods called in the constructor

ERROR: PropertyNotSetInConstructor - 29:11 - Property Jorpo\Concerto\FieldTypes\EnableFieldType\Model\SomeEvent::$when is not defined in constructor of Jorpo\Concerto\FieldTypes\EnableFieldType\Model\SomeEvent and in any methods called in the constructor

INFO: UnusedClass - 29:11 - Class Jorpo\Concerto\FieldTypes\EnableFieldType\Model\SomeEvent is never used

[weirdan]

@designermonkey that's not supposed to work. You need to declare your initializer methods final, otherwise a descendant may override them. However Psalm should not warn when property is initialized in inherited final protected method, and it currently does: https://psalm.dev/r/20ede335c0

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/20ede335c0

<?php
    abstract class Event {
        protected string $uniqueId;
        
        final protected function generateUniqueId(): void {
            $this->uniqueId = 'test';
        }
    }

    final class SomeEvent extends Event {
        public function __construct(string $someValue) {
            $this->generateUniqueId();
        }
    }

Psalm output (using commit ff00255):

ERROR: PropertyNotSetInConstructor - 10:17 - Property SomeEvent::$uniqueId is not defined in constructor of SomeEvent and in any methods called in the constructor

[designermonkey]

Interesting. Could you point me to some further reading about why thta need to be final?

Either way, the error returned isn't exactly very clear though. Is it supposed to be returning a different one in my use case?

[weirdan]

Interesting. Could you point me to some further reading about why thta need to be final?

Well, that's fairly simple. Given class C:

class C {
   private int $prop;

   protected function initProp(): void { $this->prop = 1; }

   public function __construct() { $this->initProp(); }

   public function getProp(): int { return $this->prop; }
}

it can be shown that $this->prop can be left uninitialized when descendant overrides initProp(), e.g.

class D extends C {
    protected function initProp(): void {}
}

(new D)->getProp(); // will fail

Psalm cannot assume this won't happen, unless the method is private or final or the class itself is final.

[rulatir]

it can be shown that $this->prop can be left uninitialized when descendant overrides initProp(), e.g.

There is no room for "can". When a descendant overrides initProp() in a way that does not guarantee that $prop is initialized, or when it fails to invoke an implementation that does provide this guarantee (because it fails to call the parent constructor or something), then report the problem for the descendant. Psalm has all the information it needs in order to figure out whether the effective class, resulting from specific, known inheritance chain and specific, known sequence of trait inclusions on that chain results in a class that will properly initialize its $prop property. Psalm also has the ability to analyze the code of methods to determine whether the (possibly inherited) constructor of a derived class guaranteedly results in initializing a property, or whether it eventually calls code that does. There is no justification for bailing out with "sorry, dunno" here.

There is no problem with class C. There is no (regular) way you can create an object of type exactly C in a way that fails to initialize $prop. Therefore this issue should not be reported for class C, full stop.

[weirdan]

Psalm has all the information it needs in order to figure out whether the effective class

I wish that was true. But Psalm does not necessarily has access to all descendants - e.g. when a library ships a non-final class meant to be extended by the consumers of that library.

[rulatir]

The crux of this bug is the incorrect belief that access to descendants is required to validate ancestors. It is not. Psalm should not be expected to prophesize problems with a class that is perfectly correct and innocent as it is, but its descendants might potentially do something wrong, like failing to call the parent constructor, or otherwise actively frustrating the base class's intention to initialize the property. When descendants actually commit those sins, Psalm should report problems about those descendants while analyzing their code, at which point Psalm will have access to the entire inheritance chain leading to the actual occurrence of the problem.

[rulatir]

Lint tools' insistence on weaving morbid prophecies about an Evil Child that will be born some day are one of the big causes of the proliferation of libraries that are cripplingly locked down using final. Developers cut down on extensibility in order to satisfy needlessly catastrophizing lint tools.

[rulatir]

Me: Is this a constructor?
Psalm: Yes.
Me: Is the property set in it?
Psalm: Yes.
Me: Then why are you saying that PropertyNotSetInConstructor?

[SCIF]

Quite a big discussion here, but looks like even final is not a solution: https://psalm.dev/r/c8408355be

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/c8408355be

<?php

trait Tr
{
    protected int $a;
    
    final protected function set(): void
    {
        $this->a = 5;
    }
}

class B
{
    use Tr;
    
    protected function __construct()
    {
        $this->set();
    }

}

final class Cl extends B
{    
}

Psalm output (using commit 5baf85e):

ERROR: PropertyNotSetInConstructor - 24:13 - Property Cl::$a is not defined in constructor of Cl or in any methods called in the constructor

[SCIF]

Hmm, probably my example is actually a next bug: #5062

[rulatir]

Seriously, this needs more urgent attention. It is absolutely unacceptable for a lint tool to force programmers to entirely throw away a huge chunk of what constitutes OO programming. This bug forces programmers to lock their libraries down with final all over the place, or engineer ultra-pedantic, bloated, resource-hungry monstrosities with 7× more classes than necessary.

[orklah]

Hey @rulatir, feel free to take a look at it. I'd be fine with not raising this error when we can show that a parent correctly initialize the property, even if a child could theoretically override the method

[rulatir]

I do feel free to "take a look at it". I don't feel able to "take a look at it", not without investing weeks to learn the basics of the project, all the while someone who already knows the codebase could probably fix this bug in 10 minutes. And this is normal. People who come to a bugtracker should be assumed to be in the position I am in: harmed by a bug, but not knowing the rocket science required to fix it.

I no longer use psalm in my projects, but this bug doesn't just harm psalm users. It harms users of all libraries developed with psalm as a linter, which end up being completely locked down with final in order to keep this overzealous rule quiet.

[orklah]

@rulatir You seem engaged in your own little crusade against final that is completely out of scope here, given we stated earlier that final doesn't even solve the issue

However, you managed to both massively overestimate the time it would take you to learn how to fix this issue and also massively underestimate the time it would take me (or anyone else in the project) to fix it. And you did this to justify your demand for other to fix a problem that doesn't even exists in a tool you don't use.

That's quite a performance there, pal.

This bug tracker currently has 900 open issues and many are more critical than this. So as I said, if you're concerned enough to do anything more than complain, feel free to take a look at it.

[rulatir]

My crusade is against false positives in general. They are massively harmful.