Skip to content

fix: filter unnecessary completions after colon #13611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 26, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion crates/ide-completion/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use syntax::{
ast::{self, AttrKind, NameOrNameRef},
AstNode,
SyntaxKind::{self, *},
SyntaxToken, TextRange, TextSize,
SyntaxToken, TextRange, TextSize, T,
};
use text_edit::Indel;

Expand Down Expand Up @@ -569,6 +569,28 @@ impl<'a> CompletionContext<'a> {
// completing on
let original_token = original_file.syntax().token_at_offset(offset).left_biased()?;

// try to skip completions on path with qinvalid colons
// this approach works in normal path and inside token tree
match original_token.kind() {
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;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Veykril Veykril Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@yue4u yue4u Nov 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in .b701b14ca5b195fd6e227b913a912a4fcc87efb0 I mean e1de04d

T![::] if !is_prev_token_valid_path_start_or_segment(&original_token) => {
return None;
}
_ => {}
}

let AnalysisResult {
analysis,
expected: (expected_type, expected_name),
Expand Down Expand Up @@ -618,6 +640,24 @@ impl<'a> CompletionContext<'a> {
}
}

fn is_prev_token_valid_path_start_or_segment(token: &SyntaxToken) -> bool {
if let Some(prev_token) = token.prev_token() {
// token before coloncolon is invalid
if !matches!(
prev_token.kind(),
// trival
WHITESPACE | COMMENT
// PathIdentSegment
| IDENT | T![super] | T![self] | T![Self] | T![crate]
// QualifiedPath
| T![>]
) {
return false;
}
}
true
}

const OP_TRAIT_LANG_NAMES: &[&str] = &[
"add_assign",
"add",
Expand Down
1 change: 0 additions & 1 deletion crates/ide-completion/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ pub fn completions(
completions::vis::complete_vis_path(&mut completions, ctx, path_ctx, has_in_token);
}
}
// prevent `(` from triggering unwanted completion noise
return Some(completions.into());
}

Expand Down
24 changes: 24 additions & 0 deletions crates/ide-completion/src/tests/attribute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,30 @@ fn attr_in_source_file_end() {
);
}

#[test]
fn invalid_path() {
check(
r#"
//- proc_macros: identity
#[proc_macros:::$0]
struct Foo;
"#,
expect![[r#""#]],
);

check(
r#"
//- minicore: derive, copy
mod foo {
pub use Copy as Bar;
}
#[derive(foo:::::$0)]
struct Foo;
"#,
expect![""],
);
}

mod cfg {
use super::*;

Expand Down
97 changes: 96 additions & 1 deletion crates/ide-completion/src/tests/special.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,22 @@

use expect_test::{expect, Expect};

use crate::tests::{check_edit, completion_list_no_kw};
use crate::tests::{check_edit, completion_list_no_kw, completion_list_with_trigger_character};

fn check(ra_fixture: &str, expect: Expect) {
let actual = completion_list_no_kw(ra_fixture);
expect.assert_eq(&actual)
}

pub(crate) fn check_with_trigger_character(
ra_fixture: &str,
trigger_character: Option<char>,
expect: Expect,
) {
let actual = completion_list_with_trigger_character(ra_fixture, trigger_character);
expect.assert_eq(&actual)
}

#[test]
fn completes_if_prefix_is_keyword() {
check_edit(
Expand Down Expand Up @@ -893,3 +902,89 @@ fn f() {
"#]],
);
}

Copy link
Contributor Author

@yue4u yue4u Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#[test]
fn completes_after_colon_with_trigger() {
check_with_trigger_character(
r#"
//- minicore: option
fn foo { ::$0 }
"#,
Some(':'),
expect![[r#"
md core
"#]],
);
check_with_trigger_character(
r#"
//- minicore: option
fn foo { /* test */::$0 }
"#,
Some(':'),
expect![[r#"
md core
"#]],
);

check_with_trigger_character(
r#"
fn foo { crate::$0 }
"#,
Some(':'),
expect![[r#"
fn foo() fn()
"#]],
);

check_with_trigger_character(
r#"
fn foo { crate:$0 }
"#,
Some(':'),
expect![""],
);
}

#[test]
fn completes_after_colon_without_trigger() {
check_with_trigger_character(
r#"
fn foo { crate::$0 }
"#,
None,
expect![[r#"
fn foo() fn()
"#]],
);

check_with_trigger_character(
r#"
fn foo { crate:$0 }
"#,
None,
expect![""],
);
}

#[test]
fn no_completions_in_invalid_path() {
check(
r#"
fn foo { crate:::$0 }
"#,
expect![""],
);
check(
r#"
fn foo { crate::::$0 }
"#,
expect![""],
);

check(
r#"
fn foo { crate:::::$0 }
"#,
expect![""],
);
}
14 changes: 1 addition & 13 deletions crates/rust-analyzer/src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use lsp_types::{
use project_model::{ManifestPath, ProjectWorkspace, TargetKind};
use serde_json::json;
use stdx::{format_to, never};
use syntax::{algo, ast, AstNode, TextRange, TextSize, T};
use syntax::{algo, ast, AstNode, TextRange, TextSize};
use vfs::AbsPathBuf;

use crate::{
Expand Down Expand Up @@ -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 {
Copy link
Contributor Author

@yue4u yue4u Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to keep the filtering here but

  1. it's not testable (here)
  2. we need to parse the file when trigger is Some(':') | None anyway and I hope this change has similar perf (filtered before context creation)
  3. actually the new behavior has nothing to do with the trigger char

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let source_file = snap.analysis.parse(position.file_id)?;
let left_token = source_file.syntax().token_at_offset(position.offset).left_biased();
let completion_triggered_after_single_colon = match left_token {
Some(left_token) => left_token.kind() == T![:],
None => true,
};
if completion_triggered_after_single_colon {
return Ok(None);
}
}

let completion_config = &snap.config.completion();
let items = match snap.analysis.completions(
completion_config,
Expand Down