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.
After the Rework commit unified both FAUX extensions and equality assertions through setRootExpr($node), the specifyOnly flag became dead code. Remove it entirely. Add PHPDoc to setRootExpr() explaining the mechanism: when rootExpr is set, ImpossibleCheckTypeHelper evaluates it in scope instead of analysing sureTypes/sureNotTypes. When rootExpr === the check node (as used by str_contains, array_key_exists, and equality assertions), isSpecified() bails out immediately, preventing false always-true reports from side-effect narrowing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When setRootExpr($node) is used, also add a sureType for the call expression with ConstantBooleanType(true). This stores the expression result in scope via filterByTruthyValue, enabling ImpossibleCheckTypeHelper to detect duplicate calls (e.g. nested identical str_ends_with inside if(str_ends_with(...))). ImpossibleCheckTypeHelper now checks scope for the expression type before the isSpecified early return. If the call result is already known (stored from a previous identical check), it reports always-true/false. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The same array_key_exists($prototypeParameterName, $prototypeMethodCalls) check was performed twice — the second was dead code. Detected by the new duplicate call detection for rootExpr-based type specifying. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…around Type-specifying extensions that narrow argument types as a side effect (e.g. str_contains narrowing haystack to non-empty-string) previously used setRootExpr($node) plus a manually-added ConstantBooleanType sureType to suppress false always-true/false reports and enable duplicate call detection. Equality assertions used the same workaround. This commit replaces that pattern with a dedicated setSideEffectOnly() flag on SpecifiedTypes: - SpecifiedTypes: new @api setSideEffectOnly() setter, isSideEffectOnly() getter, propagated through all immutable-copy methods (OR semantics in unionWith/intersectWith, preserved through normalize and setRootExpr) - ImpossibleCheckTypeHelper: dedicated sideEffectOnly block before the rootExpr block — checks hasExpressionType for duplicate detection, otherwise returns null (no always-true/false report) - MutatingScope: filterByTruthyValue/filterByFalseyValue automatically store the call's boolean result via TypeSpecifier::create when sideEffectOnly is set, enabling duplicate detection without manual ConstantBooleanType sureTypes in each extension - Extensions simplified: StrContainingTypeSpecifyingExtension: ->setSideEffectOnly() ArrayKeyExistsFunctionTypeSpecifyingExtension: ->setSideEffectOnly() PregMatchTypeSpecifyingExtension: ->setSideEffectOnly() TypeSpecifier::specifyTypesFromAsserts: ->setSideEffectOnly() All 12540 tests pass. PHPStan self-analysis reports no errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
str_contains, str_starts_with, and str_ends_with don't exist in PHP 7.4,
so the test data is split into bug-14705.php (strpos, array_key_exists,
equality assertions) and bug-14705-php8.php (str_* functions). The PHP 8+
test method uses #[RequiresPhp('>= 8.0')] attribute.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The term "side effect" is overloaded — in programming it refers to function impurity, while here it meant "type narrowings that are a consequence of the check." Rename to specifyOnly which fits the SpecifiedTypes domain: these types only specify (narrow) types, they don't determine the check outcome in ImpossibleCheckTypeHelper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
The specifyOnly duplicate-detection marker is keyed by the exact condition node. After an early-continue on `!array_key_exists(...)`, only the BooleanNot node records a boolean (false); the bare positive call is a different node that was never a condition, so it keeps its declared bool return type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 ?
When a specifyOnly condition is a BooleanNot (e.g. `!array_key_exists(...)`), store the boolean result marker for the underlying call rather than for the negated form. The negated form is then derived from the inner value instead of being capped at bool, so the bare positive call narrows to true. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…medNamedArgumentRule The new BooleanNot-aware specifyOnly duplicate detection flags this genuine duplicate guard as always-true. The second identical check was dead code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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. |
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