RedundantPropertyInitializationCheck in constructor

[janopae]

Imagine the following example:

https://psalm.dev/r/a95dce64af

It contains a library, that has a not so Psalm-friendly implementation: Nobody knows whether the properties will be initialised. But this should be solvable by adding a lot of checks to the child class constructor, which ensure everything that Psalm needs to continue working.

But Psalm tells me, these checks were redundant. Which they aren't, because they're the only thing that ensures that Psalm's assumption that properties will be initialised in constructor is true. It tells me that they were redundant, because this should already be ensured in the constructor. Well, this is the constructor.

So I don't think RedundantPropertyInitializationCheck should apply to constructors. Instead, in the constructor, these checks should be able to replace an explicit assingment.

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/a95dce64af

<?php

namespace Library {
	class ParentClass {
        /**
        * @param array<string, string> $options
        */
    	public function __construct(array $options)
        {
            foreach ($options as $name => $value) {
                $this->$name = $value;
            }
        }
    }
}

namespace App {
    use Library\ParentClass;
    use InvalidArgumentException;
    
    class MyClass extends ParentClass {
        public string $a;
        public string $b;

		/**
        * @param array<string, string> $options
        */
    	public function __construct(array $options)
        {
            parent::__construct($options);
            
            if (!isset($this->a)) {
            	throw new InvalidArgumentException('a is mandatory');
            }

            if (!isset($this->b)) {
            	throw new InvalidArgumentException('b is mandatory');
            }
        }
    }
}

Psalm output (using commit 7c4228f):

ERROR: RedundantPropertyInitializationCheck - 32:18 - Property $this->a with type string should already be set in the constructor

ERROR: RedundantPropertyInitializationCheck - 36:18 - Property $this->b with type string should already be set in the constructor

[AndrolGenhald]

I thought this might be fine, but since the library isn't even defining the properties that's not going to work. You're probably right that the best way to handle this is to disable that check for __construct, it should still handle the case where the check is redundant or impossible.

Dynamic properties are being deprecated in PHP 8.2 by the way, so that library will need to change, either by adding actual properties (like they arguably should), or by using #[AllowDynamicProperties].

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/7bd13eda9b

<?php

namespace Library {
	class ParentClass {
        /**
        * @param array<string, string> $options
        */
    	public function __construct(array $options)
        {
            foreach ($options as $name => $value) {
                $this->$name = $value;
            }
        }
    }
}

namespace App {
    use Library\ParentClass;
    use InvalidArgumentException;
    
    class MyClass extends ParentClass {
        public string $a;
        public string $b;

		/**
        * @param array<string, string> $options
        */
    	public function __construct(array $options)
        {
            parent::__construct($options);
            
            if (!isset($options["a"])) {
            	throw new InvalidArgumentException('a is mandatory');
            }

            if (!isset($options["b"])) {
            	throw new InvalidArgumentException('b is mandatory');
            }
        }
    }
}

Psalm output (using commit 7c4228f):

ERROR: PropertyNotSetInConstructor - 22:23 - Property App\MyClass::$a is not defined in constructor of App\MyClass or in any methods called in the constructor

ERROR: PropertyNotSetInConstructor - 23:23 - Property App\MyClass::$b is not defined in constructor of App\MyClass or in any methods called in the constructor

https://psalm.dev/r/62cf8f5904

<?php

namespace Library {
	class ParentClass {
        /**
        * @param array<string, string> $options
        */
    	public function __construct(array $options)
        {
            foreach ($options as $name => $value) {
                $this->$name = $value;
            }
        }
    }
}

namespace App {
    use Library\ParentClass;
    use InvalidArgumentException;
    
    class MyClass extends ParentClass {
        public string $a;
        public string $b;

		/**
         * @param array<string, string> $options
         * @psalm-suppress RedundantPropertyInitializationCheck
         */
    	public function __construct(array $options)
        {
            parent::__construct($options);
            
            if (!isset($this->a)) {
            	throw new InvalidArgumentException('a is mandatory');
            }

            if (!isset($this->b)) {
            	throw new InvalidArgumentException('b is mandatory');
            }

            if (!isset($this->a)) {
            }
            if (isset($this->b)) {
            }
        }
    }
}

Psalm output (using commit 7c4228f):

ERROR: ParadoxicalCondition - 41:17 - Condition ($this->a is not isset) contradicts a previously-established condition ($this->a is isset)

ERROR: RedundantCondition - 43:17 - $this->b is isset has already been asserted

[janopae]

Dynamic properties are being deprecated in PHP 8.2 by the way, so that library will need to change, either by adding actual properties (like they arguably should), or by using #[AllowDynamicProperties].

The idea of this library is not to use dynamic properties, but rather to auto-map property values provided in the $options array to public properties defined in the sub-class (actually, there are also some defined in the base class, but I left them out in my example). I think, the library would be fine if providing an array key that has no corresponding property emitted an error.

(The library I talk about is Symfony/Validator btw., and the Parent class is Constraint)

[janopae]

In case any one else stumbles upon this issue while this is not fixed:

I worked around this by redundantly assinging the parameters in the sub class – so instead of

        public function __construct(array $options)
        {
            parent::__construct($options);
            
            if (!isset($this->a)) {
            	throw new InvalidArgumentException('a is mandatory');
            }

            if (!isset($this->b)) {
            	throw new InvalidArgumentException('b is mandatory');
            }
        }

I went with

        /**
        * @param array {a: string, b: string} $options
        */
        public function __construct(array $options)
        {
            parent::__construct($options);

            $this->a  = $options['a']
            $this->a  = $options['a']
        }

However, this bypasses the normalisation handled in the parent constructor.