feat: Implement completion for the callable fields. #15879

dfireBird · 2023-11-12T11:27:17Z

PR is opened with basic changes. It could be improved by having a new SymbolKind for the callable fields and implementing a separate render function similar to the render_method for the new SymbolKind.
It could also be done without any changes to the SymbolKind of course, have the new function called based on the type of field.
I prefer the former method.

Please give any thoughts or changes you think is appropriate for this method. I could start working on that in this same PR.

DropDemBits

Some nits and notes, but overall LGTM.

crates/ide-completion/src/render.rs

crates/ide-completion/src/completions/dot.rs

dfireBird · 2023-11-15T16:18:55Z

I've now added the tests for both the cases you mentioned and made the refactors as well.

DropDemBits

Just one last nit that I missed in the first review, and otherwise LGTM.

crates/ide-completion/src/render.rs

Veykril

There is one interesting case that we should also have a test for which is foo.bar::<>, I think this should result in a DotAccessKind::Method { has_parens: false } (the only case where its false I believe as otherwise missing parens will result in a field access node).

crates/ide-completion/src/completions/dot.rs

crates/ide-completion/src/render.rs

bors · 2023-11-21T10:08:56Z

☔ The latest upstream changes (presumably #15825) made this pull request unmergeable. Please resolve the merge conflicts.

dfireBird · 2023-11-23T07:24:48Z

For the foo.bar::<> case, it should complete to (foo).bar::<>() right?
In current implementation it completes to (foo).bar()::<>.

Veykril · 2023-11-23T09:15:35Z

For the foo.bar::<> case, it should complete to (foo).bar::<>() right? In current implementation it completes to (foo).bar()::<>.

I should complete to foo.bar::<>(), no reason to parenthesize the receiver

variable name change for clearer usage indication

Before it was inserting whenever function field is found but it should happend only in the case of function call.

dfireBird · 2023-11-28T16:54:02Z

Sorry I was busy with some thing and couldn't find time to fix the tests. I have fixed the tests now.

I'll create a follow up PR for the foo.baz::<> case, once this PR is merged.

Veykril

Sorry for all the review rounds and thanks for sticking with it, this should be the last :)

Veykril · 2023-11-30T20:20:35Z

crates/ide-completion/src/completions/dot.rs

+        |acc, field, ty| acc.add_tuple_field(ctx, None, field, &ty),
+        is_field_access,
+    );
+
    if let DotAccessKind::Method { .. } = dot_access.kind {
        cov_mark::hit!(test_no_struct_field_completion_for_method_call);


This cov mark makes little sense to be tested like this now (since the else branch is gone, which was what this mark wanted to assert is not being called)

Veykril · 2023-11-30T20:22:35Z

crates/ide-completion/src/completions/dot.rs

+"#,
+            r#"
+struct S { va_field: u32, fn_field: fn() }
+fn foo() { (S { va_field: 0, fn_field: || {} }).fn_field() }


This is why I wanted the check_edit test :^) The output here is wrong, this should be

Suggested change

fn foo() { (S { va_field: 0, fn_field: || {} }).fn_field() }

fn foo() { (S { va_field: 0, fn_field: || {} }.fn_field)() }

This is how you call fields in rust (to disambiguate from method calls)

Sorry for all the review rounds and thanks for sticking with it, this should be the last :)

No worries, this is just lesson for me to put good a PR next time.

I keep getting confused where to put the end parenthesis. I somehow finally decided this was correct thing, my bad.

No worries, this is just lesson for me to put good a PR next time.

All good, the r-a code base takes time to get accustomed to, and the ide-completion crate is a part where I put more review rounds in in terms of quality than ide-assists for example, so I am a bit more thorough here

I keep getting confused where to put the end parenthesis. I somehow finally decided this was correct thing, my bad.

Understandable, it is a rarely used quirk of the language.

Veykril · 2023-11-30T20:29:56Z

crates/ide-completion/src/render.rs

+                if let Some(receiver) = ctx.completion.sema.original_ast_node(receiver.clone()) {
+                    let range = receiver.syntax().text_range();
+                    builder.insert(range.start(), "(".to_string());
+                    builder.insert(range.end(), ")".to_string());


To fix the wrong edit we should probably just check the parent of the (original ast node) receiver here, if its a field add the closing parenthesis after the field etc.

Yeah, I should do something like this, right? I suppose storing the range is kinda useless now.

builder.insert(receiver.syntax().text_range().start(), "(".to_string()); builder.insert(ctx.source_range().end(), ")".to_string());

Ye that should work I think

Veykril · 2023-12-01T16:19:27Z

Thanks!
@bors r+

bors · 2023-12-01T16:19:30Z

📌 Commit b7effe5 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-12-01T16:20:39Z

⌛ Testing commit b7effe5 with merge e402c49...

bors · 2023-12-01T16:31:25Z

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

lnicola · 2023-12-04T07:11:20Z

fix: make callable fields not complete in method access no parens case Follow up PR for #15879 Fixes the callable field completion appearing in the method access with no parens case.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2023

DropDemBits reviewed Nov 14, 2023

View reviewed changes

DropDemBits approved these changes Nov 15, 2023

View reviewed changes

crates/ide-completion/src/render.rs Outdated Show resolved Hide resolved

Veykril reviewed Nov 21, 2023

View reviewed changes

crates/ide-completion/src/completions/dot.rs Outdated Show resolved Hide resolved

crates/ide-completion/src/completions/dot.rs Show resolved Hide resolved

crates/ide-completion/src/render.rs Outdated Show resolved Hide resolved

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2023

dfireBird force-pushed the fix-14656 branch from 52de9ae to 75fec72 Compare November 23, 2023 07:28

dfireBird added 8 commits November 28, 2023 22:21

add different completion for fn fields

a0e690a

implement completion render for callable fields

7cf3ab4

add tests for the completion of the callable field

5bcafd7

refactor obtaining receivers into idiomatic way

eedeb58

variable name change for clearer usage indication

add tests for tuple fields and expect fn type cases

aa1cf8d

fix the insertion of the surronding parens

8296b16

Before it was inserting whenever function field is found but it should happend only in the case of function call.

refactor complete_fn_fields function and correct branch checks

5c0c8ce

update dot tests function with check_edit

21c09eb

dfireBird force-pushed the fix-14656 branch from 75fec72 to 21c09eb Compare November 28, 2023 16:52

dfireBird requested a review from Veykril November 28, 2023 17:00

Veykril reviewed Nov 30, 2023

View reviewed changes

fix close parens position to move after field access

b7effe5

Veykril approved these changes Dec 1, 2023

View reviewed changes

bors merged commit e402c49 into rust-lang:master Dec 1, 2023
10 checks passed

lnicola changed the title ~~Implement completion for the callable fields.~~ feat: Implement completion for the callable fields. Dec 3, 2023

dfireBird mentioned this pull request Dec 4, 2023

Completing callable fields inside struct initialization will insert parenthesis after field name #16014

Closed

dfireBird deleted the fix-14656 branch December 5, 2023 16:18

dfireBird mentioned this pull request Dec 8, 2023

fix: make callable fields not complete in method access no parens case #16049

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement completion for the callable fields. #15879

feat: Implement completion for the callable fields. #15879

dfireBird commented Nov 12, 2023

DropDemBits left a comment

dfireBird commented Nov 15, 2023

DropDemBits left a comment

Veykril left a comment

bors commented Nov 21, 2023

dfireBird commented Nov 23, 2023 •

edited

Veykril commented Nov 23, 2023

dfireBird commented Nov 28, 2023 •

edited

Veykril left a comment •

edited

Veykril Nov 30, 2023

Veykril Nov 30, 2023

dfireBird Dec 1, 2023 •

edited

Veykril Dec 1, 2023

Veykril Nov 30, 2023

dfireBird Dec 1, 2023 •

edited

Veykril Dec 1, 2023

Veykril commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

lnicola commented Dec 4, 2023

	fn foo() { (S { va_field: 0, fn_field: \|\| {} }).fn_field() }
	fn foo() { (S { va_field: 0, fn_field: \|\| {} }.fn_field)() }

feat: Implement completion for the callable fields. #15879

feat: Implement completion for the callable fields. #15879

Conversation

dfireBird commented Nov 12, 2023

DropDemBits left a comment

Choose a reason for hiding this comment

dfireBird commented Nov 15, 2023

DropDemBits left a comment

Choose a reason for hiding this comment

Veykril left a comment

Choose a reason for hiding this comment

bors commented Nov 21, 2023

dfireBird commented Nov 23, 2023 • edited

Veykril commented Nov 23, 2023

dfireBird commented Nov 28, 2023 • edited

Veykril left a comment • edited

Choose a reason for hiding this comment

Veykril Nov 30, 2023

Choose a reason for hiding this comment

Veykril Nov 30, 2023

Choose a reason for hiding this comment

dfireBird Dec 1, 2023 • edited

Choose a reason for hiding this comment

Veykril Dec 1, 2023

Choose a reason for hiding this comment

Veykril Nov 30, 2023

Choose a reason for hiding this comment

dfireBird Dec 1, 2023 • edited

Choose a reason for hiding this comment

Veykril Dec 1, 2023

Choose a reason for hiding this comment

Veykril commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

lnicola commented Dec 4, 2023

dfireBird commented Nov 23, 2023 •

edited

dfireBird commented Nov 28, 2023 •

edited

Veykril left a comment •

edited

dfireBird Dec 1, 2023 •

edited

dfireBird Dec 1, 2023 •

edited