#[macro] impl Trait for Struct misses autocompletion for fn's

[tcanabrava]

steps to reproduce:

Download the demo project for godot-rust.

https://github.com/godot-rust/demo-projects.git

Open the rust lib on a editor that has rust-analyzer enabled.
Open any rust file, find code that starts with:

#[godot_api]
impl IArea2D for SomeStruct { 
}

Invoke the completion list inside of the impl block.

Vscode (Rust Analyzer):

Rust Rover (Non Rust Analyzer):

See that the completion list for Rust Analyzer is a Generic list (it has even println! and assert!, while the list provided by RustRover are the functions of the trait.

[ShoyuVanilla]

We have similar issues like #18497 and I think maybe we should do something about them, like supporting rust-analyzer attribute to proc macro developers that regard them as if they were "plain item without proc macro" when completing 🤔

[tcanabrava]

with the importance of games, for rust, I believe that not having RA working on impls like this will be a bummer. could look to implement this with a bit of handholding, not really afraid of hard code. but I have never touched RA in my life. :)

[ShoyuVanilla]

Any contributions or helps would be appreciated but we should track down the real blockers and do some design discussions before doing actual works.

(proc) macro things are quite hard ones to correctly handle in IDEs and you may check that with this video or relatively recent zulip discussion.

I cannot really give you helpful guidelines because I'm quite ignorant on macro handlings yet 😅
Only thing I could say is that it would be helpful to look into other proc-macro labeled issues to follow previous contexts and ask on zulip for detailed discussions

[ChayimFriedman2]

There is a little r-a bug here, but even if it will be fixed this will still not be expanded, and the failure can be attributed mainly to the macro.

The r-a bug (in short) is that we attach the trivia to the last function, and so in the actual expansion we're inside it while in the speculative expansion we're directly under the impl, causing a mismatch between them. I can give more info if someone is interested in fixing it, and if not it'll be fixed when we switch to Salsa's speculative execution.

But the macro does not lend itself towards having great IDE support. When it fails to parse its input, it panics (try to insert raCompletionIdentifier in the impl after the last function and "Expand macro recursively"). This is the worst thing a macro can do. It should return a compile_error!(), and importantly include its complete input in the output.

While perhaps r-a could do something here, it's very hard. The best thing to do is to fix the macro itself.

[flodiebold]

But the macro does not lend itself towards having great IDE support. When it fails to parse its input, it panics (try to insert raCompletionIdentifier in the impl after the last function and "Expand macro recursively").

Hmm, an attribute macro shouldn't need to do that, we shouldn't be passing invalid syntax to it. There may be something missing in our syntax fixup if this is the case.

[tcanabrava]

so, for my particular usecase:

I don't really care that the macro is broken (in the sense it's foreign API, I need to work with it. of course I can send a patch for godot-rust fixing the macro (if I know what to fix), but that won't be a proper fix since it other macros (specially targeting specific FFI-API's) could appear in the future.

since rustrover works flawessly for this, I know it's possible - of course I don't know what to do as I have never touched a complex rust project before. (for those who don't know me, I'm a KDE/C++ dev, trying to learn rust.) - my biggest patch for rust was for axum (the vpath! macro)

and as I said, I'm ok implementing this - with a bit of handholding untill I feel confortable with the source.

[ChayimFriedman2]

Hmm, an attribute macro shouldn't need to do that, we shouldn't be passing invalid syntax to it. There may be something missing in our syntax fixup if this is the case.

Our fixup infra is very incomplete.

[ChayimFriedman2]

BTW, I noticed that we emit an (experimental) error for the macro in player.rs "this type alias takes 1 lifetime argument but 0 lifetime arguments were supplied". I will try to investigate that.

[flodiebold]

Hmm, an attribute macro shouldn't need to do that, we shouldn't be passing invalid syntax to it. There may be something missing in our syntax fixup if this is the case.

Our fixup infra is very incomplete.

True. But that's where this needs to be fixed, the macro is not the problem.

@tcanabrava if you want to improve this, the place to look at would be the syntax fixup infrastructure in fixup.rs. It's responsible for taking invalid syntax, and turning it into valid syntax in a way that's reversible ideally even if a macro has transformed it a bit. A test case like

impl Foo {
    bar
}

might be a good place to start. I think we currently do nothing outside of expressions. For a case like this, I think we can't just remove the token (because we wouldn't know how to add it back), so maybe the best approach is to replace it by fn bar {} for example. Or maybe an easier way would be to replace the whole { bar } by just { }.