Fix duplicate lsp completion requests

[babyccino]

Sorry I accidentally closed this PR so I'm making it again. Fixes completion requests being made twice upon input when completion menu is already open. I can't see why this would be necessary

Release Notes:

• Fixes bug where lsp completion requests were being made twice

[SomeoneToIgnore]

Thank you for finding this.
Two notes:

1. I'm not sure that did_trigger_completions as an extra field in the editor is a good solution to that though — in our case, we query the duplicate completion wrongly only in the case "editor got an edit with the completions menu opened".

Such duplication happens only in

zed/crates/editor/src/editor.rs

Line 2844 in f2e7c63

this.change_selections(Some(Autoscroll::fit()), cx, |s| s.select(new_selections));

just a few lines before the "proper" completion trigger:

zed/crates/editor/src/editor.rs

Lines 2859 to 2861 in f2e7c63

	let trigger_in_words = !had_active_inline_completion;
	this.trigger_completion_on_input(&text, trigger_in_words, cx);
	this.refresh_inline_completion(true, cx);

So what we can do instead is to call a "special" method in this context, that would do the very same as change_selections does but without the extra completion triggering logic (that is being called self.selections_did_change).
I would've had a shared function with an extra parameter, pub fn change_selections that calls it with one parameter, and our place that calls it with another.

2. Let's add a test for that — this bug is subtle and we'd better ensure it does not degrade over time.
   editor_tests.rs in the editor crate have examples of setting up a fake language server, we can do that and add a counter into it, open the completions menu and ensure that completion requests are not re-queried too much.

Let me know if you need help with either, happy to work on this more and find a good fix.

[babyccino]

Thank you for finding this. Two notes:

1. I'm not sure that did_trigger_completions as an extra field in the editor is a good solution to that though — in our case, we query the duplicate completion wrongly only in the case "editor got an edit with the completions menu opened".

Such duplication happens only in

zed/crates/editor/src/editor.rs

Line 2844 in f2e7c63

this.change_selections(Some(Autoscroll::fit()), cx, |s| s.select(new_selections));

just a few lines before the "proper" completion trigger:

zed/crates/editor/src/editor.rs

Lines 2859 to 2861 in f2e7c63

	let trigger_in_words = !had_active_inline_completion;
	this.trigger_completion_on_input(&text, trigger_in_words, cx);
	this.refresh_inline_completion(true, cx);

So what we can do instead is to call a "special" method in this context, that would do the very same as change_selections does but without the extra completion triggering logic (that is being called self.selections_did_change). I would've had a shared function with an extra parameter, pub fn change_selections that calls it with one parameter, and our place that calls it with another.

2. Let's add a test for that — this bug is subtle and we'd better ensure it does not degrade over time.
   editor_tests.rs in the editor crate have examples of setting up a fake language server, we can do that and add a counter into it, open the completions menu and ensure that completion requests are not re-queried too much.

Let me know if you need help with either, happy to work on this more and find a good fix.

Yeah I didn't really like doing it with an added parameter but I just wanted to change as little as possible in case there were any complex interactions I wasn't seeing. I'm a bit confused on your solution though. Is what I just pushed approximately what you were thinking?

[SomeoneToIgnore]

Thank you, looks much better now.

to change as little as possible

Funny, but now the diff is less than before, with the parameter

complex interactions

Those usually grow faster from a bloated state, when a struct has a lot of parameters which interact with each other around the methods.
Now though, we just have two tracks: for selecting with completion re-querying and without, no extra state added.

approximately what you were thinking

Yes, looks very close to!
I would hide the whole // -- change_selections without triggering show_completions section into a method.
Something like

// existing public API, used in many places, we keep its signature, but change the body
pub fn change_selections<R>(
        &mut self,
        autoscroll: Option<Autoscroll>,
        cx: &mut ViewContext<Self>,
        change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R,
    ) -> R {
    // old body is moved into that inner function and adjusted a bit
    self.change_selections_inner(autoscroll, true, cx, change)
}

pub fn change_selections_inner<R>(
        &mut self,
        autoscroll: Option<Autoscroll>,
        request_completions: bool,
        cx: &mut ViewContext<Self>,
        change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R,
    ) -> R {
    // here goes old `change_selections` logic + new parameter handling
}

// somewhere in the handle_edit place, we use self.change_selections_inner(autoscroll, false, cx, change)

But that's style nits (albeit will make the code more readable) — what's more important is to combine a new test for that.
We would need to create a fake language server and assign a message handler on it, like this:

zed/crates/editor/src/editor_tests.rs

Lines 5804 to 5805 in b53d58e

	fake_server
	.handle_request::<lsp::request::Formatting, _, _>(move |params, _| async move {

We can return anything hardcoded here, as we do not care to apply anything in this test.
One caveat here for our case would be not to .await the request handling before the edit is made, otherwise nothing will send the LSP request and .await'inig will not work.

other tests around use cx.handle_request that handles the request once, we instead, want to have a test request counter, so cx method is not good for us:

zed/crates/editor/src/inlay_hint_cache.rs

Line 1435 in b53d58e

let lsp_request_count = Arc::new(AtomicU32::new(0));

when we have both, we can trigger completions and check for completion menu to appear similar to

zed/crates/editor/src/editor_tests.rs

Line 8425 in b53d58e

cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"});

test.

Uniting that together into a new test, we can check that on N cx.simulate_keystroke with the context menu open, we send N completion requests exactly.

[babyccino]

Thank you, looks much better now.

to change as little as possible

Funny, but now the diff is less than before, with the parameter

complex interactions

Those usually grow faster from a bloated state, when a struct has a lot of parameters which interact with each other around the methods. Now though, we just have two tracks: for selecting with completion re-querying and without, no extra state added.

approximately what you were thinking

Yes, looks very close to! I would hide the whole // -- change_selections without triggering show_completions section into a method. Something like

// existing public API, used in many places, we keep its signature, but change the body
pub fn change_selections<R>(
        &mut self,
        autoscroll: Option<Autoscroll>,
        cx: &mut ViewContext<Self>,
        change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R,
    ) -> R {
    // old body is moved into that inner function and adjusted a bit
    self.change_selections_inner(autoscroll, true, cx, change)
}

pub fn change_selections_inner<R>(
        &mut self,
        autoscroll: Option<Autoscroll>,
        request_completions: bool,
        cx: &mut ViewContext<Self>,
        change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R,
    ) -> R {
    // here goes old `change_selections` logic + new parameter handling
}

// somewhere in the handle_edit place, we use self.change_selections_inner(autoscroll, false, cx, change)

But that's style nits (albeit will make the code more readable) — what's more important is to combine a new test for that. We would need to create a fake language server and assign a message handler on it, like this:

zed/crates/editor/src/editor_tests.rs

Lines 5804 to 5805 in b53d58e

	fake_server
	.handle_request::<lsp::request::Formatting, _, _>(move |params, _| async move {

We can return anything hardcoded here, as we do not care to apply anything in this test. One caveat here for our case would be not to .await the request handling before the edit is made, otherwise nothing will send the LSP request and .await'inig will not work.

other tests around use cx.handle_request that handles the request once, we instead, want to have a test request counter, so cx method is not good for us:

zed/crates/editor/src/inlay_hint_cache.rs

Line 1435 in b53d58e

let lsp_request_count = Arc::new(AtomicU32::new(0));

when we have both, we can trigger completions and check for completion menu to appear similar to

zed/crates/editor/src/editor_tests.rs

Line 8425 in b53d58e

cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"});

test.

Uniting that together into a new test, we can check that on N cx.simulate_keystroke with the context menu open, we send N completion requests exactly.

Just added those comments to check in with you I was going to get rid of them 😅... if I remembered

I've already done the test pretty much exactly as you've said. I'll push it in a sec

[babyccino]

I've added a test specifically for checking for duplicate requests as well as ammending another completions test to also check for duplicate requests. Not sure if that's completely necessary, maybe you can delete the new test? Both of these tests fail if you change !completions_triggered to true here which is the old behaviour

zed/crates/editor/src/editor.rs

Line 2861 in ddc196c

this.selections_did_change(true, &old_cursor_position, !completions_triggered, cx);

[SomeoneToIgnore]

Nice, thank you so much for testing the other code too.
And a simpler way to test that too!

I think the logic is a bit odd still, let's fix it and correct the atomics, and we're ready to merge.

[SomeoneToIgnore]

Nice, now we have a test (that takes most of the change), and a small production code change, no extra state.
One small note would be that
// old body is moved into that inner function and adjusted a bit comment is not really needed, but it does not matter at this point.

Thank you for doing all this!

[babyccino]

Nice, now we have a test (that takes most of the change), and a small production code change, no extra state. One small note would be that // old body is moved into that inner function and adjusted a bit commend is not really needed, but it does not matter at this point.

Thank you for doing all this!

Oops I copied your code above and forgot to remove it 😅