feat: Go to implementation of trait methods

[bitgaoshu]

try goto where the trait method implies, #4558

[flodiebold]

Hi, thanks for working on this and sorry for the delay!

I've taken the liberty of changing some things in the hir_ty parts of this, mostly renaming things and removing Valid because it didn't really do anything except make the diff harder to read. I also removed the usage of is_valid_candidate because as far as I can tell, it's not necessary when we already know the trait that is being used.

I'm still a bit unsure about the frontend parts though. For one thing, it's a bit awkward to special-case this for functions, because it should just as well work for associated constants and types. Also, finding the implementation should really already happen in IdentClass::classify_token and not after the fact (try_find_trait_item_definition is IMO not the right place for this, but maybe @Veykril has some thoughts on this).

[Veykril]

classify_token_to_impl shouldn't be a thing like that. This resolution should be done in NameRefClass::classify, in fact I don't think there should be any changes to the defs.rs module at all. This PR should affect Semantics::resolve_method_call (for method calls) and Semantics::resolve_path (for function calls) directly or indirectly by changing what these two methods make use of.

So I'd expect the following things to be "specialized" for this:

rust-analyzer/crates/hir/src/source_analyzer.rs

Lines 241 to 248 in 6fc5c3c

	pub(crate) fn resolve_method_call(
	&self,
	db: &dyn HirDatabase,
	call: &ast::MethodCallExpr,
	) -> Option<(FunctionId, Substitution)> {
	let expr_id = self.expr_id(db, &call.clone().into())?;
	self.infer.as_ref()?.method_resolution(expr_id)
	}

and

rust-analyzer/crates/hir/src/source_analyzer.rs

Lines 341 to 346 in 6fc5c3c

	if let Some(path_expr) = parent().and_then(ast::PathExpr::cast) {
	let expr_id = self.expr_id(db, &path_expr.into())?;
	let infer = self.infer.as_ref()?;
	if let Some(assoc) = infer.assoc_resolutions_for_expr(expr_id) {
	return Some(PathResolution::Def(AssocItem::from(assoc).into()));
	}

[bitgaoshu]

finding the implementation should really already happen in IdentClass::classify_token

This resolution should be done in NameRefClass::classify, in fact I don't think there should be any changes to the defs.rs module at all

yes, my first thought is like that. Then I find it would affect other functionality, like rename.
Then I need tell IdentClass::classify_token , "I want function's impl" , or would affect other functionality.

which is better? in goto_.rs or defs.rs or else?

[Veykril]

rename currently has some special casing around the old behaviour and I imagine the goto stuff might as well (or it just has tests that need to be updated). You would need to update those special cases, we should still do the change source_analyzer

[bitgaoshu]

I run cargo test in local succeed.

[Veykril]

The failing tests in CI are slow-tests which don't run by default as they are stubbed out if a specific ENV var isn't set.

[Veykril]

One more small nit from me.

[Veykril]

@flodiebold is this also okay from your PoV? With my nit fixed I'd be inclinced to merge this in that case (after tomorrows release, just to be safe)

[flodiebold]

Yes, the HIR parts are fine for me.

Since this PR has been a long time coming, maybe we could err on the side of merging and fix the nit separately 😉

[Veykril]

Sure, I can fix that one up myself, though I'd still like to wait with merging until after the next release just to be safe :)

[Veykril]

Let's get this merged now, very excited about this :)
@bors r+

[bors]

📌 Commit dcb4837 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit dcb4837 with merge c40a60b...

[bors]

💔 Test failed - checks-actions

[Veykril]

Looks like something was made private that the PR is using, will fix that up

[Veykril]

@bors r+

[bors]

📌 Commit 38c11be has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit 38c11be with merge 22e53f1...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 22e53f1 to master...

[Veykril]

So this is what I feared, this makes some IDE stuff rather slow. Syntax highlighting in bigger files is even worse now than it already is 😓

[Veykril]

574ms - trait_impls_in_deps_query (162 calls)
156ms - trait_solve::wait (1418 calls)

Is this just validation taking ages again? We get this on every keystroke even though trait_impls_in_deps_query shouldnt change I think?

[bitgaoshu]

sorry about that. i looked back my commits. My initial commit is only affect goto type. but now it seems not. it always try resolve impl when need resolve function . and i think it’s no need.

[Veykril]

Yes, the thing is the way its done now is the proper way, name classification should work they way it does now after this PR. It's just unfortunate that for highlighting this isn't really relevant at all and kind of expensive.

[lnicola]

trait-to-impl.mp4