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.

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

Tracking issue for ide-completion refactor #12144

Closed
12 of 14 tasks
Veykril opened this issue May 3, 2022 · 1 comment
Closed
12 of 14 tasks

Tracking issue for ide-completion refactor #12144

Veykril opened this issue May 3, 2022 · 1 comment
Labels
A-completion autocompletion S-actionable Someone could pick this issue up and work on it right now

Comments

@Veykril
Copy link
Member

Veykril commented May 3, 2022

In #11397 I started refactoring the completions crate a bit and I haven't gotten back to finishing this up, so the crate is in a weird state where we use 3 differnt things to determine where certain completions apply. So with this I'll write down what I had in mind and where we are currently at in regards to that plan.

Completions are generated by fist calculating a CompletionContext which analyzes the surrounding context of the cursor position which we can then derive the completions from. Following that we just execute a bunch of functions that fill the completions accumulator with completions.

completions::attribute::complete_attribute(&mut acc, &ctx);
completions::attribute::complete_derive(&mut acc, &ctx);
completions::attribute::complete_known_attribute_input(&mut acc, &ctx);
completions::dot::complete_dot(&mut acc, &ctx);
completions::extern_abi::complete_extern_abi(&mut acc, &ctx);
completions::flyimport::import_on_the_fly(&mut acc, &ctx);
completions::fn_param::complete_fn_param(&mut acc, &ctx);
completions::format_string::format_string(&mut acc, &ctx);
completions::inferred_type(&mut acc, &ctx);
completions::keyword::complete_expr_keyword(&mut acc, &ctx);
completions::lifetime::complete_label(&mut acc, &ctx);
completions::lifetime::complete_lifetime(&mut acc, &ctx);
completions::mod_::complete_mod(&mut acc, &ctx);
completions::pattern::complete_pattern(&mut acc, &ctx);
completions::postfix::complete_postfix(&mut acc, &ctx);
completions::qualified_path::complete_qualified_path(&mut acc, &ctx);
completions::record::complete_record_literal(&mut acc, &ctx);
completions::record::complete_record(&mut acc, &ctx);
completions::snippet::complete_expr_snippet(&mut acc, &ctx);
completions::snippet::complete_item_snippet(&mut acc, &ctx);
completions::trait_impl::complete_trait_impl(&mut acc, &ctx);
completions::unqualified_path::complete_unqualified_path(&mut acc, &ctx);
completions::use_::complete_use_tree(&mut acc, &ctx);
completions::vis::complete_vis(&mut acc, &ctx);

The here was to have a function calculating completions for a specific "thing", this "thing" varies widely as to what it applies though. My goal with the refactor was to make this "thing" more consistent in what it covers.

We can group the things we actually complete into roughly the following:

  • keywords
  • Names
  • Lifetimes
  • NameRefs (paths and a few exceptios)
  • snippets
  • very specialized things (string literals, fmt strings, ...)

Obviously grouping the completion generation by just these is too rough, we want to do it differently. Completing by Names and NameRefs alone is messy, as we need to know about the context of where we are completing in (are we inside a type position, expression position, function return type, etc...), but this is what we kind of are doing right now if you take a look at the unqualified path module or the qualfied path module (these are practically the NameRef case). There are a bunch of different checks regarding where we are currently at placed around at various places making the code a mess.

So what I instead envision is for us to split these modules apart according to their position they apply to, with branches for the different thinsg that can be completed there (with a few exceptions for special completions). I've started with this for paths, where I have split out attributes, visibility paths, use paths and pattern paths.
This structure allows one to more easily reason about what completions apply where.

This refactor should get us closer to do "perfect completions".

I'll fully fill out the list of finer grained tasks here later this week:

  • Split apart [unqualfied path]https://github.com/rust-lang/rust-analyzer/blob/0ee4e6a22d79f58b6b459dbc874d6b90a4495d83/crates/ide-completion/src/completions/unqualified_path.rs) and qualfied path into the remaining variants
    • Expressions / Statements
    • Type
    • Derive
    • Attribute
    • Visibility
    • Pattern
    • Use
    • ItemList / AssocItemList (module and sourcefile / trait and impl, macro calls can appear there)
  • Split up keyword module
    • Implement proper keyword completions for items, respecting their order and similar. That is in unsafe const $0 we should only propose fn, not const or unsafe again.
  • Properly handle completions in generic argument position, currently its offered via type path completions with some special cases...
  • Split flyimport into the corresponding modules, this should probably be done last. We need to verify that the modules doing flyimport are disjoint so we don't do this multiple times.
  • Remove is_path_disallowed and other CompletionContext functions that look like temporary hacks.
@Veykril Veykril added A-completion autocompletion S-actionable Someone could pick this issue up and work on it right now labels May 3, 2022
bors added a commit that referenced this issue May 5, 2022
internal: Lift out item list path completions from (un)qualified_path

cc #12144
bors added a commit that referenced this issue May 5, 2022
internal: Lift out item list path completions from (un)qualified_path

cc #12144
bors added a commit that referenced this issue May 5, 2022
internal: Remove `unqualified_path` completions module

cc #12144
bors added a commit that referenced this issue May 30, 2022
fix: Retrigger visibility completion after parentheses

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 #12144
bors added a commit that referenced this issue Jun 3, 2022
internal: Clean up keyword completion handling

#12144
@Veykril
Copy link
Member Author

Veykril commented Jun 17, 2022

With #12560 #12565 #12564 #12562 and #12573 the main refactors in this issue are now done.

I've split the two other checkboxes into separate issues as htey are less of a refactoring task and more of an implementation thing, #12567 #12568

The last refactor we need to do now is #12571 and then we should have reached a state where we can perfect our completions on

@Veykril Veykril closed this as completed Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-completion autocompletion S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

1 participant