Skip to content

Disable auto-complete on comments #5089

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 2 commits into from
Jul 2, 2020

Conversation

BGluth
Copy link
Contributor

@BGluth BGluth commented Jun 27, 2020

Resolves #4907 by disabling any auto-completion on comments.

As flodiebold pointed out, in the future we may want to support some form of auto-completion within doc comments, but for now it was suggested to just disable auto-completion on them entirely.

The implementation involves adding a new field is_comment to CompletionContext and checking if the immediate token we auto-completed on is a comment. I couldn't see a case where we need to check any of the ancestors, but let me know if this is not sufficient. I also wasn't sure if it was necessary to add a new field to this struct, but I decided it's probably the best option if we want to potentially do auto-completion on doc comments in the future.

Finally, the three tests I added should I think ideally not filter results by CompletionKind::Keyword, but if I want to get unfiltered results, I need access to a non-public function get_all_completion_items which I don't know if I should make public just for this.

@BGluth BGluth changed the title Disable autocomplete on comments Disable auto-complete on comments Jun 27, 2020
@matklad
Copy link
Member

matklad commented Jun 27, 2020

I think only keyword completion is triggered in comments, so let’s at least move the check to keywords.

All the other completion contributors are more specific, and just don’t match comments at all.

I wonder why keywords match thoug? Perhaps there’s some bug?

What I am getting at is that “should this completion be triggered here” check is supposed to be precise, if keywords trigger in comments, there might be some additional cases where they falsely trigger, and we need to avoid that as well.

@BGluth
Copy link
Contributor Author

BGluth commented Jun 27, 2020

Good call. Somehow it didn't occur to me as odd that only one set of checks (complete_expr_keyword) was returning completions in comments...

However, I still think explicitly checking if we are on a comment in complete_expr_keyword may be a good solution. The checks that are currently used for keyword completion items I feel are probably correct in the majority of cases, although I'm not certain. The checks for all keyword completion items are just checking the state of various combinations of CompletionContext fields. For example, these are the only two requirements to add the const and type completion items:

if ctx.has_item_list_or_source_file_parent || ctx.block_expr_parent {
    add_keyword(ctx, acc, "const", "const ");
    add_keyword(ctx, acc, "type", "type ");
}

It's easy to see that something like:

fn foo() {
    x = 2 // A comment<|>
}

will still add const and type to the list of completion items because the parent is a block (ctx.has_item_list_or_source_file_parent and ctx.block_expr_parent are actually actually true/false respectively ATM which I'm pretty sure is wrong, but I can create a separate PR for this). Other completion functions (ie. complete_fn_param, complete_qualified_path) end up ignoring comments by doing an early return at the beginning. For example, complete_qualified_path returns early if it's not dealing with a path.

Because the rules to add keywords in complete_expr_keyword seem correct when not dealing with comments, I think adding an early return if we are dealing with a comment might be appropriate. Unlike the other completion functions, this one is active in multiple contexts where we can't just do an early return if we're not dealing with a function/path/record/etc. I think here we probably should explicitly check for these edge cases.

@matklad
Copy link
Member

matklad commented Jun 30, 2020

scoping is comment check to only keywords seems fine by me!

@BGluth BGluth force-pushed the 4907_comments_autocomplete_disable branch from e06671c to c03deed Compare July 1, 2020 21:17
@BGluth BGluth force-pushed the 4907_comments_autocomplete_disable branch from c03deed to cc77bdf Compare July 1, 2020 21:20
@BGluth
Copy link
Contributor Author

BGluth commented Jul 1, 2020

@matklad I changed the implementation to just check if the current token is a comment in complete_expr_keyword.

Hope it's ok to do a direct token comparison here. I can't see another way to check if it's a comment.

Also, it looks like npm audit is failing for ubuntu-latest, but I don't think this is from any of my changes.

@matklad
Copy link
Member

matklad commented Jul 2, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 2, 2020

@bors bors bot merged commit 57ed622 into rust-lang:master Jul 2, 2020
@BGluth BGluth deleted the 4907_comments_autocomplete_disable branch July 7, 2020 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Top level keywords show up as completions in non-code comments
2 participants