diff --git a/src/references/mod.rs b/src/references/mod.rs index 1edd0dd5..e784d622 100644 --- a/src/references/mod.rs +++ b/src/references/mod.rs @@ -50,6 +50,31 @@ impl Backend { content: &str, position: Position, include_declaration: bool, + ) -> Option> { + self.find_references_with_member_mode(uri, content, position, include_declaration, true) + } + + /// Like [`find_references`], but excludes unresolved member-call matches. + /// + /// Rename uses this stricter mode so same-named methods on unrelated or + /// unknown receiver types are not renamed conservatively. + pub(crate) fn find_references_for_rename( + &self, + uri: &str, + content: &str, + position: Position, + include_declaration: bool, + ) -> Option> { + self.find_references_with_member_mode(uri, content, position, include_declaration, false) + } + + fn find_references_with_member_mode( + &self, + uri: &str, + content: &str, + position: Position, + include_declaration: bool, + allow_unresolved_member_subjects: bool, ) -> Option> { let start_total = std::time::Instant::now(); tracing::info!( @@ -76,6 +101,7 @@ impl Backend { content, sym.start, include_declaration, + allow_unresolved_member_subjects, ); tracing::info!( "Find References: total time for {:?}: {:?}", @@ -118,6 +144,7 @@ impl Backend { content: &str, span_start: u32, include_declaration: bool, + allow_unresolved_member_subjects: bool, ) -> Vec { match kind { SymbolKind::Variable { name } => { @@ -146,11 +173,15 @@ impl Backend { // Resolve the enclosing class to scope the search. let hierarchy = self.resolve_member_declaration_hierarchy(uri, span_start, name, is_static); + let declaration_scope = + self.resolve_member_declaration_scope(uri, span_start, name, is_static); return self.find_member_references( name, is_static, include_declaration, hierarchy.as_ref(), + declaration_scope.as_ref(), + allow_unresolved_member_subjects, ); } self.find_variable_references(uri, content, name, span_start, include_declaration) @@ -177,7 +208,7 @@ impl Backend { } => { // Resolve the subject to determine the class hierarchy // so we only return references on related classes. - let hierarchy = self.resolve_member_access_hierarchy( + let (hierarchy, declaration_scope) = self.resolve_member_access_scopes( uri, subject_text, *is_static, @@ -190,6 +221,8 @@ impl Backend { *is_static, include_declaration, hierarchy.as_ref(), + declaration_scope.as_ref(), + allow_unresolved_member_subjects, ) } SymbolKind::FunctionCall { name, .. } => { @@ -204,11 +237,15 @@ impl Backend { // Resolve the enclosing class to scope the search. let hierarchy = self.resolve_member_declaration_hierarchy(uri, span_start, name, *is_static); + let declaration_scope = + self.resolve_member_declaration_scope(uri, span_start, name, *is_static); self.find_member_references( name, *is_static, include_declaration, hierarchy.as_ref(), + declaration_scope.as_ref(), + allow_unresolved_member_subjects, ) } SymbolKind::SelfStaticParent(ssp_kind) => { @@ -820,6 +857,8 @@ impl Backend { target_is_static: bool, include_declaration: bool, hierarchy: Option<&HashSet>, + declaration_scope: Option<&HashSet>, + allow_unresolved_subjects: bool, ) -> Vec { let mut locations = Vec::new(); @@ -916,15 +955,15 @@ impl Backend { span.start, content, ); - if !subject_fqns.is_empty() - && !subject_fqns.iter().any(|fqn| hier.contains(fqn)) - { + if subject_fqns.is_empty() { + if !allow_unresolved_subjects { + continue; + } + } else if !subject_fqns.iter().any(|fqn| hier.contains(fqn)) { // Subject resolved but none of the resolved // classes are in the target hierarchy — skip. continue; } - // If subject_fqns is empty, we couldn't resolve - // the subject — include conservatively. } if file_content.is_none() { @@ -949,7 +988,12 @@ impl Backend { } // Check if the enclosing class is in the hierarchy. - if let Some(hier) = hierarchy { + let declaration_filter = if *is_static == target_is_static { + declaration_scope.or(hierarchy) + } else { + hierarchy + }; + if let Some(hier) = declaration_filter { let ctx = file_ctx_cell.get_or_init(|| self.file_context(file_uri)); let enclosing = find_class_at_offset(&ctx.classes, span.start).or_else(|| { @@ -996,7 +1040,7 @@ impl Backend { if include_declaration && let Some(classes) = self.get_classes_for_uri(file_uri) { for class in &classes { // Filter by hierarchy when available. - if let Some(hier) = hierarchy { + if let Some(hier) = declaration_scope.or(hierarchy) { let class_fqn = class.fqn().to_string(); if !hier.contains(&class_fqn) { continue; @@ -1261,22 +1305,27 @@ impl Backend { /// /// Returns `Some(set_of_fqns)` when the subject can be resolved to at /// least one class, or `None` when resolution fails entirely. - fn resolve_member_access_hierarchy( + fn resolve_member_access_scopes( &self, uri: &str, subject_text: &str, is_static: bool, span_start: u32, member_name: &str, - ) -> Option> { + ) -> (Option>, Option>) { let ctx = self.file_context(uri); - let content = self.get_file_content(uri)?; + let Some(content) = self.get_file_content(uri) else { + return (None, None); + }; let fqns = self.resolve_subject_to_fqns(subject_text, is_static, &ctx, span_start, &content); if fqns.is_empty() { - return None; + return (None, None); } - Some(self.collect_hierarchy_for_fqns(&fqns, Some(member_name), is_static)) + ( + Some(self.collect_hierarchy_for_fqns(&fqns)), + self.collect_declaring_seed_scope(&fqns, member_name, is_static), + ) } /// Resolve the class hierarchy for a `MemberDeclaration` at a given offset. @@ -1286,8 +1335,8 @@ impl Backend { &self, uri: &str, offset: u32, - member_name: &str, - is_static: bool, + _member_name: &str, + _is_static: bool, ) -> Option> { let classes: Vec> = self .uri_classes_index @@ -1306,7 +1355,34 @@ impl Backend { .min_by_key(|c| c.start_offset) })?; let fqn = current_class.fqn().to_string(); - Some(self.collect_hierarchy_for_fqns(&[fqn], Some(member_name), is_static)) + Some(self.collect_hierarchy_for_fqns(&[fqn])) + } + + fn resolve_member_declaration_scope( + &self, + uri: &str, + offset: u32, + member_name: &str, + is_static: bool, + ) -> Option> { + let classes: Vec> = self + .uri_classes_index + .read() + .get(uri) + .cloned() + .unwrap_or_default(); + let current_class = find_class_at_offset(&classes, offset).or_else(|| { + classes + .iter() + .map(|c| c.as_ref()) + .filter(|c| c.keyword_offset > 0 && offset < c.start_offset) + .min_by_key(|c| c.start_offset) + })?; + self.collect_declaring_seed_scope( + &[current_class.fqn().to_string()], + member_name, + is_static, + ) } /// Resolve a member access subject to zero or more class FQNs. @@ -1372,12 +1448,7 @@ impl Backend { /// - All ancestor FQNs (parent chain, interfaces, traits) /// - All descendant FQNs (classes that extend/implement any class in /// the hierarchy) - fn collect_hierarchy_for_fqns( - &self, - seed_fqns: &[String], - member_name: Option<&str>, - is_static: bool, - ) -> HashSet { + fn collect_hierarchy_for_fqns(&self, seed_fqns: &[String]) -> HashSet { let mut hierarchy = HashSet::new(); let class_loader = |name: &str| -> Option> { self.find_or_load_class(name) }; @@ -1406,15 +1477,21 @@ impl Backend { extensions.push(normalize_fqn(builder_fqn).to_string()); } } - for ext_fqn in extensions { + for ext_fqn in &extensions { if hierarchy.insert(ext_fqn.clone()) { - self.collect_ancestors(&ext_fqn, &class_loader, &mut hierarchy); + self.collect_ancestors(ext_fqn, &class_loader, &mut hierarchy); } } // Bridge Laravel Builders back to their Models. - // If a class in the hierarchy is a custom builder, find which models - // use it and add them to the hierarchy. + // Only builder roots that are actually part of the original lookup + // should contribute models. A custom builder's ancestors include the + // base Eloquent builder, but that must not fan out into every model. + let builder_roots: HashSet = seed_fqns + .iter() + .map(|fqn| normalize_fqn(fqn).to_string()) + .chain(extensions.iter().cloned()) + .collect(); let mut model_seeds = Vec::new(); { let class_index = self.fqn_class_index.read(); @@ -1426,10 +1503,10 @@ impl Backend { .and_then(|b| b.base_name()) .map(normalize_fqn) { - if hierarchy.contains(normalized.as_str()) { + if builder_roots.contains(normalized.as_str()) { model_seeds.push(class_fqn.clone()); } - } else if hierarchy + } else if builder_roots .contains(crate::virtual_members::laravel::ELOQUENT_BUILDER_FQN) { // All models use the base Eloquent Builder by default. @@ -1438,24 +1515,24 @@ impl Backend { } } } - for model_fqn in model_seeds { - if hierarchy.insert(normalize_fqn(&model_fqn).to_string()) { - self.collect_ancestors(&model_fqn, &class_loader, &mut hierarchy); + for model_fqn in &model_seeds { + if hierarchy.insert(normalize_fqn(model_fqn).to_string()) { + self.collect_ancestors(model_fqn, &class_loader, &mut hierarchy); } } - // Walk down: collect all descendants using the global GTI index. - // We only walk down from a class if it actually defines the member - // (or if no member name was provided). + // Walk down: collect descendants from the original target classes, + // not every ancestor. This keeps a concrete class rename from + // fanning out through an implemented interface into sibling classes. let mut queue: std::collections::VecDeque = std::collections::VecDeque::new(); - if let Some(name) = member_name { - for fqn in &hierarchy { - if self.defines_member(fqn, name, is_static, &class_loader) { - queue.push_back(fqn.clone()); - } - } - } else { - queue.extend(hierarchy.iter().cloned()); + for fqn in seed_fqns { + queue.push_back(normalize_fqn(fqn).to_string()); + } + for ext_fqn in &extensions { + queue.push_back(ext_fqn.clone()); + } + for model_fqn in &model_seeds { + queue.push_back(normalize_fqn(model_fqn).to_string()); } let gti = self.gti_index.read(); @@ -1473,6 +1550,40 @@ impl Backend { hierarchy } + fn collect_declaring_seed_scope( + &self, + seed_fqns: &[String], + member_name: &str, + is_static: bool, + ) -> Option> { + let class_loader = |name: &str| -> Option> { self.find_or_load_class(name) }; + let declaring_seeds: Vec = seed_fqns + .iter() + .map(|fqn| normalize_fqn(fqn).to_string()) + .filter(|fqn| self.defines_member(fqn, member_name, is_static, &class_loader)) + .collect(); + + if declaring_seeds.is_empty() { + return None; + } + + let mut scope: HashSet = declaring_seeds.iter().cloned().collect(); + let mut queue: std::collections::VecDeque = declaring_seeds.into(); + let gti = self.gti_index.read(); + while let Some(fqn) = queue.pop_front() { + if let Some(descendants) = gti.get(&fqn) { + for desc in descendants { + let normalized = normalize_fqn(desc).to_string(); + if scope.insert(normalized.clone()) { + queue.push_back(normalized); + } + } + } + } + + Some(scope) + } + fn defines_member( &self, fqn: &str, @@ -1484,7 +1595,6 @@ impl Backend { return false; }; - // Real methods if cls .methods .iter() @@ -1493,25 +1603,19 @@ impl Backend { return true; } - // Laravel forwarded methods if let Some(laravel) = cls.laravel() { if let Some(builder_cls) = laravel .custom_builder .as_ref() .and_then(|b| b.base_name()) .and_then(class_loader) - { - // Forwarded methods are instance methods on the builder - // but called statically on the model. - if builder_cls + && builder_cls .methods .iter() .any(|m| m.name.eq_ignore_ascii_case(name) && (!is_static || !m.is_static)) - { - return true; - } + { + return true; } - // Standard builder forwarding if class_loader(crate::virtual_members::laravel::ELOQUENT_BUILDER_FQN) .filter(|bc| { bc.methods diff --git a/src/rename/mod.rs b/src/rename/mod.rs index bc3d0f9e..fc356d89 100644 --- a/src/rename/mod.rs +++ b/src/rename/mod.rs @@ -130,7 +130,7 @@ impl Backend { let class_rename_fqn = self.resolve_class_rename_fqn(&span.kind, uri, span.start); // Find all references (including the declaration). - let locations = self.find_references(uri, content, position, true)?; + let locations = self.find_references_for_rename(uri, content, position, true)?; if locations.is_empty() { return None; diff --git a/src/rename/tests.rs b/src/rename/tests.rs index f4e9d7c0..84d9df7a 100644 --- a/src/rename/tests.rs +++ b/src/rename/tests.rs @@ -795,6 +795,343 @@ async fn rename_method_does_not_leak_to_unrelated_class() { ); } +#[tokio::test] +async fn rename_private_method_excludes_unresolved_same_name_calls() { + let backend = Backend::new_test(); + let uri = Url::parse("file:///test.php").unwrap(); + let text = concat!( + "recalculate();\n", + " }\n", + "}\n", + "class Other {\n", + " public function recalculate(): void {}\n", + "}\n", + "function demo($unknown, Other $other): void {\n", + " $unknown->recalculate();\n", + " $other->recalculate();\n", + "}\n", + ); + + open_file(&backend, &uri, text).await; + + let edit = rename(&backend, &uri, 2, 25, "calculate").await; + assert!(edit.is_some(), "Rename should produce edits"); + + let file_edits = edits_for_uri(&edit.unwrap(), &uri); + let result = apply_edits(text, &file_edits); + + assert!( + result.contains("private function calculate(): void {}"), + "Private declaration should be renamed; got:\n{}", + result + ); + assert!( + result.contains("$this->calculate()"), + "$this call should be renamed; got:\n{}", + result + ); + assert!( + result.contains("$unknown->recalculate()"), + "Unresolved same-name call should remain unchanged; got:\n{}", + result + ); + assert!( + result.contains("$other->recalculate()"), + "Resolved unrelated class call should remain unchanged; got:\n{}", + result + ); +} + +#[tokio::test] +async fn rename_listener_private_method_cross_file_stays_scoped() { + let backend = Backend::new_test(); + let uri_a = Url::parse("file:///Listener.php").unwrap(); + let uri_b = Url::parse("file:///Other.php").unwrap(); + let uri_c = Url::parse("file:///Use.php").unwrap(); + + let text_a = concat!( + "recalculate();\n", + " }\n", + "}\n", + ); + + let text_b = concat!( + "recalculate();\n", + " $service->recalculate();\n", + "}\n", + ); + + open_file(&backend, &uri_a, text_a).await; + open_file(&backend, &uri_b, text_b).await; + open_file(&backend, &uri_c, text_c).await; + + let edit = rename(&backend, &uri_a, 2, 25, "calculate").await; + assert!(edit.is_some(), "Rename should produce edits"); + + let edit = edit.unwrap(); + let edits_a = edits_for_uri(&edit, &uri_a); + let edits_b = edits_for_uri(&edit, &uri_b); + let edits_c = edits_for_uri(&edit, &uri_c); + + let result_a = apply_edits(text_a, &edits_a); + let result_b = apply_edits(text_b, &edits_b); + let result_c = apply_edits(text_c, &edits_c); + + assert!( + result_a.contains("private function calculate(): void {}"), + "Listener private method should be renamed; got:\n{}", + result_a + ); + assert!( + result_a.contains("$this->calculate()"), + "Listener self-call should be renamed; got:\n{}", + result_a + ); + assert!( + result_b.contains("public function recalculate(): void {}"), + "Unrelated class declaration should stay unchanged; got:\n{}", + result_b + ); + assert!( + result_c.contains("$unknown->recalculate()"), + "Unknown receiver call should stay unchanged; got:\n{}", + result_c + ); + assert!( + result_c.contains("$service->recalculate()"), + "Unrelated typed receiver call should stay unchanged; got:\n{}", + result_c + ); +} + +#[tokio::test] +async fn rename_method_on_implementation_does_not_leak_to_sibling_implementors() { + let backend = Backend::new_test(); + let uri = Url::parse("file:///test.php").unwrap(); + let text = concat!( + "recalculate();\n", + " $worker->recalculate();\n", + "}\n", + ); + + open_file(&backend, &uri, text).await; + + let edit = rename(&backend, &uri, 5, 20, "calculate").await; + assert!(edit.is_some(), "Rename should produce edits"); + + let file_edits = edits_for_uri(&edit.unwrap(), &uri); + let result = apply_edits(text, &file_edits); + + assert!( + result.contains( + "class Listener implements Recalculates {\n public function calculate(): void {}" + ), + "Implementation declaration should be renamed; got:\n{}", + result + ); + assert!( + result.contains("$listener->calculate()"), + "Listener call should be renamed; got:\n{}", + result + ); + assert!( + result.contains("interface Recalculates {\n public function recalculate(): void;"), + "Interface declaration should remain unchanged; got:\n{}", + result + ); + assert!( + result.contains( + "class Worker implements Recalculates {\n public function recalculate(): void {}" + ), + "Sibling implementation should remain unchanged; got:\n{}", + result + ); + assert!( + result.contains("$worker->recalculate()"), + "Sibling call should remain unchanged; got:\n{}", + result + ); +} + +#[tokio::test] +async fn rename_interface_method_updates_implementations_and_calls() { + let backend = Backend::new_test(); + let uri = Url::parse("file:///test.php").unwrap(); + let text = concat!( + "recalculate();\n", + " $listener->recalculate();\n", + " $worker->recalculate();\n", + "}\n", + ); + + open_file(&backend, &uri, text).await; + + let edit = rename(&backend, &uri, 2, 20, "calculate").await; + assert!(edit.is_some(), "Rename should produce edits"); + + let file_edits = edits_for_uri(&edit.unwrap(), &uri); + let result = apply_edits(text, &file_edits); + + assert!( + result.contains("interface Recalculates {\n public function calculate(): void;"), + "Interface declaration should be renamed; got:\n{}", + result + ); + assert!( + result.contains( + "class Listener implements Recalculates {\n public function calculate(): void {}" + ), + "First implementation should be renamed; got:\n{}", + result + ); + assert!( + result.contains( + "class Worker implements Recalculates {\n public function calculate(): void {}" + ), + "Second implementation should be renamed; got:\n{}", + result + ); + assert!( + result.contains("$item->calculate()"), + "Interface-typed call should be renamed; got:\n{}", + result + ); + assert!( + result.contains("$listener->calculate()"), + "Listener call should be renamed; got:\n{}", + result + ); + assert!( + result.contains("$worker->calculate()"), + "Worker call should be renamed; got:\n{}", + result + ); +} + +#[tokio::test] +async fn rename_child_override_stays_on_child_branch() { + let backend = Backend::new_test(); + let uri = Url::parse("file:///test.php").unwrap(); + let text = concat!( + "recalculate();\n", + " }\n", + "}\n", + "class OrderListener extends BaseListener {\n", + " protected function recalculate(): void {\n", + " $this->recalculate();\n", + " }\n", + "}\n", + ); + + open_file(&backend, &uri, text).await; + + let edit = rename(&backend, &uri, 5, 24, "calculate").await; + assert!(edit.is_some(), "Rename should produce edits"); + + let file_edits = edits_for_uri(&edit.unwrap(), &uri); + let result = apply_edits(text, &file_edits); + + assert!( + result.contains("class InvoiceListener extends BaseListener {\n protected function calculate(): void {\n $this->calculate();"), + "Child override should be renamed with its self-call; got:\n{}", + result + ); + assert!( + result.contains("class BaseListener {\n protected function recalculate(): void {}"), + "Parent declaration should remain unchanged; got:\n{}", + result + ); + assert!( + result.contains("class OrderListener extends BaseListener {\n protected function recalculate(): void {\n $this->recalculate();"), + "Sibling override branch should remain unchanged; got:\n{}", + result + ); +} + +#[tokio::test] +async fn rename_parent_method_updates_overrides_and_subclass_calls() { + let backend = Backend::new_test(); + let uri = Url::parse("file:///test.php").unwrap(); + let text = concat!( + "recalculate();\n", + " }\n", + " public function run(): void {\n", + " $this->recalculate();\n", + " }\n", + "}\n", + ); + + open_file(&backend, &uri, text).await; + + let edit = rename(&backend, &uri, 2, 24, "calculate").await; + assert!(edit.is_some(), "Rename should produce edits"); + + let file_edits = edits_for_uri(&edit.unwrap(), &uri); + let result = apply_edits(text, &file_edits); + + assert!( + result.contains("class BaseJob {\n protected function calculate(): void {}"), + "Parent declaration should be renamed; got:\n{}", + result + ); + assert!( + result.contains("class ChildJob extends BaseJob {\n protected function calculate(): void {\n $this->calculate();\n }\n public function run(): void {\n $this->calculate();"), + "Override and subclass calls should be renamed; got:\n{}", + result + ); +} + #[tokio::test] async fn rename_method_includes_inherited_class() { // Renaming a method on a parent class should also rename it on