Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
102 changes: 57 additions & 45 deletions crates/ty_ide/src/goto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@ pub use crate::goto_type_definition::goto_type_definition;

use crate::find_node::covering_node;
use crate::stub_mapping::StubMapper;
use ruff_db::parsed::ParsedModuleRef;
use ruff_db::parsed::{ParsedModuleRef, parsed_module};
use ruff_python_ast::{self as ast, AnyNodeRef};
use ruff_python_parser::TokenKind;
use ruff_text_size::{Ranged, TextRange, TextSize};
use ty_python_semantic::types::Type;
use ty_python_semantic::{HasType, SemanticModel};
use ty_python_semantic::types::definitions_for_keyword_argument;
use ty_python_semantic::{HasType, SemanticModel, definitions_for_name};

#[derive(Clone, Copy, Debug)]
pub(crate) enum GotoTarget<'a> {
Expand Down Expand Up @@ -150,15 +151,19 @@ impl GotoTarget<'_> {
use ruff_python_ast as ast;

match self {
// For names, find the definitions of the symbol
GotoTarget::Expression(expression) => {
if let ast::ExprRef::Name(name) = expression {
Self::get_name_definition_targets(name, file, db, stub_mapper)
} else {
// For other expressions, we can't find definitions
None
}
}
GotoTarget::Expression(expression) => match expression {
ast::ExprRef::Name(name) => definitions_to_navigation_targets(
db,
stub_mapper,
definitions_for_name(db, file, name),
),
ast::ExprRef::Attribute(attribute) => definitions_to_navigation_targets(
db,
stub_mapper,
ty_python_semantic::definitions_for_attribute(db, file, attribute),
),
_ => None,
},

// For already-defined symbols, they are their own definitions
GotoTarget::FunctionDef(function) => {
Expand Down Expand Up @@ -195,40 +200,30 @@ impl GotoTarget<'_> {
None
}

// TODO: Handle attribute and method accesses (y in `x.y` expressions)
// TODO: Handle keyword arguments in call expression
// TODO: Handle multi-part module names in import statements
// TODO: Handle imported symbol in y in `from x import y as z` statement
// TODO: Handle string literals that map to TypedDict fields
_ => None,
}
}
// Handle keyword arguments in call expressions
GotoTarget::KeywordArgument(keyword) => {
// Find the call expression that contains this keyword
let module = parsed_module(db, file).load(db);
Comment on lines +205 to +206
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this seems a bit unfortunate that we need to go and search the covering node again. Would it make sense to instead store the CallExpr in KeywordArgument alongside the keyword?

Getting the function there should be cheaper because CoveringNode allows upward traversal whereas calling covering_node requires an AST traversal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd advise against doing preemptive micro-optimizations here. This is not a hot code path, so using more memory in an attempt to speed it up is probably not the right tradeoff.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a micro optimization. It just seems unnecessary if we can get this information when extracting the GoToDefinitionTarget. It's right there and I find it even less confusing


/// Get navigation targets for definitions associated with a name expression
fn get_name_definition_targets(
name: &ruff_python_ast::ExprName,
file: ruff_db::files::File,
db: &dyn crate::Db,
stub_mapper: Option<&StubMapper>,
) -> Option<crate::NavigationTargets> {
use ty_python_semantic::definitions_for_name;
// Use the keyword's range to find the containing call expression
let covering_node = covering_node(module.syntax().into(), keyword.range())
.find_first(|node| matches!(node, AnyNodeRef::ExprCall(_)))
.ok()?;

// Get all definitions for this name
let mut definitions = definitions_for_name(db, file, name);
if let AnyNodeRef::ExprCall(call_expr) = covering_node.node() {
let definitions =
definitions_for_keyword_argument(db, file, keyword, call_expr);
return definitions_to_navigation_targets(db, stub_mapper, definitions);
}

// Apply stub mapping if a mapper is provided
if let Some(mapper) = stub_mapper {
definitions = mapper.map_definitions(definitions);
}
None
}

if definitions.is_empty() {
return None;
// TODO: Handle multi-part module names in import statements
// TODO: Handle imported symbol in y in `from x import y as z` statement
// TODO: Handle string literals that map to TypedDict fields
_ => None,
}

// Convert definitions to navigation targets
let targets = convert_resolved_definitions_to_targets(db, definitions);

Some(crate::NavigationTargets::unique(targets))
}
}

Expand Down Expand Up @@ -279,18 +274,35 @@ fn convert_resolved_definitions_to_targets(
full_range: full_range.range(),
}
}
ty_python_semantic::ResolvedDefinition::ModuleFile(module_file) => {
// For module files, navigate to the beginning of the file
ty_python_semantic::ResolvedDefinition::FileWithRange(file_range) => {
// For file ranges, navigate to the specific range within the file
crate::NavigationTarget {
file: module_file,
focus_range: ruff_text_size::TextRange::default(), // Start of file
full_range: ruff_text_size::TextRange::default(), // Start of file
file: file_range.file(),
focus_range: file_range.range(),
full_range: file_range.range(),
}
}
})
.collect()
}

/// Shared helper to map and convert resolved definitions into navigation targets.
fn definitions_to_navigation_targets<'db>(
db: &dyn crate::Db,
stub_mapper: Option<&StubMapper<'db>>,
mut definitions: Vec<ty_python_semantic::ResolvedDefinition<'db>>,
) -> Option<crate::NavigationTargets> {
if let Some(mapper) = stub_mapper {
definitions = mapper.map_definitions(definitions);
}
if definitions.is_empty() {
None
} else {
let targets = convert_resolved_definitions_to_targets(db, definitions);
Some(crate::NavigationTargets::unique(targets))
}
}

pub(crate) fn find_goto_target(
parsed: &ParsedModuleRef,
offset: TextSize,
Expand Down
Loading
Loading