Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autocompletion does not work for function-local variable when there is a macro on a function #10788

Closed
gyzerok opened this issue Nov 17, 2021 · 22 comments
Labels
A-completion autocompletion A-macro macro expansion C-bug Category: bug S-actionable Someone could pick this issue up and work on it right now

Comments

@gyzerok
Copy link

gyzerok commented Nov 17, 2021

Hello everyone! Love the work you are doing for the community!

Setup

VSCode plugin: v0.2.817
Rust: 1.56.1 (59eed8a2a 2021-11-01)
Cargo: 1.56.0 (4ed5d137b 2021-10-04)
Rust Analyzer: 73668334f 2021-11-15 stable

Problem

I can confirm it happening in NeoVim as well, so it's not unique to VSCode setup.

Screen.Recording.2021-11-17.at.18.34.58.mov

Reproduction

Here is the smallest reproducable example I can get:

  1. Do cargo init rust-analyzer-autocomplete-bug
  2. Do cargo add tracing
  3. Add following to your main.rs and try to type test where comment is
  4. Observe things not working
  5. Remove #[tracing::instrument] and try again to type test where comment is
  6. Observe everything working fine
// main.rs
use std::collections::HashSet;

fn main() {
    println!("Hello, world!");
}

struct Test;

impl Test {
    fn test() -> HashSet<String> {
        HashSet::new()
    }
}

#[tracing::instrument]
async fn buggy_autocomplete() {
    let test = Test::test();
    // cannot autocomplete test here
}

I'd be happy to provide any additional details if needed. Really am excited about your project getting better every weak! 🎉

@jonas-schievink jonas-schievink added A-completion autocompletion A-macro macro expansion C-bug Category: bug labels Nov 17, 2021
@gyzerok
Copy link
Author

gyzerok commented Nov 18, 2021

Hello @jonas-schievink! Do you need some extra information for the issue to become actionable?

@gyzerok gyzerok changed the title Autocompletion does not work for local variable Autocompletion does not work for function-local variable when there is a macro on a function Nov 18, 2021
@Veykril Veykril added the S-actionable Someone could pick this issue up and work on it right now label Nov 18, 2021
@ngryman
Copy link

ngryman commented Nov 18, 2021

It seems to only break on full path macro names.

This breaks:

However, this works:

@Veykril
Copy link
Member

Veykril commented Nov 18, 2021

It seems to only break on full path macro names.

That one is actually due to us always resolving #[test] to the builtin test attribute, which is fixed in current nightly/next stable.

So your problem is probably the async_std attributes not being input error tolerant, same probably for the tracing ones(though I think they fixed that so it might be something else there).

@Veykril
Copy link
Member

Veykril commented Nov 18, 2021

Filed async-rs/async-std#998 for the async-std problem.

Regarding the original issue, this is interesting, typing just t gives the me autocompletion for test, but if I type te it disappears.

@ngryman
Copy link

ngryman commented Nov 18, 2021

I don't know if that helps but it happens with async-trait as well.
I'm pretty sure auto-completion was working there in previous versions of rust-analyzer.

@lnicola
Copy link
Member

lnicola commented Nov 18, 2021

All proc macros using syn have this problem, unless they handle it as a special case. See dtolnay/async-trait#178 for async-trait.

@ngryman
Copy link

ngryman commented Nov 18, 2021

Meanwhile, for folks interested in bringing back the auto-completion, unchecking the Proc Attr Macros option made it work for me.

Given the fact that this option breaks auto-completion with what seems to be a good number of macros out there, and it's marked as "experimental", shouldn't it be an opt-in instead of an opt-out?

@lnicola
Copy link
Member

lnicola commented Nov 18, 2021

We should probably remove the "experimental" bit. The problem is that the macro crashes on invalid input, which is what you have while typing code.

@ngryman
Copy link

ngryman commented Nov 18, 2021

@lnicola Thanks for the additional context 👍

In the current state of things, wouldn't that be more beneficial for the average user to disable it by default?

@lnicola
Copy link
Member

lnicola commented Nov 18, 2021

I don't know. Most beneficial for the community at large would probably be for proc macro authors to stop using syn for macros which don't need to look at the function body.

Using syn means that old proc macro crates might not work any more when they encounter new syntax (such as on a new edition). This was a big problem last time, with a lot of projects depending on two versions of syn. It also slows down the compile times by a lot.

In my code I stopped using async-trait exactly for these reasons.

@ngryman
Copy link

ngryman commented Nov 18, 2021

@lnicola Got it, thanks.

It looks like that from a technical standpoint, you're 💯 right, but from a user perspective, it's a different story. My only concern is the time it will take for the community to adapt. Meanwhile, I feel like rust-analyzer will have folks like me reporting what seems to be a regression from their perspective.

That's why I was proposing to swap this option to an opt-in, at least the time the community adapts to what you're suggesting. If you want to keep it that way, that's fine too, as there's an easy workaround. I just wanted to bring an average user perspective to the discussion.

@flodiebold
Copy link
Member

The problem is that not enabling this option leads to other code not being handled correctly. Before the option was enabled by default, we had people reporting those problems instead. It might have been less prevalent, but if we revert the setting, it also means no-one notices the problem.

@lnicola
Copy link
Member

lnicola commented Nov 18, 2021

I get what you're saying, but:

  • for every user having trouble with async-trait, there will be two tokio users complaining that IDE features don't work with #[tokio::main]
  • we enable features by default, for discoverability and to get bug reports when something doesn't work right
  • the other crates we've filed issues against fixed it (by returning the original function body on parse errors); async-trait is the only crate where the maintainer refused
  • we generally want to push the whole ecosystem forward, not to hang back until everyone catches up

@ngryman
Copy link

ngryman commented Nov 18, 2021

That makes sense, I wasn't fully aware of the tradeoff that has been made. Thanks for the explanation!

@gyzerok
Copy link
Author

gyzerok commented Nov 18, 2021

I am getting a feeling that my issue became a discussion for a completely separate issue. Is it so? If yes, can you please open another issue and move your discussion there?

@ngryman
Copy link

ngryman commented Nov 19, 2021

@gyzerok I'm curious what makes you feel that way? Unless I misunderstood something, it looks like to me that I'm facing the exact same issue with the async_trait macro for example.

@Veykril
Copy link
Member

Veykril commented Nov 19, 2021

While it looks like that on first glance, their issue is slightly different.

Regarding the original issue, this is interesting, typing just t gives the me autocompletion for test, but if I type te it disappears.

@norman784
Copy link

norman784 commented Nov 27, 2021

Using syn means that old proc macro crates might not work any more when they encounter new syntax (such as on a new edition). This was a big problem last time, with a lot of projects depending on two versions of syn. It also slows down the compile times by a lot.

@lnicola it would be interesting if there is something I can read about, is there an alternative to syn? I recently started using macros in my crate and I'm now having the issue that autocompletion doesn't work inside my macros, I read your response but I don't fully get how I can accomplish that..

returning the original function body on parse errors

What is your approach when developing macros? in my case I don't care about the function body, but the function signature (visibility, name, generics, arguments, return value, etc, about the function body I don't care).

Edit: Im using proc_macro and proc_macro_attributes

@lnicola
Copy link
Member

lnicola commented Nov 28, 2021

@norman784 https://github.com/matklad/xflags is an example of a proc-macro crate that does not use syn.

in my case I don't care about the function body, but the function signature

That was proposed for syn (and rejected) in dtolnay/syn#1086. The other approach (returning the original tokens) is for example tokio-rs/tokio#4162.

But things here might still change, so we might be able to find a solution that doesn't require such workarounds from proc macro authors.

@norman784
Copy link

Thanks, I will check those out.

@jcowgar
Copy link

jcowgar commented Dec 2, 2021

I am experiencing something similar, but a macro at the front or a Result return at the bottom causes the error. My example is in an actix_web project.

https://asciinema.org/a/P3T2FdAS591VQJYPSQYKgDtTq?t=10

Notice I can leave in the macro, or I can remove the string return and type completion works. With either or both of them in, it does not.

Is this the same error simply manifesting a little differently?

I am using neovim 0.6.0, rust-analyzer 2021-11-22 and nightly 2021-12-02, both exhibit the same problem, in Neovim, Kakoune or Helix.

@Veykril
Copy link
Member

Veykril commented Dec 14, 2021

Closing in favor of #11014

@Veykril Veykril closed this as completed Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-completion autocompletion A-macro macro expansion C-bug Category: bug S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

8 participants