Skip to content

Keep the array part of a union after unset() of an offset - #6722

Open
SanderMuller wants to merge 3 commits into
phpstan:2.3.xfrom
SanderMuller:unset-offset-keeps-union
Open

SanderMuller wants to merge 3 commits into
phpstan:2.3.xfrom
SanderMuller:unset-offset-keeps-union

Conversation

@SanderMuller

Copy link
Copy Markdown
Contributor

unset($x['key']) on a union that has a member without offsets turned the whole variable into *ERROR*. That is the case for array|false, array|string, array|int and similar unions. PHPStan reported nothing on the unset, and every later check on the variable went silent.

/** @param array{port?: int, path?: string}|false $parsed */
function foo($parsed): void {
	unset($parsed['port']);
	\PHPStan\dumpType($parsed); // before: *ERROR*, after: array{path?: string}|false
}

UnionType::unsetOffset() mapped every member through unsetOffset(). false, int, string and the other scalars return ErrorType there, and the union of an array with ErrorType is ErrorType. Now the members that return ErrorType are left out, the same way UnionType::getOffsetValueType() already does it. The result is ErrorType only when no member is left.

At runtime, unset($x['k']) leaves false as it is, with a deprecation since PHP 8.1. It throws an Error for true, int, float, string and objects without ArrayAccess. So ConstantBooleanType(false) now returns itself from unsetOffset(), as NullType already does, and BooleanType returns false. Without that, $x === false after the unset() was reported as always false.

BenevolentUnionType::unionTypes() already drops ErrorType results, so benevolent unions did not have this bug. BenevolentUnionType now overrides unsetOffset() with the old unionTypes() call, so its result stays benevolent.

I found this while comparing PHPStan and Mago findings on WordPress. One WordPress bug fix was for a missing 'path' key on a parse_url() result in redirect_canonical(). PHPStan reported nothing on that line before the fix, because an unset( $redirect['port'] ) a few lines earlier had turned $redirect into *ERROR*. With this change, PHPStan reports the line. On WordPress trunk from 2026-10-07, the change adds one error. In sanitize_trackback_urls(), preg_split() can return false, and that value reaches array_map() after a loop that unsets offsets.

UnsetRule still reports nothing for unset() on array|false, because it reports only when the offset can never be accessed. Reporting the "maybe" case would add new errors, so I left it for a separate change.

UnionType, BenevolentUnionType, BooleanType and ConstantBooleanType are shadowed by the Turbo extension, so the change is ported to their .cpp files, and the declarations are regenerated. The existing bool, false, scalars and nullableInt subjects in type-family.php diverge between the two implementations if only one side has the change. I added array|false, array|string and a benevolent array|false|null subject as well. Analysis output on WordPress is identical with and without the extension.

🤖 Generated with Claude Code

SanderMuller and others added 2 commits October 9, 2026 23:59
UnionType::unsetOffset() mapped every member through unsetOffset(). A
member without offsets (false, int, string, ...) returns ErrorType there,
and the union with ErrorType made the whole variable *ERROR*, so every
later check on it went silent. Leave those members out, as
getOffsetValueType() already does, and return ErrorType only when no
member is left.

unset() of an offset leaves false as it is (a deprecation since PHP 8.1)
and throws on true, so false now keeps itself, as null already does, and
bool becomes false.

BenevolentUnionType keeps its previous unionTypes() call, which already
leaves out ErrorType results and keeps the result benevolent.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@SanderMuller

Copy link
Copy Markdown
Contributor Author

The shopware/shopware integration job fails with one new error, and I think it is correct:

tests/integration/Core/Content/ImportExport/Api/ImportExportProfileApiTest.php:86
Parameter #3 $parameters of method ...TestBrowser::jsonRequest() expects array<int|string, mixed>, array<string, mixed>|false given.

The test does $entry = current($this->prepareImportExportProfileTestData());, which is array|false, then unset($entry[$property]);, then passes $entry on. Before this change the unset() turned $entry into *ERROR*, so the false from current() went unreported. It needs an entry in shopware-baseline.neon in phpstan/phpstan. I can open that PR if you want it.

The other red checks fail on other open 2.3.x PRs as well (for example #6721 and #6723).


public function unsetOffset(Type $offsetType): Type
{
if ($this->value) {

@staabm staabm Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ($this->value) {
/** unset() of an offset leaves false as it is (deprecated since PHP 8.1) and throws on true. see https://3v4l.org/mHHkL#veol */
if ($this->value) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied in c946474. The snippet also shows that before PHP 8.1, unset() on true does nothing. unsetOffset() does not know the PHP version, so it still returns ErrorType for true, the same as before this PR.

}

/**
* @param array<string, int>|int|false $value

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please also test with ArrayAccess|false variants

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in c946474: ArrayAccess|false (PHPDoc and native), ArrayAccess|array|false, ArrayAccess|string and ArrayAccess|bool. All five gave *ERROR* without this change.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.

2 participants