Skip to content

Collapse ObjectShapeType against HasPropertyType regardless of member order - #6184

Open
SanderMuller wants to merge 2 commits into
phpstan:2.2.xfrom
SanderMuller:fix-intersection-hasproperty-order
Open

Collapse ObjectShapeType against HasPropertyType regardless of member order#6184
SanderMuller wants to merge 2 commits into
phpstan:2.2.xfrom
SanderMuller:fix-intersection-hasproperty-order

Conversation

@SanderMuller

Copy link
Copy Markdown
Contributor

When a parameter is typed \stdClass&object{u?:int} and its value goes through isset()/?? narrowing, PHPStan should resolve the optional key to its declared type. Since 2.2.6 it fell back to mixed instead:

abstract class AbstractJsonRepresentation
{
    /** @param \stdClass $data */
    abstract protected static function fromObjectInternal(\stdClass $data): self;
}

final class MissalYearLimits extends AbstractJsonRepresentation
{
    private ?int $until_year;

    /** @param \stdClass&object{since_year:int,until_year?:int} $data */
    protected static function fromObjectInternal(\stdClass $data): self
    {
        // 2.2.5: int|null   2.2.6+: mixed  -> "argument.type" false positive at level 10
        return new self($data->until_year ?? null);
    }
}

Root cause

isset($data->until_year) narrows $data by intersecting it with a HasPropertyType for the key, and TypeCombinator::intersect() is supposed to collapse object{until_year?:int} & hasProperty(until_year) into object{until_year:int}, so the read is int.

That collapse lived in the pairwise reduction loop, reached only after the generic supertype dedup. The parameter also carries stdClass, a dynamic-property class that reports every property as present, so hasProperty(until_year) is a supertype of it and the dedup removes the HasPropertyType as redundant against stdClass. Whether the dedup drops it first or the object shape collapses it first depends on the order of the members in the intersection.

While intersection members were still sorted in place, describing the type happened to reorder them so the object shape came first and won the race. Once that in-place mutation was removed (correctly, so a type's value no longer depends on what has been called on it) the construction order took over, stdClass came first, and its mixed dominated.

Note the bare read $data->until_year without isset/?? was already mixed before and after 2.2.6. That is a separate, pre-existing gap and not what this fixes; this change is about the isset/?? narrowing path only.

Fix

Move the object-shape / HasPropertyType collapse into its own pass before the reduction loop, so member order no longer decides the outcome. Two details keep it behaviour-preserving otherwise:

  • It only fires when the shape has, or may have, the key (hasInstanceProperty(...) is not no). A sealed shape intersected with a HasPropertyType for a key it cannot have is left alone, so it still reduces to never in the loop below, as it did before.
  • The pass is gated on a HasPropertyType being present in the intersection, a flag computed in the existing flatten loop, so an intersection without one pays only that flag check and never enters the extra loop.

The array analogue (ConstantArrayType & HasOffsetType) is not affected: there is no universal-offset type that reports every offset as present, so nothing absorbs the HasOffsetType before the offset is made required. Verified against real 2.2.5/2.2.6 builds.

Closes phpstan/phpstan#15047

*/
protected static function fromObjectInternal(stdClass $data): self
{
assertType('object{since_year: int, until_year?: int}&stdClass', $data);

@staabm staabm Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't object{...} already imply stdClass? can't we just drop stdClass from the intersection when building the type for the phpdoc as it is already redundant?

skip that. I missremembered

… order

An intersection of an object shape carrying an optional key with a
HasPropertyType for that key - as produced by isset()/?? narrowing, e.g.
`stdClass&object{u?:int}` narrowed by `isset($x->u)` - is supposed to resolve
the optional key to its declared type, so that `$x->u ?? null` is `int|null`.

That relied on member order. The collapse
`ObjectShapeType & HasPropertyType -> makePropertyRequired()` sat in the
reduction loop, reached only after the generic supertype dedup. When the
intersection also contains a dynamic-property class such as stdClass, which
reports every property as present, HasPropertyType is a supertype of it and the
dedup splices HasPropertyType out before it is ever paired with the object
shape. Which of the two fires first depends on the order of the members, so the
optional key stayed optional whenever stdClass happened to come first and the
read fell back to the class's mixed. While intersection members were still
sorted in place this was masked - describing the type reordered them so the
shape came first; once that mutation was removed the construction order won.

Move the collapse into its own pass before the reduction loop so member order
no longer decides the result. Guard it with hasInstanceProperty(): when the
shape does not have the key it is left untouched, so a sealed shape intersected
with a HasPropertyType for a key it cannot have still reduces to never in the
loop below, as before.

The array analogue (ConstantArrayType & HasOffsetType) is unaffected: there is
no universal-offset crate reporting every offset as present, so nothing absorbs
the HasOffsetType before the offset is made required.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@staabm
staabm force-pushed the fix-intersection-hasproperty-order branch from 4372b52 to fb4d864 Compare August 10, 2026 14:08

@staabm staabm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the initial issue is about a rule error false positive. we need a rule test showing such error is no longer emitted

The nsrt test covers the inferred type; this asserts the reported error is
gone at the level that emitted it. The false positive only appears with
checkImplicitMixed (level 10), so InstantiationRuleTest gains the same
toggle the other rule tests already expose.
@SanderMuller

Copy link
Copy Markdown
Contributor Author

Good call, added in 29b5e27.

tests/PHPStan/Rules/Classes/data/bug-15047.php plus InstantiationRuleTest::testBug15047(), expecting no errors. The message that used to be emitted is the one from the issue:

Parameter #1 $untilYear of class Bug15047Instantiation\MissalYearLimits constructor expects int|null, mixed given.

It comes from InstantiationRule, hence that test class. Checked both directions: the test passes on this branch and fails on the parent commit with exactly that error, so it does pin the false positive rather than just sitting there.

One wrinkle worth mentioning. The error only appears with checkImplicitMixed, so at level 10 and not at 8 or 9, and InstantiationRuleTest hardcoded checkImplicitMixed: false. Without wiring that up the new test would have passed on the parent commit too and proved nothing. It now has the same $checkImplicitMixed toggle that CallMethodsRuleTest, CallStaticMethodsRuleTest and a few others already expose, defaulted to false so the existing cases are untouched.

Gates: full suite green (21304 tests), self-analysis clean, phpcs clean, InstantiationRuleTest green (47 tests). I also measured the TypeCombinator::intersect change on a doctrine/symfony codebase, three interleaved rounds against the fix's parent commit rather than the merge base, since the branch sits on top of #6203, #6205 and #6206 and comparing against the merge base would have credited their gains here: 138.8s against 139.1s median, with an 8.8s spread inside a single build, so no measurable cost.

@SanderMuller
SanderMuller requested a review from staabm August 10, 2026 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression (2.2.6+): overriding method's narrower @param object shape is ignored when the abstract parent declares a wider @param

2 participants