Skip to content

Commit 655f99b

Browse files
MrSrsenAJenbo
authored andcommitted
Substitute generic return types when the native hint is nullable
`should_override_type_typed` used `unwrap_nullable()`, which only strips the `?Foo` (Nullable) form and not the `Foo|null` (Union-with-null) form. A nullable-union native such as `object|null` therefore reached the union branch with its `null` member attached; since `object` and `null` are both scalar names, the branch judged the type unrefinable and discarded a generic docblock return like `@psalm-return ?T`, leaving the bare native. Use `non_null_type()` (strips null from both Nullable and Union forms) so the non-null part is analysed, matching the function's documented `Foo|null → Foo` intent. Fixes inherited generic returns such as Doctrine's `ServiceEntityRepository<T>::find(): ?T` resolving to `object|null`. Adds an assert_type regression fixture. Signed-off-by: Anders Jenbo <anders@jenbo.dk>
1 parent e8b829e commit 655f99b

3 files changed

Lines changed: 75 additions & 2 deletions

File tree

docs/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
4343
- **`@method` tags override inherited methods of the same name.** A `@method` annotation on a class now takes precedence over a method inherited from a more distant ancestor. The common repository pattern, where a base repository declares `@method Entity|null findOneBy(...)` while its vendor parent returns a generic `object`, now resolves to the concrete entity type, so members accessed on the result are no longer flagged as unverifiable.
4444
- **`??=` keeps the resolved type.** After `$x ??= new Foo()`, the variable resolves to `Foo` (or the union of its existing non-null type and the assigned value), so property and method access on `$x` is no longer reported as unresolvable.
4545
- **Generics with fewer arguments than parameters.** `@extends Collection<User>` against `Collection<TKey, TValue>` now binds `User` to the value parameter, so inherited element types resolve correctly.
46+
- **Nullable generic return types resolve through inheritance.** A method whose native return hint is nullable (`object|null`) and whose docblock returns a template (`@return ?T`) now resolves to the bound type, so a repository's `find()` returns `Entity|null` instead of the bare `object|null`. Contributed by @MrSrsen in https://github.com/PHPantom-dev/phpantom_lsp/pull/152.
4647
- **Conditional `is null` return types** resolve consistently regardless of how the call site is parsed, and an explicitly passed `null` now selects the null branch.
4748
- **Go-to-definition, rename, and highlight accuracy.** References in `@see` tags to qualified names like `App\Foo::bar()` now land on the correct location, and renaming a property selects the whole `$name` instead of `$nam`.
4849
- **`@phpstan-require-extends` and `@phpstan-require-implements` navigation.** Class and interface names in these trait constraint tags now support go-to-definition and hover, and an import used only by such a tag is no longer flagged as unused. Contributed by @calebdw in https://github.com/PHPantom-dev/phpantom_lsp/pull/172.

src/docblock/tags.rs

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1297,8 +1297,19 @@ pub fn should_override_type_typed(docblock_type: &PhpType, native_type: &PhpType
12971297

12981298
// Unwrap nullable wrappers for further analysis. `?Foo` → `Foo`,
12991299
// `Foo|null` → `Foo`. For non-nullable types, use as-is.
1300-
let doc_inner = docblock_type.unwrap_nullable();
1301-
let native_inner = native_type.unwrap_nullable();
1300+
//
1301+
// `non_null_type()` strips nullability from BOTH representations — the
1302+
// `?Foo` (`Nullable`) form and the `Foo|null` (`Union` with a `null`
1303+
// member) form. Plain `unwrap_nullable()` only handled the former, so a
1304+
// nullable-union native such as `object|null` reached the union branch
1305+
// below with its `null` member still attached. Since `object` and `null`
1306+
// are both "scalar names", that branch then judged the whole type
1307+
// unrefinable and discarded a generic docblock return like
1308+
// `@psalm-return ?T`, leaving the bare native (`object|null`).
1309+
let doc_owned = docblock_type.non_null_type();
1310+
let doc_inner = doc_owned.as_ref().unwrap_or(docblock_type);
1311+
let native_owned = native_type.non_null_type();
1312+
let native_inner = native_owned.as_ref().unwrap_or(native_type);
13021313

13031314
// If the docblock type is a bare, unparameterised primitive scalar
13041315
// (`int`, `string`, `bool`, etc.), there's no value in overriding.
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
<?php declare(strict_types = 1);
2+
3+
// Regression: a class-level generic return type whose method has a *nullable*
4+
// native return hint (e.g. `object|null`) must still be resolved through the
5+
// `@extends`/`@psalm-return` template binding, not collapse to the native hint.
6+
//
7+
// This is the Doctrine `ServiceEntityRepository<T>::find(): ?T` shape: before
8+
// the fix, `should_override_type_typed` analysed `object|null` with its `null`
9+
// member attached, judged it unrefinable (both `object` and `null` are "scalar
10+
// names"), and discarded the generic docblock return — so `$repo->find()`
11+
// resolved to `object|null` instead of `Entity|null`.
12+
13+
namespace InheritedGenericReturnNullable;
14+
15+
use function PHPStan\Testing\assertType;
16+
17+
class Entity {}
18+
19+
/**
20+
* @template T of object
21+
*/
22+
class EntityRepository
23+
{
24+
/**
25+
* @return object|null
26+
* @psalm-return ?T
27+
*/
28+
public function find(mixed $id): object|null {}
29+
}
30+
31+
/**
32+
* @template T of object
33+
* @template-extends EntityRepository<T>
34+
*/
35+
class ServiceEntityRepository extends EntityRepository {}
36+
37+
/** @extends ServiceEntityRepository<Entity> */
38+
class EntityRepo extends ServiceEntityRepository {}
39+
40+
/** @extends EntityRepository<Entity> */
41+
class DirectRepo extends EntityRepository {}
42+
43+
/** @template V */
44+
class CollectionNonNull
45+
{
46+
/** @return V */
47+
public function get(): object {}
48+
}
49+
50+
/** @extends CollectionNonNull<Entity> */
51+
class EntityCollection extends CollectionNonNull {}
52+
53+
function t(EntityRepo $multi, DirectRepo $single, EntityCollection $coll): void
54+
{
55+
// Two-level @extends (Doctrine's exact shape): native object|null + @psalm-return ?T
56+
assertType('Entity|null', $multi->find(1));
57+
// Single-level @extends: native object|null + @psalm-return ?T
58+
assertType('Entity|null', $single->find(1));
59+
// Control: non-nullable native object + @return V already worked before the fix
60+
assertType('Entity', $coll->get());
61+
}

0 commit comments

Comments
 (0)