Skip to content

fix Type aliases are expanded in the hover text #2707 #3018

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asukaminato0721 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix Type aliases are expanded in the hover text #2707 #3018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 111 additions & 66 deletions pyrefly/lib/lsp/wasm/hover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use pyrefly_python::docstring::parse_parameter_documentation;
use pyrefly_python::ignore::Ignore;
use pyrefly_python::ignore::Tool;
use pyrefly_python::ignore::find_comment_start_in_line;
use pyrefly_python::module::Module;
use pyrefly_python::symbol_kind::SymbolKind;
use pyrefly_types::callable::Callable;
use pyrefly_types::callable::FunctionKind;
Expand Down Expand Up @@ -431,6 +432,30 @@ fn parameter_definition_documentation(
docs.get(key).cloned().map(|doc| (key.to_owned(), doc))
}

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()
Comment on lines +435 to +438
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
.find_map(|node| match node {
AnyNodeRef::StmtFunctionDef(function_def)
if function_def.name.range() == definition_range =>
{
Some(function_def)
}
_ => None,
})?;
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} ..."))
Comment on lines +447 to +456
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

/// Check if the cursor position is on the `in` keyword within a for loop or comprehension.
/// Returns Some(iterable_range) if found, None otherwise.
fn in_keyword_in_iteration_at(
Expand Down Expand Up @@ -513,7 +538,7 @@ pub fn get_hover(
});
}

// Otherwise, fall through to the existing type hover logic
// Otherwise, fall through to the existing type hover logic.
let mut type_ = transaction.get_type_at(handle, position)?;

// Helper function to check if we're hovering over a callee and get its range
Expand Down Expand Up @@ -549,42 +574,49 @@ pub fn get_hover(
}

let fallback_name_from_type = fallback_hover_name_from_type(&type_);
let (kind, name, docstring_range, module) = if let Some(FindDefinitionItemWithDocstring {
metadata,
definition_range: definition_location,
module,
docstring_range,
display_name,
}) = transaction
.find_definition(
handle,
position,
FindPreference {
prefer_pyi: false,
..Default::default()
},
)
.map(Vec1::into_vec)
.unwrap_or_default()
// TODO: handle more than 1 definition
.into_iter()
.next()
{
let kind = metadata.symbol_kind();
let name = {
let snippet = module.code_at(definition_location);
if snippet.chars().any(|c| !c.is_whitespace()) {
Some(snippet.to_owned())
} else if let Some(name) = display_name.clone() {
Some(name)
} else {
fallback_name_from_type
}
let (kind, name, definition_range, docstring_range, module) =
if let Some(FindDefinitionItemWithDocstring {
metadata,
definition_range: definition_location,
module,
docstring_range,
display_name,
}) = transaction
.find_definition(
handle,
position,
FindPreference {
prefer_pyi: false,
..Default::default()
},
)
.map(Vec1::into_vec)
.unwrap_or_default()
// TODO: handle more than 1 definition
.into_iter()
.next()
{
let kind = metadata.symbol_kind();
let name = {
let snippet = module.code_at(definition_location);
if snippet.chars().any(|c| !c.is_whitespace()) {
Some(snippet.to_owned())
} else if let Some(name) = display_name.clone() {
Some(name)
} else {
fallback_name_from_type
}
};
(
kind,
name,
Some(definition_location),
docstring_range,
Some(module),
)
} else {
(None, fallback_name_from_type, None, None, None)
};
(kind, name, docstring_range, Some(module))
} else {
(None, fallback_name_from_type, None, None)
};

let name = name.or_else(|| identifier_text_at(transaction, handle, position));

Expand All @@ -593,39 +625,52 @@ pub fn get_hover(
&& !transaction
.identifier_at(handle, position)
.is_some_and(|id| matches!(id.context, IdentifierContext::ClassDef { .. }));
let type_display = transaction.ad_hoc_solve(handle, "hover_display", {
let mut cloned = type_.clone();
move |solver| {
if show_constructor {
let constructor = match cloned {
Type::ClassDef(ref cls)
if !solver.get_metadata_for_class(cls).is_typed_dict() =>
{
Some(solver.type_order().constructor_to_callable(
&solver.promote_nontypeddict_silently_to_classtype(cls),
))
}
Type::Type(box Type::ClassType(ref cls)) => {
Some(solver.type_order().constructor_to_callable(cls))
let type_display = if callee_range_opt.is_none() {
match (&type_, module.as_ref(), definition_range) {
(Type::Function(_), Some(module), Some(definition_range)) => {
declared_function_hover_display(module, definition_range)
}
_ => None,
}
} else {
None
}
.or_else(|| {
transaction.ad_hoc_solve(handle, "hover_display", {
let mut cloned = type_.clone();
move |solver| {
if show_constructor {
let constructor = match cloned {
Type::ClassDef(ref cls)
if !solver.get_metadata_for_class(cls).is_typed_dict() =>
{
Some(solver.type_order().constructor_to_callable(
&solver.promote_nontypeddict_silently_to_classtype(cls),
))
}
Type::Type(box Type::ClassType(ref cls)) => {
Some(solver.type_order().constructor_to_callable(cls))
}
_ => None,
};
if let Some(mut constructor) = constructor {
constructor.transform_toplevel_callable(|c| {
expand_callable_kwargs_for_hover(&solver, c)
});
return constructor.as_lsp_string_with_fallback_name(
name_for_display.as_deref(),
LspDisplayMode::Hover,
);
}
_ => None,
};
if let Some(mut constructor) = constructor {
constructor.transform_toplevel_callable(|c| {
expand_callable_kwargs_for_hover(&solver, c)
});
return constructor.as_lsp_string_with_fallback_name(
name_for_display.as_deref(),
LspDisplayMode::Hover,
);
}
cloned
.transform_toplevel_callable(|c| expand_callable_kwargs_for_hover(&solver, c));
cloned.as_lsp_string_with_fallback_name(
name_for_display.as_deref(),
LspDisplayMode::Hover,
)
}
cloned.transform_toplevel_callable(|c| expand_callable_kwargs_for_hover(&solver, c));
cloned.as_lsp_string_with_fallback_name(
name_for_display.as_deref(),
LspDisplayMode::Hover,
)
}
})
});

let docstring = if let (Some(docstring), Some(module)) = (docstring_range, module) {
Expand Down
32 changes: 32 additions & 0 deletions pyrefly/lib/test/lsp/hover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,38 @@ greeter("hi")
);
}

#[test]
fn hover_preserves_type_aliases_in_function_signatures() {
let code = r#"
from typing import TypeAlias

Messages: TypeAlias = 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 alias name, got: {report}"
);
assert!(
!report.contains("def func(msgs: list[str]) -> None: ..."),
"Expected hover not to expand the alias, got: {report}"
);
}

#[test]
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
#[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]

Copilot uses AI. Check for mistakes.
fn hover_over_inline_ignore_comment() {
let code = r#"
Expand Down
Loading