fix Implement type hierarchy #2264#2278
fix Implement type hierarchy #2264#2278asukaminato0721 wants to merge 3 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds end-to-end LSP type hierarchy support (prepare + supertypes + subtypes) and wires up capability advertisement so LSP clients can enable the feature.
Changes:
- Implement
textDocument/typeHierarchyhandlers in the non-wasm LSP server (prepare/supertypes/subtypes). - Add AST helpers for locating/collecting class definitions to support hierarchy queries.
- Add an LSP integration test and fixture Python file for type hierarchy behavior.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/lsp/non_wasm/server.rs | Adds type hierarchy request handling and injects typeHierarchyProvider into initialize response. |
| pyrefly/lib/lsp/non_wasm/type_hierarchy.rs | Adds helpers to find class defs at positions and build TypeHierarchyItems. |
| pyrefly/lib/lsp/non_wasm/mod.rs | Exposes the new type_hierarchy module. |
| pyrefly/lib/commands/lsp.rs | Passes a type_hierarchy_provider flag into initialization. |
| pyrefly/lib/commands/tsp.rs | Passes a type_hierarchy_provider flag into initialization for TSP mode. |
| pyrefly/lib/test/lsp/lsp_interaction/type_hierarchy.rs | Adds an integration test covering prepare/supertypes/subtypes. |
| pyrefly/lib/test/lsp/lsp_interaction/test_files/type_hierarchy_test/classes.py | Adds a simple inheritance fixture (A <- B <- C). |
| pyrefly/lib/test/lsp/lsp_interaction/mod.rs | Registers the new test module. |
| Cargo.lock | Updates lockfile due to dependency resolution changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Serialize)] | ||
| struct InitializeResultWithTypeHierarchy<'a> { | ||
| capabilities: ServerCapabilitiesWithTypeHierarchy<'a>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| server_info: Option<ServerInfo>, | ||
| } |
There was a problem hiding this comment.
InitializeResultWithTypeHierarchy is missing #[serde(rename_all = "camelCase")] (or a field-level rename) so server_info will serialize as server_info instead of LSP's required serverInfo. This breaks the initialize response schema for clients that read serverInfo.
Add #[serde(rename_all = "camelCase")] to InitializeResultWithTypeHierarchy (or #[serde(rename = "serverInfo")] on the field) to keep the JSON wire format compliant.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
| &uri, | ||
| params.item.selection_range.start, | ||
| FindPreference::default(), | ||
| move |transaction, handle, definition| { |
There was a problem hiding this comment.
Can we try to break this up or move it into a helper function. This is over 100 lines and at this point becoming a bit difficult to read.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| indexing_mode: IndexingMode, | ||
| initialization_params: &InitializeParams, | ||
| ) -> ServerCapabilities { | ||
| ) -> ServerCapabilitiesWithTypeHierarchy { |
There was a problem hiding this comment.
Sorry maybe I was not clear but this is not exactly what I meant. On line 842 of fbcode/pyrefly/pyrefly/lib/lsp/non_wasm/server.rs we add the call hierarchy provider. We should be able to add the TypeHierarchyProvider in a very similar way. Once we do that we will not need ServerCapabilitiesWithTypeHierarchy since type hierarchy will already be included in the existing ServerCapabilities
There was a problem hiding this comment.
The problem is:
we have https://docs.rs/lsp-types/latest/lsp_types/enum.CallHierarchyServerCapability.html
but we don't have TypeHierarchyServerCapability in lsp_types
There was a problem hiding this comment.
Ah good catch! In that case I think this is fine for now.
| pub fn initialize_finish( | ||
| #[derive(Serialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct ServerCapabilitiesWithTypeHierarchy { |
There was a problem hiding this comment.
I still think we can implement this in a way where these structs are not required. See my other comment.
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
4 participants
Summary
Fixes #2264
Added LSP type hierarchy support end‑to‑end: prepare/supertypes/subtypes handlers backed by class MRO + reverse‑dep scanning, and injected typeHierarchyProvider into the initialize response so clients can enable the feature.
Test Plan
added lsp test.