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

Auto imports in completion #6553

Merged
merged 18 commits into from
Nov 17, 2020
Merged

Auto imports in completion #6553

merged 18 commits into from
Nov 17, 2020

Conversation

SomeoneToIgnore
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore commented Nov 14, 2020 •
edited
Loading

completion

Closes #1062 but does not handle the completion order, since it's a separate task for #4922 , #4922 and maybe something else.

2 quirks in the current implementation:

  • traits are not auto imported during method completion

If I understand the current situation right, we cannot search for traits by a part of a method name, we need a full name with correct case to get a trait for it.

  • VSCode (?) autocompletion is not as rigid as in Intellij Rust as you can notice on the animation.

Intellij is able to refresh the completions on every new symbol added, yet VS Code does not query the completions on every symbol for me.
With a few debug prints placed in RA, I've observed the following behaviour: after the first set of completion suggestions is received, next symbol input does not trigger a server request, if the completions contain this symbol.
When more symbols added, the existing completion suggestions are filtered out until none are left and only then, on the next symbol it queries for completions.
It seems like the only alternative to get an updated set of results is to manually retrigger it with Esc and Ctrl + Space.

Despite the eerie latter bullet, the completion seems to work pretty fine and fast nontheless, but if you have any ideas on how to make it more smooth, I'll gladly try it out.

@kjeremy
Copy link
Contributor

kjeremy commented Nov 14, 2020 via email

@SomeoneToIgnore
Copy link
Contributor Author

I've tried to emphasise this by adding a full qualifier to the completion label (we don't do that for anything else), but can add something else too, any particular ideas?

@matklad
Copy link
Member

matklad commented Nov 16, 2020

The last bullet is #6565

matklad
matklad reviewed Nov 16, 2020
View reviewed changes
Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Nice!

I like how its' not that much code in the end: we did build some nice infrastructure for this!

crates/completion/src/completions/unqualified_path.rs Outdated Show resolved Hide resolved
crates/ide_db/src/imports_locator.rs Outdated
local_query.limit(40);
local_query
},
ExternalImportablesQuery::new(name_to_import).anchor_end().case_sensitive().limit(40),
Copy link
Member

Choose a reason for hiding this comment

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

Why we don't set exact for External imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Query objects have different methods, this one has no exact, but has achor_end with the documentation:

/// Only returns items whose paths end with the (case-insensitive) query string as their last
/// segment.

crates/completion/src/item.rs Outdated Show resolved Hide resolved
crates/ide_db/src/imports_locator.rs Outdated Show resolved Hide resolved
@flodiebold
Copy link
Member

traits are not auto imported during method completion
If I understand the current situation right, we cannot search for traits by a part of a method name, we need a full name with correct case to get a trait for it.

If you mean the iterate_method_candidates function, that's not how we would want to start the search anyway. We should first look for candidates using the index, and then confirm them using iterate_method_candidates. There's no need to set the name parameter to iterate_method_candidates if the list of trait candidates is pre-filtered to only contain those with methods we're interested in, I think.

@flodiebold
Copy link
Member

... we might need a separate index for that though, indexing traits by their contained methods.

@SomeoneToIgnore
Copy link
Contributor Author

not that much code in the end

And also the code that I've used, was very performant this time, no issues with the lookup at all.
There are so many people made this simple PR happen, amazing.

... we might need a separate index for that though, indexing traits by their contained methods.

I've meant that we miss this index, yes.
If I understand it correctly, we need to change the code somewhere around here https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/hir_def/src/import_map.rs#L70

@SomeoneToIgnore
Copy link
Contributor Author

The last bullet is #6565

I've rebased this PR on top and rechecked, looks like the behaviour did not change much.

@matklad
Copy link
Member

matklad commented Nov 17, 2020

@SomeoneToIgnore I mean you need to flip is_incomplete: false to is_incomplete: true to actually change the behavior.

@SomeoneToIgnore
Copy link
Contributor Author

Oh, feels so much better now.

I'm still amazed that I don't get any "overly long loop turn" messages when I do this on a project with hundreds of crates:

another_completion

@matklad
Copy link
Member

matklad commented Nov 17, 2020

Well, if we are doing good job, we shouldn't see those messages even if every IDE feature is horrible slow :-)

This messages track not whether features are fast, but whether they block the main loop.

@matklad
Copy link
Member

matklad commented Nov 17, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 17, 2020

Build succeeded:

@bors bors bot merged commit 156f7d6 into rust-lang:master Nov 17, 2020
@SomeoneToIgnore SomeoneToIgnore deleted the auto-imports-in-completion branch November 17, 2020 18:31
@kiljacken
Copy link
Contributor

This is so amazing! Great work @SomeoneToIgnore

@SomeoneToIgnore
Copy link
Contributor Author

My feelings are quite mixed when I hear this, so I'll allow myself a little praise on the topic once to explain why I as a contributor would prefer to be ignored in this situation.
(but thank you nontheless :) )

Since my early days of participation in this project, I wanted to implement this feature, it felt cool and charming to be able to deliver such magic to hundreds of people across the globe.

Overall, I've taken 3 serious stabs at this issue and every time it was a different experience:

  • For the first time, all that had existed in the project was a text search index, no macro expansion results or any other fancy stuff and the search was imprecise and laggy, not mentioning the amount of bloat code I had to do in the completions side to just display anything. Still, big chunks of logic were completely absent, for instance, no way I could check if an import path is accessible within the current module.

  • During the second time, we've got macro expansion and a great FST index for external crates, magic module path resolution with all the relativity batteries included.
    Yet, it was very slow by that time and I could not make it smooth enough, even though technically had got something working finally.

  • And by the charmy third time, we've got lots of performance improvements and lots of cool reusable API such as insert import and the ast tree diff ones.
    (not really related to the import index, but seeing the project highlighting and hints appear before all its dependencies being processed was astonishing)
    And only then it clicked and worked so simple and nice, that it's even a bit underwhelming.

Almost 2 years since the original ticket, people were pursuing their own needs, investing into RA infra, their work was supported, PRs reviewed and merged.
Additionally, the other features did not explode and did not break anything, some (such as type inference) had even improved and helped too in the end.
All those efforts unintendedly led to one of the most dull PRs enabling one of the most cool features for me.

I think, this is a remarkable (and relatively rare) sign that we're on the right track and we need to thank everybody who participates in this development.

@georgewfraser
Copy link
Contributor

@SomeoneToIgnore This appears to be the cause of the performance regression described in #6612. Reverting this PR, building and installing rust-analyzer locally fixed it for me.

@kjeremy
Copy link
Contributor

kjeremy commented Nov 24, 2020 •
edited
Loading

Can we move the work into completionItem/resolve for clients that support it?

See #6366

@SomeoneToIgnore
Copy link
Contributor Author

I feel like an "experimental completions" toggle be quicker to add for me first, we'll need it anyway later.
The resolve approach would be my next step later, then.

This was referenced Nov 27, 2020
Merged
Closed
bors bot added a commit that referenced this pull request Nov 28, 2020
@bors @SomeoneToIgnore
Merge #6650
b7ece77 
6650: Make completion and assists module independent r=matklad a=SomeoneToIgnore

A follow-up of #6553 (comment)

Move the common code for both assists and completion modules into a separate crate.

Co-authored-by: Kirill Bulatov <mail4score@gmail.com>
@dialtone dialtone mentioned this pull request Jan 1, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad left review comments

@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.

Auto-complete and auto-import multi-segment path
7 participants
@SomeoneToIgnore @kjeremy @matklad @flodiebold @kiljacken @georgewfraser @Veykril