Skip to content

Fix phpstan/phpstan#14352: "Cannot re-assign $this." false-positive#5281

Merged
staabm merged 11 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-t6vskzw
Mar 23, 2026
Merged

Fix phpstan/phpstan#14352: "Cannot re-assign $this." false-positive#5281
staabm merged 11 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-t6vskzw

Conversation

@phpstan-bot
Copy link
Collaborator

@phpstan-bot phpstan-bot commented Mar 23, 2026

closes phpstan/phpstan#14352

fixes the CI build

@clxmstaab clxmstaab force-pushed the create-pull-request/patch-t6vskzw branch 3 times, most recently from ad07aca to 38aade2 Compare March 23, 2026 13:33
@clxmstaab clxmstaab force-pushed the create-pull-request/patch-t6vskzw branch from 38aade2 to 4f11780 Compare March 23, 2026 13:34
@staabm staabm requested a review from VincentLanglet March 23, 2026 13:55
Copy link
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this = $value should still be reported

@staabm
Copy link
Contributor

staabm commented Mar 23, 2026

$this = $value should still be reported

yes. thats tested via bug-14349.php which was added in the inital implementation

@VincentLanglet
Copy link
Contributor

$this = $value should still be reported

yes. thats tested via bug-14349.php which was added in the inital implementation

I mean $this = $value when $this is an ArrayAccess

@staabm
Copy link
Contributor

staabm commented Mar 23, 2026

I see. added another test showing it still errors

staabm and others added 2 commits March 23, 2026 15:09
…his reassignment

$this = $value should still be reported even when $value implements ArrayAccess.
The fix now tracks whether the VariableAssignNode originated from a dimension-fetch
assignment ($this[$key] = $value) vs a direct assignment ($this = $value).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Collaborator Author

Pushed successfully. Here's a summary of the changes:

Problem: The original fix checked if the assigned expression's type implements ArrayAccess to suppress the "Cannot re-assign $this" error. But as the reviewer pointed out, $this = $arrayAccessValue should still be reported as an error.

Solution: Added a dimFetch flag to VariableAssignNode that tracks whether the assignment originated from a dimension-fetch expression ($this[$key] = $value) vs a direct assignment ($this = $value). The rule now:

  1. Skips the error only when it's a dim-fetch AND $this implements ArrayAccess
  2. Still reports $this = $value even when the value implements ArrayAccess
  3. Still reports $this[$key] = $value on classes that don't implement ArrayAccess

Comment on lines +41 to +47
class TestPlain
{
public function doFoo(string $key, string $value): void
{
$this[$key] = $value;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the case

class TestPlain
{
	public function doFoo(string $key, string $value): void
	{
		$this[$key] = $value;
	}
}

might sometimes works if we extends this class and implement ArrayAccess.

But on the opposide, the code

$t = new TestPlain();

$t->doFoo('', '');

will crash.

IMHO ->yes() should be !->no()
and we will have a behavior similar to https://phpstan.org/r/ae445bd2-da35-4b9f-b0cb-a44c0d1b71e8

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed. Here's a summary of the changes:

Problem: The reviewer pointed out that for non-final classes like TestPlain, $this[$key] = $value should not error because a subclass might implement ArrayAccess. The check should use !->no() instead of ->yes().

Changes:

  1. VariableAssignNode — Added dimFetch flag to distinguish $this[$key] = $value (dim-fetch) from $this = $value (direct assignment)

  2. AssignHandler — Pass dimFetch: true when creating VariableAssignNode from dim-fetch assignment paths

  3. InvalidVariableAssignRule — Changed the check to:

    • Use $node->isDimFetch() to only suppress errors for dim-fetch assignments
    • Check $scope->getType($variable) (the type of $this) instead of the assigned expression type
    • Use !->no() instead of ->yes() — this allows the "maybe" case for non-final classes (line 614 of ObjectType::isSuperTypeOf returns Maybe when checking an interface against a non-final class)
  4. Test data — Added TestArrayAccessReassign case to verify $this = $other still errors even when $other implements ArrayAccess

Behavior:

  • TestArrayAccess (implements ArrayAccess): $this[$key] = $value → no error, $this = $value → error
  • TestArrayAccessReassign: $this = $other → error (even though $other is ArrayAccess)
  • FinalTestPlain (final, no ArrayAccess): $this[$key] = $value → error
  • TestPlain (non-final, no ArrayAccess): $this[$key] = $valueno error (subclass might implement ArrayAccess)

phpstan-bot and others added 5 commits March 23, 2026 14:27
…eassignment

Instead of checking the assigned expression type, use a dimFetch flag on
VariableAssignNode to distinguish $this[$key] = $value from $this = $value.
Check $this type with !->no() so non-final classes that might implement
ArrayAccess through subclasses are not falsely reported.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@staabm staabm merged commit 6895e54 into phpstan:2.1.x Mar 23, 2026
651 checks passed
@staabm staabm deleted the create-pull-request/patch-t6vskzw branch March 23, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants