Completion context expected type #8008

JoshMcguigan · 2021-03-14T07:19:25Z

Currently there are two ways completions use to determine the expected type. There is the expected_type field on the CompletionContext, as well as the expected_name_and_type method on the RenderContext. These two things returned slightly different results, and their results were only valid if you had pre-checked some (undocumented) invariants. A simple combination of the two approaches doesn't work because they are both too willing to go far up the syntax tree to find something that fits what they are looking for.

This PR makes the following changes:

Updates the algorithm that sets expected_type on CompletionContext
Adds expected_name field to CompletionContext
Re-writes the expected_name_and_type method to simply return the underlying fields from CompletionContext (I'd like to save actually removing this method for a follow up PR just to keep the scope of the changes down)
Adds unit tests for the expected_type/expected_name fields

All the existing unit tests still pass (unmodified), but this new algorithm certainly has some gaps (although I believe all the FIXME introduced in this PR are also flaws in the current code). I wanted to stop here and get some feedback though - is this approach fundamentally sound?

crates/ide_completion/src/context.rs

matklad · 2021-03-14T15:20:15Z

I'd like to save actually removing this method for a follow up PR just to keep the scope of the changes down

👍

The overall approach here does seem sound to me!

crates/ide_completion/src/context.rs

matklad · 2021-03-14T15:47:05Z

crates/ide_completion/src/context.rs

+                            // Is there some way to directly look up the type of the
+                            // NameRef `a`?


You might be looking for https://github.com/rust-analyzer/rust-analyzer/blob/0c2f570151749d14e351f30fa234693b36bc4e72/crates/syntax/src/ast/node_ext.rs#L275-L282

Due to field shorthand, the AST situation here is non-trivial.

also, a bunch of map flattens can be converted to and_thens

You might be looking for

I don't think this is what I need. I already have some code which gets the ast::RecordExprField, but then Semantics::resolve_record_field returns None because in this case my record field has no expr.

This is when the source code looks like:

struct A { a: u32 } fn foo() { A { a: $0 }; }

I'm fairly certain the existing implementation (before this PR) has the same problem here, so I'd be happy to push this off to a future PR if the solution is complex.

yeah, plus one to postponing this!

This has been resolved after rebasing since #8025 merged 🎉

I've updated the tests and removed the FIXME comments in both places!

matklad · 2021-03-14T15:49:43Z

crates/ide_completion/src/context.rs

+                            cov_mark::hit!(expected_type_if_let_with_leading_char);
+                            cov_mark::hit!(expected_type_if_let_without_leading_char);
+                            cov_mark::hit!(expected_type_match_arm_with_leading_char);


I am a bit wary about several hit!s at the same place. To my mind, the difference between zero and one mark is huge, because that lets you to quickly identify the interesting test. Additional marks generally don't add as much value (other tests are probably nearby anyway), but they make the code more verbose.

Yeah, I wasn't sure about this. I went forward with it because:

These marks were genuinely useful in helping me reason about this code during development, and I figure if they are useful now as I am writing this code they will be at least 2x as useful when myself or someone else needs to change it in the future.

I considered perhaps only having marks on some of the tests but then I'd need to arbitrarily draw the line somewhere, which seems harder than putting them in each test.

That said, I can remove them if you prefer that. Although if we want to remove them, I'd prefer to remove them all rather than only some of them (as discussed in point 2 above).

I am fine with keeping them all! I'd be strongly against removing all, as having just one mark is hugely helpful.

In the future, if someone sees this logic, they might wonder "ok, what's the actual rust code that causes us to hit this case?" By grepping for a mark, they can quickly identify one example.

Anyway, that's not really all that important

bors r+

matklad · 2021-03-14T18:19:46Z

I usually just not print anything, so that the expected output is an empty string in case the expectations are empty

…

On Sunday, 14 March 2021, Josh Mcguigan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/ide_completion/src/context.rs <#8008 (comment)> : > + expected_type: Option<&str>, + expected_name: Option<&str>, Thanks for the suggestion - this is much better! I'm not sure if there is a standard way used across the codebase for printing optional types. I didn't think the Some/None strings were very useful, so I just print the value if it is Some, or else print ?. But let me know if there is some other standard I should be following. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8008 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M6XUOCA47SSJLITR53TDT3RPANCNFSM4ZEVX2FQ> .

JoshMcguigan · 2021-03-14T19:00:09Z

I usually just not print anything, so that the expected output is an empty string in case the expectations are empty

This seems tricky if we want the expect test to be one line, which seems nice since it is so short. The problem is that in theory either field could be None, so if you just put both fields on one line without some labels or placeholders reading the tests could be difficult.

I think the current approach provides enough context to make the tests readable, without overwhelming with the amount of output. But of course this format is now easy to change, so I'm open to alternatives here.

JoshMcguigan · 2021-03-14T19:14:28Z

@matklad is there anything else you'd for sure want to see before this merges? I know there is probably a lot that can be done to improve it, but I believe at this point it meets or exceeds the existing behavior in all cases.

Merging this sooner than later allows parallelizing two streams of work - one is further improvements of the calculations for expected_type and expected_name, and the other is further completion work (such as #7992) which would build on this.

matklad · 2021-03-15T12:53:58Z

is there anything else you'd for sure want to see before this merges?

Ah, sorry, I though I did give an d+ here, but apparently I didn't. Guess it's easier to invite you as a colaborator, so that you have r+ :)

matklad · 2021-03-15T12:56:22Z

bors r+

?

bors · 2021-03-15T12:56:23Z

Already running a review

lnicola · 2021-03-15T13:02:19Z

changelog internal clean up CompletionContext::expected_type

bors · 2021-03-15T13:06:48Z

Build succeeded:

…f using fields added in rust-lang#8008

8027: Completion context remove exact match method in favor of fields r=JoshMcguigan a=JoshMcguigan This is a minor cleanup PR following #8008. It removes the `expected_name_and_type` method on completion context in favor of using the fields. I thought this method was used in more places, or else it may have just made sense to make this change directly in #8008 🤷 Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>

JoshMcguigan mentioned this pull request Mar 14, 2021

improved completion sorting for functions and methods #7992

Merged

JoshMcguigan force-pushed the completion-expected-type branch from 06825d7 to 97a8199 Compare March 14, 2021 07:37

matklad reviewed Mar 14, 2021

View reviewed changes

crates/ide_completion/src/context.rs Outdated Show resolved Hide resolved

matklad reviewed Mar 14, 2021

View reviewed changes

crates/ide_completion/src/context.rs Outdated Show resolved Hide resolved

matklad reviewed Mar 14, 2021

View reviewed changes

JoshMcguigan force-pushed the completion-expected-type branch 2 times, most recently from 6eb09ab to 7a521e6 Compare March 14, 2021 18:18

JoshMcguigan force-pushed the completion-expected-type branch from 7a521e6 to acac3f4 Compare March 14, 2021 18:33

update algorithm for determining expected type of completion

d91151c

JoshMcguigan force-pushed the completion-expected-type branch from acac3f4 to d91151c Compare March 15, 2021 12:38

JoshMcguigan mentioned this pull request Mar 15, 2021

Goto definition works for S { a: } case #8025

Merged

bors bot merged commit 0ac7a19 into rust-lang:master Mar 15, 2021

JoshMcguigan added a commit to JoshMcguigan/rust-analyzer that referenced this pull request Mar 15, 2021

remove expected_name_and_type method on completion context in favor o…

67d59ae

…f using fields added in rust-lang#8008

JoshMcguigan mentioned this pull request Mar 15, 2021

Completion context remove exact match method in favor of fields #8027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Completion context expected type #8008

Completion context expected type #8008

JoshMcguigan commented Mar 14, 2021 •

edited

matklad commented Mar 14, 2021

matklad Mar 14, 2021

JoshMcguigan Mar 14, 2021

matklad Mar 15, 2021

JoshMcguigan Mar 15, 2021

matklad Mar 14, 2021

JoshMcguigan Mar 14, 2021

matklad Mar 15, 2021

matklad commented Mar 14, 2021 via email

JoshMcguigan commented Mar 14, 2021

JoshMcguigan commented Mar 14, 2021

matklad commented Mar 15, 2021

matklad commented Mar 15, 2021

bors bot commented Mar 15, 2021

lnicola commented Mar 15, 2021

bors bot commented Mar 15, 2021

		// Is there some way to directly look up the type of the
		// NameRef `a`?

Completion context expected type #8008

Completion context expected type #8008

Conversation

JoshMcguigan commented Mar 14, 2021 • edited

matklad commented Mar 14, 2021

matklad Mar 14, 2021

Choose a reason for hiding this comment

JoshMcguigan Mar 14, 2021

Choose a reason for hiding this comment

matklad Mar 15, 2021

Choose a reason for hiding this comment

JoshMcguigan Mar 15, 2021

Choose a reason for hiding this comment

matklad Mar 14, 2021

Choose a reason for hiding this comment

JoshMcguigan Mar 14, 2021

Choose a reason for hiding this comment

matklad Mar 15, 2021

Choose a reason for hiding this comment

matklad commented Mar 14, 2021 via email

JoshMcguigan commented Mar 14, 2021

JoshMcguigan commented Mar 14, 2021

matklad commented Mar 15, 2021

matklad commented Mar 15, 2021

bors bot commented Mar 15, 2021

lnicola commented Mar 15, 2021

bors bot commented Mar 15, 2021

JoshMcguigan commented Mar 14, 2021 •

edited