Report always-true in_array(..., true) when every finite needle value is guaranteed in the haystack - #5944

Do we have test for this branch ?

Also, I'm not sure about it since array<int, 1|2> doesn't enforce every finite typed to be present.

I've addressed both of VincentLanglet's review comments. Here's a summary of what I did and why.

Comment 1 — ImpossibleCheckTypeHelper.php: "Do we have a test for this branch? … array<int, 1|2> doesn't enforce every finite type to be present"

The reviewer is correct on both counts.

Soundness: The else branch collected every finite type of a general array's value type into $guaranteedValueTypes, then treated them as guaranteed-present. That's unsound — array<int, 1|2> could be [1], [2], or even empty, so it guarantees neither 1 nor 2. With a 1|2 needle this would wrongly prove always-true.

Reachability: I instrumented the branch and ran the suite — it's never reached. As my experiments confirmed, TypeCombinator always collapses a constant-array | general-array union into a single general array, and a purely general haystack is already handled earlier (the isConstantArray()->no() block at lines 162-181). So the branch only survives as a defensive path for non-collapsing union haystacks.

Rather than keep speculative, untestable code that guesses guarantees from a general array, I replaced it with a conservative return null (inconclusive) plus a comment explaining exactly when the branch is reached and why a general array can't guarantee a value's presence. I also added explicit generalArrays() coverage to nsrt/bug-14873.php asserting that array<int, 1|2> / non-empty-array<int, 1|2> needles infer bool (not always-true), while non-empty-array<int, 1> correctly stays true.

Comment 2 — InArrayFunctionTypeSpecifyingExtension.php: "Why do we need setRootExpr?"

Added a comment: each per-item SpecifiedTypes carries its own $needle === $item as its root expression; once multiple items are combined, that per-item root expression no longer describes the whole in_array() call, and leaving it set lets ImpossibleCheckTypeHelper read a single item's comparison type and draw a wrong conclusion about the whole call. Clearing it removes the leak.

Verification

• make phpstan — clean.
• NodeScopeResolverTest (1664), TypeSpecifierTest, all Rules/Comparison + Rules/Functions (1012) — all pass.
• The three new generalArrays() assertions fail appropriately if the old unsound collection is restored.

Committed as two logical commits and pushed.

Pushed. Here's my summary of how this review is addressed.

Response to VincentLanglet's review

Both comments were on an earlier version of the diff (the quoted hunk still shows the old getIterableValueType()->getFiniteTypes() loop). They have since been addressed by the two most recent commits, and I've now also added rule-level test coverage.

Comment 1 — ImpossibleCheckTypeHelper.php: "Do we have a test for this branch? Also, I'm not sure about it since array<int, 1|2> doesn't enforce every finite type to be present."

You're right — the old else branch collected every finite type of a general array's value type and treated each as guaranteed-present, which is unsound: array<int, 1|2> can be [1], [2], or empty, so it guarantees neither value.

Fixed in a9635ab28: the general-array branch now bails conservatively with return null instead of guessing guarantees. A general (non-constant) array can never guarantee a specific value is present, and a purely general haystack is already handled earlier (the isConstantArray()->no() block at lines 162–181), so the else is only reachable for non-collapsing constant | general union haystacks — which TypeCombinator collapses in practice, making it a defensive path.

Tests:

• nsrt/bug-14873.php::generalArrays() asserts array<int, 1|2> and non-empty-array<int, 1|2> needles infer bool (not true), while non-empty-array<int, 1> correctly stays true.
• I've now also added generalArrays() to the rule test fixture (tests/PHPStan/Rules/Comparison/data/bug-14873.php, commit 55d090143) confirming the helper itself reports no false always-true for general arrays — this is the rule-level counterpart to the type-inference test, directly answering your "do we have a test" question.

The still-active constant-array branch is sound and tested: it only marks a value guaranteed when its position has exactly one finite type (count(...) === 1), so array{1|2} is never treated as guaranteeing 1 or 2, whereas array{1, 2} correctly guarantees both. Covered by variableHaystack()/integers()/enums() and the rule test's lines 17/21.

Comment 2 — InArrayFunctionTypeSpecifyingExtension.php: "Why do we need setRootExpr?"

Explained in code in 565576e63: each per-item SpecifiedTypes from specifyTypesInCondition(Identical($needle, $item)) carries its own $needle === $item as its root expression. For a single-element array that root expr correctly describes the whole call, but once multiple items are combined it no longer does — and mergeRootExpr(null, X) lets the last item's comparison re-surface as the combined root expr. ImpossibleCheckTypeHelper then reads getType(that single comparison) (line 339), which for a subset needle is the constant false, and wrongly concludes the whole in_array() is always-false. Clearing it (only when multiple items were combined) removes that leak.

All checks green: ImpossibleCheckTypeFunctionCallRuleTest (100), ParameterCastableToStringRuleTest (8), NodeScopeResolverTest (1664), and make phpstan clean.

Why do we need setRootExpr ?

Use a shorter comment

Done. I addressed VincentLanglet's review comment by shortening the multi-line comment on the general-array haystack branch in ImpossibleCheckTypeHelper.php to a single concise line:

} else {
    // A general array cannot guarantee any specific value is present.
    return null;
}

make phpstan is clean and ImpossibleCheckTypeFunctionCallRuleTest passes (100/100). The change was committed and pushed (after rebasing on top of remote work that landed in the meantime).