Set RUST_IDE_PROC_MACRO_COMPLETION and RUST_IDE_PROC_MACRO_COMPLETION_DUMMY_IDENTIFIER for proc macros

[lnicola]

CC intellij-rust/intellij-rust#9711, yewstack/yew#2972

[Veykril]

The way this is done I think will break for r-a due to how our completions work 😕 We expand proc-macros twice, once properly and once speculatively(this is where we would set the env vars), but the infra falls down if the expansions start to differ in structure too much I believe. We also would need to re-expand the non-speculative one for completions which seems not necessarily ideal.

[flodiebold]

Yeah, gating error resilience behind a special flag is the opposite of helpful :/ The dummy identifier thing we can do though.

[vlad20012]

Btw in IntelliJ Rust we also expand proc-macros twice, but use for completion purposes only speculatively run (with dummy identifier inserted). I think we could use both as well, but I'd anyway set environment variables only during the speculative run.

[Veykril]

The problem is we can't use the speculative one for semantic analysis due to how salsa works, thats where the original expansion comes in. And therefor if the two expansions differ too much we run into problems (which tbf already happens in some scenarios where in the original expansion an identifier is missing)

[vlad20012]

And therefor if the two expansions differ too much we run into problems (which tbf already happens in some scenarios where in the original expansion an identifier is missing)

Could you explain this a little more? I think this is absolutely not a problem for IntelliJ Rust, in out case they can differ in any way.

[flodiebold]

I don't understand the notion that error resilience is only necessary for completion anyway. We also want e.g. go to definition to work in an incomplete macro.

[vlad20012]

The main idea behind it is custom completion, not error resilience. For example, standard tag name completion in yew (div, span, etc), or attribute names completion in yew (<div st/*caret*/>). I only did the basic PoC in yew which looks like error resilience at first glance, but in general the idea is a custom completion

[vlad20012]

Also,

1. changing the macro behavior explicitly for IDE purposes should be more psychologically comfortable for proc macro authors
2. returning the expansion in addition to compile_error!() goes against usual architecture of syn-based proc macros and leads to lot of code changes; hence this is another source of discomfort and doubt for proc macro authors, I guess

[flodiebold]

We're all for supporting custom completions, but that only requires RUST_IDE_PROC_MACRO_COMPLETION_DUMMY_IDENTIFIER.

changing the macro behavior explicitly for IDE purposes should be more psychologically comfortable for proc macro authors

I don't know if that's true. But "for IDE" and "for completions" are still different; I can somewhat see the point of letting the proc macro know it's running in an IDE, but then we would set the variable all the time and not just for completions. Which might be what ends up happening if enough macros do gate error resilience on the variable.

returning the expansion in addition to compile_error!() goes against usual architecture of syn-based proc macros and leads to lot of code changes; hence this is another source of discomfort and doubt for proc macro authors, I guess

I don't know how the RUST_IDE_PROC_MACRO_COMPLETION variable helps with that, you have to implement the error resilience either way. I think the usual architecture of syn-based proc macros is just bad for IDEs, and there's not much we can do about it (for function-based proc macros).

[Veykril]

returning the expansion in addition to compile_error!() goes against usual architecture of syn-based proc macros and leads to lot of code changes; hence this is another source of discomfort and doubt for proc macro authors, I guess

imo this is a design flaw of syn it works great for the compiler, but not for IDEs even if the IDE fixes up syntax errors in attribute inputs.

changing the macro behavior explicitly for IDE purposes should be more psychologically comfortable for proc macro authors

Here I agree with what flodiebold said, if the IDE is expanding a proc-macro it should unconditionally set this env var, not just when completing. There are more features where I can think of a proc-macro helping out the IDE (like re-mapping spans for input tokens that usually disappear in the expansion, think (non-derive) helper attributes of attributes like salsa uses). I don't think setting this solely for completions is more beneficial than always setting it when the IDE does an expansion opposed to the compiler (ignoring the fact that r-a has trouble with the completion only var or not).

Also just a general aside, I like that you are exploring ways to make proc-macros help out IDEs :) This has been something I've been thinking about for a while as well, but never tackled since r-a's token map needs a rewrite before we can do any of the ideas properly I had so far in mind.

[vlad20012]

We're all for supporting custom completions

Also just a general aside, I like that you are exploring ways to make proc-macros help out IDEs :)

Glad to hear! Thank you, guys!
My current ideas on this are very... blurry. I look at this as some kind of experiment and am always ready to change the approach if we find a better idea. And of course, I do not believe proc macro authors will start doing something special for IntelliJ Rust if RA does not support it 😅

but that only requires RUST_IDE_PROC_MACRO_COMPLETION_DUMMY_IDENTIFIER

Well, strictly speaking, just the presence of RUST_IDE_PROC_MACRO_COMPLETION_DUMMY_IDENTIFIER is equivalent to RUST_IDE_PROC_MACRO_COMPLETION=1

returning the expansion in addition to compile_error!() goes against usual architecture of syn-based proc macros and leads to lot of code changes; hence this is another source of discomfort and doubt for proc macro authors, I guess

I don't know how the RUST_IDE_PROC_MACRO_COMPLETION variable helps with that, you have to implement the error resilience either way.

You can compare my approach in yew (yewstack/yew#2972) and your approach in tokio (tokio-rs/tokio#4162). The difference is that during completion you shouldn't care about returning compile_error!() at all. You can just skip strict checking of the input and do best-effort parsing&transcribing (because the expansion will anyway used only for completion but not for error checking or any other kind of analysis). This usually leads to much fewer changes in a macro code compared to returning both best-effort expansion and compile_error!() (please take a closer look at yewstack/yew#2972). Yes, this sounds less powerful than returning compile_error!() with best-effort expansion all the time, but I think this approach is easier to use and therefore more likely to spread throughout the ecosystem.

I can somewhat see the point of letting the proc macro know it's running in an IDE, but then we would set the variable all the time and not just for completions.

To be honest, we set such a variable since the very beginning, but its name is not very generic 🥶.
And again, I think expanding macros to best-effort expansion coupled with compile_error!() is much more difficult for authors of syn-based proc macros, so I don't believe this approach will work. I'm also concerned that users may be misled if macros are expanded differently (at least, a user can see an expansion of a macro in an IDE). This is not an issue for completion.

At least we could just start with completion, it looks like the least controversial place.

[flodiebold]

Well, strictly speaking, just the presence of RUST_IDE_PROC_MACRO_COMPLETION_DUMMY_IDENTIFIER is equivalent to RUST_IDE_PROC_MACRO_COMPLETION=1

As mentioned, RA mainly uses the results from expanding the macro as written without the dummy identifier for analysis during completion (scope, types etc); the dummy identifier is only used to get the syntactic context. We can't currently do semantic analysis on a "fake" (speculative) file where the dummy identifier is inserted (because that would involve changing the salsa inputs, potentially blowing up all caches, and then changing them back). Which also means that if there are further macro calls inside the speculative expansion, we can only resolve them if they also exist inside the normal expansion; those are the problems Veykril was referring to when the two expansions differ too much, I think.