Don't eagerly instantiate CompletionItems

[Veykril]

Currently when calculating completions we run our different functions to calculate the many different completions we want to offer, immediately turning all of them into the same CompletionItems. This has the downside that we lose a lot of valuable information, requiring us to do upfront decision on some things.

The main pain point here is the auto-insertion of borrows which is currently partially disabled, and for the small subset where it is enabled it is still bugged. What we can do instead is collect all calculated completions into some intermediate representation first which we can then render into the full items. This extra step would allow us to properly split items into multiple ones where desired, as is the case with auto-borrow completions. Similarly this would then also allow us to split completions into simple ones and ones with call argument/pattern destructuring snippets which appear and disappear every now and then due to some refactors.

This, after #12570 should be the last major refactoring required (in regards to what my view on the current completion infra is at least). Then we should be pretty damn close to being able to iron out all the small special cases for "perfect completions".

[matklad]

Hm, I think the original design was for CompletionItem to be exactly that thing. That is, I think this doc comment doesn't really reflect the intention:

/// `CompletionItem` describes a single completion variant in the editor pop-up.
/// It is basically a POD with various properties. To construct a
/// `CompletionItem`, use `new` method and the `Builder` struct.

This should be not a single item in pop-up, but a single "logical entity" which you are completing. Like, "completing a field". Whether to add & should be a property of the CI which the caller should decide for themselves if they want to do or not.

In this sense, we do compute actual items a bit lazily, and we only realize them in the to_proto layer, where, eg, a single item can split into two LSP completions:

rust-analyzer/crates/rust-analyzer/src/to_proto.rs

Lines 302 to 319 in 427061d

	if let Some((mutability, relevance)) = item.ref_match() {
	let mut lsp_item_with_ref = lsp_item.clone();
	set_score(&mut lsp_item_with_ref, max_relevance, relevance);
	lsp_item_with_ref.label =
	format!("&{}{}", mutability.as_keyword_for_ref(), lsp_item_with_ref.label);
	if let Some(it) = &mut lsp_item_with_ref.text_edit {
	let new_text = match it {
	lsp_types::CompletionTextEdit::Edit(it) => &mut it.new_text,
	lsp_types::CompletionTextEdit::InsertAndReplace(it) => &mut it.new_text,
	};
	*new_text = format!("&{}{}", mutability.as_keyword_for_ref(), new_text);
	}
	acc.push(lsp_item_with_ref);
	};
	acc.push(lsp_item);

What's the specific problem we are solving here? What information do we loose when we create a CI? I think at the point where we create CI we have all the information there is, so, presumably, we don't have to loose anything?

[Veykril]

Ye that sounds like roughly what I was thinking of, though I didn't think of to_proto being responsible for the "splitting" (which makes sense now that you pointed that out).

I think the main pain point we have right now is the rendering of completion items happening before we split the completions, that at least is the cause for most misplaced & from what I can tell, and to me it does feel a bit wrong that we render the completion item and then try to split it and adjust the already rendered thing. That is, the rendering causes us to lose information I believe.

[matklad]

Uh, yeah. To maybe rephrase that -- completion items shouldn't necessary include an edit, but, rather, it should include all the necessary info to compute it (or potentially a batch of edits). Or maybe it should include a Vec<_> of edits.

The fact that we try to manufacture a new edit for this purpose in to_proto is definitely wrong.

[matklad]

To maybe give a minimal API, something like this would work:

struct CompletionItem {
  text_edit: TextEdit,
  autoref_text_edit: Option<TextEdit>,
}

[matklad]

and to give a bit more context:

Another invariant I think we should preserve is CI being pods. That is, that we don't include hir items in completions.

This is in contrast to IntelliJ architecture, where completion item is essentially a decorated hir type. I don't like IntelliJ approach, because it's not universal (some completions don't), and because it creates the problems similar to this one: if you include hir in CI, you presumably going to use that hir later for "something". But at that later point you'll loose the info from the call-site where CI was created!

So, on the contrary, it's better to eagerly compute all possible attributes when creating a CI.

[Veykril]

That is a very good point indeed, I assumed the IntelliJ approach would be the nicer one as it feels more "proper" on first thought. You are right though it isn't consistent since not all completions are HIR based and you'll lose the "call-site" info later on.

With that I think I got a pretty good view on what to change now.

Though now I do wonder what the benefits are with "splitting" in to_proto opposed to when we initially render the completions. Though I think that won't be a big relevant part of the changes do it probably doesn't matter too much right now.

[matklad]

Though now I do wonder what the benefits are with "splitting"

This comes from thinking about what the ideal client would do. The ideal client probably won't render

self.foo
&mut self.foo

it'll probably render just

&mut self.foo

and would have some other means for the user to express whether they want &mut or not. For example, there can be different keys to finish "full" or "just the ident" completion, or & can serve as a completion character which inserts the ref.

[flodiebold]

Another example of a completion item that could be 'split' in that way would be allowing users to choose between importing a flyimport item and fully qualifying it instead 🤔 (We don't provide the latter at all since actually duplicating all items in the list this way would probably be way too much)