fix Type aliases are expanded in the hover text #2707#3018
fix Type aliases are expanded in the hover text #2707#3018asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve Pyrefly’s LSP hover output for function signatures so that user-authored type alias names (e.g. Messages) are preserved in hover text instead of being expanded (e.g. list[str]), addressing issue #2707.
Changes:
- Add a hover regression test ensuring function signature hover preserves a
TypeAliasname. - Update hover rendering to prefer displaying the declared function header text (from source) for non-call-site hovers, instead of the fully-resolved type display.
- Refactor hover definition lookup plumbing to retain the definition range/module for the new display path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyrefly/lib/test/lsp/hover.rs |
Adds a test asserting hover shows def func(msgs: Messages) -> None: ... and not the expanded alias. |
pyrefly/lib/lsp/wasm/hover.rs |
Introduces source-based function header extraction to preserve alias names in hover signatures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let body_start = function_def | ||
| .body | ||
| .first() | ||
| .map(Ranged::range) | ||
| .map(|range| range.start()) | ||
| .unwrap_or(function_def.range.end()); | ||
| let header = module | ||
| .code_at(TextRange::new(function_def.range.start(), body_start)) | ||
| .trim_end(); | ||
| header.ends_with(':').then(|| format!("{header} ...")) |
There was a problem hiding this comment.
declared_function_hover_display slices from the start of the StmtFunctionDef to the start of the first statement in the body. Because comments/blank lines are not AST statements, this slice can include leading body comments (e.g. def f(...):\n # comment\n pass), causing trim_end() to end with # comment instead of :, so the function returns None and hover falls back to the type-based display (re-expanding aliases). Consider finding the end of the signature colon directly (e.g., by tokenizing or scanning the slice for the signature-terminating : while skipping trailing whitespace/comments) rather than using the first body statement as the cutoff.
| fn declared_function_hover_display(module: &Module, definition_range: TextRange) -> Option<String> { | ||
| let (ast, _, _) = Ast::parse(module.contents(), module.source_type()); | ||
| let function_def = Ast::locate_node(&ast, definition_range.start()) | ||
| .into_iter() |
There was a problem hiding this comment.
This introduces an Ast::parse(module.contents(), ...) on the hover path for functions. Hover can be called very frequently, and parsing the full module each time can become a noticeable CPU cost on larger files. If possible, reuse an already-cached AST (e.g. use transaction.get_ast(handle) when the definition module is the current handle; or extend the definition result to carry a handle/key that lets you retrieve a cached AST) and only fall back to parsing when no cached AST is available.
| ); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
The new test covers a TypeAlias-style alias and a simple body (pass). Given the signature extraction logic in declared_function_hover_display, it would be good to add coverage for (1) type Messages = list[str] aliases (PEP 695) and (2) a function whose body begins with a comment/blank line before the first statement, which currently affects whether the signature header is detected and aliases are preserved.
| #[test] | |
| #[test] | |
| fn hover_over_function_preserves_pep695_type_alias() { | |
| let code = r#" | |
| type Messages = list[str] | |
| def func(msgs: Messages) -> None: | |
| pass | |
| func | |
| #^ | |
| "#; | |
| let report = get_batched_lsp_operations_report(&[("main", code)], |state, handle, position| { | |
| match get_hover(&state.transaction(), handle, position, false) { | |
| Some(Hover { | |
| contents: HoverContents::Markup(markup), | |
| .. | |
| }) => markup.value, | |
| _ => "None".to_owned(), | |
| } | |
| }); | |
| assert!( | |
| report.contains("def func(msgs: Messages) -> None: ..."), | |
| "Expected hover to preserve the PEP 695 alias name, got: {report}" | |
| ); | |
| assert!( | |
| !report.contains("def func(msgs: list[str]) -> None: ..."), | |
| "Expected hover not to expand the PEP 695 alias, got: {report}" | |
| ); | |
| } | |
| #[test] | |
| fn hover_over_function_with_leading_blank_and_comment_preserves_alias() { | |
| let code = r#" | |
| from typing import TypeAlias | |
| Messages: TypeAlias = list[str] | |
| def func(msgs: Messages) -> None: | |
| # leading comment before the first statement | |
| pass | |
| func | |
| #^ | |
| "#; | |
| let report = get_batched_lsp_operations_report(&[("main", code)], |state, handle, position| { | |
| match get_hover(&state.transaction(), handle, position, false) { | |
| Some(Hover { | |
| contents: HoverContents::Markup(markup), | |
| .. | |
| }) => markup.value, | |
| _ => "None".to_owned(), | |
| } | |
| }); | |
| assert!( | |
| report.contains("def func(msgs: Messages) -> None: ..."), | |
| "Expected hover to preserve the alias name when the body starts with blank/comment lines, got: {report}" | |
| ); | |
| assert!( | |
| !report.contains("def func(msgs: list[str]) -> None: ..."), | |
| "Expected hover not to expand the alias when the body starts with blank/comment lines, got: {report}" | |
| ); | |
| } | |
| #[test] |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes #2707
non-call hovers on plain functions render the declared source header instead of the normalized solved signature.
That preserves aliases like Messages in hover text while leaving call-site hover behavior unchanged for constructors,
__call__, and expanded**kwargs.Test Plan
add test