Psalm: PropertyNotSetInConstructor oddity

[SignpostMarv]

Not certain if this is an error/info thing that needs to be triggered- testing the same class as in #127 , I get an info crop up in the log on code something like this:

/**
* @var Foo
*/
protected $Foo;

public function DoesSomethingWithFoo(...$argsNotRelevantToIssue) : string
{
    if (!isset($this->Foo)) {
        $this->Foo = new Foo($this);
    }

    return $this->Foo->someMethodOnFoo(...$argsNotRelevantToIssue);
}

Property Response::$Foo is not defined in constructor of Response or in any private methods called in the constructor

The following change squishes the info notice:

/**
* @var Foo
*/
protected $Foo = null;

I'm wondering if this is some weirdness between void in PHP 7.1 and null in prior versions ?

[SignpostMarv]

I'm also wondering if this would go away if the property was private, but I tend to leave the properties protected out of habit, as it's a pet peeve of mine to have to update a package just to change a variable from private to protected.

[muglug]

The following change squishes the info notice:

/**
* @var Foo
*/
protected $Foo = null;

That code should trigger an InvalidPropertyAssignment issue (which maybe you're suppressing).

Anyway, you have the right idea – if a non-nullable property is not defined in the constructor this issue will be triggered.

Simplest way to fix is with

/**
* @var ?Foo
*/
protected $Foo;

which tells Psalm that the property can be null (as your usage of it, with that isset check, indicates)

[SignpostMarv]

Prior to the fix, I added an additional corresponding property that made things a little more clearer:

/**
* @var Foo|null
*/
protected $Foo = null;

/**
* @var bool
*/
protected $FooHasBeenSet = false;

public function DoesSomethingWithFoo(...$argsNotRelevantToIssue) : string
{
    if (!$this->FooHasBeenSet ) {
        $this->Foo = new Foo($this);
        $this->FooHasBeenSet  = true;
    }

    return $this->Foo->someMethodOnFoo(...$argsNotRelevantToIssue);
}

While this approach is a little more readable, it works best with scalar/array types, especially since I'm working in an environment where PHP 7.1 is not guaranteable (so trying to avoid confusing others with 7.1 syntax peppered amongst code/comments that's only ever going to be run under 7.0

/**
* @var array
*/
protected $Foo = [];

/**
* @var bool
*/
protected $FooHasBeenSet = false;

public function DoesSomethingWithFoo(...$argsNotRelevantToIssue) : string
{
    if (!$this->FooHasBeenSet ) {
        $this->Foo = [$argsNotRelevantToIssue];
        $this->FooHasBeenSet  = true;
    }

    // return something from $this->Foo here
}

[muglug]

Well, given there's no support for property types at this juncture, I feel like the annotations can contain whatever syntax we like (with guidance from Phpdoc)

[SignpostMarv]

I'm peppering the code with notes along the lines of @todo when we can support php 7.1, change return type to "void", @todo when we can support php 7.1, change param type to "? Foo", etc :D

[dgunay]

Gonna just post this here since my issue is similar so I don't know if it warrants a new Issue. I get a PropertyNotSetInConstructor with the following code:

<?php declare (strict_types = 1);

class Foo
{
  /** @var string $bar */
  protected $bar;

  protected const REQUIRED = 'bar';

  /**
   * @param mixed[string] $params
   */
  public function __construct(array $params)
  {
    $this->validateBarExistsInArray($params);
    $this->setBar($params['bar']);
  }

  /**
   * @throws \InvalidArgumentException
   * @param mixed[string] $params
   * @return void
   */
  protected function validateBarExistsInArray(array $params): void
  {
    if (!isset($params[self::REQUIRED])) {
      throw new \InvalidArgumentException();
    }
  }

  /**
   * @param string $value
   * @return void
   */
  protected function setBar(string $value): void
  {
    $this->bar = $value;
  }
}

It goes away if I change either $bar or setBar() to private.

If the worry is that subclasses might not call the parent's constructor or use the protected setter method, shouldn't it be their fault and not the parent class?

[weirdan]

@dgunay subclasses can override setBar, and fail to set the property. You can keep it protected if you declare it final: https://psalm.dev/r/a03a4da977

[muglug]

Yeah, but Psalm should actually be able to work around that.

[weirdan]

Is it actually possible to work around that? Imagine the library package providing the base class to be extended by consumer packages. final setter seems to be a good middle ground, unfortunately it can't be used in some use cases (like, doctrine entities).

[muglug]

I don't think there's ever a situation where a consumer package would think "I can do whatever I want in the extended method, the base class is type checked so I don't need to type check the child class"

[muglug]

fixed in https://github.com/vimeo/psalm/commit/5c76ab35c8952e6f8f6363c66e0a0f721b7b99cd