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 all commits
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
28 changes: 27 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,32 @@ 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 invalid 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;
}

// has 3 colon or 2 coloncolon in a row
// special casing this as per discussion in https://github.com/rust-lang/rust-analyzer/pull/13611#discussion_r1031845205
// and https://github.com/rust-lang/rust-analyzer/pull/13611#discussion_r1032812751
if prev_token
.prev_token()
.map(|t| t.kind() == T![:] || t.kind() == T![::])
.unwrap_or(false)
{
return None;
}
}
_ => {}
}

let AnalysisResult {
analysis,
expected: (expected_type, expected_name),
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
90 changes: 89 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,82 @@ 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![""],
)
}
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