Code completion not working in #[tokio::main]

[chanced]

rust-analyzer version: rust-analyzer version: 0.4.1232-standalone (476d043 2022-10-04)
rustc version: rustc 1.64.0 (a55dd71d5 2022-09-19)
relevant settings:

---

I'm not getting code completion in #[tokio::main] but it seems based on old issues that it should work?

Screen.Recording.2022-10-05.at.8.00.43.PM.mov

This is happening in a fresh project with the following Cargo.toml:

[package]
name = "example"
version = "0.1.0"
edition = "2021"

[dependencies]
tokio = { version = "1.21.2", features = ["full"] }

Sorry if I should have resurrected an old issue instead of starting a new one.

[fasterthanlime]

I can reproduce this with 0.3.1229-standalone on rustc 1.64 (and rustc 2022-10-08), even though rust-analyzer is able to expand the main! macro successfully, there's no completions in there, cf. #13373 (comment)

[fasterthanlime]

I've travelled to the past (through VSCode's "Install another version" menu) to confirm completions in tokio::main used to work:

That's with 0.3.1123 - so it broke sometime between 0.3.1123 and 0.3.1162 - will try to narrow it down further.

edit: 0.3.1131 still works.

edit: 0.3.1140 still works.

[fasterthanlime]

I've narrowed it down to:

• 0.3.1140-standalone (7e2b983fd 2022-07-24) (still works), and
• 0.3.1148-standalone (2b472f668 2022-07-31) (doesn't work)

Here's the relevant diff: 7e2b983...2b472f6

I'm going to try and bisect this.

Edit: while bisecting, I encountered "Broken pipe (os error 32)" at dadb832, so I'm going to try and switch to 1.63.0 for the rest of the bisection and hope it still tells us what we want to know.

[Veykril]

From a quick look (inserting a dbg!) it seems that our completion "analysis"(the part figuring out where we are completing things) is returning None (that is it fails to analyse the current position). A lot of things changed in that code path so its most likely a faulty ? placed somewhere

[fasterthanlime]

My bisection pointed to fba6adf, so I must've messed something up, unless doc comments are somehow load-bearing. Starting again... (with 1.63.0 the whole time).

[Veykril]

We are hitting this early return which disables completions

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

Line 421 in 61504c8

return None;

Which is kind of weird, because I don't think any code regarding this has changed there?

[Veykril]

Okay this seems to be a problem with the token mapping again, for some reason we are looking at the offset of the let keyword in this expansion instead of the s in the async block

[fasterthanlime]

New bisect result:

The merge base 977e12a is bad.
This means the bug has been fixed between 977e12a and [7e2b983].

Not sure what's going on with my git bisect usage today lol, it thinks I'm looking for a commit that fixed a bug?

[Veykril]

Ye those commits are not related I'm failry certain, so from what I've checked, the problem is that we map the token we are completing to the let keyword, because the tokio proc-macro re-uses the function body spans for the new output body and we just pick the first token there (not a lot we can do there better really I believe). What surprises me is why this only fails now, this behavior should've been the same even before ...

[fasterthanlime]

• 0.3.1140-standalone (7e2b983fd 2022-07-24) (still works), and

Upon closer inspection, it doesn't work with this version either (at least, with rustc 1.63.0). I'm going to stop posting comments until I can pinpoint it to one commit.

edit: I'm keeping track of my bisect work here https://gist.github.com/fasterthanlime/43142d7bb3bcae4869441852bcee830b

edit: latest bisect points to ebfbb31 (but that's for rustc 1.63.0, it seems that broke at a different place)

git bisect bad
ebfbb314c03cd8e70198eebf5a96ece8a2f79e51 is the first bad commit
commit ebfbb314c03cd8e70198eebf5a96ece8a2f79e51
Author: Jonas Schievink <jonas.schievink@ferrous-systems.com>
Date:   Tue Jul 12 15:13:04 2022 +0200

    Update 1.63 proc macro ABI to match rustc

 .../src/abis/abi_1_63/proc_macro/bridge/client.rs  |   8 --
 .../src/abis/abi_1_63/proc_macro/bridge/handle.rs  |  22 +++-
 .../src/abis/abi_1_63/proc_macro/bridge/mod.rs     | 136 ++++++++++++---------
 .../src/abis/abi_1_63/proc_macro/bridge/rpc.rs     |  53 ++++----
 .../src/abis/abi_1_63/proc_macro/bridge/server.rs  |  22 ++--
 .../src/abis/abi_1_63/proc_macro/mod.rs            |  96 ++++++++++++---
 .../src/abis/abi_1_63/rustc_server.rs              |  87 +++++++------
 7 files changed, 264 insertions(+), 160 deletions(-)

I can confirm that caf23f2 works, but ebfbb31 doesn't.

[Veykril]

I assume proc-macro server changes might've changed span association for tokens for r-a (not sure why or how, but it's suspicious given that the commit there removes randomness so I wouldn't be surprised). Tokio with the following patch works as intended tokio-rs/tokio@f307531 (which makes it skip the span on the initial tokens).

So I wouldn't consider this a regression here (I am honestly baffled it supposedly worked for people before?), as this was just sheer luck that it didn't break so far.

Also note, this only affects completions for the tail expression. If you are trying to complete in non-tail position completions work fine, since those spans aren't being re-used.

[fasterthanlime]

I'm glad you were able to narrow it down so much @Veykril but idk how I feel about a patch to tokio — a fix in rust-analyzer would fix it for all tokio versions published so far, so I'd rather have that (and a regression test) if possible.

I'm not sure how to write such a test right now.

[Veykril]

Well to be honest, I don't think we can fix it in r-a, unless we hardcode for the tokio proc-macro which I will absolutely fight against doing. I am still puzzled how it worked before ebfbb314c03cd8e70198eebf5a96ece8a2f79e51though, so maybe I am missing something obvious here. But from what I know the problem is just the fact that tokio re-uses the span, which is the only element r-a can use to calculate completions for proc-macro expansions.

[fasterthanlime]

unless we hardcode for the tokio proc-macro which I will absolutely fight against doing

Oh I'm right there with you, that would be awful.

You certainly have a much better understanding of the actual completion code than I do, my observations are only based on the fact that you could get completions in caf23f2, but you can't anymore in ebfbb314c03cd8e70198eebf5a96ece8a2f79e511, so my immediate instinct would be to go and diff the two expansions and see what changed, because I think we're in either of those situations:

• the old behavior was correct and span handling was accidentally broken in that commit (in a way that only affects tokio's tokio::main macro, as far as we can tell right now)
• the new behavior is more correct and the only fix is to patch tokio itself

If it can be fixed by somehow returning to the older behavior, that'd mean tokio doesn't need to be patched and that seems like a better outcome. Of course someone needs to put in the work to do that research!

[fasterthanlime]

Note that even with the tokio patch, completion is not behaving as expected. The first half of this video is "without tokio::main", and typing connector.connect highlights the connect method as expected. The second half doesn't narrow down anything after the dot, and when picking a completion, it's inserted after the currently typed method (not shown in the video):

Completion-In-Tokio-Main-1080p.mp4

(I've verified that completion works as expected with rustc 1.63 and caf23f2)

[fasterthanlime]

Oh, completion works in caf23f2 because... expansion fails:

thread 'MacroExpander' panicked at 'assertion failed: `(left != right)`
  left: `0`,
 right: `0`', crates/proc-macro-srv/src/abis/abi_1_63/proc_macro/bridge/handle.rs:22:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
   4: proc_macro_srv::abis::abi_1_63::proc_macro::bridge::handle::InternedStore<T>::new
   5: proc_macro_srv::abis::abi_1_63::proc_macro::bridge::client::HandleStore<S>::new
   6: proc_macro_srv::abis::abi_1_63::proc_macro::bridge::server::run_server
   7: proc_macro_srv::abis::abi_1_63::proc_macro::bridge::server::<impl proc_macro_srv::abis::abi_1_63::proc_macro::bridge::client::Client<(proc_macro_srv::abis::abi_1_63::proc_macro::TokenStream,proc_macro_srv::abis::abi_1_63::proc_macro::TokenStream),proc_macro_srv::abis::abi_1_63::proc_macro::TokenStream>>::run
   8: proc_macro_srv::abis::abi_1_63::Abi::expand
   9: proc_macro_srv::abis::Abi::expand
  10: proc_macro_srv::dylib::Expander::expand
  11: proc_macro_srv::ProcMacroSrv::expand
  12: proc_macro_srv::cli::run
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[fasterthanlime]

More data: the completion failure seems to only happen on the last line of main. With any other line, it's fine:

Another interesting thing: rename also fails on the last line:

But any other line is fine:

[Veykril]

because I think we're in either of those situations:

• the old behavior was correct and span handling was accidentally broken in that commit (in a way that only affects tokio's tokio::main macro, as far as we can tell right now)
• the new behavior is more correct and the only fix is to patch tokio its

The latter is the case, the problem is that tokio is emitting its extra tokens with the span of the tail expression (for diagnostics purposes when a type mismatch happens), this unfortunately trips up r-a because it uses the spans to figure out a lot of things (completions is just one of the things, as you see renaming is also affected and a lot of other things). r-a usually just picks the first token that matches for a span, which in tokio's case will always be a let token as that is the first token we see that has the corresponding span which is why my patch fixes the problem, as the input function body becomes the first tokens instead again. This is basically a shotcoming of spans I suppose, I am unsure if there are ways we can make this smarter than just picking the first or not.

[bjorn3]

Would it be possible to give each token and token tree passed to the proc macro a unique id and remap based on that? If a proc macro passes a part of the input back unchanged it would have the same unique id, and it it changes it, a different id would be assigned to the part that was changed.

[flodiebold]

Or maybe we could just prefer tokens that are the same as the original token if we have multiple choices?

[fasterthanlime]

which is why my patch fixes the problem

I think there's more going on though — as mentioned in #13355 (comment), your patch doesn't quite fix the problem.

[Veykril]

Ye completions still acting funky comes from the fact that we are now mapping the identifier to a semicolon which has me very confused .. god I hate macros

[Veykril]

Okay, so I went digging in completions a bit and found some general problems there that I fixed in #13386, with this, completions work correctly with the tokio patch applied. I still have no idea why the behavior changed though, will do some digging for that next, although I still don't consider that a bug. So I might instead just look into ways of making the token mapping here smarter for completions as that is probably a better use of the time.

[fasterthanlime]

Yay! Fwiw my position has changed since I discovered the only reason it /did/ work was because macro expansion didn't work at all. The tokio patch seems like the way to go now, to me.

[Veykril]

Ye I'll probably PR the patch to tokio anyways since it does simplify things in general.

Or maybe we could just prefer tokens that are the same as the original token if we have multiple choices?

We can most likely do this here.

Would it be possible to give each token and token tree passed to the proc macro a unique id and remap based on that? If a proc macro passes a part of the input back unchanged it would have the same unique id, and it it changes it, a different id would be assigned to the part that was changed.

Isn't this what are basically doing already? Our span is a token id, its just the fact that tokio copies the span and re-uses it for unrelated tokens.

[bjorn3]

its just the fact that tokio copies the span and re-uses it for unrelated tokens.

My suggestion is to tie the token id to the specific token value, so it can't be re-used for unrelated tokens no matter what a proc macro does.

[Veykril]

I guess that would be something for #9403 then. We want both behaviors in a sense, completion really wants to drill down the unique identity of a token (for now, ideally we would inspect all usages of a token in expansions, though maybe only for decl macros), but we also want to see span re-uses for some other features.

[Veykril]

With #13392, completions somewhat work in current tokio as well now, there are a few exceptions where it breaks due to #13388 still.

[Veykril]

I'll close this as its mostly fixed, and the problematic parts are tracked in the linked issue now.

[Veykril]

fwiw I opened a PR for the span patch tokio-rs/tokio#5092 though it would be fully understandable for them to not merge this given its a workaround for a r-a bug

[fasterthanlime]

fwiw I opened a PR for the span patch tokio-rs/tokio#5092 though it would be fully understandable for them to not merge this given its a workaround for a r-a bug

Yeah exactly, one big reason I wanted to make sure this couldn't be fixed on the r-a side is that I think (no matter how useful r-a is) there's limited political capital to spend and get crate maintainers to merge fixes "purely for r-a". From previous experience, I can totally see some of them going "rustc likes my code, that's good enough for me".

[chanced]

Good work ya'll.