fix(completion): make the expected type a tad smarter with `Fn`s #16136

saiintbrisson · 2023-12-16T15:11:36Z

This commit changes how the expected type is calculated when
working with Fn pointers, making the parenthesis stop vanishing
when completing the function name.

I've been bugged by the behavior of parenthesis completion for
a long while now. R-a assumes that the LetStmt type is the same
as the function type I've just written. Worse is that all parenthesis
vanish, even from functions that have completely different signatures.
It will now verify if the signature is the same.

While working on this, I noticed that record fields behave the same,
so I also made it prioritize the field type instead of the current
expression when possible, but I'm unsure if this is OK, so input is
appreciated.

ImplTraits as return types will still behave weirdly because lowering
is disallowed at the time it resolves the function types.

saiintbrisson · 2023-12-16T15:13:07Z

crates/ide-completion/src/context/analysis.rs

+                    let field_ty = sema.resolve_record_field(&it).map(|(_, _, ty)| ty);
+                    let field_name = it.field_name().map(NameOrNameRef::NameRef);
                    if let Some(expr) = it.expr() {
                        cov_mark::hit!(expected_type_struct_field_with_leading_char);
-                        (
-                            sema.type_of_expr(&expr).map(TypeInfo::original),
-                            it.field_name().map(NameOrNameRef::NameRef),
-                        )
+                        let ty = field_ty
+                            .or_else(|| sema.type_of_expr(&expr).map(TypeInfo::original));
+                        (ty, field_name)
                    } else {
                        cov_mark::hit!(expected_type_struct_field_followed_by_comma);
-                        let ty = sema.resolve_record_field(&it)
-                            .map(|(_, _, ty)| ty);
-                        (
-                            ty,
-                            it.field_name().map(NameOrNameRef::NameRef),
-                        )
+                        (field_ty, field_name)


I couldn't think of a reason as to why this was done this way, so please enlighten me if there is indeed a reason.

Unsure why either, your changes look correct here to me.

saiintbrisson · 2023-12-16T15:44:20Z

crates/ide-completion/src/context/analysis.rs

@@ -359,7 +359,8 @@ fn expected_type_and_name(
                    let ty = it.pat()
                        .and_then(|pat| sema.type_of_pat(&pat))
                        .or_else(|| it.initializer().and_then(|it| sema.type_of_expr(&it)))
-                        .map(TypeInfo::original);
+                        .map(TypeInfo::original)
+                        .filter(|ty| it.ty().is_some() || !ty.is_fn());


What I'm doing here is only taking Fn types into account for the let statement if it is explicitly defined, stopping r-a from stripping the parenthesis. Is there a case I'm not seeing where this will be problematic?

Don't think so but please add a commend for why this is done.

Veykril · 2024-01-02T12:17:45Z

crates/ide-completion/src/context/analysis.rs

@@ -359,7 +359,8 @@ fn expected_type_and_name(
                    let ty = it.pat()
                        .and_then(|pat| sema.type_of_pat(&pat))
                        .or_else(|| it.initializer().and_then(|it| sema.type_of_expr(&it)))
-                        .map(TypeInfo::original);
+                        .map(TypeInfo::original)
+                        .filter(|ty| it.ty().is_some() || !ty.is_fn());


Don't think so but please add a commend for why this is done.

saiintbrisson · 2024-01-02T13:07:36Z

@Veykril done!

Veykril · 2024-01-02T13:09:24Z

Thanks!
@bors r+

bors · 2024-01-02T13:09:26Z

📌 Commit d9d4c5c has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-01-02T13:10:34Z

⌛ Testing commit d9d4c5c with merge c090c56...

…h-fns, r=Veykril fix(completion): make the expected type a tad smarter with `Fn`s This commit changes how the expected type is calculated when working with Fn pointers, making the parenthesis stop vanishing when completing the function name. I've been bugged by the behavior of parenthesis completion for a long while now. R-a assumes that the `LetStmt` type is the same as the function type I've just written. Worse is that all parenthesis vanish, even from functions that have completely different signatures. It will now verify if the signature is the same. While working on this, I noticed that record fields behave the same, so I also made it prioritize the field type instead of the current expression when possible, but I'm unsure if this is OK, so input is appreciated. ImplTraits as return types will still behave weirdly because lowering is disallowed at the time it resolves the function types. ![image](https://github.com/rust-lang/rust-analyzer/assets/29989290/c06d6c93-5cac-4ebe-a93b-923017a6ae8c) ![image](https://github.com/rust-lang/rust-analyzer/assets/29989290/31594d82-fa4d-446c-a77e-47e9de1a9a67) ![image](https://github.com/rust-lang/rust-analyzer/assets/29989290/cf33856e-a485-411b-91af-11090d78a44e)

bors · 2024-01-02T13:16:03Z

💔 Test failed - checks-actions

Veykril · 2024-01-02T13:28:02Z

Needs a rebase + reblessing the tests it seems as the output has changed.

This commit changes how the expected type is calculated when working with Fn pointers, making the parenthesis stop vanishing when completing the function name. I've been bugged by the behaviour on parenthesis completion for a long while now. R-a assumes that the `LetStmt` type is the same as the function type I've just written. Worse is that all parenthesis vanish, even from functions that have completely different signatures. It will now verify if the signature is the same. While working on this, I noticed that record fields behave the same, so I also made it prioritize the field type instead of the current expression when possible, but I'm unsure if this is OK, so input is appreciated. ImplTraits as return types will still behave weirdly because lowering is disallowed at the time it resolves the function types.

Veykril · 2024-01-02T13:34:36Z

@bors r+

bors · 2024-01-02T13:34:38Z

📌 Commit 9a36bc3 has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-01-02T13:35:45Z

⌛ Testing commit 9a36bc3 with merge 7659109...

bors · 2024-01-02T13:45:31Z

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

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

saiintbrisson commented Dec 16, 2023

View reviewed changes

Veykril approved these changes Jan 2, 2024

View reviewed changes

saiintbrisson force-pushed the fix/completion/a-tad-smarter-with-fns branch from 7d6e292 to d9d4c5c Compare January 2, 2024 13:07

saiintbrisson added 2 commits January 2, 2024 10:28

fix(completion): render fn type

9a36bc3

saiintbrisson force-pushed the fix/completion/a-tad-smarter-with-fns branch from d9d4c5c to 9a36bc3 Compare January 2, 2024 13:31

bors merged commit 7659109 into rust-lang:master Jan 2, 2024
10 checks passed

bors mentioned this pull request Jan 2, 2024

Draft Callable type matching #16205

Closed

Veykril mentioned this pull request Jan 2, 2024

Completion type match calculations for function items should consider the signature compatability #16053

Closed

This was referenced Jun 11, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#445

Merged

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(completion): make the expected type a tad smarter with `Fn`s #16136

fix(completion): make the expected type a tad smarter with `Fn`s #16136

saiintbrisson commented Dec 16, 2023

saiintbrisson Dec 16, 2023

Veykril Jan 2, 2024

saiintbrisson Dec 16, 2023 •

edited

Loading

Veykril Jan 2, 2024

Veykril Jan 2, 2024

saiintbrisson commented Jan 2, 2024

Veykril commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

Veykril commented Jan 2, 2024

Veykril commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

fix(completion): make the expected type a tad smarter with Fns #16136

fix(completion): make the expected type a tad smarter with Fns #16136

Conversation

saiintbrisson commented Dec 16, 2023

saiintbrisson Dec 16, 2023

Choose a reason for hiding this comment

Veykril Jan 2, 2024

Choose a reason for hiding this comment

saiintbrisson Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Veykril Jan 2, 2024

Choose a reason for hiding this comment

Veykril Jan 2, 2024

Choose a reason for hiding this comment

saiintbrisson commented Jan 2, 2024

Veykril commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

Veykril commented Jan 2, 2024

Veykril commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

fix(completion): make the expected type a tad smarter with `Fn`s #16136

fix(completion): make the expected type a tad smarter with `Fn`s #16136

saiintbrisson Dec 16, 2023 •

edited

Loading