Readonly properties should be assignable outside the constructor

Problem

class Thing
{
    public readonly string $name;

    public function __construct()
    {
    }
	
	public function populate(string $name): void
	{
		$this->name = $name;
	}
}

Psalm (according to the web analyzer on the home page) is objecting to this construction. I believe it is incorrect.

While setting a readonly property from the constructor is certainly the most common case, it is by no means the only. There are numerous counter-examples of reasonable usage that set a property elsewhere that should be allowed.

This example is a simplified version of real code I have that uses multiple stages to build up a reflection/attributes-based analysis of an object. In my specific case, I have an attribute class that, after it's instantiated, is passed the Reflection object from which it was spawned and can pull data off of it to cache. Readonly properties are a very good use case here, but Psalm objects.

In another example, we're using readonly public properties as a lazy-instantiation mechanism. (See https://peakd.com/hive-168588/@crell/php-tricks-lazy-public-readonly-properties) However, it's getting rejected for the same reason despite this being an entirely valid use for readonly properties.

class Person 
{
    public readonly string $full;
    
    public function __construct(
        public readonly string $first,
        public readonly string $last,
    ) {
        unset($this->full);
    }
    
    public function __get(string $key): mixed
    {
        print __FUNCTION__ . PHP_EOL;
        if ($key === 'full') {
            return $this->full = "$this->first $this->last";
        }
    }
}

$c = new Person('Larry', 'Garfield');

print $c->first . PHP_EOL;
print $c->full . PHP_EOL;
print $c->full . PHP_EOL;

Feb 08 '22 17:02 Crell

Hey @Crell, can you reproduce the issue on https://psalm.dev ?

Feb 08 '22 17:02 psalm-github-bot[bot]

cf:

Setting readonly properties outside the constructor: https://psalm.dev/r/33b562cf8e

Lazy loading readonly properties: https://psalm.dev/r/3d6f55d9a6

Feb 08 '22 17:02 Crell

I found these snippets:

<?php

class Thing
{
    public readonly string $name;

    public function __construct()
    {
    }
	
	public function populate(string $name): void
	{
		$this->name = $name;
	}
}

Psalm output (using commit efb1464):

ERROR: InaccessibleProperty - 13:3 - Thing::$name is marked readonly

ERROR: PropertyNotSetInConstructor - 5:28 - Property Thing::$name is not defined in constructor of Thing or in any methods called in the constructor

<?php

class Person 
{
    public readonly string $full;
    
    public function __construct(
        public readonly string $first,
        public readonly string $last,
    ) {
        unset($this->full);
    }
    
    public function __get(string $key): mixed
    {
        print __FUNCTION__ . PHP_EOL;
        if ($key === 'full') {
            return $this->full = "$this->first $this->last";
        }
    }
}

$c = new Person('Larry', 'Garfield');

print $c->first . PHP_EOL;
print $c->full . PHP_EOL;
print $c->full . PHP_EOL;

Psalm output (using commit efb1464):

ERROR: InaccessibleProperty - 18:20 - Person::$full is marked readonly

ERROR: InvalidReturnType - 14:41 - Not all code paths of Person::__get end in a return statement, return type mixed expected

ERROR: PropertyNotSetInConstructor - 5:28 - Property Person::$full is not defined in constructor of Person or in any methods called in the constructor

Feb 08 '22 17:02 psalm-github-bot[bot]

That's an interesting construction!

But you're kinda missing the point of what Psalm is trying to do here.

Uninitialized properties are basically impossible for SA to analyse. Psalm can't see in advance the order in which you'll call your methods and won't ever be able to guarantee that your property is initialized if it's in a random method. Likewise, it can't tell if you're possibly going to write on a readonly property twice, unless you write it in a place that can only be called once (the __construct).

That means it can either give up on reporting initialization issue, or it can go overboard and straight up requiring that all properties must be initialized in the constructor. By enforcing this, Psalm can then assume your properties are initialized and won't crash your code. This pattern works for the big majority of modern codebases and it allow Psalm to make checks here.

However, there are code, like yours, that relies on having uninitialized properties at one point after the constructor. Issues reported by Psalm for such code are effectively noise that should be suppressed because they won't be relevant anyway as they go against the core assumption Psalm makes.

This means, InaccessibleProperty, PropertyNotSetInConstructor, MissingConstructor, RedundantPropertyInitializationCheck, UninitializedProperty and possibly some other are just irrelevant.

By silencing Psalm on those issues, you're basically telling it that you'll handle the checks on properties yourself because it's not able to do it automatically

Feb 08 '22 20:02 orklah

There was a similar report for phpstan/phpstan at https://github.com/phpstan/phpstan/issues/6562 (just for the SA records)

Feb 09 '22 08:02 ohader

Fun fact: The constructor can be called multiple times. (I can't imagine you ever would, but the engine allows it.)

https://3v4l.org/lJWCr

I get that the common case is constructor-only. However, I'm working on a library that by necessity does two-stage construction. It's a reflection/attributes library that powers the serialization library I built for TYPO3, so it's going to come up a ton. So (for Psalm or PHPStan) the problem is a legitimate use case forces users to either:

1. Disable an otherwise useful-80%-of-the-time flag (or several).
2. Litter their code with "Don't SA the last line, trust me", which many will forget, or will take as a sign that the library is inherently wrong/buggy (which is not true).
3. Not use readonly properties in a situation very well suited to them just because their SA is overly-protective.
4. Not use SA tools at all.

None of these are good options right now.

For reference, the attribute/reflection code looks vaguely like this (very simplified):

https://psalm.dev/r/1a5f998cd0

(Users of the library would be writing attributes that use the fromReflection() style method, so it's not just my own code that would need to opt-out.)

Feb 09 '22 16:02 Crell

I found these snippets:

<?php

class Analyzer {
    /**
     * @param class-string $class
     * $param class-string $attribute
     */   
    public function analyze(string $class, string $attribute): ?object
    {
        $r = new \ReflectionClass($class);

        $a =  $r->getAttributes($attribute, \ReflectionAttribute::IS_INSTANCEOF);
        
        if (!isset($a[0])) { return null; }
        
        /** @var MyAttrib $attrib */
        $attrib = $a[0]->newInstance();
        
        $attrib->fromReflectionClass($r);
        
        return $attrib;
    }
}

#[\Attribute]
class MyAttrib
{
    public readonly string $phpName;
    
    public function __construct(public readonly string $key) {}
    
    public function fromReflectionClass(ReflectionClass $r): void
    {
        $this->phpName = $r->getName();
    }
}

#[MyAttrib(key: 'a')]
class A
{
}

$analyzer = new Analyzer();

$attrib = $analyzer->analyze(A::class, MyAttrib::class);

print_r($attrib->phpName);

Psalm output (using commit f5a28b6):

ERROR: PossiblyNullPropertyFetch - 47:9 - Cannot get property on possibly null variable $attrib of type null|object

ERROR: ArgumentTypeCoercion - 12:33 - Argument 1 of ReflectionClass::getAttributes expects class-string|null, parent type string provided

ERROR: InaccessibleProperty - 34:9 - MyAttrib::$phpName is marked readonly

ERROR: PropertyNotSetInConstructor - 28:28 - Property MyAttrib::$phpName is not defined in constructor of MyAttrib or in any methods called in the constructor

Feb 09 '22 16:02 psalm-github-bot[bot]

The way I see it there are two options:

• If users store those class in a specific directory (like entities for example), then Psalm allow for path-based suppression of some issue
• You can provide a TYPO3 Psalm plugin, the same way we have a doctrine plugin that suppress or solves specific issues (possibly based on attributes user add to their code)

I don't see a solution for your use-case that wouldn't worsen the analysis for the rest of the users here unfortunately...

Feb 09 '22 18:02 orklah

How about an annotation for the property that names the method where it is initialised?

In theory it could be more than one method, but allowing only one makes IMHO the code cleaner.

Aug 02 '22 11:08 masi

@masi Still a no-go. If you're passing around objects we'd have to have some additional state there to track which properties have been initialized: https://psalm.dev/r/44a7cd39e2

You could do something like Foo<false> if Foo::$bar hasn't been initialized and Foo<true> if it has, but when you have 10 readonly properties that's a lot of extra state to track, and I don't think anyone wants to start annotating functions with @return Foo<true, true, true, false, true, false, false, false, true>.

Aug 02 '22 15:08 AndrolGenhald

I found these snippets:

<?php

/** @psalm-suppress MissingConstructor */
class Foo
{
    public readonly string $bar;
    
    public function initBar(): void
    {
        /** @psalm-suppress InaccessibleProperty */
        $this->bar = "baz";
    }
}

function getFoo(): Foo
{
    $foo = new Foo();
    // $foo->initBar();
    return $foo;
}

$foo = getFoo(); // How is Psalm supposed to know if $foo->bar has been initialized?

Psalm output (using commit 24f7920):

No issues!

Aug 02 '22 15:08 psalm-github-bot[bot]

I'll close this, we can't do much here

Jan 12 '23 20:01 orklah

I've got a #[Required] function in Symfony, and I assign my readonly properties there, phpstan does not like this, but it's a valid usecase as '#[Required]' in symfony is kinda meant for that (will put in Dependency Injection as well). Can we at least have a config option to disable the scan other than literring the code with // @phpstan-ignore-next-line etc?

Edit: Couldn't really comment on the other report, as it was locked.

Feb 18 '24 04:02 cyraid