-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
feat: Add the ability to jump from into
to from
definitions
#18934
feat: Add the ability to jump from into
to from
definitions
#18934
Conversation
into
to from
definitionsinto
to from
definitions
crates/ide/src/goto_definition.rs
Outdated
// FIXME: This condition does not work for complicated cases such as | ||
// receiver_type: Vec<i64> | ||
// arg.ty(): T: IntoIterator<Item = i64> | ||
args.first().is_some_and(|arg| receiver_type.could_coerce_to(db, arg.ty())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this works in many cases but does not cover all cases.
If someone knows how to check type compatibilities, please comment.
Maybe this feature is required to cover all cases.
Feel free to add them to minicore (behind flags). |
crates/ide/src/goto_definition.rs
Outdated
// - return_type is B (type of b) | ||
// We will find the definition of B::from(a: A). | ||
let method_call = ast::MethodCallExpr::cast(original_token.parent()?.parent()?)?; | ||
let receiver_type = sema.type_of_expr(&method_call.receiver()?)?.original(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we actually need the adjusted type for the receiver.
crates/ide/src/goto_definition.rs
Outdated
let method_call = ast::MethodCallExpr::cast(original_token.parent()?.parent()?)?; | ||
let receiver_type = sema.type_of_expr(&method_call.receiver()?)?.adjusted(); | ||
let return_type = sema.type_of_expr(&method_call.clone().into())?.original(); | ||
|
||
let (search_method, search_trait, return_type) = match method_call.name_ref()?.text().as_str() { | ||
"into" => ("from", FamousDefs(sema, krate).core_convert_From()?, return_type), | ||
// If the method is try_into() or parse(), return_type is Result<T, Error>. | ||
// Get T from type arguments of Result<T, Error>. | ||
"try_into" => ( | ||
"try_from", | ||
FamousDefs(sema, krate).core_convert_TryFrom()?, | ||
return_type.type_arguments().next()?, | ||
), | ||
"parse" => ( | ||
"from_str", | ||
FamousDefs(sema, krate).core_str_FromStr()?, | ||
return_type.type_arguments().next()?, | ||
), | ||
_ => return None, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use Semantics::resolve_method_call_as_callable
here, that gives us a bunch of the things here already like receiver param/type, function id and return type.
crates/ide/src/goto_definition.rs
Outdated
let from_impls = Impl::all_for_type(db, return_type) | ||
.into_iter() | ||
.filter(|impl_| impl_.trait_(db).is_some_and(|trait_| trait_ == search_trait)); | ||
let from_methods = from_impls.flat_map(|impl_| impl_.items(db)).filter_map(|item| match item { | ||
AssocItem::Function(function) if function.name(db).as_str() == search_method => { | ||
Some(function) | ||
} | ||
_ => None, | ||
}); | ||
let target_method = from_methods.into_iter().find(|method| { | ||
let args = method.assoc_fn_params(db); | ||
|
||
// FIXME: This condition does not work for complicated cases such as | ||
// receiver_type: Vec<i64> | ||
// arg.ty(): T: IntoIterator<Item = i64> | ||
args.first().is_some_and(|arg| receiver_type.could_coerce_to(db, arg.ty())) | ||
})?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be going through the (currently unexposed) SourceAnalyzer::resolve_impl_method_or_trait_def
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll touch up on this a bit in a follow up PR for some style nits, but otherwise lgtm 👍
sema: &Semantics<'_, RootDatabase>, | ||
original_token: &SyntaxToken, | ||
) -> Option<Vec<NavigationTarget>> { | ||
let method_call = ast::MethodCallExpr::cast(original_token.parent()?.parent()?)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw we should also make this work with non method call syntax, but that can wait for a follow up PR
close #18316
close #15315
(maybe related to #4558)
This PR add the ability to jump from
into
tofrom
definitions.into
tofrom
try_into
totry_from
parse
tofrom_str
Test
I can't write unit tests for
TryFrom
andFromStr
becauseminicore
doesn't contain them.