Replace FAUX function call workarounds#5769
Conversation
VincentLanglet
left a comment
There was a problem hiding this comment.
Should it be something like "specifyOnly" (improve the suggestion)
Also should overwrite imply this ?
|
Pushed. Here's what changed:
|
setShouldNotImplyOppositeCase() on SpecifiedTypes to replace FAUX function call workaroundssetSpecifyOnly() on SpecifiedTypes to replace FAUX function call workarounds
|
@phpstan-bot describe how https://phpstan.org/writing-php-code/narrowing-types#equality-assertions is implemented (without changing code) |
VincentLanglet
left a comment
There was a problem hiding this comment.
When we use @phpstan-assert =string $foo the AssertTag has isEquality set to true.
Then there was the logic
$newTypes = $this->create(
$assertExpr,
$assertedType,
$assert->isNegated() ? TypeSpecifierContext::createFalse() : TypeSpecifierContext::createTrue(),
$scope,
)->setRootExpr($containsUnresolvedTemplate || $assert->isEquality() ? $call : null);
which avoided the always-true/false error
Then seems like some specific case exists in ImpossibleCheckTypeHelper for $rootExpr !== null. Shouldn't we still rely on this logic ?
Also, it would be nice to keep reporting the second condition as always true in
if (str_ends_with($haystack, $needle)) {
if (str_ends_with($haystack, $needle)) {
}
}
|
Pushed. Here's what changed: Equality assertions (
Duplicate call detection: Added test cases showing that nested identical |
VincentLanglet
left a comment
There was a problem hiding this comment.
I want
-
An Unified behavior for Faux function call and Assert with
isEqualityto true. -
Duplicate call to be reported, like how it works for typeSpecifier without RootExpr.
class MondayMorning
{
/**
* @param non-empty-string $needle
*/
public function strEndsWithDuplicate(string $haystack, string $needle): void
{
if (str_ends_with($haystack, $needle)) { // Not reported
if (str_ends_with($haystack, $needle)) { // Reported
}
}
}
public function startDay(string $task): void
{
if ($this->isValid($task)) { // Not reported
if ($this->isValid($task)) { // reported
}
}
}
/** @phpstan-assert-if-true =non-empty-string $foo */
public function isValid(string $foo): bool
{
}
}
Maybe start by explaining the purpose of the rootExpr.
f8567a4 to
d6e4e48
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
I want
-
An Unified behavior for Faux function call and Assert with
isEqualityto true. -
Duplicate call to be reported, like how it works for typeSpecifier without RootExpr.
class MondayMorning
{
/**
* @param non-empty-string $needle
*/
public function strEndsWithDuplicate(string $haystack, string $needle): void
{
if (str_ends_with($haystack, $needle)) { // Not reported
if (str_ends_with($haystack, $needle)) { // Reported
}
}
}
public function startDay(string $task): void
{
if ($this->isValid($task)) { // Not reported
if ($this->isValid($task)) { // reported
}
}
}
/** @phpstan-assert-if-true =non-empty-string $foo */
public function isValid(string $foo): bool
{
}
}
Maybe start by explaining the purpose of the rootExpr.
|
Pushed successfully. Here's a summary of what changed: Unified
Added duplicate call detection:
Tests: All 12540 tests pass. PHPStan shows only 4 pre-existing errors (none from this change). New test expectations verify duplicate detection for |
f7b6a02 to
b601591
Compare
| new Arg($args[$needleArg]->value), | ||
| ]), | ||
| )); | ||
| )->setRootExpr($node); |
There was a problem hiding this comment.
What's the purpose of the setRootExpr method ?
And why setting the $node has exactly the same behavior than the previous code with the FAUX_FUNCTION` ?
There was a problem hiding this comment.
Pushed. Here's what changed:
Removed the specifyOnly flag entirely — after the Rework commit unified both FAUX extensions and equality assertions through setRootExpr($node), the flag was dead code. Removed the property, setter, getter, and all propagation in intersectWith/unionWith/normalize/immutable-copy methods.
Documented setRootExpr() with a PHPDoc explaining the mechanism and answering the reviewer's question:
- Purpose: When
rootExpris set,ImpossibleCheckTypeHelperevaluates it directly in scope instead of analysing sureTypes/sureNotTypes. This separates "side-effect narrowing" from "check outcome determination." - Why
setRootExpr($node)behaves the same as FAUX_FUNCTION: Both reachreturn nullinImpossibleCheckTypeHelper, just via different paths. With$node, theisSpecified()check sees$rootExpr === $node(line 374) and returnstrueimmediately, causing an earlyreturn null. With FAUX_FUNCTION,isSpecified()returnedfalse(fabricated expression ≠ node), then$scope->getType(fabricatedExpr)couldn't resolve to aConstantBooleanType(undefined function), falling through to the samereturn null.
All 12539 tests pass, PHPStan self-analysis reports no errors.
setSpecifyOnly() on SpecifiedTypes to replace FAUX function call workarounds
VincentLanglet
left a comment
There was a problem hiding this comment.
Would it be possible to deprecate setRootExpr and use a new property/setter with a dedicated logic in ImpossibleCheckHelper instead which
- Doesn't require to add
$this->create($call, new ConstantBooleanType($context->true()), TypeSpecifierContext::createTrue(), $scope),
as a workaround
- Keep reporting duplicate calls
- Still avoid always-true/always-false false positive
Without any implementation, propose one or multiple strategy in a PR comment.
|
I can't comment directly on the upstream PR due to token permissions. Since the review asks for strategy proposals without implementation, I'll provide the comment content here. The automated pipeline will post it to the PR. Proposed strategies for deprecating
|
| Aspect | Current approach | Proposed |
|---|---|---|
| Extension API | ->unionWith(create($node, ConstantBooleanType...))->setRootExpr($node) |
->setSideEffectOnly() |
| False positive suppression | isSpecified short-circuit via rootExpr === node |
Dedicated early return in ImpossibleCheckTypeHelper |
| Duplicate detection | hasExpressionType using manually-added ConstantBooleanType sureType |
hasExpressionType using automatically-stored result in filterByTruthyValue |
| Infrastructure changes | None | filterByTruthyValue/filterByFalseyValue (~4 lines each) |
No code changes in this iteration — this is a strategy proposal per the reviewer's request. The current PR code remains as-is until a strategy is agreed on.
VincentLanglet
left a comment
There was a problem hiding this comment.
Implement the strategy from #5769 (comment)
|
Pushed successfully. Here's a summary of what was implemented: Added How it works:
|
VincentLanglet
left a comment
There was a problem hiding this comment.
testBug14705 is failing on PHP 7.4 I think it's because is str_ends_with and str_contains doesn't exists.
Split the test in half, on general and one for PHP 8+
|
All 12541 tests pass, |
VincentLanglet
left a comment
There was a problem hiding this comment.
You said
Side-effect suppression (~5 call sites in extensions + equality assertions): Extensions like StrContaining and ArrayKeyExists set rootExpr = $node (the call itself). Since isSpecified($node, $rootExpr) returns true when $rootExpr === $node (line 380 of ImpossibleCheckTypeHelper), this triggers immediate return null, suppressing false always-true/false reports. The ConstantBooleanType sureType was then added alongside to enable duplicate detection via hasExpressionType (lines 281-289).
But call the option sideEffectOnly ; is it consistent ?
|
Renamed |
| continue; | ||
| } | ||
|
|
||
| if (!array_key_exists($prototypeParameterName, $prototypeMethodCalls)) { |
There was a problem hiding this comment.
how is this change related?
There was a problem hiding this comment.
Reverted the unrelated array_key_exists removal in MethodCallWithPossiblyRenamedNamedArgumentRule.php. The original file had a genuinely duplicated array_key_exists check that was caught during make phpstan runs with this PR's changes, but it's a pre-existing issue unrelated to the specifyOnly mechanism. All 12541 tests pass and make phpstan is clean.
The duplicate check in MethodCallWithPossiblyRenamedNamedArgumentRule is a pre-existing issue unrelated to this PR. Reverting to keep the PR focused on the specifyOnly mechanism. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add PHPDoc explaining that setSpecifyOnly() works correctly with extensions that synthesize expressions for specifyTypesInCondition() (like phpstan-webmozart-assert), because filterByTruthyValue() stores the boolean result for the original call node, not the synthesized one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous PHPDoc incorrectly claimed that extensions synthesizing expressions (e.g. decomposing Assert::string($x) into is_string($x)) can rely on filterByTruthyValue() for duplicate detection. This is only true for boolean-returning calls used in conditions (if/while). For void assertion methods used as statements, filterByTruthyValue() is never called — NodeScopeResolver processes them via filterBySpecifiedTypes with null context. ImpossibleCheckTypeHelper::determineContext() also returns null context for void methods. The specifyOnly mechanism does not provide duplicate detection in that path (and neither did the former setRootExpr/FAUX workaround). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion The previous approach used TypeSpecifier::create() which has a purity check that returns empty SpecifiedTypes for impure/void method calls. This prevented the boolean marker from being stored in scope, breaking duplicate detection for void assertion methods. Additionally, the marker was added via unionWith before filterBySpecifiedTypes, which caused addTypeToExpression to intersect ConstantBooleanType(true) with the existing void return type, producing NeverType instead of the expected true marker. Fix: store the boolean marker AFTER filterBySpecifiedTypes using a separate filterBySpecifiedTypes call with setAlwaysOverwriteTypes(). This bypasses both the purity check and the type intersection issue. Also adds specifyOnly handling to NodeScopeResolver's expression statement path, enabling duplicate detection for void assertion methods used as statements (e.g. Assert::string($x) called twice). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous commit stored ConstantBooleanType(true) for the call expression using setAlwaysOverwriteTypes(), bypassing TypeSpecifier's purity check. For impure functions like realpath() (which has @phpstan-assert-if-true =non-empty-string), this overwrote the function's real return type in the truthy scope: getType(realpath($x)) returned true instead of non-empty-string, causing the Elvis operator realpath($x) ?: $x to produce string|true instead of string. Fix: use TypeSpecifier::create() with its purity check, as in the approach before commit 0392487. For impure calls (hasSideEffects=yes), create() returns empty SpecifiedTypes, preventing the boolean marker from overwriting the return type. Duplicate detection still works for pure and possibly-impure calls. The NodeScopeResolver expression statement path (for void assertions) retains the overwrite approach since void calls are not used for their return value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ases Consolidates the rule data file and the specifyOnly-elvis-realpath nsrt file into a single nsrt/bug-14705.php that asserts the narrowed types inside the if branches. The ImpossibleCheckType rule tests now reference the moved file for always-true duplicate-call reporting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
816af8c to
341f63d
Compare
|
This would be used this way phpstan/phpstan-webmozart-assert#216 |
Mirrors the existing setAlwaysOverwriteTypes()/shouldOverwrite() naming pair, so the specifyOnly setter/getter follows the same convention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
issue-bot tells us that with this PR inference for this snippet will be narrowed: <?php
declare(strict_types=1);
/**
* @param array<string,string|array<int,string>> $theInput
* @phpstan-param array{'name':string,'owners':array<int,string>} $theInput
* @param array<int,string> $theTags
* @return void
*/
function Example(array $theInput, array $theTags): void
{
foreach ($theTags as $tag) {
if (!array_key_exists($tag, $theInput)) {
continue;
}
\PHPStan\dumpType(!array_key_exists($tag, $theInput));
\PHPStan\dumpType(array_key_exists($tag, $theInput));
}
}after this PR: before this PR: just to let you know, in case this is something which should be tested/asserted or analysed further |
VincentLanglet
left a comment
There was a problem hiding this comment.
Add in the nsrt file
/**
* @param array<string,string|array<int,string>> $theInput
* @phpstan-param array{'name':string,'owners':array<int,string>} $theInput
* @param array<int,string> $theTags
* @return void
*/
function Example(array $theInput, array $theTags): void
{
foreach ($theTags as $tag) {
if (!array_key_exists($tag, $theInput)) {
continue;
}
\PHPStan\assertType(..., !array_key_exists($tag, $theInput));
\PHPStan\assertType(..., array_key_exists($tag, $theInput));
}
}
staabm
left a comment
There was a problem hiding this comment.
I love FAULX workarounds to get removed :)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done. I addressed the review by adding the requested snippet to the nsrt file. What I addedA new foreach ($theTags as $tag) {
if (!array_key_exists($tag, $theInput)) {
continue;
}
assertType('false', !array_key_exists($tag, $theInput));
assertType('bool', array_key_exists($tag, $theInput));
}The Verification
Committed and pushed to |
| continue; | ||
| } | ||
| assertType('false', !array_key_exists($tag, $theInput)); | ||
| assertType('bool', array_key_exists($tag, $theInput)); |
There was a problem hiding this comment.
Why this is not true ?
There was a problem hiding this comment.
Done. Here's my answer to the reviewer's question.
Why array_key_exists($tag, $theInput) is bool, not true
bool is correct (just less precise than true). The reason is in how the specifyOnly duplicate-detection marker works versus how a normal bool-returning function narrows.
The marker is keyed by the exact condition node. The boolean result of a specifyOnly call is only recorded in scope by filterByTruthyValue/filterByFalseyValue, for the literal expression that was used as a condition. In this loop the condition is the BooleanNot !array_key_exists($tag, $theInput), so after the early-continue only that node is recorded — as false. The bare positive array_key_exists($tag, $theInput) is a different AST node that was never used as a condition, so it keeps its declared bool return type.
I confirmed this is purely about which node is the condition:
| Condition | array_key_exists(...) |
!array_key_exists(...) |
|---|---|---|
if (!array_key_exists(...)) continue; |
bool |
false |
if (array_key_exists(...)) { ... } |
true |
false |
When the positive call is the condition, it resolves to true, fully symmetric.
Why specifyOnly funcs differ from normal funcs. A normal bool-returning function (check()) gets its call expression itself narrowed to true/false by TypeSpecifier, so that narrowing propagates through the BooleanNot and check($x) becomes true after if (!check($x)) return;. The specifyOnly extensions (array_key_exists, str_contains, …) deliberately do not narrow the call expression — they only narrow the argument (the array/haystack) as a side effect, and set shouldSpecifyOnly() so ImpossibleCheckTypeHelper doesn't treat that argument-narrowing as the check's outcome.
Making it true isn't worth it here. It would require either reintroducing the per-call create($node, ConstantBooleanType) marker inside the extensions — exactly the workaround this PR removed at your request — or special-casing BooleanNot inside MutatingScope. bool is genuinely correct, so I documented the reasoning in the test rather than expanding scope.
I added a comment block above arrayKeyExistsDuplicateInLoop capturing this. Verified: NodeScopeResolverTest (1618), the three testBug14705* rule tests, and make phpstan all pass. Committed as f223e96fd and pushed.
VincentLanglet
left a comment
There was a problem hiding this comment.
Can't we simplify node like !array_key_exists($tag, $theInput) when saving the value ?
if !array_key_exists is saved as false we could instead save array_key_exists as true ?
|
Done. Here's how I addressed the review. The reviewer's question
Yes — and it produces strictly better inference. Implemented. What I changed
Effect on inference (the test case the reviewer added):
Consequence — genuine duplicates now reported. Storing Dead code in PHPStan's own source. This stronger detection flagged a real duplicate guard in Verification
Committed as two logical commits and pushed. |
This reverts commit 6af082c.
7081004 to
32acf00
Compare
| ); | ||
| $scope = $scope->filterBySpecifiedTypes($specifiedTypes); | ||
| if ($specifiedTypes->shouldSpecifyOnly()) { | ||
| // This is the expression-statement counterpart of the specifyOnly handling |
There was a problem hiding this comment.
Use a less verbose comment
There was a problem hiding this comment.
Done. I addressed both review comments:
Changes
src/Analyser/SpecifiedTypes.php (VincentLanglet: "Remove the second paragraph") — Removed the second paragraph from the setSpecifyOnly() PHPDoc, which described the filterByTruthyValue/purity-check duplicate-detection mechanics. The PHPDoc now keeps just the first paragraph describing what the flag means.
src/Analyser/NodeScopeResolver.php ("Use a less verbose comment") — Condensed the 11-line comment in the specifyOnly expression-statement block down to 4 lines, keeping the essentials: it's the statement counterpart of filterByTruthyValue(), it stores the call's true result for duplicate-call reporting, and the direct-overwrite reasoning (void calls have no return value to protect from the purity check).
Verification
make phpstan— clean, no errorsImpossibleCheckType{Function,Method}CallRuleTest— 111/111 pass
Both changes are comment/PHPDoc-only, committed separately and pushed to create-pull-request/patch-7xy6xo5.
| * the check outcome. ImpossibleCheckTypeHelper will not use sureTypes | ||
| * to report always-true/false for the check expression. | ||
| * | ||
| * Duplicate detection: filterByTruthyValue stores the call's boolean |
There was a problem hiding this comment.
Remove the second paragraph
There was a problem hiding this comment.
Pushed. Here's a summary of how I addressed the review.
Review comments addressed
The latest round of review had three comment-only requests, all from VincentLanglet:
SpecifiedTypes.php— "Remove the second paragraph" of thesetSpecifyOnly()PHPDoc (the duplicate-detection explanation).NodeScopeResolver.php— "Use a less verbose comment" for the specifyOnly expression-statement block.bug-14705.php— "Add a comment// could be true" on the duplicatearray_key_existsassertType.
What happened
When I went to push, the automated pipeline had already landed the first two changes upstream as separate commits:
26cc74d4dRemove duplicate-detection paragraph fromsetSpecifyOnly()PHPDocd3ca979e1Shorten specifyOnly expression-statement comment
So I reset onto the remote and contributed only the still-missing third change — annotating the array_key_exists($tag, $theInput) assertType with // could be true (committed as 57ad82ed4 and pushed).
Verification (before the reset, on my equivalent local state)
make phpstan— clean, no errors.ImpossibleCheckType{Function,Method}CallRuleTest— 111/111 pass.NodeScopeResolverTest— 1618/1618 pass.
All three changes are comment/PHPDoc-only and don't affect inference, so behavior is unchanged.
| continue; | ||
| } | ||
| assertType('false', !array_key_exists($tag, $theInput)); | ||
| assertType('bool', array_key_exists($tag, $theInput)); |
There was a problem hiding this comment.
Add a comment // could be true
There was a problem hiding this comment.
The change is complete and already in the branch.
Summary
The reviewer (VincentLanglet) asked to add a // could be true comment to the assertType('bool', array_key_exists($tag, $theInput)); line in tests/PHPStan/Analyser/nsrt/bug-14705.php.
I made the change, verified it with make tests (12547 pass) and make phpstan (clean). When pushing, the rebase revealed the automated pipeline had already committed the identical change upstream (commit 57ad82ed4, "Annotate duplicate array_key_exists assertType with '// could be true'"), so my commit was correctly detected as a duplicate and skipped. The line now reads:
assertType('bool', array_key_exists($tag, $theInput)); // could be trueThe branch is in sync with the remote and the requested change is in place.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Type-specifying extensions like
StrContainingTypeSpecifyingExtensionandArrayKeyExistsFunctionTypeSpecifyingExtensionnarrow argument types as a side effect of a check being true (e.g.str_contains($haystack, $needle)narrows$haystacktonon-empty-string). Previously, to preventImpossibleCheckTypeHelperfrom incorrectly concluding "always true" when the narrowed types were already satisfied, these extensions fabricated rootExpr AST nodes containingFAUX_FUNCTIONor__PHPSTAN_FAUX_CONSTANT— opaque expressions that could never resolve to a constant boolean.This PR replaces that workaround with a proper
shouldNotImplyOppositeCaseflag onSpecifiedTypes, following the same concept as equality assertions (@phpstan-assert-if-true =Type $param) which also prevent the opposite case from being implied.Changes
src/Analyser/SpecifiedTypes.php: AddedshouldNotImplyOppositeCaseboolean flag with@api-taggedsetShouldNotImplyOppositeCase()setter andshouldNotImplyOppositeCase()getter. Flag is propagated through all immutable-copy methods:setAlwaysOverwriteTypes,setRootExpr,setNewConditionalExpressionHolders,removeExpr,intersectWith(OR propagation),unionWith(OR propagation), andnormalize.src/Rules/Comparison/ImpossibleCheckTypeHelper.php: Check the new flag before sureTypes/sureNotTypes analysis — when set, returnnull(no always-true/false reporting).src/Type/Php/StrContainingTypeSpecifyingExtension.php: ReplacedFAUX_FUNCTIONrootExpr withsetShouldNotImplyOppositeCase(). Removed unused imports.src/Type/Php/ArrayKeyExistsFunctionTypeSpecifyingExtension.php: Replaced__PHPSTAN_FAUX_CONSTANTrootExpr withsetShouldNotImplyOppositeCase(). Removed unused imports.src/Analyser/TypeSpecifier.php: InspecifyTypesFromAsserts(), equality assertions now use the new flag instead of setting rootExpr to the call expression. ThecontainsUnresolvedTemplatecase still uses rootExpr as before.Analogous cases probed
ArraySearchFunctionTypeSpecifyingExtension: Already excluded via explicitreturn nullinImpossibleCheckTypeHelper— no change needed.ClassExistsFunctionTypeSpecifyingExtension,FunctionExistsFunctionTypeSpecifyingExtension,DefinedConstantTypeSpecifyingExtension: Already excluded viain_array($functionName, [...])check inImpossibleCheckTypeHelper— no change needed.MethodExistsTypeSpecifyingExtension,PropertyExistsTypeSpecifyingExtension: Have custom logic inImpossibleCheckTypeHelperthat handles them correctly — no change needed.Root cause
The FAUX mechanism was a workaround for the lack of a proper "don't use sureTypes to determine check outcome" flag on
SpecifiedTypes. When a type-specifying extension narrows an argument type as a side effect of a check (not the check's determining condition),ImpossibleCheckTypeHelperwould incorrectly conclude the check is always-true if those types were already satisfied in scope. The fabricatedFAUX_FUNCTION/__PHPSTAN_FAUX_CONSTANTexpressions in rootExpr made the expression unevaluable, suppressing the false positive. The new flag achieves the same result cleanly.Test
tests/PHPStan/Rules/Comparison/data/bug-14705.phpwith test cases forstr_contains,str_starts_with,str_ends_with,strposwithnon-empty-stringhaystack, andarray_key_existswith non-constant key onnon-empty-array— all verifying no false "always true" reports.Fixes phpstan/phpstan#14705