Fix false-positive type errors on generic class methods via return types

Author: AJenbo

docs/CHANGELOG.md

@@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Exception types in `catch` clauses matched incorrectly across namespaces.** Thrown exception types are now matched against `catch` clause types regardless of whether short names or fully-qualified names are used. The `@throws` tag comparison in docblock update actions now resolves names through the class loader, fixing false "missing @throws" suggestions for root-namespace exceptions (e.g. `RuntimeException`) in namespaced files without an explicit `use` import.
 - **False "undefined variable" diagnostics for by-reference parameters in inherited static methods and constructors.** By-reference parameters in parent class static methods and constructors are now correctly resolved when called on a child class, preventing false diagnostics for variables defined via the call. Class lookup for by-reference resolution now tries the fully-qualified name first, preventing incorrect matches when multiple classes share a short name across namespaces.
 - **Nested `match(true)` expressions.** Multiple `match(true)` expressions sharing narrowed variable names no longer produce incorrect diagnostics.
+- **False-positive type errors on generic class methods resolved through return types.** When a method returns a generic class (e.g. `@return HasMany<Translation, Tag>`), calling a method on the result whose parameter references a class-level template parameter (e.g. `@param TRelatedModel $model`) no longer produces a spurious "expects TRelatedModel, got Translation" diagnostic. The return type's generic arguments are now preserved and used for template substitution.
 - **Variables prefixed with `$this` receiving stale type information.** Variables like `$thisItem` or `$thisValue` no longer receive stale type information from `$this`-related narrowing.
 - **False-positive type errors on generic class methods and templated static methods.** Class-level `@template` parameters (e.g. `Collection<User>`) are now substituted into method parameter types before checking argument compatibility. Method-level `@template` parameters (e.g. PHPUnit's `assertSame`) are resolved from call-site argument types, covering literals, variables, enum cases, property accesses, and method call return types.
 

docs/todo/bugs.md

@@ -133,22 +133,32 @@ foreach ($regexes as $type => $regex) {
 }
 ```
 
-## B5 — Unresolved function-level template return type leaks through to argument diagnostics
+## B5 — Unresolved template parameters leak raw names into argument diagnostics
 
-When a function declares `@template TReduceReturnType` and
-`@return TReduceReturnType` but no argument binds the template
-parameter, the return type resolves to the raw template name
-`TReduceReturnType` instead of `mixed` or remaining unresolved.
-When the result is passed to a function expecting a concrete type
-(e.g. `takes_int(int $x)`), the type error checker sees
-"TReduceReturnType vs int" and fires a false positive diagnostic.
+When a template parameter is not bound to a concrete type (either
+because no argument carries the binding, or because the class was
+instantiated without a generic annotation), the raw template name
+(e.g. `TValue`, `TKey`, `TReduceReturnType`, `TClosure`) leaks
+through to the type error checker. The diagnostic then reports
+"expects TValue, got string" instead of recognising the parameter
+as unresolved and suppressing the check.
 
-Template substitution should either resolve the return type from
-the call-site arguments or fall back to the template's bound
-(defaulting to `mixed`) so the raw name never leaks through to
-downstream diagnostics.
+This affects both function-level and class-level templates:
 
-Reproducer:
+- **Function-level:** `@template TReduceReturnType` with
+  `@return TReduceReturnType` where no argument binds the param.
+- **Class-level:** `Collection<TKey, TValue>` where the Collection
+  was created without a generic annotation (e.g. `collect([])`,
+  `new Collection()`). Methods like `push($item)` still have
+  `@param TValue $item` with the raw template name, so passing
+  a `string` fires "expects TValue, got string".
+
+Template substitution should either resolve the parameter from the
+call-site arguments / class-level generic annotation, or fall back
+to the template's bound (defaulting to `mixed`) so the raw name
+never leaks through to downstream diagnostics.
+
+Reproducer (function-level):
 
 ```php
 /**
@@ -165,45 +175,20 @@ function test(): void {
 }
 ```
 
-## B6 — Class-level template substitution doesn't propagate to parameter types in type error checker
-
-When a generic class like `HasMany<TRelatedModel, TDeclaringModel>`
-has a method with `@param TRelatedModel $model`, and the class is
-instantiated with concrete type arguments (e.g.
-`HasMany<Translation, Tag>`), the type error checker does not
-substitute `TRelatedModel → Translation` in the parameter type.
-The diagnostic sees "expects TRelatedModel, got Translation" and
-fires a false positive.
-
-The completion/hover pipeline already performs class-level template
-substitution via `build_substitution_map` in `inheritance.rs`, but
-the type error diagnostic collector does not apply the same
-substitution when comparing argument types against parameter types
-on generic class methods.
-
-Reproducer:
+Reproducer (class-level):
 
 ```php
 /**
- * @template TRelatedModel
- * @template TDeclaringModel
+ * @template TValue
  */
-class HasMany {
-    /** @param TRelatedModel $model */
-    public function save($model): void {}
-}
-
-class Translation {}
-class Tag {
-    /** @return HasMany<Translation, Tag> */
-    public function translations(): HasMany { return new HasMany(); }
+class Collection {
+    /** @param TValue $item */
+    public function push($item): void {}
 }
 
 function test(): void {
-    $tag = new Tag();
-    $translation = new Translation();
-    $tag->translations()->save($translation);
-    // false positive: "expects TRelatedModel, got Translation"
+    $items = new Collection();   // no generic annotation
+    $items->push('hello');       // false positive: "expects TValue, got string"
 }
 ```
 
@@ -242,3 +227,56 @@ class FooTest extends TestCase {
 }
 ```
 
+## B8 — Class-level template parameters lost through chained method calls
+
+When a method returns a generic class (e.g. `Collection<Product>`)
+and the next method in the chain returns `self<TItem>` or another
+type referencing a class-level template parameter, the template
+substitution is lost because `resolve_call_return_types_expr`
+converts intermediate `ResolvedType`s (which carry generic args) to
+`Vec<Arc<ClassInfo>>` via `into_arced_classes`, discarding the
+`type_string` field that holds the generic parameters.
+
+The first call in the chain now correctly propagates generics (B6
+fix), but the second call resolves the base through
+`resolve_call_return_types_expr` → `MethodCall` →
+`into_arced_classes`, which strips the generic info before the
+method's return type can be template-substituted.
+
+Fixing this requires threading `ResolvedType` (with its
+`type_string`) through the `MethodCall` arm of
+`resolve_call_return_types_expr` so that class-level template
+arguments survive into the method return-type resolution step.
+
+Reproducer:
+
+```php
+/**
+ * @template TItem
+ */
+class Collection {
+    /** @param TItem $item */
+    public function add($item): void {}
+
+    /** @return self<TItem> */
+    public function filter(): self { return $this; }
+}
+
+class Product {}
+
+class Store {
+    /** @return Collection<Product> */
+    public function products(): Collection { return new Collection(); }
+}
+
+function test(): void {
+    $store = new Store();
+    $product = new Product();
+    // First level works (B6 fix): $store->products()->add($product)
+    // Second level fails: $store->products()->filter()->add($product)
+    // false positive: "expects TItem, got Product"
+    $store->products()->filter()->add($product);
+}
+```
+
+

src/completion/call_resolution.rs

@@ -500,10 +500,17 @@ impl Backend {
     /// [`SubjectExpr::StaticMethodCall`], [`SubjectExpr::FunctionCall`],
     /// [`SubjectExpr::Variable`], or [`SubjectExpr::NewExpr`].
     /// Any other variant falls through to `resolve_target_classes_expr`.
-    pub(crate) fn resolve_call_return_types_expr(
+    ///
+    /// Like [`resolve_call_return_types_expr`](Self::resolve_call_return_types_expr)
+    /// but also captures the raw return type hint (before class resolution)
+    /// into `return_type_hint_out` when provided.  This preserves generic
+    /// type parameters (e.g. `HasMany<Translation, Tag>`) that would
+    /// otherwise be lost when converting to `Vec<Arc<ClassInfo>>`.
+    pub(crate) fn resolve_call_return_types_expr_with_hint(
         callee: &SubjectExpr,
         text_args: &str,
         ctx: &ResolutionCtx<'_>,
+        mut return_type_hint_out: Option<&mut Option<PhpType>>,
     ) -> Vec<Arc<ClassInfo>> {
         match callee {
             // ── Instance method call: base->method(…) ───────────────
@@ -515,8 +522,37 @@ impl Backend {
                     super::resolver::resolve_target_classes_expr(base, AccessKind::Arrow, ctx),
                 );
 
+                // Capture the raw return type hint while we iterate
+                // the owner classes below.  We grab it from the first
+                // owner that has a matching method — before the return
+                // type gets flattened into ClassInfo.
+                let mut hint_captured = false;
                 let mut results = Vec::new();
+
                 for owner in &lhs_classes {
+                    // Capture the return type hint from the first owner
+                    // that has the method, before class resolution loses
+                    // generic parameters.  The resolve_class_fully call
+                    // is cached, so this doesn't duplicate work done by
+                    // resolve_method_return_types_with_args below.
+                    if !hint_captured && let Some(ref mut hint_out) = return_type_hint_out {
+                        let merged = crate::virtual_members::resolve_class_fully_maybe_cached(
+                            owner,
+                            ctx.class_loader,
+                            ctx.resolved_class_cache,
+                        );
+                        if let Some(m) = merged
+                            .methods
+                            .iter()
+                            .find(|m| m.name.eq_ignore_ascii_case(method_name))
+                        {
+                            if let Some(ref ret) = m.return_type {
+                                **hint_out = Some(ret.clone());
+                            }
+                            hint_captured = true;
+                        }
+                    }
+
                     let split_args = split_text_args(text_args);
                     let arg_refs = split_args.to_vec();
                     let template_subs =
@@ -559,6 +595,25 @@ impl Backend {
                 };
 
                 if let Some(ref owner) = owner_class {
+                    // Capture return type hint for static method calls.
+                    // The resolve_class_fully call is cached, so this
+                    // doesn't duplicate work.
+                    if let Some(ref mut hint_out) = return_type_hint_out {
+                        let merged = crate::virtual_members::resolve_class_fully_maybe_cached(
+                            owner,
+                            ctx.class_loader,
+                            ctx.resolved_class_cache,
+                        );
+                        if let Some(m) = merged
+                            .methods
+                            .iter()
+                            .find(|m| m.name.eq_ignore_ascii_case(method_name))
+                            && let Some(ref ret) = m.return_type
+                        {
+                            **hint_out = Some(ret.clone());
+                        }
+                    }
+
                     let split_args = split_text_args(text_args);
                     let arg_refs = split_args.to_vec();
                     let template_subs =
@@ -698,6 +753,10 @@ impl Backend {
                     }
 
                     if let Some(ref ret) = func_info.return_type {
+                        // Capture the function's return type hint.
+                        if let Some(ref mut hint_out) = return_type_hint_out {
+                            **hint_out = Some(ret.clone());
+                        }
                         return super::type_resolution::type_hint_to_classes_typed(
                             ret,
                             "",
@@ -858,6 +917,17 @@ impl Backend {
         }
     }
 
+    /// Thin wrapper around [`resolve_call_return_types_expr_with_hint`]
+    /// that discards the return type hint.  Existing callers that don't
+    /// need generic type preservation use this.
+    pub(crate) fn resolve_call_return_types_expr(
+        callee: &SubjectExpr,
+        text_args: &str,
+        ctx: &ResolutionCtx<'_>,
+    ) -> Vec<Arc<ClassInfo>> {
+        Self::resolve_call_return_types_expr_with_hint(callee, text_args, ctx, None)
+    }
+
     /// Resolve a method call's return type, taking into account PHPStan
     /// conditional return types when `text_args` is provided, and
     /// method-level `@template` substitutions when `template_subs` is

src/completion/resolver.rs

@@ -408,11 +408,37 @@ pub(crate) fn resolve_target_classes_expr(
 
         // ── Call expression ─────────────────────────────────────
         SubjectExpr::CallExpr { callee, args_text } => {
+            // First pass: resolve the call to classes (cheap).
             let classes = Backend::resolve_call_return_types_expr(callee, args_text, ctx);
-            classes
-                .into_iter()
-                .map(|arc| ResolvedType::from_class(Arc::unwrap_or_clone(arc)))
-                .collect()
+            // Only capture the raw return type hint when at least one
+            // resolved class has template parameters.  Non-generic
+            // classes don't benefit from the hint, and skipping it
+            // avoids an extra resolve_class_fully lookup on every
+            // call expression in builder chains.
+            let needs_hint = classes.iter().any(|c| !c.template_params.is_empty());
+            if needs_hint {
+                let mut hint: Option<PhpType> = None;
+                let classes2 = Backend::resolve_call_return_types_expr_with_hint(
+                    callee,
+                    args_text,
+                    ctx,
+                    Some(&mut hint),
+                );
+                if let Some(h) = hint {
+                    let class_vec: Vec<ClassInfo> =
+                        classes2.into_iter().map(Arc::unwrap_or_clone).collect();
+                    return ResolvedType::from_classes_with_hint(class_vec, h);
+                }
+                classes2
+                    .into_iter()
+                    .map(|arc| ResolvedType::from_class(Arc::unwrap_or_clone(arc)))
+                    .collect()
+            } else {
+                classes
+                    .into_iter()
+                    .map(|arc| ResolvedType::from_class(Arc::unwrap_or_clone(arc)))
+                    .collect()
+            }
         }
 
         // ── Property chain ──────────────────────────────────────