phan icon indicating copy to clipboard operation
phan copied to clipboard

Redundant Condition Detection doesn't account for initial 'unset' state from `!isset($this->y)`

Open DeveloperRob opened this issue 3 years ago • 2 comments

Code Snippet: https://3v4l.org/lEbfg (full code also provided at end of issue) Phan version used: 5.4.1 Observed behavior:

test.php:10 PhanRedundantCondition Redundant attempt to cast $this->y of type string to isset

Expected behavior: No errors (in the real world case, then $this->y might or might not be set based on a more complex set of ifs in the constructor, and the property is readonly; so I can't set it at the top of the function and then set it to a different value later.

Full test code:

<?php
class y{
	public string $y;
	
	public function __construct(bool $b){
		if($b){
			$this->y = "hello";
		}
		
		if(!isset($this->y)){
			$this->y = "goodbye";
		}
	}
}

DeveloperRob avatar Aug 30 '22 12:08 DeveloperRob

In any context other than isset/empty/??/?->, the access would throw Typed property y::$y must not be accessed before initialization, so

  • Instance and static property access should be special cased?
  • This should continue to be check for property overrides in the local scope $this->y = 'foo'; if (!isset($this->y)) {...} that are unconditionally there and non-null

TysonAndre avatar Aug 30 '22 12:08 TysonAndre

Since Phan prefers to minimize false positives, implicitly treating all typed properties as potentially unset (including things like $this->y = 'foo'; if (!isset($this->y)) {...}) would still be closer to this goal than the current state. By the way, properties can be unset after being set in non-trivial ways, like:

function foo() {
	unset(A::$instance->str);
}

class A {
	public static self $instance;

	public string $str;

	public function bar() {
		$this->str = 'str';
		foo();
		if (!isset($this->str)) {
			// this will be true
		}
	}
}
A::$instance = new A;
A::$instance->bar();

tacsipacsi avatar Feb 06 '23 09:02 tacsipacsi