performance: Speed up Method Completions By Taking Advantage of Orphan Rules

[davidbarsky]

(Continues #16498)

This PR speeds up method completions by doing two things without regressing analysis-stats¹:

• Filter candidate traits prior to calling iterate_path_candidates by relying on orphan rules (see below for a slightly more in-depth explanation). When generating completions on slog::Logger in oxidecomputer/omicron as a test, this PR halved my completion times—it's now 454ms cold and 281ms warm. Before this PR, it was 808ms cold and 579ms warm.
• Inline some of the method candidate checks into is_valid_method_candidate and remove some unnecessary visibility checks. This was suggested by @Veykril in this comment.

We filter candidate traits by taking advantage of orphan rules. For additional details, I'll rely on @WaffleLapkin's explanation from Zulip:

A type A can only implements traits which

1. Have a blanket implementation (impl<T> Trait for T {})
2. Have implementation for A (impl Trait for A {})

Blanket implementation can only exist in Trait's crate. Implementation for A can only exist in A's or Trait's crate.

Big thanks to Waffle for its keen observation!

---

I think some additional improvements are possible:

• for_trait_and_self_ty seemingly does not distinguish between &T, &mut T, or T, resulting in seemingly irrelevant traits like tokio::io::AsyncWrite being being included for, e.g., &slog::Logger. I don't know they're being considered due to the autoref/autoderef behavior, but I wonder if it'd make sense to filter by mutability earlier and not consider trait implementations that require &mut T when we only have a &T.
• The method completions spend a lot of time in unification, and while there might be low-hanging fruit there, it might make more sense to wait for the new trait solver in rustc. I dunno.

Footnotes

1. The filtering occurs outside of typechecking, after all. ↩

[davidbarsky]

Right, okay: I addressed the review feedback, except the big one from Waffle. I'm pretty sure I've got a handle on checking fundamental types (I'll pilfer from it from the orphan implementation check), but I'm struggling to get a test with fundamental types working (see the changes in crates/ide-completion/src/tests/flyimport.rs). The expect side of things isn't the correct correct, but I can't seem to get the fixtures to be aware of a #[fundamental] Box—any tips?

[Veykril]

Aren't you missing the fundamental check? The fixture seems correct to me

[davidbarsky]

I am, yeah. I just want to write some tests that invoke fundamental trait impls and then ensure the impl works afterwards (in this PR, to be clear). i’m viewing those tests as a baseline.

[Veykril]

Oh wait, your test fixture is not using the fundamental type in any impl, so how should it show anything?

[davidbarsky]

Maybe a better of phrasing my question is "do I need to do anything special to introduce a new fundamental type in a fixture, or is the #[fundamental] annotation sufficient". Looking at some of the other tests, I'm inclined to say no.

[Veykril]

Just using the attribute is enough yes, see the orphan check diagnostic fixtures

[davidbarsky]

Alright, I updated the PR to include tests for impl ForeignTrait for Box<LocalStruct>, impl ForeignTrait for &LocalStruct, and impl Into<A> for (). The first two tests pass without issue (in that the filtering already considers fundamental types), but the third test seemingly does not work, despite .into() showing up in completions when I build a rust-analyzer binary with these exact changes.

Notably, I didn't need to make additional changes when filtering to consider fundamental types.

[Veykril]

Lgtm with the imports tidied up
@bors delegate+

[bors]

✌️ @davidbarsky, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

[davidbarsky]

@bors r=@Veykril

[bors]

📌 Commit c246a93 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit c246a93 with merge 5fead60...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 5fead60 to master...