Skip to content

[ty] Improve lsp handling of hover/goto on imports #21572

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

Merged
merged 2 commits into from
Nov 22, 2025
Merged

[ty] Improve lsp handling of hover/goto on imports #21572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
240f79f
Improve lsp handling of hover/goto on imports
Gankra Nov 21, 2025
d53f1f4
fiuxp
Gankra Nov 21, 2025
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
148 changes: 73 additions & 75 deletions crates/ty_ide/src/goto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ pub(crate) enum GotoTarget<'a> {
/// ```
ImportModuleComponent {
module_name: String,
level: u32,
component_index: usize,
component_range: TextRange,
},
Expand Down Expand Up @@ -302,12 +303,21 @@ impl GotoTarget<'_> {
// (i.e. the type of `MyClass` in `MyClass()` is `<class MyClass>` and not `() -> MyClass`)
GotoTarget::Call { callable, .. } => callable.inferred_type(model),
GotoTarget::TypeParamTypeVarName(typevar) => typevar.inferred_type(model),
GotoTarget::ImportModuleComponent {
module_name,
component_index,
level,
..
} => {
// We don't currently support hovering the bare `.` so there is always a name
let module = import_name(module_name, *component_index);
model.resolve_module_type(Some(module), *level)?
}
// TODO: Support identifier targets
GotoTarget::PatternMatchRest(_)
| GotoTarget::PatternKeywordArgument(_)
| GotoTarget::PatternMatchStarName(_)
| GotoTarget::PatternMatchAsName(_)
| GotoTarget::ImportModuleComponent { .. }
| GotoTarget::TypeParamParamSpecName(_)
| GotoTarget::TypeParamTypeVarTupleName(_)
| GotoTarget::NonLocal { .. }
Expand Down Expand Up @@ -353,37 +363,30 @@ impl GotoTarget<'_> {
/// as just returning a raw `NavigationTarget`.
pub(crate) fn get_definition_targets<'db>(
&self,
file: ruff_db::files::File,
db: &'db dyn crate::Db,
model: &SemanticModel<'db>,
alias_resolution: ImportAliasResolution,
) -> Option<DefinitionsOrTargets<'db>> {
use crate::NavigationTarget;
let db = model.db();
let file = model.file();

match self {
GotoTarget::Expression(expression) => definitions_for_expression(db, file, expression)
.map(DefinitionsOrTargets::Definitions),
GotoTarget::Expression(expression) => {
definitions_for_expression(model, expression).map(DefinitionsOrTargets::Definitions)
}

// For already-defined symbols, they are their own definitions
GotoTarget::FunctionDef(function) => {
let model = SemanticModel::new(db, file);
Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Definition(function.definition(&model)),
]))
}
GotoTarget::FunctionDef(function) => Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Definition(function.definition(model)),
])),

GotoTarget::ClassDef(class) => {
let model = SemanticModel::new(db, file);
Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Definition(class.definition(&model)),
]))
}
GotoTarget::ClassDef(class) => Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Definition(class.definition(model)),
])),

GotoTarget::Parameter(parameter) => {
let model = SemanticModel::new(db, file);
Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Definition(parameter.definition(&model)),
]))
}
GotoTarget::Parameter(parameter) => Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Definition(parameter.definition(model)),
])),

// For import aliases (offset within 'y' or 'z' in "from x import y as z")
GotoTarget::ImportSymbolAlias {
Expand All @@ -404,24 +407,18 @@ impl GotoTarget<'_> {
GotoTarget::ImportModuleComponent {
module_name,
component_index,
level,
..
} => {
// Handle both `import foo.bar` and `from foo.bar import baz` where offset is within module component
let components: Vec<&str> = module_name.split('.').collect();

// Build the module name up to and including the component containing the offset
let target_module_name = components[..=*component_index].join(".");

// Try to resolve the module
definitions_for_module(db, &target_module_name)
// We don't currently support hovering the bare `.` so there is always a name
let module = import_name(module_name, *component_index);
definitions_for_module(model, Some(module), *level)
}

// Handle import aliases (offset within 'z' in "import x.y as z")
GotoTarget::ImportModuleAlias { alias } => {
if alias_resolution == ImportAliasResolution::ResolveAliases {
let full_module_name = alias.name.as_str();
// Try to resolve the module
definitions_for_module(db, full_module_name)
definitions_for_module(model, Some(alias.name.as_str()), 0)
} else {
let alias_range = alias.asname.as_ref().unwrap().range;
Some(DefinitionsOrTargets::Targets(
Expand All @@ -444,9 +441,8 @@ impl GotoTarget<'_> {

// For exception variables, they are their own definitions (like parameters)
GotoTarget::ExceptVariable(except_handler) => {
let model = SemanticModel::new(db, file);
Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Definition(except_handler.definition(&model)),
ResolvedDefinition::Definition(except_handler.definition(model)),
]))
}

Expand Down Expand Up @@ -478,9 +474,9 @@ impl GotoTarget<'_> {
//
// Prefer the function impl over the callable so that its docstrings win if defined.
GotoTarget::Call { callable, call } => {
let mut definitions = definitions_for_callable(db, file, call);
let mut definitions = definitions_for_callable(model, call);
let expr_definitions =
definitions_for_expression(db, file, callable).unwrap_or_default();
definitions_for_expression(model, callable).unwrap_or_default();
definitions.extend(expr_definitions);

if definitions.is_empty() {
Expand All @@ -491,18 +487,15 @@ impl GotoTarget<'_> {
}

GotoTarget::BinOp { expression, .. } => {
let model = SemanticModel::new(db, file);

let (definitions, _) =
ty_python_semantic::definitions_for_bin_op(db, &model, expression)?;
ty_python_semantic::definitions_for_bin_op(db, model, expression)?;

Some(DefinitionsOrTargets::Definitions(definitions))
}

GotoTarget::UnaryOp { expression, .. } => {
let model = SemanticModel::new(db, file);
let (definitions, _) =
ty_python_semantic::definitions_for_unary_op(db, &model, expression)?;
ty_python_semantic::definitions_for_unary_op(db, model, expression)?;

Some(DefinitionsOrTargets::Definitions(definitions))
}
Expand Down Expand Up @@ -632,6 +625,7 @@ impl GotoTarget<'_> {
{
return Some(GotoTarget::ImportModuleComponent {
module_name: full_name.to_string(),
level: 0,
component_index,
component_range,
});
Expand Down Expand Up @@ -672,14 +666,12 @@ impl GotoTarget<'_> {
// Handle offset within module name in from import statements
if let Some(module_expr) = &from.module {
let full_module_name = module_expr.to_string();

if let Some((component_index, component_range)) = find_module_component(
&full_module_name,
module_expr.range.start(),
offset,
) {
if let Some((component_index, component_range)) =
find_module_component(&full_module_name, module_expr.start(), offset)
{
return Some(GotoTarget::ImportModuleComponent {
module_name: full_module_name,
level: from.level,
component_index,
component_range,
});
Expand Down Expand Up @@ -876,27 +868,26 @@ fn convert_resolved_definitions_to_targets(

/// Shared helper to get definitions for an expr (that is presumably a name/attr)
fn definitions_for_expression<'db>(
db: &'db dyn crate::Db,
file: ruff_db::files::File,
model: &SemanticModel<'db>,
Copy link
Member

@MichaReiser MichaReiser Nov 22, 2025

Choose a reason for hiding this comment

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

Sort of funny that SemanticModel survived to this day. I introduced it in one of the very earliest prototypes of a type aware linter back when ty was barely able to infer ints. I'm not sure it's the right API but good to see that it's not complete rubbish and is useful in cases like this :)

Copy link
Contributor Author

@Gankra Gankra Nov 22, 2025

Choose a reason for hiding this comment

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

Yeah between this PR and the string annotations one I am Fully embracing it as an actually useful abstraction!

expression: &ruff_python_ast::ExprRef<'_>,
) -> Option<Vec<ResolvedDefinition<'db>>> {
match expression {
ast::ExprRef::Name(name) => Some(definitions_for_name(db, file, name)),
ast::ExprRef::Name(name) => Some(definitions_for_name(model.db(), model.file(), name)),
ast::ExprRef::Attribute(attribute) => Some(ty_python_semantic::definitions_for_attribute(
db, file, attribute,
model.db(),
model.file(),
attribute,
)),
_ => None,
}
}

fn definitions_for_callable<'db>(
db: &'db dyn crate::Db,
file: ruff_db::files::File,
model: &SemanticModel<'db>,
call: &ast::ExprCall,
) -> Vec<ResolvedDefinition<'db>> {
let model = SemanticModel::new(db, file);
// Attempt to refine to a specific call
let signature_info = call_signature_details(db, &model, call);
let signature_info = call_signature_details(model.db(), model, call);
signature_info
.into_iter()
.filter_map(|signature| signature.definition.map(ResolvedDefinition::Definition))
Expand Down Expand Up @@ -947,29 +938,25 @@ pub(crate) fn find_goto_target(
}

let covering_node = covering_node(parsed.syntax().into(), token.range())
.find_first(|node| node.is_identifier() || node.is_expression())
.find_first(|node| {
node.is_identifier() || node.is_expression() || node.is_stmt_import_from()
Copy link
Member

@MichaReiser MichaReiser Nov 22, 2025

Choose a reason for hiding this comment

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

I wonder if it makes sense to change this to node.is_stmt. I don't think we ever want to go beyond the closest statement (my, now proven incorrect, assumption was that expression is a sufficient boundary but turns out it isn't)

Copy link
Contributor Author

@Gankra Gankra Nov 22, 2025

Choose a reason for hiding this comment

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

Hmm I'll think about it

})
.ok()?;

GotoTarget::from_covering_node(&covering_node, offset, parsed.tokens())
}

/// Helper function to resolve a module name and create a navigation target.
fn definitions_for_module<'db>(
db: &'db dyn crate::Db,
module_name_str: &str,
model: &SemanticModel,
module: Option<&str>,
level: u32,
) -> Option<DefinitionsOrTargets<'db>> {
use ty_python_semantic::{ModuleName, resolve_module};

if let Some(module_name) = ModuleName::new(module_name_str) {
if let Some(resolved_module) = resolve_module(db, &module_name) {
if let Some(module_file) = resolved_module.file(db) {
return Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Module(module_file),
]));
}
}
}
None
let module = model.resolve_module(module, level)?;
let file = module.file(model.db())?;
Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Module(file),
]))
}

/// Helper function to extract module component information from a dotted module name
Expand All @@ -983,9 +970,7 @@ fn find_module_component(

// Split the module name into components and find which one contains the offset
let mut current_pos = 0;
let components: Vec<&str> = full_module_name.split('.').collect();

for (i, component) in components.iter().enumerate() {
for (i, component) in full_module_name.split('.').enumerate() {
let component_start = current_pos;
let component_end = current_pos + component.len();

Expand All @@ -1004,3 +989,16 @@ fn find_module_component(

None
}

/// Helper to get the module name up to the given component index
fn import_name(module_name: &str, component_index: usize) -> &str {
// We want everything to the left of the nth `.`
// If there's no nth `.` then we want the whole thing.
let idx = module_name
.match_indices('.')
.nth(component_index)
.map(|(idx, _)| idx)
.unwrap_or(module_name.len());

&module_name[..idx]
}
5 changes: 3 additions & 2 deletions crates/ty_ide/src/goto_declaration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{Db, NavigationTargets, RangedValue};
use ruff_db::files::{File, FileRange};
use ruff_db::parsed::parsed_module;
use ruff_text_size::{Ranged, TextSize};
use ty_python_semantic::ImportAliasResolution;
use ty_python_semantic::{ImportAliasResolution, SemanticModel};

/// Navigate to the declaration of a symbol.
///
Expand All @@ -18,8 +18,9 @@ pub fn goto_declaration(
let module = parsed_module(db, file).load(db);
let goto_target = find_goto_target(&module, offset)?;

let model = SemanticModel::new(db, file);
let declaration_targets = goto_target
.get_definition_targets(file, db, ImportAliasResolution::ResolveAliases)?
.get_definition_targets(&model, ImportAliasResolution::ResolveAliases)?
.declaration_targets(db)?;

Some(RangedValue {
Expand Down
6 changes: 3 additions & 3 deletions crates/ty_ide/src/goto_definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{Db, NavigationTargets, RangedValue};
use ruff_db::files::{File, FileRange};
use ruff_db::parsed::parsed_module;
use ruff_text_size::{Ranged, TextSize};
use ty_python_semantic::ImportAliasResolution;
use ty_python_semantic::{ImportAliasResolution, SemanticModel};

/// Navigate to the definition of a symbol.
///
Expand All @@ -18,9 +18,9 @@ pub fn goto_definition(
) -> Option<RangedValue<NavigationTargets>> {
let module = parsed_module(db, file).load(db);
let goto_target = find_goto_target(&module, offset)?;

let model = SemanticModel::new(db, file);
let definition_targets = goto_target
.get_definition_targets(file, db, ImportAliasResolution::ResolveAliases)?
.get_definition_targets(&model, ImportAliasResolution::ResolveAliases)?
.definition_targets(db)?;

Some(RangedValue {
Expand Down
Loading
Loading