Unable to infer type when using a getter

[Jean85]

Summary of a problem or a feature request

I encountered this problem when checking this little piece of code inside a Symfony controller:

        if ($this->tokenStorage->getToken()) {
            return $this->tokenStorage->getToken()->getUser();
        }

PHPStan should infer that, since we checked the getter inside the if, we can safely exclude that the getter returns null here.

Code snippet that reproduces the problem

The code to reproduce the issue is here: https://phpstan.org/r/10c11895e307276cd46b1501d63e1cba

<?php declare(strict_types = 1);

class HelloWorld
{
	private $a;

	/**
	 * @return \DateTimeInterface|null
	 */
	public function getA() 
	{
		return $this->a;
	}
}

$class = new HelloWorld();
if ($class->getA()) {
	echo $class->getA()->format('Y-m-d');
}

Expected output

The expected output should be no errors, instead we get Cannot call method format() on DateTimeInterface|null.

[ondrejmirtes]

Some people would disagree with you, like @muglug 😊
Basically you can't be sure that the method will do the same thing the second time you call it.

In the future, I want to have a concept of "pure" methods - has no side effects, returns the same thing unless you call an impure method in the object meanwhile. I'll add MethodReflection::isPure(). I'm still not sure whether I'll inspect the method body to see if it just returns something, or I will rely on naming conventions (like only methods that start with get/is/has are pure).

Right now, you can help yourself with a hack:

if ($class->getA() !== null) {
	echo $class->getA()->format('Y-m-d');
}

...as a more explicit check still works.

[Jean85]

Thanks for the quick fix!

[muglug]

Yeah, this is a risky thing for a static analysis tool to assume, I think. I decided that people should explicitly have to opt-in (via a config flag) to this behaviour.

However, there's other behaviour that I think is less risky that I decided people should opt out of (via a different config flag). With that config flag on, this code

class X {
  /** @var ?int **/
  private $x;

  public function getX(): int {
    $this->x = 5;
    $this->foo();
    return $this->x;
  }

  public function foo(): void {}
}

must be rewritten as

class X {
  /** @var ?int **/
  private $x;

  public function getX(): int {
    $x = $this->x = 5;
    $this->foo();
    return $x;
  }

  public function foo(): void {}
}

to pass checks, because the function X::foo may make some modifications.

As I said, Psalm doesn't force you to write code like that by default, but Hack does, and I like the absolute strictness of it (no side-effects allowed).

I ticketed a similar (opt-in) feature for PHPStan here: #929

[jdeniau]

@ondrejmirtes I understand your point.
Your quickfix with the null test should return an error too though as getA may return a DateTime the first time and null the second time.

[JanJakes]

@ondrejmirtes I understand that detecting pure methods may be hard. However, I don't understand why $a->getB() !== null ? $a->getB()->getC() : null passes but $a->getB() ? $a->getB()->getC() : null doesn't. What's the reasoning behind this? They both eliminate null and they both have the same danger of side effects. Why such inconsistency? And how can you assume that !== null is OK when you don't detect pure functions? It makes me think... how many similar assumptions PHPStan is doing without actually knowing the truth?

[ondrejmirtes]

@JanJakes Yeah, it's currently a hack that will be solved later with pure functions/methods. I can't think of any other example like this.

[JanJakes]

@ondrejmirtes Thanks for the clarification. In the meantime I might be better not to assume anything (also for !==) but I understand 0.* versions are basically experimental.

[ghost]

@ondrejmirtes Great to hear that you're considering pure functions!

Just wanted to add a side note that whilst pure functions in the traditional sense do provide a solution to a related problem, it would likely not fix the problem in the common case of getters with setters (which can still be fixed by putting it in a local variable, as already suggested).

This is because pure functions should not just not produce any side effects, but also return the same return value for the exact same set of arguments. This, of course, depends on your definition of pure functions, as you may also see pure functions as those just not having side effects.

In the mentioned traditional sense, as a getter has no arguments, the return value should thus be constant (e.g. constexpr in C++, I believe). This is however impossible to guarantee without returning a property that is constant (or, in PHP, making it a constant), which then removes its usefulness as object state and prevents its use in setters. It however may do the trick in immutable classes, as you can guarantee the value does not change afterwards (or at least, it shouldn't, as in PHP we can't guarantee that syntax-wise, yet 😄 ).

Just wanted to add these two cents in the hope the difference will be made clear in the naming if it is implemented 😄 .

[ondrejmirtes]

Fixed: phpstan/phpstan-src@d4edc59

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.