-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Changes from 1 commit
f26d548
a6d0e34
7a568f7
e1de04d
1ca5cb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ use syntax::{ | |
ast::{self, AttrKind, NameOrNameRef}, | ||
AstNode, | ||
SyntaxKind::{self, *}, | ||
SyntaxToken, TextRange, TextSize, | ||
SyntaxToken, TextRange, TextSize, T, | ||
}; | ||
use text_edit::Indel; | ||
|
||
|
@@ -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; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can just return None if we find a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parser will produce two continuous There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated in |
||
T![::] if !is_prev_token_valid_path_start_or_segment(&original_token) => { | ||
Veykril marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return None; | ||
} | ||
_ => {} | ||
} | ||
|
||
let AnalysisResult { | ||
analysis, | ||
expected: (expected_type, expected_name), | ||
|
@@ -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", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
|
@@ -893,3 +902,89 @@ fn f() { | |
"#]], | ||
); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
#[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![""], | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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::{ | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible to keep the filtering here but
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Uh oh!
There was an error while loading. Please reload this page.