Skip to content

fix: fn_param completion lookup #12085

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 1 commit into from
Apr 27, 2022
Merged

fix: fn_param completion lookup #12085

merged 1 commit into from
Apr 27, 2022

Conversation

yue4u
Copy link
Contributor

@yue4u yue4u commented Apr 26, 2022

close #12073 caused by #12040

@@ -56,7 +55,7 @@ fn fill_fn_params(
ctx: &CompletionContext,
function: &ast::Fn,
param_list: &ast::ParamList,
mut add_new_item_to_acc: impl FnMut(&str, String),
mut add_new_item_to_acc: impl FnMut(&str),
) {
let mut file_params = FxHashMap::default();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we still needs this to be a FxHashMap for removing duplicates

let param = ctx.token.ancestors().find(|node| node.kind() == SyntaxKind::PARAM)?;

let is_mut_token = (ctx.token.kind() == SyntaxKind::MUT_KW).then(|| &ctx.token);
Copy link
Contributor Author

@yue4u yue4u Apr 26, 2022

Choose a reason for hiding this comment

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

I discovered this regression during testing: if current token is exactly mut kw, for example fn foo(mut$0), the ancestor PARAM will include the R_PAREN and removes it incorrectly.

[crates/ide_completion/src/completions/fn_param.rs:160] &param = [email protected]
  [email protected]
    [email protected] "mut"
    [email protected]
      [email protected] ")"

I decided just to special case it but not sure if there are more situations the ancestor PARAM range should be adjusted.

Copy link
Member

Choose a reason for hiding this comment

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

Oh that is a bug in the parser I am pretty sure. Iirc we have the invariant that nodes have balanced parentheses which isn't the case here

Copy link
Member

@Veykril Veykril Apr 26, 2022

Choose a reason for hiding this comment

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

So let's revert this special casing, as we should fix this in the parser instead, I'll look into that actually

Copy link
Member

Choose a reason for hiding this comment

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

Should be fixed with #12090

Copy link
Contributor Author

@yue4u yue4u Apr 27, 2022

Choose a reason for hiding this comment

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

Thanks! I tried #12090 locally, while it fails the fuzz test("the parser seems stuck") it does fix this issue. I removed the kw test 5d63e7a#diff-52c63bea78a6963ad84e5744682632d05437285113ee1c1f3a5521e57518e111R597-R607 from previous commit as I think it'll be better to cover it in the parser.

@yue4u yue4u changed the title fix: fn_param lookup fix: fn_param completion lookup Apr 27, 2022
@Veykril
Copy link
Member

Veykril commented Apr 27, 2022

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Apr 27, 2022

📌 Commit c1685e5 has been approved by Veykril

@bors
Copy link
Contributor

bors commented Apr 27, 2022

⌛ Testing commit c1685e5 with merge 0b49c93...

@bors
Copy link
Contributor

bors commented Apr 27, 2022

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

@bors bors merged commit 0b49c93 into rust-lang:master Apr 27, 2022
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.

parameter completion are no longer being filtered by the client
3 participants