fix: filter unnecessary completions after colon #13611

yue4u · 2022-11-12T13:55:30Z

close #13597
related: #10173

This PR also happens to fix two extra issues:

The test case in https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-completion/src/tests/attribute.rs#L778-L801 was never triggered in previous behavior.

after:

tt.mp4

2. completions were triggered even in invalid paths, like
fn main() { core:::::$0 }

#[:::::$0] struct X;

only ::: and :::: is excluded as discussed in #13611 (comment)

yue4u · 2022-11-12T13:58:45Z

crates/rust-analyzer/src/handlers.rs

@@ -812,18 +812,6 @@ pub(crate) fn handle_completion(
    let completion_trigger_character =
        params.context.and_then(|ctx| ctx.trigger_character).and_then(|s| s.chars().next());

-    if Some(':') == completion_trigger_character {


It's possible to keep the filtering here but

it's not testable (here)

we need to parse the file when trigger is Some(':') | None anyway and I hope this change has similar perf (filtered before context creation)

actually the new behavior has nothing to do with the trigger char

the filtering definitely should not be happening here, so moving the handling into the ide-completion crate seems proper to me 👍

yue4u · 2022-11-12T14:00:46Z

crates/ide-completion/src/tests/special.rs

@@ -893,3 +902,89 @@ fn f() {
        "#]],
    );
 }
+


not sure if putting tests here (special.rs) is better than https://github.com/rust-lang/rust-analyzer/blob/45ec315e01dc8dd1146dfeb65f0ef6e5c2efed78/crates/ide-completion/src/tests.rs
I didn't find any expect_test usage there and not sure if there are any reasons for that

Veykril

Apologies, I thought I had already reviewed this last week

Veykril · 2022-11-19T12:56:11Z

crates/rust-analyzer/src/handlers.rs

@@ -812,18 +812,6 @@ pub(crate) fn handle_completion(
    let completion_trigger_character =
        params.context.and_then(|ctx| ctx.trigger_character).and_then(|s| s.chars().next());

-    if Some(':') == completion_trigger_character {


the filtering definitely should not be happening here, so moving the handling into the ide-completion crate seems proper to me 👍

crates/ide-completion/src/context.rs

Veykril · 2022-11-19T13:01:17Z

crates/ide-completion/src/context.rs

+            T![:] => {
+                // return if no prev token before colon
+                let prev_token = original_token.prev_token()?;
+
+                // only has a single colon
+                if prev_token.kind() != T![:] {
+                    return None;
+                }
+
+                if !is_prev_token_valid_path_start_or_segment(&prev_token) {
+                    return None;
+                }
+            }


I think we can just return None if we find a T![:] without any additional checks? The previous token can't be another T![:] as the parser should produce a T![::] then I think?

Parser will produce two continuous T![:] if we are inside token streams and that's the case I wanted to support as the video in the description.

However that will produce too much noise if we are accidentally typing like :::. Every single : after will trigger the completion again. So I introduced is_prev_token_valid_path_start_or_segment to solve that. I know it's invalid input but felt this behavior pretty annoying during testing without the check.

Oh I see, ye then we need something like that here. Though I think this is still too restrictive, so maybe let's instead check that we either have a ::, or exactly two : tokens in a row (but not three)? I think that's a decent trade off unless I am missing something else. We can't make completions in macros perfect unfortunately. In the future if we know about fragment types for inputs this can be potentially refined.

updated in ~~.b701b14ca5b195fd6e227b913a912a4fcc87efb0~~ I mean e1de04d

crates/ide-completion/src/context.rs

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

yue4u · 2022-11-19T16:36:58Z

I removed the check before :: in 7a568f7 but still not sure about #13611 (comment)

if we don't bother completing inside token tree then it's ok to remove the checking (same as current behavior)
if we want to completing inside token tree
1. I think it's better to deal with the ::: case but it's not necessary
2. more invalid completions might be triggered than before

yue4u · 2022-11-19T16:44:18Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

yue4u · 2022-11-26T17:16:27Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

Veykril · 2022-11-26T17:21:08Z

crates/ide-completion/src/tests/special.rs

-    );
-    check(
-        r#"
-fn foo { crate::::$0 }


Ah wait, we no longer catch this because the token here is a :: again right? Let's add that to the check as well as it is simple, I forgot that :) That is, t.kind() == T![:] || t.kind() == T![::] for the previous token check

done 1ca5cb7

Veykril · 2022-11-26T17:21:54Z

@bors delegate+

bors · 2022-11-26T17:21:56Z

✌️ @yue4u can now approve this pull request

yue4u · 2022-11-26T17:54:55Z

@bors r+

bors · 2022-11-26T17:54:57Z

📌 Commit 1ca5cb7 has been approved by yue4u

It is now in the queue for this repository.

bors · 2022-11-26T17:55:03Z

⌛ Testing commit 1ca5cb7 with merge 34e2bc6...

bors · 2022-11-26T18:04:38Z

☀️ Test successful - checks-actions
Approved by: yue4u
Pushing 34e2bc6 to master...

fix: filter unnecessary completions after colon

f26d548

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2022

yue4u commented Nov 12, 2022

View reviewed changes

yue4u marked this pull request as draft November 12, 2022 14:02

yue4u marked this pull request as ready for review November 12, 2022 14:06

Veykril reviewed Nov 19, 2022

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2022

yue4u and others added 2 commits November 20, 2022 00:58

Update crates/ide-completion/src/context.rs

a6d0e34

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

fix: remove insufficient check for coloncolon

7a568f7

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 19, 2022

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2022

fix: only special casing 3 colon in a row

e1de04d

yue4u force-pushed the fix/completion-after-colon branch from b701b14 to e1de04d Compare November 26, 2022 16:53

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 26, 2022

Veykril reviewed Nov 26, 2022

View reviewed changes

fix: also exclude 2 coloncolon in a row

1ca5cb7

bors merged commit 34e2bc6 into rust-lang:master Nov 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: filter unnecessary completions after colon #13611

fix: filter unnecessary completions after colon #13611

yue4u commented Nov 12, 2022 •

edited

Loading

yue4u Nov 12, 2022 •

edited

Loading

Veykril Nov 19, 2022

yue4u Nov 12, 2022 •

edited

Loading

Veykril left a comment

Veykril Nov 19, 2022

Veykril Nov 19, 2022

yue4u Nov 19, 2022

yue4u Nov 19, 2022

Veykril Nov 24, 2022 •

edited

Loading

yue4u Nov 26, 2022 •

edited

Loading

yue4u commented Nov 19, 2022

yue4u commented Nov 19, 2022

yue4u commented Nov 26, 2022

Veykril Nov 26, 2022

yue4u Nov 26, 2022

Veykril commented Nov 26, 2022

bors commented Nov 26, 2022

yue4u commented Nov 26, 2022

bors commented Nov 26, 2022

bors commented Nov 26, 2022

bors commented Nov 26, 2022

@@ @@ -893,3 +902,89 @@ fn f() { @@
                       "#]],
                   );
               }

fix: filter unnecessary completions after colon #13611

fix: filter unnecessary completions after colon #13611

Conversation

yue4u commented Nov 12, 2022 • edited Loading

yue4u Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

Veykril Nov 19, 2022

Choose a reason for hiding this comment

yue4u Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

Veykril left a comment

Choose a reason for hiding this comment

Veykril Nov 19, 2022

Choose a reason for hiding this comment

Veykril Nov 19, 2022

Choose a reason for hiding this comment

yue4u Nov 19, 2022

Choose a reason for hiding this comment

yue4u Nov 19, 2022

Choose a reason for hiding this comment

Veykril Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

yue4u Nov 26, 2022 • edited Loading

Choose a reason for hiding this comment

yue4u commented Nov 19, 2022

yue4u commented Nov 19, 2022

yue4u commented Nov 26, 2022

Veykril Nov 26, 2022

Choose a reason for hiding this comment

yue4u Nov 26, 2022

Choose a reason for hiding this comment

Veykril commented Nov 26, 2022

bors commented Nov 26, 2022

yue4u commented Nov 26, 2022

bors commented Nov 26, 2022

bors commented Nov 26, 2022

bors commented Nov 26, 2022

yue4u commented Nov 12, 2022 •

edited

Loading

yue4u Nov 12, 2022 •

edited

Loading

yue4u Nov 12, 2022 •

edited

Loading

Veykril Nov 24, 2022 •

edited

Loading

yue4u Nov 26, 2022 •

edited

Loading