Deduplicate scope map helpers for extract-function, extract-variable,

and inline-variable

Author: AJenbo

docs/todo/refactor.md

@@ -223,17 +223,17 @@ already tracks this information during its top-to-bottom pass.
 ### `extract_closure_return_type_from_assignment`
 
 Uses `rfind("$fn = ")` backward from cursor, then parses closure return type
-from raw text. The forward walker already processes closure assignments and
-extracts return types from the AST node's type hint. Callers in
-`call_resolution.rs` (L1272) and `rhs_resolution.rs` (L2695) should read the
-variable's type from the scope map instead.
+from raw text. The forward walker does NOT currently store callable return type
+info — it stores the variable as plain `Closure` via `resolve_rhs_expression`.
+Eliminating this backward walker requires teaching `resolve_rhs_expression` (or
+`resolve_rhs_with_scope`) to produce a `Callable(params, return_type)` PhpType
+for closure/arrow-function expressions.
 
 ### `extract_first_class_callable_return_type`
 
 Uses `rfind("$fn = ")` backward, then resolves `Foo::bar(...)` callable return
-type from text. The forward walker already handles first-class callable
-assignments. Callers in `call_resolution.rs` (L1292) and `rhs_resolution.rs`
-(L2718) should use the scope map.
+type from text. Same situation: the forward walker stores plain `Closure` for
+first-class callable assignments. Needs the same `Callable` type support.
 
 ### `extract_function_return_from_source`
 
@@ -252,8 +252,11 @@ re-parse the AST to extract information the SymbolMap already has.
 
 ### Scope boundary detection
 
-The SymbolMap stores `scopes` (function/closure boundaries) and provides
-`find_enclosing_scope()`. Yet several code action helpers and diagnostic modules
-manually walk the AST to find the enclosing function/method body (e.g.
-`build_scope_map` in extract variable, inline variable, undefined variable
-diagnostics).
+The three `build_scope_map` helpers in extract-function, extract-variable, and
+inline-variable have been unified into a single
+`scope_collector::build_scope_map_for_offset` function. These code actions still
+re-parse the AST (via `with_parsed_program`, which caches) rather than using the
+SymbolMap's `find_enclosing_scope()`, because they need the full `ScopeMap`
+(variable reads/writes/frames) which the SymbolMap does not provide. Further
+consolidation would require either enriching the SymbolMap with variable
+access tracking or caching the `ScopeMap` per file.

src/code_actions/extract_function.rs

@@ -22,9 +22,7 @@ use crate::code_actions::{CodeActionData, make_code_action_data};
 use crate::completion::phpdoc::generation::enrichment_plain;
 use crate::completion::resolver::Loaders;
 use crate::php_type::PhpType;
-use crate::scope_collector::{
-    FrameKind, ScopeMap, collect_function_scope, collect_function_scope_with_kind, collect_scope,
-};
+use crate::scope_collector::ScopeMap;
 use crate::types::ClassInfo;
 use crate::util::{find_class_at_offset, offset_to_position, position_to_byte_offset};
 
@@ -312,100 +310,11 @@ fn build_scope_map(content: &str, offset: u32) -> ScopeMap {
     let arena = Bump::new();
     let file_id = mago_database::file::FileId::new("extract_fn_scope");
     let program = mago_syntax::parser::parse_file_content(&arena, file_id, content);
-
-    for stmt in program.statements.iter() {
-        if let Some(map) = try_build_scope_from_statement(stmt, offset) {
-            return map;
-        }
-    }
-
-    // Fallback: top-level scope.
-    let body_end = content.len() as u32;
-    collect_scope(program.statements.as_slice(), 0, body_end)
-}
-
-/// Recursively try to build a scope map from a statement.
-fn try_build_scope_from_statement(stmt: &Statement<'_>, offset: u32) -> Option<ScopeMap> {
-    match stmt {
-        Statement::Function(func) => {
-            let body_start = func.body.left_brace.start.offset;
-            let body_end = func.body.right_brace.end.offset;
-            if offset >= body_start && offset <= body_end {
-                return Some(collect_function_scope(
-                    &func.parameter_list,
-                    func.body.statements.as_slice(),
-                    body_start,
-                    body_end,
-                ));
-            }
-        }
-        Statement::Class(class) => {
-            for member in class.members.iter() {
-                if let ClassLikeMember::Method(method) = member
-                    && let MethodBody::Concrete(block) = &method.body
-                {
-                    let body_start = block.left_brace.start.offset;
-                    let body_end = block.right_brace.end.offset;
-                    if offset >= body_start && offset <= body_end {
-                        return Some(collect_function_scope_with_kind(
-                            &method.parameter_list,
-                            block.statements.as_slice(),
-                            body_start,
-                            body_end,
-                            FrameKind::Method,
-                        ));
-                    }
-                }
-            }
-        }
-        Statement::Trait(tr) => {
-            for member in tr.members.iter() {
-                if let ClassLikeMember::Method(method) = member
-                    && let MethodBody::Concrete(block) = &method.body
-                {
-                    let body_start = block.left_brace.start.offset;
-                    let body_end = block.right_brace.end.offset;
-                    if offset >= body_start && offset <= body_end {
-                        return Some(collect_function_scope_with_kind(
-                            &method.parameter_list,
-                            block.statements.as_slice(),
-                            body_start,
-                            body_end,
-                            FrameKind::Method,
-                        ));
-                    }
-                }
-            }
-        }
-        Statement::Enum(en) => {
-            for member in en.members.iter() {
-                if let ClassLikeMember::Method(method) = member
-                    && let MethodBody::Concrete(block) = &method.body
-                {
-                    let body_start = block.left_brace.start.offset;
-                    let body_end = block.right_brace.end.offset;
-                    if offset >= body_start && offset <= body_end {
-                        return Some(collect_function_scope_with_kind(
-                            &method.parameter_list,
-                            block.statements.as_slice(),
-                            body_start,
-                            body_end,
-                            FrameKind::Method,
-                        ));
-                    }
-                }
-            }
-        }
-        Statement::Namespace(ns) => {
-            for inner in ns.statements().iter() {
-                if let Some(map) = try_build_scope_from_statement(inner, offset) {
-                    return Some(map);
-                }
-            }
-        }
-        _ => {}
-    }
-    None
+    crate::scope_collector::build_scope_map_for_offset(
+        program.statements.as_slice(),
+        offset,
+        content.len() as u32,
+    )
 }
 
 // ─── Type resolution ────────────────────────────────────────────────────────

src/code_actions/extract_variable.rs

@@ -12,7 +12,7 @@ use tower_lsp::lsp_types::*;
 use crate::Backend;
 use crate::code_actions::{CodeActionData, make_code_action_data};
 use crate::parser::with_parsed_program;
-use crate::scope_collector::{ScopeMap, collect_function_scope, collect_scope};
+use crate::scope_collector::ScopeMap;
 use crate::util::{find_identical_occurrences, offset_to_position, position_to_byte_offset};
 
 // ─── Name generation ────────────────────────────────────────────────────────
@@ -583,80 +583,12 @@ fn find_enclosing_statement_line(content: &str, selection_start: usize) -> (usiz
 /// This finds the enclosing function/method scope or falls back to
 /// top-level scope.
 fn build_scope_map(content: &str, offset: u32) -> ScopeMap {
-    use mago_syntax::ast::*;
-
-    with_parsed_program(content, "extract_variable", |program, _content| {
-        // Try to find the enclosing function or method.
-        for stmt in program.statements.iter() {
-            if let Statement::Function(func) = stmt {
-                let body_start = func.body.left_brace.start.offset;
-                let body_end = func.body.right_brace.end.offset;
-                if offset >= body_start && offset <= body_end {
-                    return collect_function_scope(
-                        &func.parameter_list,
-                        func.body.statements.as_slice(),
-                        body_start,
-                        body_end,
-                    );
-                }
-            }
-            if let Statement::Class(class) = stmt {
-                for member in class.members.iter() {
-                    if let ClassLikeMember::Method(method) = member
-                        && let MethodBody::Concrete(block) = &method.body
-                    {
-                        let body_start = block.left_brace.start.offset;
-                        let body_end = block.right_brace.end.offset;
-                        if offset >= body_start && offset <= body_end {
-                            return collect_function_scope(
-                                &method.parameter_list,
-                                block.statements.as_slice(),
-                                body_start,
-                                body_end,
-                            );
-                        }
-                    }
-                }
-            }
-            if let Statement::Namespace(ns) = stmt {
-                for inner_stmt in ns.statements().iter() {
-                    if let Statement::Function(func) = inner_stmt {
-                        let body_start = func.body.left_brace.start.offset;
-                        let body_end = func.body.right_brace.end.offset;
-                        if offset >= body_start && offset <= body_end {
-                            return collect_function_scope(
-                                &func.parameter_list,
-                                func.body.statements.as_slice(),
-                                body_start,
-                                body_end,
-                            );
-                        }
-                    }
-                    if let Statement::Class(class) = inner_stmt {
-                        for member in class.members.iter() {
-                            if let ClassLikeMember::Method(method) = member
-                                && let MethodBody::Concrete(block) = &method.body
-                            {
-                                let body_start = block.left_brace.start.offset;
-                                let body_end = block.right_brace.end.offset;
-                                if offset >= body_start && offset <= body_end {
-                                    return collect_function_scope(
-                                        &method.parameter_list,
-                                        block.statements.as_slice(),
-                                        body_start,
-                                        body_end,
-                                    );
-                                }
-                            }
-                        }
-                    }
-                }
-            }
-        }
-
-        // Fallback: top-level scope
-        let body_end = content.len() as u32;
-        collect_scope(program.statements.as_slice(), 0, body_end)
+    with_parsed_program(content, "extract_variable", |program, content| {
+        crate::scope_collector::build_scope_map_for_offset(
+            program.statements.as_slice(),
+            offset,
+            content.len() as u32,
+        )
     })
 }
 

src/code_actions/inline_variable.rs

@@ -27,7 +27,7 @@ use tower_lsp::lsp_types::*;
 use crate::Backend;
 use crate::code_actions::{CodeActionData, make_code_action_data};
 use crate::parser::with_parsed_program;
-use crate::scope_collector::{AccessKind, ScopeMap, collect_function_scope, collect_scope};
+use crate::scope_collector::{AccessKind, ScopeMap};
 use crate::util::{offset_to_position, position_to_byte_offset};
 
 // ─── AST helpers ────────────────────────────────────────────────────────────
@@ -487,104 +487,15 @@ fn expression_has_side_effects(expr: &Expression<'_>) -> bool {
 /// Build a `ScopeMap` for the file by walking the AST, identical to the
 /// approach used in extract_variable.
 fn build_scope_map(content: &str, offset: u32) -> ScopeMap {
-    with_parsed_program(content, "inline_variable", |program, _content| {
-        // Try to find the enclosing function or method.
-        for stmt in program.statements.iter() {
-            if let Some(map) = try_build_scope_from_statement(stmt, offset) {
-                return map;
-            }
-        }
-
-        // Fallback: top-level scope.
-        let body_end = content.len() as u32;
-        collect_scope(program.statements.as_slice(), 0, body_end)
+    with_parsed_program(content, "inline_variable", |program, content| {
+        crate::scope_collector::build_scope_map_for_offset(
+            program.statements.as_slice(),
+            offset,
+            content.len() as u32,
+        )
     })
 }
 
-fn try_build_scope_from_statement(stmt: &Statement<'_>, offset: u32) -> Option<ScopeMap> {
-    match stmt {
-        Statement::Function(func) => {
-            let body_start = func.body.left_brace.start.offset;
-            let body_end = func.body.right_brace.end.offset;
-            if offset >= body_start && offset <= body_end {
-                return Some(collect_function_scope(
-                    &func.parameter_list,
-                    func.body.statements.as_slice(),
-                    body_start,
-                    body_end,
-                ));
-            }
-            None
-        }
-        Statement::Class(class) => {
-            for member in class.members.iter() {
-                if let ClassLikeMember::Method(method) = member
-                    && let MethodBody::Concrete(block) = &method.body
-                {
-                    let body_start = block.left_brace.start.offset;
-                    let body_end = block.right_brace.end.offset;
-                    if offset >= body_start && offset <= body_end {
-                        return Some(collect_function_scope(
-                            &method.parameter_list,
-                            block.statements.as_slice(),
-                            body_start,
-                            body_end,
-                        ));
-                    }
-                }
-            }
-            None
-        }
-        Statement::Trait(tr) => {
-            for member in tr.members.iter() {
-                if let ClassLikeMember::Method(method) = member
-                    && let MethodBody::Concrete(block) = &method.body
-                {
-                    let body_start = block.left_brace.start.offset;
-                    let body_end = block.right_brace.end.offset;
-                    if offset >= body_start && offset <= body_end {
-                        return Some(collect_function_scope(
-                            &method.parameter_list,
-                            block.statements.as_slice(),
-                            body_start,
-                            body_end,
-                        ));
-                    }
-                }
-            }
-            None
-        }
-        Statement::Enum(en) => {
-            for member in en.members.iter() {
-                if let ClassLikeMember::Method(method) = member
-                    && let MethodBody::Concrete(block) = &method.body
-                {
-                    let body_start = block.left_brace.start.offset;
-                    let body_end = block.right_brace.end.offset;
-                    if offset >= body_start && offset <= body_end {
-                        return Some(collect_function_scope(
-                            &method.parameter_list,
-                            block.statements.as_slice(),
-                            body_start,
-                            body_end,
-                        ));
-                    }
-                }
-            }
-            None
-        }
-        Statement::Namespace(ns) => {
-            for inner_stmt in ns.statements().iter() {
-                if let Some(map) = try_build_scope_from_statement(inner_stmt, offset) {
-                    return Some(map);
-                }
-            }
-            None
-        }
-        _ => None,
-    }
-}
-
 // ─── Line deletion helpers ──────────────────────────────────────────────────
 
 /// Compute the byte range for deleting an entire statement line.