Another bucket of refactoring to compleation and diagnostics

Author: AJenbo

docs/CHANGELOG.md

@@ -66,6 +66,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **`@switch`/`@case` with a class-constant case value no longer reports a syntax error.** `@case (Some\Namespaced\Enum::VALUE)` now translates to a valid `case` arm instead of silently corrupting the rest of the file.
 - **Generated return types and `@return` tags understand every kind of return expression, not just literals and variables.** Inferring a missing return type now resolves method/function calls, ternaries, matches, property access, and array literals split across multiple lines through the same type engine as hover, instead of degrading to `mixed` (or, for multi-line array literals, to a coarser `array<mixed>`) for anything beyond a simple literal, `new`, or a plain variable.
 - **Calls to functions declared in another `namespace` block of the same file resolve their return type.** In a file that declares more than one `namespace`, a call to a function from a later block used to leave the returned value untyped unless the function carried an `@return` docblock. The call and its return type now resolve regardless, so completion, hover, and diagnostics see the value's type.
+- **`@template` bindings resolve correctly when a call uses named arguments.** A generic function or method called with named arguments (for example `process(class: Product::class, flag: true, function ($p) { ... })`) previously bound its template parameter from the wrong argument whenever the named arguments were out of declaration order, leaving closure parameters untyped. Named arguments now route to the parameter they actually target.
 
 ## [0.9.0] - 2026-07-20
 

docs/todo/bugs.md

@@ -7,4 +7,59 @@ pipeline so it produces correct data. Downstream consumers
 (diagnostics, hover, completion, definition) should never need
 to second-guess upstream output.
 
-No outstanding items.
+## B115. Closure-parameter inference uses call-position instead of the declared parameter index, so named arguments that reorder a call break it
+
+**Severity: Low-Medium (closure/arrow-function parameters silently lose
+their inferred type — falling back to no completions/hover — for any
+call that uses named arguments to reorder or skip parameters ahead of
+the callable argument) · Discovered while deduplicating the receiver
+resolution helpers shared between `closure_resolution.rs` and
+`forward_walk/callable_inference.rs`; confirmed with a regression test**
+
+```php
+/**
+ * @template T
+ * @param bool $flag
+ * @param class-string<T> $class
+ * @param callable(T): void $cb
+ */
+function process(bool $flag, string $class, callable $cb): void {}
+
+process(class: Product::class, flag: true, function ($p) {
+    $p-> // no completions: T never binds to Product
+});
+```
+
+The forward walker's closure-argument inference
+(`walk_closures_in_call_args` and `walk_closure_in_partial_call_args` in
+`src/completion/variable/forward_walk/diagnostic_walk.rs`, and the
+completion/hover-path counterpart `infer_callable_params_for_call` in
+`src/completion/variable/forward_walk/callable_inference.rs`) computes
+`arg_idx` from `arguments.iter().enumerate()` — the argument's position
+in the *call*. That index is then used directly as the *declared*
+parameter index, both to decide whether the argument at that slot is a
+closure and to look up `fi.parameters[arg_idx]` /
+`class.get_method(..).parameters[arg_idx]` for the callable's own
+`callable(...)`/`Closure(...)` type hint (see
+`extract_callable_params_at_fw` and its callers).
+
+PHP argument binding does not guarantee call-position equals declared
+position once named arguments are involved: a named argument can appear
+anywhere in the call, and named arguments before the closure can be
+reordered or a preceding optional parameter can be omitted entirely.
+Whenever that happens, `arg_idx` no longer identifies the closure's own
+parameter, so `extract_callable_params_at_fw` looks up the wrong
+parameter (typically a non-callable one), returns no callable param
+types, and the closure gets none of its parameters inferred — even
+though the same call resolves correctly when arguments are positional
+and in declaration order.
+
+Fix by resolving each argument's *declared* parameter index (via
+something like the existing `bind_text_args_to_params` /
+`param_name_bare` machinery in `src/call_args.rs`, which already knows
+how to route named arguments to their declared slot) before using that
+index to look up the callable's own parameter type or to decide which
+argument is "the closure at position N". This affects both the
+diagnostic-path walker (`diagnostic_walk.rs`) and the completion/hover
+path (`callable_inference.rs`), which must stay in sync per the
+project's shared-pipeline convention.

docs/todo/refactor.md

@@ -214,79 +214,54 @@ Each item must include:
 
 # Outstanding items
 
-## Deduplicate parallel helpers inside the resolution pipeline
-
-**What to do.** The resolution files contain several
-copies-for-another-code-path that should collapse onto one
-implementation (do this after — or as part of — the splits above):
-
-1. **Callable-param inference.** The `*_fw`-suffixed family in
-   `forward_walk/` parallels the logic in
-   `completion/variable/closure_resolution.rs`. The suffix itself marks
-   a copy; unify them.
-2. **`$this`/`self`/`static` resolution.** ~32 call sites spread across
-   `util.rs` (`is_self_or_static`, `resolve_class_keyword`),
-   `call_resolution/callable_target.rs` (`resolve_class_name_keyword`),
-   `resolver/mod.rs` (`resolve_static_owner_class`), and `forward_walk/`
-   (`seed_this`), plus hand-rolled `== "$this"` checks. Back them with
-   one helper module.
-3. **Subclass checks.** `is_subclass_of` (`forward_walk/`),
-   `is_type_subclass_of` and `is_valid_virtual_narrowing`
-   (`call_resolution/return_types.rs`), and `util::is_subtype_of*`
-   overlap; route through the `util`/`php_type` versions.
-4. **Property-assignment scanning.** The
-   `find_*_this_property_assignment*` family
-   (`rhs_resolution/property_access.rs`) and the
-   `walk_property_narrowing_*` family (`resolver/property_narrowing.rs`)
-   walk class members and statements with near-identical skeletons for
-   different outputs. Share the traversal.
-5. **Argument-text extraction.** `extract_argument_texts_fw` /
-   `extract_first_arg_string_fw` (`forward_walk/`) vs
-   `extract_first_arg_text` / `resolve_inline_arg_raw_type`
-   (`call_resolution/arg_type_resolution.rs`) vs
-   `resolve_arg_text_to_type` (`call_resolution/template_subs.rs`) vs
-   `resolve_arg_raw_type` (`resolution.rs`).
-
-**Why it matters.** These duplications are exactly the "parallel type
-resolution systems" the conventions forbid, just internal to the
-pipeline: a narrowing or generics fix applied to one copy silently
-misses the others.
-
----
-
-## Shared AST walker for the hand-rolled traversals
-
-**What to do.** At least six modules hand-roll the same giant
-`match` over `Statement`/`Expression` variants, each independently
-re-typing the `IfBody`/`ForeachBody`/`WhileBody`/`SwitchBody` recursion:
-`symbol_map/extraction.rs` (28 statement / 101 expression matches),
-`selection_range.rs` (33/34), the anonymous-class walker in
-`parser/classes.rs` (33/36), `completion/types/narrowing.rs` (64
-expression matches), and the six boolean/name-set detector walker pairs
-in `diagnostics/undefined_variables.rs` (dynamic-vars, extract(),
-compact(), get_defined_vars(), `@`-suppression, isset/empty guards)
-plus the structural halves of `unused_variables.rs` and
-`type_errors.rs`.
-
-`mago-syntax` already ships a generated `Walker` trait
-(`walker/mod.rs`, per-node `walk_in_*`/`walk_out_*` hooks with full
-recursive traversal) that none of these use. Migrate incrementally,
-starting where the payoff is largest and the risk lowest:
-
-1. The six detector pairs in `undefined_variables.rs` — each becomes a
-   ~10-line visitor, removing ~1,400 lines of traversal boilerplate.
-2. The statement-dispatch halves of `unused_variables.rs` and
-   `type_errors.rs`.
-3. The anonymous-class walker in `parser/classes.rs` (~1,000 lines).
-4. `symbol_map/extraction.rs` and `selection_range.rs` as follow-ups.
-
-The forward walker is explicitly out of scope here (its traversal is
-interleaved with scope-state mutation; see the split item above).
+## Shared AST walker — remaining candidates (needs a decision)
+
+**Done so far.** The stateless boolean/name-set/collection walkers have
+been migrated to the generated `mago-syntax` `Walker` trait (each is now
+a small visitor over a shared traversal): the six detector pairs in
+`diagnostics/undefined_variables/` (dynamic-vars, `extract()`,
+`compact()`, `get_defined_vars()`, `@`-suppression, isset/empty guards),
+the statement-dispatch halves of `diagnostics/unused_variables.rs` and
+`diagnostics/type_errors.rs`, and the anonymous-class walker in
+`parser/anonymous.rs`. That removed ~1,900 lines of hand-rolled
+traversal.
+
+**What remains — and why it is not a clean fit.** The remaining
+hand-rolled walkers are *not* stateless collectors; they are
+positional/order-sensitive, which the typed `Walker` cannot express
+without re-typing per-node logic (it has no single "visit any node"
+hook, only per-node-type `walk_in_*`):
+
+- `selection_range.rs` pushes **synthetic** spans (brace pairs, paren
+  pairs, block interiors) that are not AST-node spans, and would need a
+  `walk_in_*` override for essentially every node type to reproduce
+  them — no net reduction.
+- `symbol_map/extraction.rs` records position-keyed symbol spans and
+  subject text; order and per-kind span shaping dominate, so the same
+  per-node-override problem applies.
+- The `property_access.rs` / `property_narrowing.rs` pair walk **from
+  the method start down to the cursor**, accumulating narrowing state in
+  source order with early returns. That interleaved-mutation,
+  cursor-bounded shape is exactly why the forward walker is out of scope
+  (below).
+
+The forward walker is explicitly out of scope (its traversal is
+interleaved with scope-state mutation).
+
+**Decision needed.** These three are arguably better left as-is: the
+`Walker` migration's payoff (delete child-dispatch boilerplate) does not
+apply when the walk pushes synthetic/positional data or mutates state in
+source order. If the goal is purely "one traversal so new AST variants
+can't create blind spots," a thin `visit_all_nodes(&Node, &mut FnMut)`
+helper over mago's `Node` enum would serve `selection_range` and
+`symbol_map/extraction` better than the typed `Walker`. Confirm whether
+to (a) leave these as-is, (b) migrate `selection_range` /
+`symbol_map/extraction` via a `Node`-enum visitor, or (c) migrate the
+property pair despite the state-threading cost.
 
 **Why it matters.** Every new mago AST node variant (new PHP syntax)
-currently needs matching arms added in six places; missing one produces
-silent blind spots in exactly one feature. One traversal, many small
-visitors is the structure all three reference projects use.
+still needs matching arms in these remaining walkers; missing one
+produces a silent blind spot in exactly one feature.
 
 ---
 

src/completion/call_resolution/template_subs.rs

@@ -632,22 +632,21 @@ impl Backend {
         {
             // self::class / static::class / parent::class resolve relative
             // to the class at the call site.
-            let class_named =
-                if name.eq_ignore_ascii_case("self") || name.eq_ignore_ascii_case("static") {
-                    ctx.current_class
-                        .map(|c| PhpType::Named(c.fqn().to_string()))
-                } else if name.eq_ignore_ascii_case("parent") {
-                    ctx.current_class
-                        .and_then(|c| c.parent_class.as_ref())
-                        .map(|p| PhpType::Named(p.to_string()))
+            let class_named = if is_self_or_static(name) {
+                ctx.current_class
+                    .map(|c| PhpType::Named(c.fqn().to_string()))
+            } else if name.eq_ignore_ascii_case("parent") {
+                ctx.current_class
+                    .and_then(|c| c.parent_class.as_ref())
+                    .map(|p| PhpType::Named(p.to_string()))
+            } else {
+                let resolved_name = if let Some(cls) = (ctx.class_loader)(name) {
+                    cls.fqn().to_string()
                 } else {
-                    let resolved_name = if let Some(cls) = (ctx.class_loader)(name) {
-                        cls.fqn().to_string()
-                    } else {
-                        name.to_string()
-                    };
-                    Some(PhpType::Named(resolved_name))
+                    name.to_string()
                 };
+                Some(PhpType::Named(resolved_name))
+            };
             return class_named.map(|n| PhpType::ClassString(Some(Box::new(n))));
         }
 

src/completion/eloquent_string.rs

@@ -382,7 +382,7 @@ impl Backend {
             // Static call: `User::with(...)` — subject is the class name.
             let fqn = self.resolve_class_name_to_fqn(subject, ctx)?;
             class_loader(&fqn)
-        } else if subject == "$this" || subject == "static" || subject == "self" {
+        } else if crate::class_lookup::is_self_or_static(subject) {
             // Inside the model class itself.
             let cursor_offset = position_to_offset(content, position);
             let current_class =

src/completion/resolver/property_narrowing.rs

@@ -126,22 +126,10 @@ fn walk_property_narrowing_in_members<'b>(
     ctx: &VarResolutionCtx<'_>,
     results: &mut Vec<ClassInfo>,
 ) {
-    use mago_syntax::cst::class_like::member::ClassLikeMember;
-    use mago_syntax::cst::class_like::method::MethodBody;
-
-    for member in members {
-        if let ClassLikeMember::Method(method) = member {
-            let body = match &method.body {
-                MethodBody::Concrete(block) => block,
-                _ => continue,
-            };
-            let body_start = body.left_brace.start.offset;
-            let body_end = body.right_brace.end.offset;
-            if ctx.cursor_offset >= body_start && ctx.cursor_offset <= body_end {
-                walk_property_narrowing_stmts(body.statements.iter(), ctx, results);
-                return;
-            }
-        }
+    if let Some(block) =
+        crate::util::find_enclosing_method_block_in_members(members, ctx.cursor_offset)
+    {
+        walk_property_narrowing_stmts(block.statements.iter(), ctx, results);
     }
 }
 

src/completion/variable/closure_resolution.rs

@@ -496,32 +496,82 @@ fn closure_this_from_function_params(
     resolve_closure_this_type(php_type, None, ctx)
 }
 
-/// Look up `closure_this_type` on an instance method's parameter at
-/// `arg_idx`, resolving the receiver from the source span.
-fn closure_this_from_receiver(
+// ─── Shared receiver resolution ─────────────────────────────────────────
+//
+// These helpers resolve "the receiver/owner class of a call's method"
+// from the AST, independent of what the caller extracts from the target
+// method once found.  Both the `@param-closure-this` resolution above
+// and the callable-parameter inference in
+// `forward_walk/callable_inference.rs` need this same receiver
+// resolution; they differ only in what they pull out of the matched
+// method (`closure_this_type` vs. `callable(...)` parameter types).
+
+/// Resolve the receiver classes of an instance/nullsafe method call from
+/// its object span.
+///
+/// Overrides `cursor_offset` to the object's start so that variable
+/// resolution looks up the scope snapshot *before* the call (where the
+/// receiver variable is actually in scope), not at a position inside a
+/// later closure argument where the variable doesn't exist yet.
+pub(in crate::completion) fn resolve_receiver_types(
     obj_start: u32,
     obj_end: u32,
-    method_name: &str,
-    arg_idx: usize,
     ctx: &ResolutionCtx<'_>,
-) -> Option<ClassInfo> {
+) -> Vec<ResolvedType> {
     let start = obj_start as usize;
     let end = obj_end as usize;
     if end > ctx.content.len() {
-        return None;
+        return vec![];
     }
     let obj_text = ctx.content[start..end].trim();
-    // Use the object's own offset as cursor_offset so that variable
-    // resolution looks up the scope snapshot *before* the closure body
-    // (where the variable is actually in scope), not at the diagnostic
-    // offset inside the closure where the variable doesn't exist.
     let obj_ctx = ResolutionCtx {
         cursor_offset: obj_start,
         ..*ctx
     };
-    let receiver_classes = ResolvedType::into_arced_classes(
-        crate::completion::resolver::resolve_target_classes(obj_text, AccessKind::Arrow, &obj_ctx),
-    );
+    crate::completion::resolver::resolve_target_classes(obj_text, AccessKind::Arrow, &obj_ctx)
+}
+
+/// Resolve the class name referenced by a static-call receiver
+/// expression (`self`, `static`, `parent`, or a class identifier).
+pub(in crate::completion) fn static_receiver_class_name(
+    class_expr: &Expression<'_>,
+    current_class: Option<&ClassInfo>,
+) -> Option<String> {
+    match class_expr {
+        Expression::Self_(_) | Expression::Static(_) => current_class.map(|cc| cc.name.to_string()),
+        Expression::Identifier(ident) => Some(bytes_to_str(ident.value()).to_string()),
+        Expression::Parent(_) => {
+            current_class.and_then(|cc| cc.parent_class.map(|a| a.to_string()))
+        }
+        _ => None,
+    }
+}
+
+/// Look up a class by name: local classes first, then the cross-file
+/// loader.
+pub(in crate::completion) fn find_owner_by_name(
+    name: &str,
+    all_classes: &[Arc<ClassInfo>],
+    class_loader: &dyn Fn(&str) -> Option<Arc<ClassInfo>>,
+) -> Option<ClassInfo> {
+    all_classes
+        .iter()
+        .find(|c| c.name == name)
+        .map(|c| ClassInfo::clone(c))
+        .or_else(|| class_loader(name).map(Arc::unwrap_or_clone))
+}
+
+/// Look up `closure_this_type` on an instance method's parameter at
+/// `arg_idx`, resolving the receiver from the source span.
+fn closure_this_from_receiver(
+    obj_start: u32,
+    obj_end: u32,
+    method_name: &str,
+    arg_idx: usize,
+    ctx: &ResolutionCtx<'_>,
+) -> Option<ClassInfo> {
+    let receiver_classes =
+        ResolvedType::into_arced_classes(resolve_receiver_types(obj_start, obj_end, ctx));
     for cls in &receiver_classes {
         let resolved = crate::virtual_members::resolve_class_fully_maybe_cached(
             cls,
@@ -546,16 +596,7 @@ fn closure_this_from_static_receiver(
     arg_idx: usize,
     ctx: &ResolutionCtx<'_>,
 ) -> Option<ClassInfo> {
-    let class_name = match class_expr {
-        Expression::Self_(_) | Expression::Static(_) => {
-            ctx.current_class.map(|cc| cc.name.to_string())
-        }
-        Expression::Identifier(ident) => Some(bytes_to_str(ident.value()).to_string()),
-        Expression::Parent(_) => ctx
-            .current_class
-            .and_then(|cc| cc.parent_class.map(|a| a.to_string())),
-        _ => None,
-    }?;
+    let class_name = static_receiver_class_name(class_expr, ctx.current_class)?;
 
     if method_name.eq_ignore_ascii_case("macro")
         && arg_idx == 1
@@ -571,12 +612,7 @@ fn closure_this_from_static_receiver(
         ));
     }
 
-    let owner = ctx
-        .all_classes
-        .iter()
-        .find(|c| c.name == class_name)
-        .map(|c| ClassInfo::clone(c))
-        .or_else(|| (ctx.class_loader)(&class_name).map(Arc::unwrap_or_clone))?;
+    let owner = find_owner_by_name(&class_name, ctx.all_classes, ctx.class_loader)?;
 
     let resolved = crate::virtual_members::resolve_class_fully_maybe_cached(
         &owner,