Reference completions for nested struct fields put & in wrong location

[JoshMcguigan]

struct Foo {
    bar: u32,
}

fn foo(f: Foo) {
    let _: &u32 = f.$0;
    // should complete to &f.bar
    // actually completes to f.&bar
}

https://github.com/rust-analyzer/rust-analyzer/blob/00c80b208bcbe52b13bbd03cb62e24b2d2075edf/crates/ide_completion/src/item.rs#L340

https://github.com/rust-analyzer/rust-analyzer/blob/00c80b208bcbe52b13bbd03cb62e24b2d2075edf/crates/rust-analyzer/src/to_proto.rs#L247-L260

Perhaps ref_match should return a new CompletionItem? Because right now to_proto is trying to make sense of how to apply the reference, but that seems like it should be the responsibility of the ide_completion crate.

I'm not going to have time to look at this too much further until early next week, so if anyone else is interested please feel free to take this one. Otherwise I'll give it a shot in a week or so.

[JoshMcguigan]

I've implemented a quick workaround for this in #8142. I plan to keep working on a proper fix, but I expect that to take a bit longer and it would be nice to fix this bug before the next weekly release.

[lnicola]

The weeklies are Sunday nightlies that get promoted, so this won't get in the release unless we merge it and make a new nightly (I think). This is a pretty annoying issue, but I'm not sure it warrants that.

@matklad any thoughts?

[matklad]

I think it's ok to live with this for another week!

[JoshMcguigan]

Now that #8142 has temporarily disabled the buggy/incorrect completions, I'd like to fix this properly so we can re-enable reference completions for struct fields/methods.

I think I'd like to start with a refactor:

1. Remove the ref_match field from CompletionItem (ref matches would be represented as a full CompletionItem)
2. Update the render_nnn methods to take in the Completions struct so they can directly add both the regular completion and the ref match completion if applicable

This should not change any behavior, but should increase the flexibility of the system. This would allow for:

3. Re-enable ref match completions for struct fields/methods, with the appropriate label/source_range/text_edit/etc.

@matklad does this seem reasonable to you?

[JoshMcguigan]

I've been digging into this a bit more.

I think I made a mistake in #8036 having render find the ref matches. This creates problems because we might want to share a render function across two things that might have different ref match needs. For example, right now there is a completion bug where match &Foo::A { $0 } will provide suggestions like &Foo::B rather than Foo::B. This isn't wrong, but it certainly isn't ideal. We never caught this bug because the test for this doesn't look at the ref_match field, which is just one of the reasons I still think ref_match should be removed from CompletionItem.

In my next attempt I'll try moving the ref_match responsibility up to the complete_nnn functions and see how that goes.

[JoshMcguigan]

In my next attempt I'll try moving the ref_match responsibility up to the complete_nnn functions and see how that goes.

I implemented this in #8393 but I didn't really like how it turned out for reasons explained in the PR.

Next I'll try just adding some marker in the ref_match for where the reference should go.

[flodiebold]

Note: we still give erroneous completions in other cases:

enum Foo {
    Bar,
}

fn foo(f: Foo) {
    let _: &Foo = Foo::$0;
    // should complete to &Foo::Bar
    // actually completes to Foo::&Bar
}

and also

mod foo {
    pub fn bar() -> u32 { 0 }
}

fn foo(f: Foo) {
    let _: &u32 = foo::$0;
    // should complete to &foo::bar()
    // actually completes to foo::&bar()
}