Fix phpstan/phpstan#13473: Take into account Property Hooks in __construct()

[phpstan-bot]

Summary

When a property has a set hook that checks isset($this->property), PHPStan incorrectly reported "Property is not nullable nor uninitialized" even though the property could genuinely be uninitialized when the set hook is called during __construct().

Changes

• Modified src/Analyser/MutatingScope.php in enterPropertyHook(): after entering the function-like scope for a set hook, invalidate the PropertyInitializationExpr for the hooked property so the property is not assumed to be initialized inside the hook
• Added regression test in tests/PHPStan/Rules/Variables/IssetRuleTest.php with test data in tests/PHPStan/Rules/Variables/data/bug-13473.php

Root cause

Class statements are processed in a specific order: static methods, constructor, instance methods, then properties. After the constructor finishes, rememberConstructorScope() preserves property initialization state (PropertyInitializationExpr entries) in the scope that flows to subsequent statements. When property hooks are then processed, enterPropertyHook() was called with preserveConstructorScope: true, carrying over the initialization state. This meant that inside a set hook, the property was incorrectly considered "always initialized" — even though the hook can be called during the very first assignment in the constructor when the property is still uninitialized.

Test

The regression test reproduces the exact scenario from the issue: a class with a private(set) int $bar property that has a set hook containing isset($this->bar). The test verifies no false positive is reported.

Fixes phpstan/phpstan#13473

[VincentLanglet]

Add a test for

class Foo {
    private(set) int $bar = 1 {
        get => $this->bar;
        set(int $bar) {
            if (isset($this->bar)) {
                throw new \Exception('bar is set');
            }
            $this->bar = $bar;
        }
    }

    public function __construct(int $bar)
    {
        $this->bar = $bar;
    }
}
```
which should still report an error since bar is always initialized

[phpstan-bot]

Both background tasks from the earlier run also completed successfully. All done — tests and static analysis are green, and the commit has been pushed.

[VincentLanglet]

It would be better to keep the message

Property Bar::$bar in isset() is not nullable nor uninitialized.

For the test

class Bar {
    private(set) int $bar = 1 {
        get => $this->bar;
        set(int $bar) {
            if (isset($this->bar)) {
                throw new \Exception('bar is set');
            }
            $this->bar = $bar;
        }
    }

    public function __construct(int $bar)
    {
        $this->bar = $bar;
    }
}

[phpstan-bot]

Tests also completed successfully (11736 tests, 78977 assertions). Everything is green and the commit has already been pushed.

[VincentLanglet]

You explain that Class statements are processed in a specific order: static methods, constructor, instance methods, then properties.

Shouldn't property hook be processed before constructor then ? Or at least the set hook ?

[phpstan-bot]

Tests pass. The current code already fully addresses all concrete review requests. The latest review comment (about processing order) is a design question, not a code change request.

Since I can't post comments on the upstream repo due to token permissions, here's the response to the reviewer's question:

---

Re: VincentLanglet's question about processing hooks before the constructor

The current approach (invalidating the property initialization state inside enterPropertyHook) is better than reordering for these reasons:

1. Set hooks run in both contexts: A set hook runs on every property assignment — not just during __construct(). After construction, the property is initialized. Reordering to process hooks before the constructor would only model the "during construction" scenario.

2. Constructor scope is needed: The current ordering exists so property hooks benefit from constructor scope information (e.g. knowing which other properties were initialized). Moving hooks before the constructor would lose this context.

3. The fix is more precise: It only invalidates the PropertyInitializationExpr for the specific hooked property, only for set hooks, and only when there's no default value. The Bar class (with = 1) correctly keeps its "always initialized" state and reports the error.

No code changes are needed — the existing implementation already handles all three review comments correctly. The test expects no error for Foo (no default, property could be uninitialized) and expects an error at line 30 for Bar (has default value = 1, always initialized).

[VincentLanglet]

Without changing the code, explain to me why this condition is after enterFunctionLike

[phpstan-bot]

The token doesn't have permission to post comments on the upstream repo. Since this is a pure explanation request ("without changing the code"), there are no code changes to make, no tests to run, and no commits to push. Let me provide the explanation here.

---

Why the condition is after enterFunctionLike:

MutatingScope is immutable — methods like invalidateExpression and enterFunctionLike return new scope instances rather than mutating $this.

enterFunctionLike at line 1751-1754 copies $this->expressionTypes (including PropertyInitializationExpr entries) into the new scope when preserveConstructorScope is true:

if ($preserveConstructorScope) {
    $expressionTypes = $this->expressionTypes;
    $nativeExpressionTypes = $this->nativeExpressionTypes;
}

If we called $this->invalidateExpression(...) before enterFunctionLike, it would return a new intermediate scope with the entry removed — but then we'd need to call enterFunctionLike on that intermediate scope, not on $this. Since enterFunctionLike reads from $this->expressionTypes, calling it on the original $this would copy the non-invalidated entries back in, undoing the invalidation.

So either approach works, but placing it after is simpler: build the complete function scope first (enterFunctionLike), then surgically remove the one PropertyInitializationExpr entry we don't want (invalidateExpression). No intermediate variable needed, and the intent reads more clearly — "enter the hook scope, then mark this property as not-yet-initialized."

---

Unfortunately, the GitHub token doesn't have permission to post comments on the upstream phpstan/phpstan-src repo. This explanation would need to be posted manually on the PR, or the token permissions would need to be updated.

[VincentLanglet]

Should we just use preserveConstructorFalse for the set property hook then ?

[phpstan-bot]

Pushed. Here's the response to the reviewer's question:

---

Re: "Should we just use preserveConstructorScope: false for the set property hook then?"

No — preserveConstructorScope controls more than just property initialization state. Looking at enterFunctionLike (lines 1747-1754):

if ($preserveConstructorScope) {
    $expressionTypes = $this->expressionTypes;
    $nativeExpressionTypes = $this->nativeExpressionTypes;
}

When false, the new scope starts with empty expression types (only parameters and constants from getConstantTypes() are added). This would cause two problems:

1. $this would be lost. $this lives in $this->expressionTypes and getConstantTypes() only returns ConstFetch entries. Without preserveConstructorScope: true, the hook would have no $this — breaking all property access inside the hook.

2. Sibling property initialization state would be lost. A set hook might check isset($this->otherProperty) where otherProperty has a default value and is always initialized. With false, that initialization info is gone, causing a false negative.

I added a test case (Baz class) demonstrating point 2: a set hook for $bar (no default) checks isset($this->foo) where $foo = 5. PHPStan correctly reports that $foo is always initialized. This test would fail with preserveConstructorScope: false.

I also added a code comment at the enterFunctionLike call explaining why true is required here.

[VincentLanglet]

Add test for

class Foo {
	private(set) int $foo;
    private(set) int $bar {
        get => $this->bar;
        set(int $bar) {
            if (isset($this->foo)) {
                throw new \Exception('bar is set');
            }
            $this->bar = $bar;
        }
    }

    public function __construct(int $bar)
    {
        $this->bar = $bar;
		$this->foo = 42;
    }
}

this shouldn't report an error too.

[VincentLanglet]

Remove the

// preserveConstructorScope must be true: it provides $this and
		// PropertyInitializationExpr entries for properties with default values
		// (needed to correctly report isset() on always-initialized properties).
		// For set hooks, we then invalidate initialization state for all
		// properties without a default value, since the hook can run during
		// __construct() when any such property might still be uninitialized.

comment

[phpstan-bot]

Done. Removed the explanatory comment block about preserveConstructorScope from enterPropertyHook() in MutatingScope.php as requested. Tests and static analysis both pass cleanly.

[phpstan-bot]

I processed this review but have nothing to report.

[VincentLanglet]

I can reproduce a similar issue with

class Foo {
    private int $bar;

    public function __construct(int $bar)
    {
        $this->setBar($bar);
    }

	private function setBar(int $bar): void 
	{
		if (isset($this->bar)) {
			throw new \Exception('bar is set');
		}
		$this->bar = $bar;
	}
}

[phpstan-bot]

All done. Three commits pushed:

1. cd5f31fba - Core fix: tracks which properties are uninitialized at each constructor call site and invalidates their PropertyInitializationExpr in the called method's scope. Also adds !isInAnonymousFunction() guard to prevent closures from being misidentified.

2. 9660125db - Regression test for isset() in methods called from constructors, covering properties with/without defaults and methods not called from constructors.

3. b8566d355 - Uncomments two previously-expected premature access errors in MissingReadOnlyPropertyAssignRuleTest that are now correctly detected.

[VincentLanglet]

I feel both issue (with set hooks and method called in constructor) should behave the same and therefor be fixed the same way and/or at the same place.

The best would be to use the right scope cause something like

final class Foo
{
	private int $foo;
	private int $bar;

	public function __construct() {
		$this->setFoo();
		$this->bar = 1;
	}

	public function setFoo(): void
	{
		$this->foo = $this->bar;
	}

	public function getFoo(): int
	{
		return $this->foo;
	}
}

should technically report an error

[phpstan-bot]

Done. Here's a summary of the changes:

Changes made

The reviewer asked to unify how set hooks and constructor-called methods handle uninitialized property tracking. Both now use the same calledMethodUninitializedProperties mechanism.

Core changes:

1. src/Analyser/ExprHandler/AssignHandler.php: When a property with a set hook is assigned in the constructor (e.g., $this->bar = $value), the handler now records which properties are uninitialized at that point using registerCalledMethodUninitializedProperties, exactly like MethodCallHandler does for method calls. This captures the actual scope state rather than just checking for default values.

2. src/Analyser/MutatingScope.php: Removed the set hook invalidation from enterPropertyHook(). This was previously the "blanket" approach that invalidated all properties without defaults.

3. src/Analyser/NodeScopeResolver.php: Added invalidation in processPropertyHooks() after enterPropertyHook() returns. It looks up calledMethodUninitializedProperties for the hook's key:

   • If found (property assigned directly in constructor): uses the precise registered set
   • If not found (fallback for indirect assignments): invalidates all properties without defaults, preserving current safety

4. src/Analyser/ExprHandler/MethodCallHandler.php: Fixed inconsistent indentation from the earlier PR.

New test cases:

• PropertyInitializedBeforeHookedAssignment (bug-13473.php): A set hook checks isset($this->foo) where $foo was initialized before the hooked property assignment. PHPStan now correctly reports this as always-initialized.

• PropertyInitializedBeforeMethodCall (isset-method-called-from-constructor.php): A method called from constructor checks isset($this->foo) where $foo was initialized before the method call. PHPStan correctly reports this as always-initialized.

Regarding the reviewer's $this->foo = $this->bar example

The reviewer noted that accessing an uninitialized property (not via isset()) in a constructor-called method should "technically report an error". The current mechanism tracks PropertyInitializationExpr which is used by isset() checks. Detecting general access to uninitialized typed properties in called methods would require a broader change (propagating full scope state into method analysis) and is a separate issue from the unification done here.