Refactor argument splitting for template substitution

AJenbo · AJenbo · commit f0a41f07cfa7 · 2026-04-11T00:02:47.000+02:00
diff --git a/src/completion/call_resolution.rs b/src/completion/call_resolution.rs
@@ -18,7 +18,7 @@
 ///   return types and template substitutions.
 /// - [`Backend::build_method_template_subs`]: builds a template
 ///   substitution map for method-level `@template` parameters from
-///   call-site argument text.
+///   pre-split call-site argument texts.
 use std::collections::HashMap;
 use std::sync::Arc;
 
@@ -374,8 +374,10 @@ impl Backend {
 
                 let mut results = Vec::new();
                 for owner in &lhs_classes {
+                    let split_args = split_text_args(text_args);
+                    let arg_refs = split_args.to_vec();
                     let template_subs =
-                        Self::build_method_template_subs(owner, method_name, text_args, ctx);
+                        Self::build_method_template_subs(owner, method_name, &arg_refs, ctx);
                     let var_resolver = build_var_resolver(ctx);
                     let mr_ctx = MethodReturnCtx {
                         all_classes: ctx.all_classes,
@@ -414,8 +416,10 @@ impl Backend {
                 };
 
                 if let Some(ref owner) = owner_class {
+                    let split_args = split_text_args(text_args);
+                    let arg_refs = split_args.to_vec();
                     let template_subs =
-                        Self::build_method_template_subs(owner, method_name, text_args, ctx);
+                        Self::build_method_template_subs(owner, method_name, &arg_refs, ctx);
                     let var_resolver = build_var_resolver(ctx);
                     let mr_ctx = MethodReturnCtx {
                         all_classes: ctx.all_classes,
@@ -519,8 +523,14 @@ impl Backend {
                         && func_info.return_type.is_some()
                         && !text_args.is_empty()
                     {
+                        let split_args: Vec<String> = split_text_args(text_args)
+                            .into_iter()
+                            .map(|s| s.to_string())
+                            .collect();
                         let subs = super::variable::rhs_resolution::build_function_template_subs(
-                            &func_info, text_args, ctx,
+                            &func_info,
+                            &split_args,
+                            ctx,
                         );
 
                         if !subs.is_empty()
@@ -1046,16 +1056,21 @@ impl Backend {
     /// Build a template substitution map for a method-level `@template` call.
     ///
     /// Finds the method on the class (or inherited), checks for template
-    /// params and bindings, resolves argument types from `text_args` using
-    /// the call resolution context, and returns a `HashMap` mapping template
-    /// parameter names to their resolved concrete types.
+    /// params and bindings, resolves argument types from the pre-split
+    /// `arg_texts` slice using the call resolution context, and returns a
+    /// `HashMap` mapping template parameter names to their resolved
+    /// concrete types.
+    ///
+    /// Callers with an AST `ArgumentList` should extract per-argument text
+    /// via [`extract_arg_texts_from_ast`] and convert to `&[&str]`.
+    /// Callers with only raw text should use [`split_text_args`] first.
     ///
     /// Returns an empty map if the method has no template params, no
     /// bindings, or if argument types cannot be resolved.
     pub(super) fn build_method_template_subs(
         class_info: &ClassInfo,
         method_name: &str,
-        text_args: &str,
+        arg_texts: &[&str],
         ctx: &ResolutionCtx<'_>,
     ) -> HashMap<String, PhpType> {
         // Find the method — first on the class directly, then via inheritance.
@@ -1082,7 +1097,6 @@ impl Backend {
             _ => return HashMap::new(),
         };
 
-        let args = split_text_args(text_args);
         let mut subs = HashMap::new();
 
         for (tpl_name, param_name) in &method.template_bindings {
@@ -1093,7 +1107,7 @@ impl Backend {
             };
 
             // Get the corresponding argument text.
-            let arg_text = match args.get(param_idx) {
+            let arg_text = match arg_texts.get(param_idx) {
                 Some(text) => text.trim(),
                 None => {
                     // No argument was provided at the call site.  If the
diff --git a/src/completion/mod.rs b/src/completion/mod.rs
@@ -84,7 +84,7 @@ pub(crate) mod use_edit;
 pub(crate) mod context;
 pub mod phpdoc;
 pub(crate) mod source;
-pub(crate) mod types;
+pub mod types;
 pub(crate) mod variable;
 
 // ─── Backward-compatible re-exports ─────────────────────────────────────────
diff --git a/src/completion/types/conditional.rs b/src/completion/types/conditional.rs
@@ -330,15 +330,40 @@ fn condition_includes_array(condition: &PhpType) -> bool {
 
 /// Split a textual argument list by commas, respecting nested parentheses
 /// so that `"foo(a, b), c"` splits into `["foo(a, b)", "c"]`.
-pub(crate) fn split_text_args(text: &str) -> Vec<&str> {
+pub fn split_text_args(text: &str) -> Vec<&str> {
     let mut result = Vec::new();
     let mut depth = 0u32;
     let mut start = 0;
-    for (i, ch) in text.char_indices() {
+    let mut in_single_quote = false;
+    let mut in_double_quote = false;
+    let mut prev_was_backslash = false;
+
+    let mut chars = text.char_indices().peekable();
+    while let Some((i, ch)) = chars.next() {
+        if prev_was_backslash {
+            prev_was_backslash = false;
+            continue;
+        }
         match ch {
-            '(' | '[' => depth += 1,
-            ')' | ']' => depth = depth.saturating_sub(1),
-            ',' if depth == 0 => {
+            '\\' => {
+                // Only treat as escape if inside a quote
+                if in_single_quote || in_double_quote {
+                    prev_was_backslash = true;
+                }
+            }
+            '\'' if !in_double_quote => {
+                in_single_quote = !in_single_quote;
+            }
+            '"' if !in_single_quote => {
+                in_double_quote = !in_double_quote;
+            }
+            '(' | '[' if !in_single_quote && !in_double_quote => {
+                depth += 1;
+            }
+            ')' | ']' if !in_single_quote && !in_double_quote => {
+                depth = depth.saturating_sub(1);
+            }
+            ',' if depth == 0 && !in_single_quote && !in_double_quote => {
                 result.push(&text[start..i]);
                 start = i + 1; // skip the comma
             }
diff --git a/src/completion/types/mod.rs b/src/completion/types/mod.rs
@@ -6,5 +6,5 @@
 /// - **narrowing**: instanceof / assert / custom type guard narrowing
 /// - **conditional**: PHPStan conditional return type resolution at call sites
 pub mod conditional;
-pub(crate) mod narrowing;
-pub(crate) mod resolution;
+pub mod narrowing;
+pub mod resolution;
diff --git a/src/completion/variable/closure_resolution.rs b/src/completion/variable/closure_resolution.rs
@@ -1501,10 +1501,8 @@ fn infer_callable_params_from_function(
         // concrete element type (`PurchaseFileProduct`).
         if !params.is_empty() && !fi.template_params.is_empty() && !fi.template_bindings.is_empty()
         {
-            let text_args = extract_argument_texts(arguments, ctx.content);
-            let text_args_joined = text_args.join(", ");
-            let subs =
-                super::rhs_resolution::build_function_template_subs(&fi, &text_args_joined, &rctx);
+            let arg_texts = extract_argument_texts(arguments, ctx.content);
+            let subs = super::rhs_resolution::build_function_template_subs(&fi, &arg_texts, &rctx);
             if !subs.is_empty() {
                 params = params.into_iter().map(|p| p.substitute(&subs)).collect();
             }
diff --git a/src/completion/variable/raw_type_inference.rs b/src/completion/variable/raw_type_inference.rs
@@ -251,21 +251,32 @@ pub(in crate::completion) fn resolve_array_func_element_type(
     None
 }
 
-/// Extract the raw text of a function/method argument list from source.
+/// Extract per-argument source text from a parsed `ArgumentList`.
 ///
-/// Returns the text between the parentheses (exclusive), trimmed.
-/// For example, an argument list `($user, $role)` returns `"$user, $role"`.
-pub(in crate::completion) fn extract_argument_text(
+/// Returns one `String` per argument by walking the AST nodes and
+/// extracting their spans. This avoids serialising the argument list
+/// to a flat string and then re-splitting with `split_text_args`.
+pub(in crate::completion) fn extract_arg_texts_from_ast(
     argument_list: &mago_syntax::ast::ArgumentList<'_>,
     content: &str,
-) -> String {
-    let left = argument_list.left_parenthesis.span().end.offset as usize;
-    let right = argument_list.right_parenthesis.span().start.offset as usize;
-    if right > left && right <= content.len() {
-        content[left..right].trim().to_string()
-    } else {
-        String::new()
-    }
+) -> Vec<String> {
+    argument_list
+        .arguments
+        .iter()
+        .map(|arg| {
+            let span = match arg {
+                mago_syntax::ast::argument::Argument::Positional(pos) => pos.value.span(),
+                mago_syntax::ast::argument::Argument::Named(named) => named.value.span(),
+            };
+            let start = span.start.offset as usize;
+            let end = span.end.offset as usize;
+            if end <= content.len() {
+                content[start..end].to_string()
+            } else {
+                String::new()
+            }
+        })
+        .collect()
 }
 
 /// Extract the output element type for `array_map($callback, $array)`.
diff --git a/src/completion/variable/rhs_resolution.rs b/src/completion/variable/rhs_resolution.rs
@@ -477,11 +477,11 @@ fn resolve_rhs_instantiation(
                 && !ctor.template_bindings.is_empty()
                 && let Some(ref arg_list) = inst.argument_list
             {
-                let text_args =
-                    super::raw_type_inference::extract_argument_text(arg_list, ctx.content);
-                if !text_args.is_empty() {
+                let arg_texts =
+                    super::raw_type_inference::extract_arg_texts_from_ast(arg_list, ctx.content);
+                if !arg_texts.is_empty() {
                     let rctx = ctx.as_resolution_ctx();
-                    let subs = build_constructor_template_subs(cls, ctor, &text_args, &rctx, ctx);
+                    let subs = build_constructor_template_subs(cls, ctor, &arg_texts, &rctx, ctx);
                     if !subs.is_empty() {
                         let type_args: Vec<PhpType> = cls
                             .template_params
@@ -569,11 +569,10 @@ fn resolve_rhs_instantiation(
 fn build_constructor_template_subs(
     _class: &ClassInfo,
     ctor: &crate::types::MethodInfo,
-    text_args: &str,
+    arg_texts: &[String],
     rctx: &crate::completion::resolver::ResolutionCtx<'_>,
     ctx: &VarResolutionCtx<'_>,
 ) -> HashMap<String, PhpType> {
-    let args = crate::completion::conditional_resolution::split_text_args(text_args);
     let mut subs = HashMap::new();
 
     for (tpl_name, param_name) in &ctor.template_bindings {
@@ -584,7 +583,7 @@ fn build_constructor_template_subs(
         };
 
         // Get the corresponding argument text.
-        let arg_text = match args.get(param_idx) {
+        let arg_text = match arg_texts.get(param_idx) {
             Some(text) => text.trim(),
             None => continue,
         };
@@ -828,8 +827,12 @@ fn resolve_generic_wrapper_template(
     let paren_end = arg_text.rfind(')')?;
     let inner_args = arg_text[paren_start + 1..paren_end].trim();
 
+    let wrapper_arg_texts = crate::completion::conditional_resolution::split_text_args(inner_args)
+        .into_iter()
+        .map(|s| s.to_string())
+        .collect::<Vec<_>>();
     let wrapper_subs =
-        build_constructor_template_subs(&wrapper_cls, wrapper_ctor, inner_args, rctx, ctx);
+        build_constructor_template_subs(&wrapper_cls, wrapper_ctor, &wrapper_arg_texts, rctx, ctx);
 
     // Find the wrapper's template param at the given position and
     // look it up in the substitution map.
@@ -1012,10 +1015,9 @@ fn classify_array_index(index: &Expression<'_>) -> ArrayBracketSegment {
 ///     positional resolution through the wrapper's generic arguments.
 pub(crate) fn build_function_template_subs(
     func_info: &crate::types::FunctionInfo,
-    text_args: &str,
+    arg_texts: &[String],
     rctx: &crate::completion::resolver::ResolutionCtx<'_>,
 ) -> HashMap<String, PhpType> {
-    let args = crate::completion::conditional_resolution::split_text_args(text_args);
     let mut subs = HashMap::new();
 
     for (tpl_name, param_name) in &func_info.template_bindings {
@@ -1028,7 +1030,7 @@ pub(crate) fn build_function_template_subs(
             None => continue,
         };
 
-        let arg_text = match args.get(param_idx) {
+        let arg_text = match arg_texts.get(param_idx) {
             Some(text) => text.trim(),
             None => continue,
         };
@@ -1353,11 +1355,13 @@ fn resolve_rhs_function_call<'b>(
             && !func_info.template_bindings.is_empty()
             && func_info.return_type.is_some()
         {
-            let text_args =
-                super::raw_type_inference::extract_argument_text(&func_call.argument_list, content);
-            if !text_args.is_empty() {
+            let arg_texts = super::raw_type_inference::extract_arg_texts_from_ast(
+                &func_call.argument_list,
+                content,
+            );
+            if !arg_texts.is_empty() {
                 let rctx = ctx.as_resolution_ctx();
-                let subs = build_function_template_subs(&func_info, &text_args, &rctx);
+                let subs = build_function_template_subs(&func_info, &arg_texts, &rctx);
                 if !subs.is_empty()
                     && let Some(ref ret) = func_info.return_type
                 {
@@ -1673,13 +1677,16 @@ fn resolve_rhs_method_call_inner<'b>(
             (classes, resolved)
         };
 
-    let text_args = super::raw_type_inference::extract_argument_text(argument_list, ctx.content);
+    let arg_texts =
+        super::raw_type_inference::extract_arg_texts_from_ast(argument_list, ctx.content);
+    let arg_refs: Vec<&str> = arg_texts.iter().map(|s| s.as_str()).collect();
+    let text_args = arg_texts.join(", ");
     let rctx = ctx.as_resolution_ctx();
     let var_resolver = build_var_resolver_from_ctx(ctx);
 
     for owner in &owner_classes {
         let template_subs =
-            Backend::build_method_template_subs(owner, &method_name, &text_args, &rctx);
+            Backend::build_method_template_subs(owner, &method_name, &arg_refs, &rctx);
         let mr_ctx = MethodReturnCtx {
             all_classes: ctx.all_classes,
             class_loader: ctx.class_loader,
@@ -1894,13 +1901,15 @@ fn resolve_rhs_static_call(
             .map(|c| ClassInfo::clone(c))
             .or_else(|| (ctx.class_loader)(&cls_name).map(Arc::unwrap_or_clone));
         if let Some(ref owner) = owner {
-            let text_args = super::raw_type_inference::extract_argument_text(
+            let arg_texts = super::raw_type_inference::extract_arg_texts_from_ast(
                 &static_call.argument_list,
                 ctx.content,
             );
+            let arg_refs: Vec<&str> = arg_texts.iter().map(|s| s.as_str()).collect();
+            let text_args = arg_texts.join(", ");
             let rctx = ctx.as_resolution_ctx();
             let template_subs =
-                Backend::build_method_template_subs(owner, &method_name, &text_args, &rctx);
+                Backend::build_method_template_subs(owner, &method_name, &arg_refs, &rctx);
             let var_resolver = build_var_resolver_from_ctx(ctx);
             let mr_ctx = MethodReturnCtx {
                 all_classes: ctx.all_classes,
diff --git a/tests/unit/conditional_split_args.rs b/tests/unit/conditional_split_args.rs
diff --git a/tests/unit/main.rs b/tests/unit/main.rs

Original file line number	Diff line number	Diff line change
`@@ -1501,10 +1501,8 @@ fn infer_callable_params_from_function(`
`1501`	`1501`	// concrete element type (`PurchaseFileProduct`).
`1502`	`1502`	`if !params.is_empty() && !fi.template_params.is_empty() && !fi.template_bindings.is_empty()`
`1503`	`1503`	`{`
`1504`		`- let text_args = extract_argument_texts(arguments, ctx.content);`
`1505`		`- let text_args_joined = text_args.join(", ");`
`1506`		`- let subs =`
`1507`		`- super::rhs_resolution::build_function_template_subs(&fi, &text_args_joined, &rctx);`
	`1504`	`+ let arg_texts = extract_argument_texts(arguments, ctx.content);`
	`1505`	`+ let subs = super::rhs_resolution::build_function_template_subs(&fi, &arg_texts, &rctx);`
`1508`	`1506`	`if !subs.is_empty() {`
`1509`	`1507`	`params = params.into_iter().map(\|p\| p.substitute(&subs)).collect();`
`1510`	`1508`	`}`