diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index e330c52c1..f0957e8af 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Implementation CodeLens.** Interfaces, classes, and methods show clickable implementation counts once the workspace index is ready. Each lens opens every exact implementation location, including members inherited from a parent or supplied by a trait. Contributed by @sidux. - **`analyze` takes more than one path.** `phpantom_lsp analyze app/ lib/Helper.php tests/` scans the union of everything named, mixing directories and single files freely, so a pre-commit hook or a CI step can hand it exactly the paths that changed instead of running the whole project or invoking the binary once per path. Overlapping arguments are reported once, and a path that does not exist still stops the run with exit code 2. Naming no path scans the entire project, as before. ### Changed diff --git a/src/code_lens.rs b/src/code_lens.rs index 9061c1d85..21b417e4f 100644 --- a/src/code_lens.rs +++ b/src/code_lens.rs @@ -6,6 +6,7 @@ use tower_lsp::lsp_types::*; use crate::Backend; use crate::atom::Atom; +use crate::definition::MemberImplementationTarget; use crate::definition::member::MemberKind; use crate::text_position::offset_to_position; use crate::types::{ClassInfo, ClassLikeKind, MAX_INHERITANCE_DEPTH}; @@ -21,6 +22,17 @@ fn line_indent(content: &str, byte_offset: usize) -> u32 { .count() as u32 } +fn implementation_lens_title(count: usize) -> String { + format!( + "{count} {}", + if count == 1 { + "implementation" + } else { + "implementations" + } + ) +} + /// Information about a prototype (ancestor) method that a local method /// overrides or implements. struct Prototype { @@ -45,9 +57,36 @@ impl Backend { }; let mut lenses = Vec::new(); + let context = self.file_context(uri); + let class_loader = self.class_loader(&context); for class in &classes { let class_fqn = class.fqn(); + let implementation_descendants = if self + .workspace_indexed + .load(std::sync::atomic::Ordering::Acquire) + { + self.implementation_descendants(class, &class_loader, true) + } else { + Vec::new() + }; + + if !implementation_descendants.is_empty() { + let locations = self.class_implementation_locations_from_descendants( + uri, + content, + class, + &implementation_descendants, + ); + if let Some(lens) = self.build_locations_lens( + uri, + offset_to_position(content, class.keyword_offset as usize), + implementation_lens_title(locations.len()), + locations, + ) { + lenses.push(lens); + } + } if let Some(lens) = self.build_covers_lens(class, uri, content) { lenses.push(lens); @@ -75,6 +114,27 @@ impl Backend { }; let proto = self.find_prototype(class, &class_fqn, &method.name, uri, content); + if !implementation_descendants.is_empty() { + let locations = self.member_implementation_locations_from_descendants( + uri, + content, + MemberImplementationTarget { + class, + name: &method.name, + kind: MemberKind::Method, + }, + &implementation_descendants, + &class_loader, + ); + if let Some(lens) = self.build_locations_lens( + uri, + pos, + implementation_lens_title(locations.len()), + locations, + ) { + lenses.push(lens); + } + } if let Some(proto) = proto { let icon = if proto.is_interface { "◆" } else { "↑" }; let title = format!("{} {}::{}", icon, proto.ancestor_name, method.name); @@ -102,6 +162,35 @@ impl Backend { } } + fn build_locations_lens( + &self, + origin_uri: &str, + source_position: Position, + title: String, + locations: Vec, + ) -> Option { + if locations.is_empty() { + return None; + } + let origin_uri = Url::parse(origin_uri).ok()?; + Some(CodeLens { + range: Range::new( + Position::new(source_position.line, 0), + Position::new(source_position.line, 0), + ), + command: Some(Command { + title, + command: "editor.action.showReferences".to_string(), + arguments: Some(vec![ + serde_json::json!(origin_uri), + serde_json::json!(source_position), + serde_json::json!(locations), + ]), + }), + data: None, + }) + } + /// Build the "which tests cover this class" lens for a class /// declaration, from the test classes whose PHPUnit coverage metadata /// (`@covers` / `@uses` / `#[CoversClass]` and friends) names it. diff --git a/src/definition/implementation.rs b/src/definition/implementation.rs index ec60275b8..9c9c50fb9 100644 --- a/src/definition/implementation.rs +++ b/src/definition/implementation.rs @@ -36,7 +36,7 @@ use std::sync::atomic::Ordering; use tower_lsp::lsp_types::*; use super::member::MemberKind; -use super::point_location; +use super::{MemberImplementationTarget, point_location}; use crate::Backend; use crate::class_lookup::find_class_at_offset; use crate::config::IndexingStrategy; @@ -147,61 +147,65 @@ impl Backend { .or_else(|| class_loader(name)) .map(Arc::unwrap_or_clone)?; - // Final classes cannot be extended, so there are no implementations. - if target.is_final { - return None; - } - - // Whether the target is a concrete (non-abstract, non-interface) - // class. When it is, we include abstract subclasses in the - // results because the user is exploring the class hierarchy - // rather than looking for instantiable implementations. - let target_is_concrete = target.kind != ClassLikeKind::Interface && !target.is_abstract; - - let target_short = target.name; - // Compute target FQN from the class's own namespace (most - // reliable), then fall back to fqn_uri_index, then to the FQN we - // resolved from the use-map, and finally to the short name. - let target_fqn = { - let from_class = crate::util::build_fqn(&target.name, target.file_namespace.as_deref()); - if from_class.contains('\\') { - from_class - } else { - self.class_fqn_for_short(&target_short).unwrap_or_else(|| { - if fqn.contains('\\') { - fqn.clone() - } else { - target_short.to_string() - } - }) - } - }; + let locations = + self.class_implementation_locations(uri, content, &target, &class_loader, false); + (!locations.is_empty()).then_some(locations) + } - let implementors = self.find_implementors( - &target_short, - &target_fqn, - &class_loader, - target_is_concrete, - false, - false, - ); + /// Return navigable declarations for the descendants of `target`. + /// + /// This is shared by go-to-implementation and code lenses + /// so their counts and targets stay identical. + pub(crate) fn class_implementation_locations( + &self, + uri: &str, + content: &str, + target: &ClassInfo, + class_loader: &dyn Fn(&str) -> Option>, + project_only: bool, + ) -> Vec { + let descendants = self.implementation_descendants(target, class_loader, project_only); + self.class_implementation_locations_from_descendants(uri, content, target, &descendants) + } - if implementors.is_empty() { - return None; + /// Resolve the descendant classes once for callers that need class and + /// member implementation data for the same declaration. + pub(crate) fn implementation_descendants( + &self, + target: &ClassInfo, + class_loader: &dyn Fn(&str) -> Option>, + project_only: bool, + ) -> Vec { + if target.is_final || matches!(target.kind, ClassLikeKind::Trait | ClassLikeKind::Enum) { + return Vec::new(); } - let mut locations = Vec::new(); - for imp in &implementors { - if let Some(loc) = self.locate_class_declaration(imp, uri, content) { - locations.push(loc); - } - } + let target_fqn = target.fqn(); + self.find_implementors( + &target.name, + &target_fqn, + class_loader, + true, + false, + project_only, + ) + } - if locations.is_empty() { - None - } else { - Some(locations) - } + pub(crate) fn class_implementation_locations_from_descendants( + &self, + uri: &str, + content: &str, + target: &ClassInfo, + descendants: &[ClassInfo], + ) -> Vec { + let include_abstract = target.kind == ClassLikeKind::Class && !target.is_abstract; + let mut locations = descendants + .iter() + .filter(|descendant| include_abstract || !descendant.is_abstract) + .filter_map(|implementor| self.locate_class_declaration(implementor, uri, content)) + .collect::>(); + sort_and_dedup_locations(&mut locations); + locations } /// Reverse jump: from a method definition in a concrete class to the @@ -395,14 +399,6 @@ impl Backend { member_name: &str, class_loader: &dyn Fn(&str) -> Option>, ) -> Option> { - let target_short = &interface_class.name; - let target_fqn = self.implementor_target_fqn(interface_class); - - // Abstract classes are included: a class being abstract says - // nothing about whether the queried method has a body in it. - let implementors = - self.find_implementors(target_short, &target_fqn, class_loader, true, false, false); - let member_kind = if interface_class .methods .iter() @@ -419,96 +415,91 @@ impl Backend { MemberKind::Constant }; - let mut locations = Vec::new(); - for imp in &implementors { - if let Some(loc) = self.locate_member_implementation( - imp, - member_name, - member_kind, - class_loader, - uri, - content, - ) && !locations.contains(&loc) - { - locations.push(loc); - } - } - - if locations.is_empty() { - None - } else { - Some(locations) - } + let locations = self.member_implementation_locations( + uri, + content, + MemberImplementationTarget { + class: interface_class, + name: member_name, + kind: member_kind, + }, + class_loader, + false, + ); + (!locations.is_empty()).then_some(locations) } - /// The location of the implementation of `member_name` that `imp` - /// provides, or `None` when it provides none. + /// Return the descendant declarations that implement or override a + /// member declared on `target`. /// - /// The definition to jump to is the one `imp` declares itself or, when - /// `imp` only inherits the member, the one declared by the nearest - /// ancestor that has a body — a concrete class that inherits a method - /// unchanged still implements it, it just implements it elsewhere. A - /// method that is only ever re-declared `abstract` is another - /// declaration rather than an implementation, so it is skipped. - fn locate_member_implementation( + /// Each target is the concrete declaration that supplies the member, + /// including a trait or parent declaration inherited by a descendant. + /// Shared inherited declarations are deduplicated, and abstract + /// re-declarations are not implementations. + pub(crate) fn member_implementation_locations( &self, - imp: &ClassInfo, - member_name: &str, - member_kind: MemberKind, + uri: &str, + content: &str, + target: MemberImplementationTarget<'_>, class_loader: &dyn Fn(&str) -> Option>, - current_uri: &str, - current_content: &str, - ) -> Option { - let declares = |cls: &ClassInfo| match member_kind { - MemberKind::Method => cls - .get_method_ci(member_name) - .is_some_and(|m| !m.is_abstract && !m.is_virtual), - MemberKind::Property => cls.properties.iter().any(|p| p.name == member_name), - MemberKind::Constant => cls.constants.iter().any(|c| c.name == member_name), - }; - - let locate = |cls: &ClassInfo| -> Option { - let cls_fqn = crate::util::build_fqn(&cls.name, cls.file_namespace.as_deref()); - let (class_uri, class_content) = - self.find_class_file_content(&cls_fqn, current_uri, current_content)?; - let member_pos = - Self::find_member_position_in_class(&class_content, member_name, member_kind, cls)?; - Some(point_location(Url::parse(&class_uri).ok()?, member_pos)) - }; - - if declares(imp) { - return locate(imp); - } + project_only: bool, + ) -> Vec { + let descendants = self.implementation_descendants(target.class, class_loader, project_only); + self.member_implementation_locations_from_descendants( + uri, + content, + target, + &descendants, + class_loader, + ) + } - let mut current = imp.parent_class; - let mut depth = 0u32; - while let Some(parent_name) = current { - if depth >= MAX_INHERITANCE_DEPTH { - break; - } - depth += 1; - let Some(parent_cls) = class_loader(&parent_name) else { - break; + pub(crate) fn member_implementation_locations_from_descendants( + &self, + uri: &str, + content: &str, + target: MemberImplementationTarget<'_>, + descendants: &[ClassInfo], + class_loader: &dyn Fn(&str) -> Option>, + ) -> Vec { + let mut locations = Vec::new(); + let target_fqn = target.class.fqn(); + for implementor in descendants { + let direct = owns_concrete_member(implementor, target.name, target.kind); + let inherited; + let (declaring_class, declaring_fqn) = if direct { + (implementor, implementor.fqn().to_string()) + } else { + let Some((declaring, declaring_fqn)) = + Self::find_declaring_class(implementor, target.name, class_loader) + else { + continue; + }; + if declaring.fqn() == target_fqn + || !owns_concrete_member(&declaring, target.name, target.kind) + { + continue; + } + inherited = declaring; + (&inherited, declaring_fqn) }; - if declares(&parent_cls) { - return locate(&parent_cls); + + if let Some((class_uri, class_content)) = + self.find_class_file_content(&declaring_fqn, uri, content) + && let Some(member_pos) = Self::find_member_position_in_class( + &class_content, + target.name, + target.kind, + declaring_class, + ) + && let Ok(parsed_uri) = Url::parse(&class_uri) + { + locations.push(point_location(parsed_uri, member_pos)); } - current = parent_cls.parent_class; } - None - } - - /// The FQN to search implementors of `cls` by: the namespace the class - /// declares itself when it has one, falling back to whatever the class - /// index knows about its short name. - fn implementor_target_fqn(&self, cls: &ClassInfo) -> String { - let from_class = crate::util::build_fqn(&cls.name, cls.file_namespace.as_deref()); - if from_class.contains('\\') { - return from_class; - } - self.class_fqn_for_short(&cls.name) - .unwrap_or_else(|| cls.name.to_string()) + sort_and_dedup_locations(&mut locations); + locations } /// Resolve implementations of a method call on an interface/abstract class. @@ -583,33 +574,20 @@ impl Backend { MemberKind::Property }; - let target_short = &candidate.name; - let target_fqn = self.implementor_target_fqn(candidate); - - let implementors = self.find_implementors( - target_short, - &target_fqn, + all_locations.extend(self.member_implementation_locations( + uri, + content, + MemberImplementationTarget { + class: candidate, + name: member_name, + kind: member_kind, + }, &class_loader, - true, false, - false, - ); - - for imp in &implementors { - if let Some(loc) = self.locate_member_implementation( - imp, - member_name, - member_kind, - &class_loader, - uri, - content, - ) && !all_locations.contains(&loc) - { - all_locations.push(loc); - } - } + )); } + sort_and_dedup_locations(&mut all_locations); if all_locations.is_empty() { return None; } @@ -1249,20 +1227,6 @@ impl Backend { }) } - /// Get the FQN for a class given its short name, by looking it up in - /// the `fqn_uri_index`. - fn class_fqn_for_short(&self, target_short: &str) -> Option { - let idx = self.symbols.fqn_uri_index.read(); - // Look for an entry whose short name matches. - for fqn in idx.keys() { - let short = short_name(fqn); - if short.eq_ignore_ascii_case(target_short) { - return Some(fqn.to_owned()); - } - } - None - } - /// Find the location of a class declaration for an implementor. fn locate_class_declaration( &self, @@ -1285,6 +1249,34 @@ impl Backend { } } +fn owns_concrete_member(class: &ClassInfo, member_name: &str, member_kind: MemberKind) -> bool { + match member_kind { + MemberKind::Method => class + .methods + .iter() + .any(|method| method.name == member_name && !method.is_virtual && !method.is_abstract), + MemberKind::Property => class + .properties + .iter() + .any(|property| property.name == member_name && !property.is_virtual), + MemberKind::Constant => class + .constants + .iter() + .any(|constant| constant.name == member_name && !constant.is_virtual), + } +} + +fn sort_and_dedup_locations(locations: &mut Vec) { + locations.sort_by(|left, right| { + left.uri + .as_str() + .cmp(right.uri.as_str()) + .then(left.range.start.line.cmp(&right.range.start.line)) + .then(left.range.start.character.cmp(&right.range.start.character)) + }); + locations.dedup(); +} + #[cfg(test)] mod tests { use std::fs; diff --git a/src/definition/mod.rs b/src/definition/mod.rs index 9e584a8dd..af2bf649d 100644 --- a/src/definition/mod.rs +++ b/src/definition/mod.rs @@ -59,6 +59,12 @@ pub(crate) mod member; mod resolve; mod type_definition; +pub(crate) struct MemberImplementationTarget<'a> { + pub(crate) class: &'a crate::types::ClassInfo, + pub(crate) name: &'a str, + pub(crate) kind: member::MemberKind, +} + /// Build an LSP `Location` with a zero-width range (start == end). /// /// Almost every "go to definition" result points to a single position diff --git a/tests/integration/code_lens.rs b/tests/integration/code_lens.rs index 651fae2a4..ecf31ed70 100644 --- a/tests/integration/code_lens.rs +++ b/tests/integration/code_lens.rs @@ -1,4 +1,5 @@ use crate::common::{create_psr4_workspace, create_test_backend}; +use tower_lsp::LanguageServer; use tower_lsp::lsp_types::*; /// Helper: open a file in the backend and return its code lenses. @@ -15,6 +16,96 @@ fn lens_titles(lenses: &[CodeLens]) -> Vec<&str> { .collect() } +async fn open_doc(backend: &phpantom_lsp::Backend, uri: Url, text: &str) { + backend + .did_open(DidOpenTextDocumentParams { + text_document: TextDocumentItem { + uri, + language_id: "php".to_string(), + version: 1, + text: text.to_string(), + }, + }) + .await; +} + +#[tokio::test] +async fn implementation_lenses_open_class_and_method_children() { + let content = r#" = serde_json::from_value( + command + .arguments + .as_ref() + .and_then(|arguments| arguments.get(2)) + .cloned() + .expect("expected implementation locations"), + ) + .expect("implementation targets should be locations"); + assert_eq!( + locations + .iter() + .map(|location| location.range.start.line) + .collect::>(), + target_lines + ); + } +} + // ─── Basic Override Detection ─────────────────────────────────────────────── #[test] diff --git a/tests/integration/implementation.rs b/tests/integration/implementation.rs index 47fb88441..66046e626 100644 --- a/tests/integration/implementation.rs +++ b/tests/integration/implementation.rs @@ -908,6 +908,48 @@ async fn test_implementation_method_only_overriders() { ); } +#[tokio::test] +async fn test_implementation_method_follows_traits_and_inherited_members() { + let backend = create_test_backend(); + + let uri = Url::parse("file:///impl_inherited.php").unwrap(); + let text = concat!( + "render();\n", // 15 + "}\n", // 16 + ); + + open(&backend, &uri, text).await; + + let locations = implementation_at(&backend, &uri, 15, 12).await; + let lines = locations + .iter() + .map(|location| location.range.start.line) + .collect::>(); + assert!( + lines.contains(&5), + "trait-provided implementation should resolve to the trait method: {lines:?}" + ); + assert!( + lines.contains(&11), + "inherited implementation should resolve to the parent method: {lines:?}" + ); +} + // ─── Server capability test ───────────────────────────────────────────────── /// The server should advertise `implementationProvider` in its capabilities.