Skip to content

Keep a by-ref writeback's native type only when the written-back type fits it - #6564

Open
SanderMuller wants to merge 2 commits into
phpstan:2.2.xfrom
SanderMuller:by-ref-native-type-internal
Open

SanderMuller wants to merge 2 commits into
phpstan:2.2.xfrom
SanderMuller:by-ref-native-type-internal

Conversation

@SanderMuller

Copy link
Copy Markdown
Contributor

A regression in 2.2.15 from #6463. With treatPhpDocTypesAsCertain: false, this reports an error that 2.2.14 did not:

function offsetCapture(string $s): ?string
{
	if (! preg_match('/(a)(b)?/', $s, $m, PREG_OFFSET_CAPTURE)) {
		return null;
	}

	return isset($m[2]) ? $m[2][0] : null;
}
Offset 2 on *NEVER* in isset() always exists and is not nullable.

array_key_exists(2, $m) and $m[2] ?? null report the same way. I found no issue for it.

Cause. Since #6463, the native type after a by-ref argument is the parameter's own type. For a builtin, that is its signature map entry, and preg_match's &$matches is string[]. With PREG_OFFSET_CAPTURE each element is an array{string, int}, so the guard intersects the offset-capture shape with array<string>, and the native type becomes *NEVER*. Without the flag the elements are strings, which is why only this case breaks. Bisected: 3f97b5a57 is clean and 091f2cfc4 is the first bad commit.

The same happens to a userland @param-out that contradicts its declaration. PHP checks a by-ref parameter's type only on the way in. So int &$v with @param-out string really does hold a string afterwards, but the native type said int.

Fix. Keep the parameter's own type as the native type only when the written-back type is a subtype of it. Otherwise fall back to mixed. Where the two agree, as in #6463's bug-15250.php cases, nothing changes.

Trade-off. The fallback is mixed, not the nearest type that fits. After a PREG_OFFSET_CAPTURE match, a later is_array($m) is therefore no longer reported as always true with treatPhpDocTypesAsCertain: false. 2.2.14 did report it. A union with the written-back type would keep that. But it would put the PHPDoc shape back into the native type, which is what #6463 set out to stop.

Tests.

  • nsrt/by-ref-writeback-native-type.php: offsetCapture() and userlandParamOut() fail without the change, with *NEVER* and int. noFlags() passes either way and guards the case that must not change.
  • IssetRuleTest::testPregMatchOffsetCaptureWithoutTreatPhpDocTypesAsCertain() fails without the change with the reported error.

What else I checked.

  • The native type after sort, preg_match without flags, preg_match_all, str_replace, exec, parse_str, similar_text, array_push and settype is the same before and after.
  • A 4.5k-file project analysed with treatPhpDocTypesAsCertain: false gives the same 3106 errors before and after.
  • make phpstan is clean. In make tests, the only failure is MissingCheckedExceptionInMethodThrowsRulePhp74Test::testInternalErrors, which fails the same way on 2.2.x without this change on my machine.
  • On 2.3.x the same code sits in ArgumentsHandler.php, so the merge-up needs the same change there.

🤖 Generated with Claude Code

… fits it

After a by-reference argument, the native type of the variable became the
parameter's own type: its declaration, or for a builtin its signature map
entry. PHP checks a declaration only on the way in and a signature map entry
not at all, so neither describes what the call writes back when the two
disagree.

preg_match() with PREG_OFFSET_CAPTURE writes arrays into a slot the signature
map declares as string[]. The guard `if (! preg_match(...))` then intersected
the offset-capture shape with array<string>, the native type became never, and
with treatPhpDocTypesAsCertain: false isset($m[2]) reported "Offset 2 on
*NEVER* in isset() always exists and is not nullable". The same happens to a
userland @param-out that contradicts its declaration.

The native type now falls back to mixed when the written-back type is not a
subtype of the declaration. Where they agree, as in phpstan#6463's own cases, nothing
changes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment thread src/Analyser/NodeScopeResolver.php Outdated
$byRefNativeType = $currentParameter instanceof ExtendedParameterReflection
? $currentParameter->getNativeType()
: $byRefType;
if (!$byRefNativeType->isSuperTypeOf($byRefType)->yes()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need to evaluate this for the ternary else (when $byRefType is assigned to $byRefNativeType). Feel free to write an ordinary if instead of the ternary above.

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.

Done in 3ee2eae.

When the parameter is not an ExtendedParameterReflection, the native type
is the written-back type itself, and the check has nothing to decide.

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