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
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 |
|
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 :)
|
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 |
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. |
7081004 to
32acf00
Compare
ondrejmirtes
left a comment
There was a problem hiding this comment.
- I don't love the
setSpecifyOnlyandshouldSpecifyOnlynames. I don't love it, by reading it I don't understand what it means. - To be fair "equality assertions" (https://phpstan.org/writing-php-code/narrowing-types#equality-assertions) are not a great name either but they do exactly what we want here: https://phpstan.org/r/bf4d5f26-b438-44a2-b980-718fb870ff2e
The error "will always evaluate to true." is not reported and $t is not narrowed in else.
To track how is this implemented we need to look where AssertTag::isEquality() is called. In TypeSpecifier around line 1882 this is done:
->setRootExpr($containsUnresolvedTemplate || $assert->isEquality() ? $call : null)
What would happen if we started to passing null there instead of a FAUX_FUNCTION FuncCall or instead of calling ->setSpecifyOnly() in the context of this PR?
Could we introduce setEquality() as an alias to setRootExpr(null)? Would it work for all use cases? Would all tests still pass?
|
Both parts of the review are addressed. Here's my response for the PR: 1) Renamed
|
511f5d0 to
a08a63e
Compare
a08a63e to
867ba24
Compare
867ba24 to
69a9e98
Compare
69a9e98 to
17d1bee
Compare
|
@VincentLanglet just saw the neos error.. is it valid? |
VincentLanglet
left a comment
There was a problem hiding this comment.
There is a false positive with
/**
* @param string $srcDirectory
*/
public function renderDirectory(string $srcDirectory): void
{
$files = scandir($srcDirectory) ?: [];
foreach ($files as $key => $value) {
which reports Empty array passed to foreach.
|
Pushed. Here's a summary of how I addressed the review. The false positive$files = scandir($srcDirectory) ?: [];
foreach ($files as $key => $value) { // "Empty array passed to foreach."
Root cause
The PR's The earlier realpath fix only covered impure functions (where FixOnly add the boolean marker when the expression is actually boolean: if ($specifiedTypes->isEquality() && $this->getType($expr)->isBoolean()->yes()) {
...
}The marker exists purely for duplicate-call detection, which is only meaningful for boolean-returning calls. Guarding on Verification
Committed as |
Kinda fun, it was related to the fact that phpstan-assert-if-true was use on a non boolean return type. |
…FAUX function call workarounds - Add `shouldNotImplyOppositeCase` flag to `SpecifiedTypes` with `@api`-tagged setter and getter methods, propagated through all immutable-copy operations (setAlwaysOverwriteTypes, setRootExpr, setNewConditionalExpressionHolders, removeExpr, intersectWith, unionWith, normalize) - Check the flag in `ImpossibleCheckTypeHelper::findSpecifiedType()` to return null early, preventing false "always true/false" reports when sureTypes are side effects of a check rather than its determining condition - Replace `FAUX_FUNCTION` rootExpr in `StrContainingTypeSpecifyingExtension` with `setShouldNotImplyOppositeCase()` - Replace `__PHPSTAN_FAUX_CONSTANT` rootExpr in `ArrayKeyExistsFunctionTypeSpecifyingExtension` with `setShouldNotImplyOppositeCase()` - Use the flag for equality assertions in `TypeSpecifier::specifyTypesFromAsserts()` instead of setting rootExpr to the call expression - Remove unused imports (Arg, BooleanAnd, NotIdentical, String_, Name, Identical, ConstFetch) from the two extension files Closes phpstan/phpstan#14705
5495f04 to
645777d
Compare
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