parameter completion are no longer being filtered by the client

[Veykril]

fn foo(foo: u32, bar: u32, baz: u32, qux: u32, ) {}

fn bar(b$0)

shows all the parameters here, but the client should be able to filter all but bar and baz out.

[yue4u]

@Veykril Sorry but I think this bug was introduced in #12040. Sadly, I still get the look_up wrong😢. #12040 (comment) cc @jonas-schievink

By changing the lookup to label or fmt(label) seems to resolve the filtering issue while rendering the correct completions. At the same time, I'm still not very clear about what's the expected value for lookup here. Could you please point me to some documentations or info about the lookup behavior?

rust-analyzer/crates/ide_completion/src/completions/fn_param.rs

Line 34 in 6869491

Some((fmt, range, lookup)) => mk_item(&fmt(label), *range).lookup_by(lookup).to_owned(),

btw, the test below will produce the full list even before that PR. Is there a way to test the filtering?

#[test]
fn filter_param() {
    check(
        r#"
fn foo(foo: u32, bar: u32, baz: u32, qux: u32, ) {}
fn bar(b$0)
}
"#,
        expect![[r#"
            bn qux: u32
            bn foo: u32
            bn baz: u32
            bn bar: u32
            kw ref
            kw mut
        "#]],
    );
}

I think it'll be a good idea to enforce the filtering in tests as #12040 (comment) said?

[Veykril]

The completion item docs for the lsp layer are here https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItem

Specifically our lookup translates to the filterText.

Sp the lookup string has to contain the identifier that is being completed. Though I am not sure how this plays out with non identifier characters being in there and the docs don't say anything about it so that might be VSCode playing wrong as well.

I think it'll be a good idea to enforce the filtering in tests as #12040 (comment) said?

We should probably add asserts in tests to check for LSP conformance there.

[yue4u]

The cause of the filtering issue is lookup changed to param.text() which in fact is the same text for each completion item, as the document for filterText says

When falsy the label is used as the filter text for this item.

Just remove the lookup in the Some case would be the fix?

edit: The original lookup in add_new_item_to_acc (https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide_completion/src/completions/fn_param.rs#L29) doesn't work because it does start with the characters in its range, so the minimum filterText will be the label (whole_param https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide_completion/src/completions/fn_param.rs#L110)?

[Veykril]

Ah the the filter text is set to the parameter that we complete, yes that would explain the weird filtering here. I don't quite see how the original lookup doesn't work though. Or rather I am not sure why the lookup had to be changed at all.

[Veykril]

Judging from the LSP docs the lookup should not affect any of this. Ah wait you changed the text range we apply the completion to

[Veykril]

I guess one easy fix would be to append the completion to the contents of what the completion text range covers and use that as the lookup? Not too fond of it but that seems like the simplest approach

[yue4u]

Given following code as an example:

fn f(foo: (), #[baz = "qux"] mut bar: u32) {}
fn g(foo: ()#[baz = "qux"] mut ba$0)

• completion text range cover: ba
• lookup from add_new_item_to_acc: mut bar
• label(whole_param): #[baz = "qux"] mut bar: u32
• full completion content: , #[baz = "qux"] mut bar: u32
• completion edit range: #[baz = "qux"] mut ba

append the completion to the contents of what the completion text range covers

I'm not very sure about why this would be better than using the label itself.

[Veykril]

Ah right, I misunderstood, ye the label itself is enough (in which case you can just drop the lookup_by I think as it defaults to the label if not set)

[yue4u]

Whether including the type or not is a different decision. Params without type (e.g #[baz = "qux"] mut bar) are also suitable for the lookup and their are closer to the behavior before. Some extra calculation are needed though. As you would expect it will enable/disable filtering by the type part. I'm ok with both myself.

---

Another (unrelated) question I had about the completions for fn params is,

rust-analyzer/crates/ide_completion/src/completions/fn_param.rs

Lines 33 to 36 in 198c075

	let item = match &comma_wrapper {
	Some((fmt, range, lookup)) => mk_item(&fmt(label), *range).lookup_by(lookup).to_owned(),
	None => mk_item(label, ctx.source_range()).lookup_by(lookup).to_owned(),
	};

In which situation would we hit the None case? I felt very unlikely when I read through this.

[Veykril]

In which situation would we hit the None case? I felt very unlikely when I read through this.

From a quick glance, never. It's just that we want to avoid unwraps so we use some defaults in these cases even if they won't be hit.

[yue4u]

From a quick glance, never.

I was asking this as if we use the label as lookup and very unlikely to hit the None case, we don't need to bother calculating and passing lookup: String to add_new_item_to_acc.

[Veykril]

Ye we don't need to I think