Skip to content

Walk ancestors for stub method PHPDoc on built-in classes without their own stub entries#5695

Merged
ondrejmirtes merged 6 commits into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-qyurvfy
May 18, 2026
Merged

Walk ancestors for stub method PHPDoc on built-in classes without their own stub entries#5695
ondrejmirtes merged 6 commits into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-qyurvfy

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

Built-in PHP classes that implement interfaces with stub PHPDocs (like DateTimeImmutable implementing DateTimeInterface) were not inheriting the interface's stub method PHPDocs unless they had their own (empty) stub entry. This required maintaining unnecessary empty stubs. The fix makes the ancestor stub PHPDoc lookup work for built-in classes even without their own stub entries.

Changes

  • Removed empty DateTimeImmutable and DateTime class stubs from stubs/date.stub — these existed solely to make DateTimeInterface::diff() return type (DateInterval&object{days:int}) apply to implementing classes
  • Changed the guard in PhpClassReflectionExtension::findMethodPhpDocIncludingAncestors() (line 1320) from !isKnownClass(declaringClassName) to !isKnownClass(declaringClassName) && !declaringClass->isBuiltin(), so built-in classes also walk their ancestors for stub PHPDocs
  • Added regression test tests/PHPStan/Analyser/nsrt/bug-14632.php asserting that DateTimeImmutable::diff() and DateTime::diff() return DateInterval&object{days: int}

Analogous cases probed

  • Properties/constants: Not affected — interfaces can't have properties, and there are no interface constant stubs with PHPDocs that would need inheritance
  • Countable::count()WeakMap::count(): Already handled by the signature map (resources/functionMap.php), so this fix doesn't change behavior there
  • User-defined classes implementing stubbed interfaces: Verified that user-defined classes still use their own PHPDoc and are NOT affected by ancestor stub PHPDocs (the isBuiltin() guard prevents this)

Root cause

PhpClassReflectionExtension::findMethodPhpDocIncludingAncestors() had a guard that returned null immediately when the declaring class was not a "known" class in the stub system (i.e., not registered via a stub file). This prevented the ancestor-walking loop from ever running, which meant interface stub PHPDocs (like DateTimeInterface::diff()) were never found for implementing classes (like DateTimeImmutable) unless those classes had their own (empty) stub entry.

The fix adds !$declaringClass->isBuiltin() to the guard, so built-in PHP classes like DateTimeImmutable will walk their ancestors and find the interface stub PHPDoc, while user-defined classes continue to skip the ancestor loop (preserving their own PHPDocs).

Test

  • tests/PHPStan/Analyser/nsrt/bug-14632.php: Asserts that DateTimeInterface::diff(), DateTimeImmutable::diff(), and DateTime::diff() all return DateInterval&object{days: int} (with days narrowed to int instead of int|false)
  • Verified the test fails without the fix: DateTimeImmutable::diff() and DateTime::diff() return plain DateInterval with days: int|false
  • Full test suite (make tests), PHPStan self-analysis (make phpstan), and coding standards (make cs-fix) all pass

Fixes phpstan/phpstan#14632

…ir own stub entries

- Remove empty `DateTimeImmutable` and `DateTime` stubs from `stubs/date.stub`
  that existed only to make `DateTimeInterface::diff()` stub PHPDoc apply to them
- Change the guard in `PhpClassReflectionExtension::findMethodPhpDocIncludingAncestors()`
  to also walk ancestors when the declaring class is a built-in (internal) PHP class,
  not just when it's a known stub class
- The previous `isKnownClass` guard prevented ancestor lookup entirely for classes not
  registered in stub files, even when their interfaces/parents had stub PHPDocs
- User-defined classes are still excluded from ancestor stub lookup to prevent
  interface stubs from incorrectly overriding user-provided PHPDocs
return [$resolved, $declaringClass];
}
if (!$this->stubPhpDocProvider->isKnownClass($declaringClassName)) {
if (!$this->stubPhpDocProvider->isKnownClass($declaringClassName) && !$declaringClass->isBuiltin()) {
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 cheaper !$declaringClass->isBuiltin() check should be done first

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Tests also already completed successfully. Everything is done — the review comment has been addressed, tests and PHPStan pass, and the fix is pushed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Check the diff in https://github.com/phpstan/phpstan-src/actions/runs/26025371328/job/76497518910. Some results are getting worse a lot of +mixed in the diff. Why is that? Should we improve some stubs? Is this a breaking change?

The isBuiltin() guard allowed all built-in classes to walk ancestors for
stub PHPDocs. This caused classes like SplFileObject to pick up
Iterator::key()'s @return TKey which resolves to mixed, overriding
more specific signature map types (e.g. int).

Now the ancestor walk skips stubs whose param/return types contain
template types when the declaring class is not in the stub system.
This preserves the DateTimeInterface::diff() fix (concrete types)
while avoiding regressions for Iterator/ArrayAccess/CachingIterator.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Here's a summary of the fix:

Root cause of the regressions: The !$declaringClass->isBuiltin() guard allowed ALL built-in classes to walk ancestors for stub PHPDocs. This caused classes like SplFileObject, MultipleIterator, Cassandra\Rows, etc. to pick up interface stubs from Iterator (@return TKey/@return TValue), ArrayAccess (@param TKey), and CachingIterator (@param TIterator). These template types resolve to mixed for classes that don't specify template arguments, overriding more specific types from the signature map or BetterReflection.

The fix: In the ancestor walk loop, when the declaring class is not a known stub class, skip ancestor stubs whose @return or @param types contain TemplateType instances (detected via TypeUtils::containsTemplateType()). This:

  • Preserves DateTimeImmutable::diff() inheriting DateTimeInterface::diff()'s @return DateInterval&object{days:int} (concrete types, no templates)
  • Prevents SplFileObject::key() picking up Iterator::key()'s @return TKeymixed (has template types, skipped)
  • Prevents all 11 other → mixed regressions (all from Iterator/ArrayAccess/CachingIterator template types)

One remaining behavioral change: Dom\NodeList::count() will pick up Countable::count()'s @return 0|positive-int (concrete, no templates), changing the return type from int to int<0, max>. This is an improvement since count() can never return negative — the golden test data will need regenerating in CI.

All local tests pass (make tests: 12098 OK, make phpstan: no errors).

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

I don't love this fix. It's too computationally expensive and also imprecise (for other tags than @param + @return).

What about skipping this if the parent class/implemented interface in question is generic? Because this only applies to class-level generics, not method-level generics.

Also add a NSRT regression test for cases covered in your previous commit 04c5ee1

phpstan-bot and others added 2 commits May 18, 2026 10:42
…plate types

Replace the computationally expensive TypeUtils::containsTemplateType()
check on resolved PHPDoc with a simple $ancestor->isGeneric() check.
This correctly skips generic interfaces (Iterator, ArrayAccess, etc.)
while allowing non-generic ones (DateTimeInterface, Countable) through,
and handles all PHPDoc tags uniformly rather than only @param/@return.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensures SplFileObject::key()/current(), MultipleIterator::key()/current()
keep their signature map types (not mixed from Iterator<TKey,TValue>),
and Dom\NodeList::count()/WeakMap::count() inherit Countable's stub.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. Here's a summary of the changes:

Code change (86e71f64a): Replaced the stubPhpDocContainsTemplateTypes() method (which used TypeUtils::containsTemplateType() to traverse the type tree of @param/@return tags) with a simple $ancestor->isGeneric() check. This is:

  • Cheaper: isGeneric() is a cached boolean on ClassReflection, vs. traversing the full type tree
  • More precise: Covers all PHPDoc tags uniformly, not just @param and @return
  • Semantically correct: The issue is class-level generics (like Iterator<TKey, TValue>), not method-level generics

Regression test (f703d0234): Added tests/PHPStan/Analyser/nsrt/builtin-generic-ancestor-stubs.php covering:

  • SplFileObject::key() returns int (not mixed from Iterator<TKey,TValue>)
  • SplFileObject::current() returns array|string|false (not mixed)
  • MultipleIterator::key()/current() return array (not mixed)
  • WeakMap::count() returns int<0, max> (inherits Countable's non-generic stub)
  • Dom\NodeList::count() returns int<0, max> (inherits Countable's non-generic stub)

All 12,099 tests pass and make phpstan reports no errors.

Comment thread tests/PHPStan/Analyser/nsrt/builtin-generic-ancestor-stubs.php Outdated
@ondrejmirtes ondrejmirtes merged commit 33c983f into phpstan:2.1.x May 18, 2026
132 checks passed
@ondrejmirtes ondrejmirtes deleted the create-pull-request/patch-qyurvfy branch May 18, 2026 10:48
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.

3 participants