Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Retrigger visibility completion after parentheses #12412

Merged
merged 1 commit into from
May 30, 2022
Merged

fix: Retrigger visibility completion after parentheses #12412

merged 1 commit into from
May 30, 2022

Conversation

yue4u
Copy link
Contributor

@yue4u yue4u commented May 29, 2022 •
edited
Loading

close #12390

This PR add ( to trigger_characters as discussed in original issue.

Some questions:

  1. Is lsp's ctx.trigger_character from params.context is the same as ctx.original_token inside actually completions?
    1. If not what's the difference?
    2. if they are the same, it's unnecessary to pass it down from handler at all.
    3. if they are the same, maybe we could parse it from fixture directly instead of using the check_with_trigger_character I added.
  2. Some completion fixtures written as ($0) ( https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-completion/src/tests/fn_param.rs#L105 as an example), If I understand correctly they are not invoked outside tests at all?
    1. using ctx.original_token directly would break these tests as well as parsing trigger_character from fixture for now.
    2. I think it make sense to allow ( triggering these cases?
  3. I hope this line up with Tracking issue for ide-completion refactor #12144

yue4u
yue4u commented May 29, 2022
View reviewed changes
crates/ide-completion/src/lib.rs
completions::r#type::complete_inferred_type(acc, ctx);
completions::use_::complete_use_tree(acc, ctx);
// prevent `(` from triggering unwanted completion noise
if trigger_character != Some("(") {
Copy link
Contributor Author

@yue4u yue4u May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Could put this down to individual tests but I think it's harder to understand and will just mess the codebase more. Current approach does not scale very well if we want to allow ( to trigger more completions though.

Copy link
Member

Choose a reason for hiding this comment

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

Ye I think putting it here for now is fine, depending on the final state of the completion architecture we might wanna move this somewhere else.

crates/rust-analyzer/src/caps.rs
@@ -29,7 +29,12 @@ pub fn server_capabilities(config: &Config) -> ServerCapabilities {
hover_provider: Some(HoverProviderCapability::Simple(true)),
completion_provider: Some(CompletionOptions {
resolve_provider: completions_resolve_provider(config.caps()),
trigger_characters: Some(vec![":".to_string(), ".".to_string(), "'".to_string()]),
trigger_characters: Some(vec![
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there some reasons for this not being a Vec<char>?

Copy link
Member

@Veykril Veykril May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

json has no notion of characters, so strings are used

crates/rust-analyzer/src/handlers.rs
let items = match snap.analysis.completions(
completion_config,
position,
completion_trigger_character.as_deref(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think trigger_character is a state as the same as position and putting it into config would be weird.

@Veykril
Copy link
Member

Veykril commented May 29, 2022

Is lsp's ctx.trigger_character from params.context is the same as ctx.original_token inside actually completions?

ctx.trigger_character is the character that triggered the completion, this is only set if the completion was in fact triggered by a character, if it was manually invoked the trigger character is none.

@Veykril
Copy link
Member

Veykril commented May 29, 2022

Seems good to me overall, will wait on merging this until tomorrow though.

@Veykril
Copy link
Member

Veykril commented May 30, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 30, 2022

📌 Commit 1b5f046 has been approved by Veykril

@bors
Copy link
Contributor

bors commented May 30, 2022

⌛ Testing commit 1b5f046 with merge e4ead8a...

@bors
Copy link
Contributor

bors commented May 30, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing e4ead8a to master...

@bors bors merged commit e4ead8a into rust-lang:master May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veykril Veykril Veykril left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pub(crate) completion leaves closing ) behind
3 participants
@yue4u @Veykril @bors