Distrust a TemplateType member's eager Yes in IntersectionType::isAcceptedBy only when that member accepts with Yes - #5962
Conversation
…eptedBy only when that member accepts with Yes - `IntersectionType::isAcceptedBy()` short-circuits lazyMaxMin to Yes, then verifies the result against the holistic `isSuperTypeOf`. Commit bad7874 distrusted that Yes whenever *any* `TemplateType` member was present and the holistic check was Maybe. - That was too broad: the eager Yes can legitimately come from a non-template member (e.g. a plain `Closure` object accepted at object level by a `Closure(static): T` signature type), while a bounded template member only accepts with Maybe. - Narrow the condition so the eager Yes is only distrusted when a `TemplateType` member itself accepts with Yes - i.e. the template's inference-mode eager acceptance is what produced the Yes. When the template accepts with Maybe, the Yes came from a genuine non-template match and is trusted. - Keeps the bug-13190 fix intact (there the template member accepts eagerly with Yes, so the return-type mismatch is still reported) while restoring acceptance for the reported argument- and return-position cases.
There was a problem hiding this comment.
Type like this is always trouble T|(Closure(static): T)
Basically the inference has to decide what T is and how greedy it should be
Because if you pass a Closure in there you need to decide whether the closure is T or whether the return type is T
Because the closure can be either T or Closure(static): T
Both sides of the union
The signatures are really problematic in this case. The other parameter type is: non-empty-string|class-string<T>
So a class-string is also a non-empty-string. PHPStan’s UnionType has to kind of decide what route to take
Same for T|array<T> for exampl
I think there’s some code to handle this but I don’t know how robust it is
The test cases should also verify what happens when you call cloneWithService with various inputs
I think this is a similar problem a1b7b38
we need tests which contain assertTypes to show the whole picture.
…h, reject mismatch) Extends the regression fixture to call offsetSet with concrete arguments on a HelloWorld<DateTimeInterface> instance, confirming the fix keeps genuine acceptance/rejection intact: a closure returning the concrete T is accepted, while a closure returning an unrelated type is still reported. Guards against the fix becoming over-permissive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I've addressed the review. Here's a summary of what I did. Response to @staabm's reviewYou're right that What I verified against the actual analyzer (throwaway probes, not committed):
So the union-inference machinery isn't disturbed by the fix — those cases were already correct and remain correct. What I committed — extended
The Regression guard confirmed: reverting the fix makes the union-passthrough cases ( Checks: The commit |
staabm
left a comment
There was a problem hiding this comment.
What I verified against the actual analyzer (throwaway probes, not committed):
* **`T|array<T>` → `T` param**: correctly _errors_ (`array<T>` is genuinely not `T`). Not over-accepted. * **`non-empty-string|class-string<T>`**: passing to a `class-string<T>` param correctly _errors_; passing to a `non-empty-string` param correctly _does not_ (a `class-string` is a `non-empty-string`). Both routes behave sensibly.
add these tests to the PR
… bug-14893 Adds the reviewer-requested probe cases as committed tests: - T|array<T> passed to a T param is still rejected (array<T> is not T). - non-empty-string|class-string<T> is rejected by a class-string<T> param but accepted by a non-empty-string param (a class-string is a non-empty-string). These lock in that the IntersectionType::isAcceptedBy fix stays precise and does not disturb the union-inference machinery. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done. I addressed staabm's review by committing the two probe cases that were previously only verified as throwaway probes. What I changed
Verification
The error messages in the expectations are the exact ones produced by the analyzer, and the Commit |
Summary
Since 2.2.3, passing a value of a union type such as
(Closure(static): T)|(Closure&T of object)to a parameter (or returning it) typedClosure(static): Tstarted producing a spuriousargument.type/return.typeerror. This restores the 2.2.2 behavior (no error) without re-breaking the bug that 2.2.3 fixed.Changes
src/Type/IntersectionType.php: inisAcceptedBy(), the block added bybad7874ecdistrusted lazyMaxMin's eager Yes whenever the intersection contained anyTemplateTypemember and the holisticisSuperTypeOfwas Maybe. The condition now additionally requires that theTemplateTypemember itself accepts with Yes before distrusting the result.tests/PHPStan/Rules/Methods/data/bug-14893.php: regression fixture reproducing the issue in both argument position (offsetSetcall) and return position (bar).tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php:testBug14893covers the argument-position case.tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php:testBug14893covers the analogous return-position case (also a regression frombad7874ec).Root cause
IntersectionType::isAcceptedBy()computes acceptance as the max over its members (A & B ⊆ Cholds ifA ⊆ CorB ⊆ C). ATemplateTypemember accepts eagerly (inference-mode Yes) even when it is not a genuine subtype, so lazyMaxMin's Yes can be untrustworthy. Commitbad7874ecaddressed this for bug 13190 by distrusting the Yes whenever aTemplateTypemember was present and the holisticisSuperTypeOfwas Maybe.That was too coarse. In the reported case the intersection
Closure & T of objectis checked againstClosure(static): T:Closuremember is accepted with Yes (a plainClosureobject is accepted at the object level by a closure-signature type),T of objectmember is accepted only with Maybe.So the overall Yes came from the genuine non-template match, not from the template's eager acceptance - yet the old condition still threw it away. The distinguishing signal is the template member's own acceptance: in bug 13190 the template accepts with Yes (eager, untrustworthy); here it accepts with Maybe. The fix keys off exactly that.
Test
CallMethodsRuleTest::testBug14893- argument position ($this->offsetSet(0, $value)); fails before the fix withParameter #2 $value of method ...::offsetSet() expects Closure(static): T, (Closure(static): T)|(Closure&T of object) given.Methods/ReturnTypeRuleTest::testBug14893- return position (return $value;); fails before the fix with the analogousshould return Closure(static): T but returns (Closure(static): T)|(Closure&T of object).Functions/ReturnTypeRuleTest::testBug13190andtestBug13190TemplateGenericstill pass, confirming the 2.2.3 fix is preserved.Countable, plainClosure,Iterator&Countable); those never triggered the block (their holisticisSuperTypeOfis Yes, not Maybe) and were already correct, so no extra tests were added for them.Fixes phpstan/phpstan#14893