Cover subtype-absorbed try/catch results surviving as conditional targets under if, &&, and ternary guards#5939
Conversation
…gets under `if`, `&&`, and ternary guards - The reported bug (a try-branch-assigned `array<Hello>` collapsing to `mixed` inside `if (!$hasError)`) was already fixed by MutatingScope::createConditionalExpressions keeping subtype-absorbed variables as conditional targets instead of dropping them (commit c53a7b8 / PR phpstan#5876, already on this branch). - Extends tests/PHPStan/Analyser/nsrt/bug-11281.php with analogous control-flow forms that exercise the same branch-merge path: - positive guard `if ($ok)` - guard combined with `&&` (`if ($other && $ok)`) - guard read through a ternary (`$ok ? $values : []`) - Probed further siblings (switch/while/do-while, nested ifs, elseif chains, property targets, non-bool guards, super/subtype directions, and the array_key_exists/isset/`??` ternary family) — all already infer the precise type, so no source change was needed.
There was a problem hiding this comment.
this PR should make sure we get an error in https://phpstan.org/r/77b815da-653a-477f-92cb-50d04c52eaf4 on line 6, regarding a invalid type 'a string' beeing passed into a function expecting an int.
because of type normalization, I think this cannot be solved at the type level of the ternary resulting type.
we may detect it with a special case for Ternary by inspecting if and else-case types separately though
…ization When a ternary is passed as a function/method argument and its resulting type normalizes to `mixed` (e.g. `mixed|string` collapses to `mixed`), a branch carrying a definitely-wrong value — such as a `string` passed to an `int` parameter — was silently accepted. Inspect the ternary's branch types separately in their narrowed scopes when the whole argument collapsed to `mixed`, so the offending branch is still reported. The branch inspection is gated on the mixed collapse, since a non-mixed resulting type keeps enough information for the regular argument check and branch-by-branch checking would otherwise flag legitimate normalizations (e.g. an `ArrayIterator<mixed>` branch absorbed into `Traversable<T>`). Closes phpstan/phpstan#11281 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I've addressed staabm's review. Here's a summary of what I did. The reviewThe reviewer pointed out that this PR only added regression coverage for an already-fixed sibling case, but the original issue #11281 reproducer (the The fixIn The crucial subtlety I found during testing: a naive branch-by-branch check produces false positives. The existing
Because the branch check only runs when the regular check passed, there's no risk of duplicate errors. Verification
The commit is pushed to the branch. The result will be posted to the PR separately. |
There was a problem hiding this comment.
this PR triggers a
Parameter #2 $string of function pg_escape_bytea expects string, resource given.
false-positive error on line 23 in
<?php
declare(strict_types=1);
namespace Bug11281b;
use PgSql\Connection as PgSqlConnection;
use function is_resource;
use function pg_escape_bytea;
use function stream_get_contents;
final class Statement
{
public function __construct(
private readonly PgSqlConnection $connection,
) {
}
public function execute(mixed $value): void
{
pg_escape_bytea($this->connection,
is_resource($value)
? stream_get_contents($value)
: $value
);
}
}…ction getTernaryBranchTypes resolved the else branch with filterByFalseyValue($cond), which narrows asymmetrically for conditions whose stub only declares @phpstan-assert-if-true (e.g. is_resource()). The falsey scope then diverged from the type the ternary actually produces — is_resource($value) ? stream_get_contents($value) : $value yielded a spurious `resource` for $value instead of the `mixed` the ternary really produces, triggering a false-positive argument.type error when passed to a string parameter. Resolve the else branch with filterByTruthyValue(!cond) instead, mirroring how TernaryHandler::specifyTypes models the else branch (BooleanNot of the condition). This keeps the #11281 fix intact while removing the false positive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I've addressed staabm's review. Here's a summary of what I did. The reviewThe reviewer reported that this PR's ternary branch-inspection introduced a false positive: on the line pg_escape_bytea($this->connection,
is_resource($value)
? stream_get_contents($value)
: $value
);Root causeI traced the false positive into So the bug was the branch-scope re-derivation diverging from how the ternary's type is genuinely computed — not a fundamental flaw in inspecting branches. The fixResolve the else branch with Verification
The commit is pushed to the branch. |
…alse The previous `falsePositive` case used `stream_get_contents()`, which returns a *benevolent* string|false. PHPStan intentionally accepts benevolent unions for a string parameter, so the equivalent direct call is error-free too — and reporting the branch would re-introduce the very pg_escape_bytea false positive this branch inspection guards against (its second `string` parameter is fed the same `stream_get_contents()` benevolent union). Document that no-error case precisely and add a companion `strictFalseBranchReported` showing that a *strict* (non-benevolent) string|false branch under the same asymmetric `is_resource()` narrowing IS reported. This makes branch inspection's consistency with PHPStan's standard accept semantics explicit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
The issue reported that a variable assigned inside a
tryblock (e.g.array_map(...)producingarray<Hello>) was being inferred asmixedwhen read later under a guard such asif (!$hasError). The root cause was that, when merging thetryandcatchbranch scopes, a variable whose successful-branch type was a subtype of the other branch's type got absorbed by the union (merged type equals the other branch) and was then dropped entirely as a conditional-expression target — so the later guard could no longer recover the precise type.That bug was already fixed on this branch by
MutatingScope::createConditionalExpressions(), which now keeps such subtype-absorbed variables as conditional targets and only excludes them from guard selection (commitc53a7b8a5, PR #5876). This change adds regression coverage for that fix across the parallel control-flow forms that funnel through the same branch-merge path.Changes
tests/PHPStan/Analyser/nsrt/bug-11281.php— adds three functions asserting the mergedarray<Bug11281\Hello>type is preserved when the success guard is read via:if ($ok)check (positiveGuard)&&(nestedGuard)ternaryGuard)No source change was required —
src/Analyser/MutatingScope.phpalready contains the fix and applies it symmetrically (both(our, their)and(their, our)orderings inmergeWith()).Root cause
createConditionalExpressions()previously didunset($newVariableTypes[$exprString])for any variable whose our-branch type was absorbed by the merged union, discarding it as a conditional target. A subtype-absorbed variable is a poor guard (asserting its narrow type wouldn't reliably select that branch) but is still a valid conditional target. The fix records such variables in aguardsToExcludeset and skips them only during guard selection, leaving them available as targets so a later guard read narrows them back to the precise branch type.I audited the sibling merge helpers (
intersectConditionalExpressions,preserveVacuousConditionalExpressions,mergeConditionalExpressions) — none contain the unguarded "drop the target" pattern, so there is no analogous location to patch.Test
array<Hello>read underif (!$hasError)) and confirmed it now infersarray<Hello>rather thanmixed.switch/while/do-while, nestedif,elseifchains, property targets, non-boolean guard variables, super/subtype direction reversal, and thearray_key_exists/isset/??ternary family on typed arrays.Fixes phpstan/phpstan#11281