diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f5319ee48..4b0574cf2 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Go-to-definition on a member at its own declaration no longer jumps to the prototype it implements.** Invoking go-to-definition on a method, constant, or enum case declaration sent the editor to the interface, parent class, or trait declaring the same member, which made "Declaration or Usages" (PHPStorm's CMD+B) and Zed's equivalent unable to show where the concrete member is used. The declaration now answers with its own location, the signal editors read as "show usages instead"; the `implements` and `extends` clauses, and Go to Implementation, remain the routes to the prototype. This is the member-level counterpart of the same fix for class names. Contributed by @EranNL. Closes #412. - **An external formatter (Pint, php-cs-fixer) that prints nothing on stdout no longer empties the file.** The stdin-driven formatters return the child process's stdout verbatim as the formatted document, with no check that anything came back. A command that formats in place and prints nothing, or that hit a read failure captured as an empty string, exited `0` with no output, and the empty result was then applied as a single edit replacing the whole document. Empty output for non-empty input is now rejected as a formatting error instead of being applied. - **Code lenses and inlay hints no longer get slower the further down a file they sit.** Each lens and each hint worked out its line by counting from the start of the file, so a large file did that once per item, on every keystroke, since both are re-pulled after every change. A lens also scanned backwards a second time to find the indentation to sit at. Both requests now read the file's line table once and answer every position from it. Following a lens was doing the same kind of avoidable work: it ran without the caches every other request sets up and copied the whole buffer to do it, and an editor asks once per lens on screen. - **Blade section and stack pairing no longer walks every template in the project for each layout in the chain.** Finding the pages that fill a layout, and deciding whether a partial is rendered by another template, both scanned the whole list of templates once per step, so an application where most pages extend one layout paid for that quadratically, on completion inside `@section`, on the section diagnostics, and on Find References alike. What extends and includes what is now recorded as the templates are read, and kept current as they are edited. diff --git a/docs/todo.md b/docs/todo.md index f30c57dca..3ebc92482 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -92,6 +92,7 @@ unlikely to move the needle for most users. | T10 | [Ternary expression as RHS of list destructuring](todo/type-inference.md#t10-ternary-expression-as-rhs-of-list-destructuring) | Low | Medium | | T11 | [Nested list destructuring](todo/type-inference.md#t11-nested-list-destructuring) | Low | Medium | | | **[Bugs](todo/bugs.md)** | | | +| B322 | [A method that implements or overrides a prototype gets no reference count](todo/bugs.md#b322-a-method-that-implements-or-overrides-a-prototype-gets-no-reference-count) | Low-Medium | Low | | | **[Diagnostics](todo/diagnostics.md)** | | | | D6 | [Unreachable code diagnostic](todo/diagnostics.md#d6-unreachable-code-diagnostic) | Low-Medium | Medium | | D16 | [`unreachable_match_arm` ignores literal subject types](todo/diagnostics.md#d16-unreachable_match_arm-ignores-literal-subject-types) | Low-Medium | Medium | diff --git a/docs/todo/bugs.md b/docs/todo/bugs.md index 025d04f12..844dcdcb4 100644 --- a/docs/todo/bugs.md +++ b/docs/todo/bugs.md @@ -54,4 +54,24 @@ No outstanding items. ## Miscellaneous -No outstanding items. +### B322. A method that implements or overrides a prototype gets no reference count + +**Impact: Low-Medium · Complexity: Low** + +`handle_code_lens` builds the reference lens for a method only when +`find_prototype` came back empty, so a method that implements an +interface or overrides a parent loses its count and keeps only the +`◆ Interface::method` navigation lens. Deleting the `implements` clause +makes the count reappear, which is how the reporter of github #412 found +it: the very methods a project most wants a usage count for, the ones +behind a contract, are the ones that never show one. + +The two lenses answer different questions and both fit on the line, the +way a property carrying a count and a class carrying an implementation +count already coexist. The gate reads as a guard against a second +resolution pass rather than a deliberate layout choice, and the counts +are already answered from the reference index, so keeping both is not +the work the gate seems to be avoiding. + +**Where to look:** `src/code_lens.rs` (`handle_code_lens`, the +`proto.is_none()` condition on `build_member_reference_lens`). diff --git a/src/definition/resolve.rs b/src/definition/resolve.rs index 521b18400..66b32300b 100644 --- a/src/definition/resolve.rs +++ b/src/definition/resolve.rs @@ -24,20 +24,12 @@ use super::point_location; use crate::Backend; use crate::class_lookup::find_class_at_offset; use crate::composer; -use crate::inheritance::find_declaring_ancestor; use crate::symbol_map::{SelfStaticParentKind, SymbolKind}; use crate::text_position::position_to_offset; use crate::types::{AccessKind, ClassInfo}; use crate::util::short_name; use crate::virtual_members::laravel; -struct MemberPrototypeSearch<'a> { - member_name: &'a str, - kind: MemberKind, - uri: &'a str, - content: &'a str, -} - impl Backend { /// Handle a "go to definition" request. /// @@ -316,35 +308,14 @@ impl Backend { .resolve_class_reference(uri, content, name, *is_fqn, cursor_offset) .map(|loc| vec![loc]), - SymbolKind::MemberDeclaration { name, is_static } => { - // If this method/property overrides a parent or implements - // an interface member, jump to the prototype declaration. - let ctx = self.file_context(uri); - let class_loader = self.class_loader(&ctx); - let current_class = - crate::class_lookup::find_class_at_offset(&ctx.classes, cursor_offset); - if let Some(cls) = current_class - && let Some(kind) = self.infer_member_declaration_kind(cls, name, *is_static) - && let Some(loc) = self.resolve_member_declaration_prototype( - uri, - content, - cls, - name, - kind, - &class_loader, - ) - { - return Some(vec![loc]); - } - - if let Some(cls) = current_class - && let Some(locs) = - self.resolve_reverse_implementation(uri, content, cls, name, &class_loader) - && !locs.is_empty() - { - return Some(locs); - } - + SymbolKind::MemberDeclaration { name, .. } => { + // Return self-location so editors detect "definition == + // cursor" and offer Find Usages instead of navigating. + // Navigating to the interface or abstract prototype from a + // declaration site makes the concrete method's usages + // unreachable; the `implements`/`extends` clause and the + // `textDocument/implementation` command handle prototype + // navigation. self.declaration_or_usages(uri, content, cursor_offset, name) } @@ -453,96 +424,6 @@ impl Backend { } } - fn infer_member_declaration_kind( - &self, - class: &ClassInfo, - member_name: &str, - is_static: bool, - ) -> Option { - if is_static - && class - .constants - .iter() - .any(|c| c.name == member_name && c.visibility != crate::types::Visibility::Private) - { - return Some(MemberKind::Constant); - } - - if class.methods.iter().any(|m| { - m.name == member_name - && m.is_static == is_static - && !m.is_virtual - && m.visibility != crate::types::Visibility::Private - }) { - return Some(MemberKind::Method); - } - - if class.properties.iter().any(|p| { - p.name == member_name - && p.is_static == is_static - && !p.is_virtual - && p.visibility != crate::types::Visibility::Private - }) { - return Some(MemberKind::Property); - } - - None - } - - fn resolve_member_declaration_prototype( - &self, - uri: &str, - content: &str, - class: &ClassInfo, - member_name: &str, - kind: MemberKind, - class_loader: &dyn Fn(&str) -> Option>, - ) -> Option { - let search = MemberPrototypeSearch { - member_name, - kind, - uri, - content, - }; - let declares = |candidate: &ClassInfo| self.class_declares_member(candidate, &search); - let (name, declaring) = find_declaring_ancestor(class, class_loader, &declares)?; - self.member_location(&name, &declaring, &search) - } - - fn class_declares_member(&self, class: &ClassInfo, search: &MemberPrototypeSearch<'_>) -> bool { - match search.kind { - MemberKind::Method => class.methods.iter().any(|m| { - m.name == search.member_name - && !m.is_virtual - && m.visibility != crate::types::Visibility::Private - }), - MemberKind::Property => class.properties.iter().any(|p| { - p.name == search.member_name - && !p.is_virtual - && p.visibility != crate::types::Visibility::Private - }), - MemberKind::Constant => class.constants.iter().any(|c| { - c.name == search.member_name && c.visibility != crate::types::Visibility::Private - }), - } - } - - fn member_location( - &self, - class_name: &str, - class: &ClassInfo, - search: &MemberPrototypeSearch<'_>, - ) -> Option { - let offset = class.member_name_offset(search.member_name, search.kind.as_str())?; - let (target_uri, target_content) = - self.find_class_file_content(class_name, search.uri, search.content)?; - let parsed_uri = Url::parse(&target_uri).ok()?; - Some(point_location( - parsed_uri, - crate::text_position::offset_to_position(&target_content, offset as usize), - )) - } - /// Return the declaration's own location for a symbol that has nowhere /// else to jump to. /// diff --git a/tests/integration/definition_members.rs b/tests/integration/definition_members.rs index bb86086ed..fe02a77fc 100644 --- a/tests/integration/definition_members.rs +++ b/tests/integration/definition_members.rs @@ -5803,3 +5803,143 @@ async fn definition_of_a_plain_parent_property_named_by_a_hook_call() { other => panic!("Expected Scalar location, got: {:?}", other), } } + +/// Regression for github #412: Ctrl+Click on a method name at its own +/// declaration site in a class that implements an interface must return the +/// concrete method's own location, not the interface declaration. +/// Editors detect "definition == cursor position" as the cue to show +/// usages; jumping to the interface makes the concrete method's usages +/// unreachable. The `implements` clause is the place that navigates to +/// the interface. +#[tokio::test] +async fn test_goto_definition_implements_method_declaration_returns_self_location() { + let (backend, dir) = create_psr4_workspace( + r#"{ + "autoload": { "psr-4": { "App\\": "src/" } } + }"#, + &[ + ( + "src/LoggerInterface.php", + concat!( + " locs, + Some(GotoDefinitionResponse::Scalar(loc)) => vec![loc], + other => panic!("Expected self-location, got: {other:?}"), + }; + assert_eq!(locations.len(), 1, "should return exactly one location"); + assert_eq!( + locations[0].uri, logger_uri, + "should return the concrete method's own location, not the interface declaration" + ); + assert_eq!( + locations[0].range.start.line, 3, + "should point back to the concrete method declaration line" + ); + assert_eq!( + ( + locations[0].range.start.character, + locations[0].range.end.character + ), + (20, 23), + "the range must cover the method name the cursor sits on, which is what \ + editors compare against the cursor to decide to show usages" + ); +} + +/// The same for a method that overrides a parent class rather than +/// implementing an interface: the declaration answers with itself, and the +/// `extends` clause is the place that navigates to the parent. +#[tokio::test] +async fn test_goto_definition_overriding_method_declaration_returns_self_location() { + let backend = create_test_backend(); + + let uri = Url::parse("file:///override_declaration.php").unwrap(); + let text = concat!( + " locs, + Some(GotoDefinitionResponse::Scalar(loc)) => vec![loc], + other => panic!("Expected self-location, got: {other:?}"), + }; + assert_eq!(locations.len(), 1, "should return exactly one location"); + assert_eq!(locations[0].uri, uri); + assert_eq!( + locations[0].range.start.line, 5, + "should point back to Dog::speak, not Animal::speak on line 2" + ); +}