fix: resolve docblock types of anonymous classes against their lexical namespace - #825
Conversation
| if ($reflection instanceof ReflectionClass && $reflection->isAnonymous()) { | ||
| // An anonymous class name carries either no namespace or the | ||
| // extended/implemented type's namespace, not reliably the lexical one. | ||
| $namespace = PhpParser::parseNamespace($reflection); |
There was a problem hiding this comment.
An alternative here would be to always recover the namespace from the source:
$namespace = PhpParser::parseNamespace($reflection);Two problems with that. Named eval()'d classes would regress: their name carries the correct namespace today, but no source is available to parse, so the lookup would come back empty. And it adds file I/O plus tokenization for every named class where none exists today. The isAnonymous() check limits the new path to the only case where the reflection name is unreliable.
This invariant is pinned by test_named_class_declared_without_source_file_can_use_type_from_its_own_namespace, added after an Infection mutant escaped by breaking exactly this case.
| if ($reflection instanceof ReflectionFunction || $reflection->isAnonymous()) { | ||
| // An anonymous class name carries either no namespace or the | ||
| // extended/implemented type's namespace, not reliably the lexical one. | ||
| $namespaceName = $tokenParser->getNamespace(); |
There was a problem hiding this comment.
An alternative here would be to derive the namespace from the token stream for all reflections, named classes included, since the content is already tokenized at this point. The whole conditional (including the ReflectionMethod reassignment above) would collapse to:
$namespaceName = $tokenParser->getNamespace();Both variants pass the full test suite (2708 tests) on this branch. The uniform variant stops relying on reflection names entirely, but it changes the code path for every named class to fix a bug that only affects anonymous ones, and it widens the mutation-testing surface under --min-msi=100. The first CI round of this PR demonstrated that mechanism: a LogicalAnd mutant escaped on the sibling condition until a dedicated test pinned it. Cost is one extra token walk per class-definition parse; the content is already read and tokenized, and the result is cached per file/line.
The PR keeps the isAnonymous() branch so named classes stay byte-identical. Switching to the uniform variant is a small change if preferred.
319c06f to
535b84b
Compare
|
AI (SLOP) Disclaimer, i'm currently building a gated, harnessed llm loop bug scout, so this is also just an experiment i took on a quick glance because @Ocramius mentioned this bug. Feel free to just toss it into the bin, no hard feelings :) |
535b84b to
bbf1ada
Compare
Note: only when imported types are used.
Why the old code was problematic: the namespace used to resolve short type names in docblocks was derived from the class name (`ReflectionClass::getNamespaceName()` / `inNamespace()`). The name of an anonymous class is a synthetic value that carries either no namespace, or — when the class extends or implements another type — that type's namespace, never the namespace the docblocks were lexically written in. Both resolution layers therefore failed: the `use` statements table came back empty, and the same-namespace fallback was never entered (or qualified symbols against the wrong namespace). Trigger scenario: mapping to an anonymous class whose constructor parameters or properties reference types by their short (imported or same-namespace) name in a docblock, e.g. `@param DummyPaginable<User>`; also closures scoped inside anonymous classes. New behaviour: for anonymous classes the lexical namespace is recovered from the tokenized source file — the same mechanism already used for functions. Named classes keep the exact previous behaviour, including named classes declared via `eval()` (no source available: short names remain unresolved as before, fully-qualified names keep working). `PhpParser::parseNamespace()` is widened to accept classes and now memoizes per file/line, mirroring `parseUseStatements()`.
bbf1ada to
1d1599b
Compare
3 participants
Builds on #824. Its reproduction test is the first commit here, authorship preserved.
Root cause
The namespace used to resolve short docblock type names comes from the class name (
ReflectionClass::getNamespaceName()/inNamespace()). For an anonymous class that name is synthetic. It carries no namespace at all, or the namespace of the extended/implemented type. It does not reliably carry the lexical namespace of the file the class was written in.This breaks both resolution layers. The
usestatements table comes back empty, becauseTokenParser::parseUseStatements()never matches the file's namespace block, so imported symbols fail. And the same-namespace fallback is never entered; for parent-prefixed synthetic names, resolution is attempted against the parent type's namespace instead.A partial-fix experiment separates the two: with only the second layer fixed, the original tests of this PR turn green while a test using an imported symbol stays red. Each layer therefore has its own regression test in this PR.
The fix
For anonymous classes, the lexical namespace is recovered from the tokenized source, behind an
isAnonymous()check. Closures already worked this way, and they are anonymous in the same sense, so both now share one code path. Named classes and named functions keep byte-identical behaviour. That includes namedeval()'d classes: no source is available for them, but their name still carries the namespace, and a dedicated test pins that.PhpParser::parseNamespace()now accepts classes and memoizes per file/line, mirroringparseUseStatements().BC note
For anonymous classes extending or implementing a namespaced type, short-name resolution was previously attempted against the parent type's namespace, and could only succeed where a class of the same name exists there. It now resolves against the declaring file's namespace, which is how PHP itself resolves type hints at the declaration site.
Pre-existing issues noticed while testing (out of scope)
eval()'d anonymous class cannot be mapped by its class name at all; its synthetic name does not survive the type-signature parser (InvalidMappingTypeSignature).eval()'d closure with an unresolvable docblock type does not round-trip through the filesystem cache (CorruptedCompiledPhpCacheFile). Whether the trigger is the missing source or the unresolvable type was not isolated.Validation
masterwithout thesrc/changes; the FQCN control passes.eval()'d classes must keep resolving through their name.test_named_class_declared_without_source_file_can_use_type_from_its_own_namespacecovers that now.