Fix phpstan/phpstan#13227: Intersections using object shapes#5152
Fix phpstan/phpstan#13227: Intersections using object shapes#5152phpstan-bot wants to merge 7 commits intophpstan:2.1.xfrom
Conversation
VincentLanglet
left a comment
There was a problem hiding this comment.
Need to be clarified first
phpstan/phpstan#13227 (comment)
- Added ObjectShapeType merging logic in TypeCombinator::intersect() so that two object shapes can be intersected by combining their properties - Properties present in both shapes have their types intersected; if the intersection is never, the whole result is never - A property is optional in the result only if it is optional in both shapes - Updated TypeCombinatorTest expectations and baseline for new instanceof count - New regression test in tests/PHPStan/Analyser/nsrt/bug-13227.php
b073731 to
bc8479f
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
Add test in TypeCombinatorTest for
object{foo: int}&object{foo?: int}object{foo: int}&object{foo: int|null}object{foo: int}&object{foo?: int|null}object{foo: int}&object{bar?: int}object{foo: int}&object{foo?: string}
Tests for: object{foo: int}&object{foo?: int}, object{foo: int}&object{foo: int|null},
object{foo: int}&object{foo?: int|null}, object{foo: int}&object{bar?: int},
and object{foo: int}&object{foo?: string}.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The |
staabm
left a comment
There was a problem hiding this comment.
should have a review from ondrej
|
I see 👍 |
ondrejmirtes
left a comment
There was a problem hiding this comment.
Three more things I'd like you to try:
- Try to make each if/else block empty in the src/ diff change. Each one of them should make at least one TypeCombinatorTest fail.
- I'd like to see what ObjectShapeType answers in
isSuperTypeOfabout other instances. Typically I testisSuperTypeOfimplementations/changes through TypeCombinatorTest, but here you're hardcoding behaviour into TypeCombinator, so I'd like to see what happens in isSuperTypeOf separately. I'll explain it below. - You added a bunch of dataIntersect tests. What happens in dataUnion? What would make sense to happen?
So, about 2). For union, the rule is - type wins in normalization if it says isSuperTypeOf yes() about the other type. So mixed|int results in mixed.
For intersection, the rule is - type wins, if it says isSuperTypeOf maybe() about the other type (and the other type says yes()). So mixed&int results in int.
If isSuperTypeOf() answers no(), the result of intersection is NeverType.
I'd like you to look at the tests you wrote and verify that isSuperTypeOf answers according to the results you expect from TypeCombinator. Of course in some cases you're combining properties from two shapes into one shape so you can't say which shape "wins" in "isSuperTypeOf". In that case it's okay to return maybe() from both sides.
…Type comparisons Object shapes are open structural types - not listing a property doesn't mean the runtime object won't have it. When comparing two ObjectShapeTypes, a missing property should return maybe() instead of no() (for required) or yes() via continue (for optional), since the actual object could have the property with any type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, and union tests - Create ObjectShapeTypeTest with isSuperTypeOf tests covering: same types, wider/narrower types, incompatible types, disjoint properties, required vs optional, empty shapes, and subset/superset relationships - Add dataIntersect tests with optional-in-i ordering to cover the isOptionalInI && !isOptionalInJ branch in TypeCombinator - Add dataUnion tests for object shapes: optional absorbs required, wider type absorbs narrower, disjoint properties stay as union, subset absorbs superset Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
left a comment
There was a problem hiding this comment.
If intersection of Two objectShapes works, the Intersection of
new ObjectShapeType(['foo' => new IntegerType()], []),
new HasPropertyType('bar'),
should not gives NEVER but object{foo: int, bar: mixed}
When intersecting an object shape with HasPropertyType, if the property doesn't exist in the shape, add it as mixed. If it exists, make it required. This is handled before isSuperTypeOf checks, similar to the ObjectShapeType-ObjectShapeType intersection. Also fix the NeverType check for overlapping properties with incompatible types: only skip if the property is optional in both shapes (not just one). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The background |
|
I processed this review but have nothing to report. |

Summary
Object shapes (
object{ a: int }) could not be intersected with each other, causing@phpstan-typealiases likeType1 & object{ b: int }to report "Type alias contains unresolvable type." This PR adds support for merging object shapes in intersections.Changes
src/Type/TypeCombinator.php(placed beforeisSuperTypeOfchecks to handle all cases)NeverType, the entire intersection returnsNeverTypeksort) for deterministic output regardless of operand ordertests/PHPStan/Type/TypeCombinatorTest.phpto expect merged object shape instead ofNeverTypefor disjoint-property intersectionsphpstan-baseline.neonto reflect additionalinstanceof ObjectShapeTypeusagesRoot cause
TypeCombinator::intersect()had no special handling for twoObjectShapeTypeinstances. When two object shapes with different properties were intersected (e.g.,object{a: int} & object{b: int}),ObjectShapeType::isSuperTypeOf()returnednobecause each shape was missing properties from the other. This causedTypeCombinatorto return an implicitNeverType, which the type alias validator flagged as unresolvable.Test
Added
tests/PHPStan/Analyser/nsrt/bug-13227.phpwith:Type1 & object{ b: int }where Type1 isobject{ a: int }— verifies merge intoobject{a: int, b: int}object{ a: int, b?: string } & object{ b: string, c?: int }— verifies optional property handling (b becomes required, c stays optional)Fixes phpstan/phpstan#13227