Fix phpstan/phpstan#6934: Nullsafe operator does not narrow down type#5322
Fix phpstan/phpstan#6934: Nullsafe operator does not narrow down type#5322phpstan-bot wants to merge 8 commits intophpstan:2.1.xfrom
Conversation
| ]); | ||
| } | ||
|
|
||
| #[RequiresPhp('>= 8.0')] |
There was a problem hiding this comment.
why does this test fail on php 8.0?
There was a problem hiding this comment.
Done. Removed the #[RequiresPhp('>= 8.0')] attribute from testBug6934() since it's unnecessary — RuleTestCase uses php-parser which can parse nullsafe ?-> syntax on any PHP version. Other nullsafe tests in the same file (e.g. testNullSafe()) already run without this attribute.
There was a problem hiding this comment.
thats not true. we need #[RequiresPhp('>= 8.0')]. tell me why the test errors on PHP 8.0
There was a problem hiding this comment.
Type inference tests also passed. All checks are green — the commit with #[RequiresPhp('>= 8.0')] restored has been pushed.
clxmstaab
force-pushed
the
create-pull-request/patch-y24thcm
branch
from
4b10170 to
fd7af45
Compare
staabm
force-pushed
the
create-pull-request/patch-y24thcm
branch
from
fd7af45 to
5ebd295
Compare
VincentLanglet
force-pushed
the
create-pull-request/patch-y24thcm
branch
from
a43dab4 to
8903c78
Compare
…method args - In nullsafe method call chains like $a?->b?->method($a), the variable $a was not narrowed to non-null when processing method arguments - Added narrowNullsafeVarChain() to NonNullabilityHelper to walk a nullsafe chain and narrow each intermediate var - Modified MethodCallHandler to apply nullsafe chain narrowing before processing arguments of virtual nullsafe method calls, then revert after - New regression test in tests/PHPStan/Analyser/nsrt/bug-6934.php
…6934 - Add CallMethodsRule test to verify no parameter type error on $node?->parentNode?->removeChild($node) - Assert $node remains DOMNode|null after the nullsafe chain - Assert $foo remains Foo|null after the nullsafe chain Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
RuleTestCase uses php-parser which can parse nullsafe syntax on any
PHP version, so #[RequiresPhp('>= 8.0')] is not needed. Other nullsafe
tests in this file (e.g. testNullSafe) run without this attribute.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test data file uses DOMNode which has version-dependent property types via #[LanguageLevelTypeAware] in phpstorm-stubs. On PHP < 8.0, the analysis may produce different errors since DOMNode::$parentNode lacks a native type declaration. The // lint >= 8.0 comment in the test data file is only processed by TypeInferenceTestCase, not by RuleTestCase::analyse(), so #[RequiresPhp] is needed to skip the test on older PHP versions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
staabm
force-pushed
the
create-pull-request/patch-y24thcm
branch
from
8903c78 to
9f51d39
Compare
|
I have a new theory why this error happens... when you debug the snippet <?php // lint >= 8.0
declare(strict_types = 1);
namespace Bug6934;
use DOMNode;
use function PHPStan\Testing\assertType;
function removeFromParent(?DOMNode $node): void {
$node?->parentNode?->removeChild($node);
}with PHP 8.0 vs. 8.5 you see that the MethodCall Handler is invoked in different order in comparison to the FunctionCallParametersCheck. I think the problem is related to whether the code is analyzed with Fibers supported or not |
|
running the above snippet with also reproduces the error with newer PHP versions, which proofs that the error we see in CI is related to fibers |
|
Should we add antoher Also, we'll need to decide if it's worth it to solve this for the non fiber versions. |
|
I'm fine if a bugfix only makes the analysis work on PHP 8.1+. |
maybe we can merge like it is and let the bot checkout whether there is a way to fix it for PHP8.x with regular NodeScopeResolver |
|
Also I'm not sure about this bugfix. When I look at NullsafeMethodCallHandler I feel like it should already do what the bug is about, so maybe the bugfix should be more minimal? |
4 participants
Summary
When using chained nullsafe operators like
$node?->parentNode?->removeChild($node), PHPStan did not narrow$nodeto non-null inside the method call arguments, even though the nullsafe chain guarantees$nodecannot be null ifremoveChild()is reached.Changes
narrowNullsafeVarChain()method tosrc/Analyser/ExprHandler/Helper/NonNullabilityHelper.phpthat walks a nullsafe chain (NullsafePropertyFetch/NullsafeMethodCall) and narrows each intermediate var to non-nullsrc/Analyser/ExprHandler/MethodCallHandler.phpto detect virtual nullsafe method calls (via thevirtualNullsafeMethodCallattribute) and apply nullsafe chain narrowing before processing arguments, then revert the narrowing afterwardtests/PHPStan/Analyser/nsrt/bug-6934.phpRoot cause
The
NullsafeMethodCallHandlerconverts aNullsafeMethodCallto a regularMethodCalland narrows the direct var expression to non-null. When theMethodCallHandlerprocesses this converted call, it first processes the var (which may contain inner nullsafe operators that temporarily narrow their vars and then revert), then processes args with the reverted scope. The inner nullsafe narrowing was lost before arguments were processed.The fix applies the nullsafe chain narrowing at the right point in the
MethodCallHandler— after the var is fully processed (so nullsafe rules fire correctly with the original scope) but before args are processed (so arguments see non-null types).Test
Added
tests/PHPStan/Analyser/nsrt/bug-6934.phpwithassertType()calls verifying that variables used in nullsafe method call chains are narrowed to non-null inside method arguments.Fixes phpstan/phpstan#6934