-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
IDEs and proc-macros #11014
Comments
An interesting bit here is that, today, rust-analyzer doesn't actually know how a valid syntax tree looks like. We never encode in a reliable manner which parts of a tree are mandatory, and which are optional. This is something ungrammar potentially can help with, as it is intended to encode valid tree structure. Though, it doesn't encode valid input on the token level. |
I'm not convinced that there's a 'contract' that attribute/derive proc macros can't receive invalid syntax. On the contrary, the fact that rustc passed invalid syntax to these macros for several versions, even if that was unintentional, means that there isn't such a contract. This plus the fact that letting proc macros handle the invalid syntax is the only way to get 100% correct behavior for some proc macros means to me that we should at least have a way to let proc macros opt in to receiving invalid syntax and handling it themselves. (Also, I still think that having a full Rust parser in every proc macro is a bad idea. Whenever Rust introduces some new syntax, these proc macros will break until their parser is updated. This could be fixed by having the compiler provide parsing functions, at the expense of hugely increasing the proc macro API interface. In that case, we could also provide a proper error-resilient parser to the proc macros.) |
If there were a way for macros to express "I am not going edit the token stream, only add around it, and I expect the stream to be normal rust code (as opposed to some arbitrary DSL)", would that case be much easier for rust-analyzer to do good completion for? As I understand it a lot of the tricky cases come from the possibility that the macro may do arbitrary edits, but a lot (most?) macros (both declarative and proc in my experience) just generate extra code to surround existing valid rust code. Either generating extra items after a struct or inserting extra control flow around a block, without actually changing the starting struct or block contents, or only changing it in a very limited way (e.g. it removes |
Macros that do just that already work very well, so such an annotation wouldn't really give us anything. |
As an FYI for everyone reaching this after having their while code (e.g. of a Cloudflare worker) marked as red with |
This comment has been minimized.
This comment has been minimized.
With #11193, it is now possible to specify macros to be replaced by dummy expanders https://rust-analyzer.github.io/thisweek/2022/01/10/changelog-111.html. If desired, can disable different kinds of proc-macros altogether for now as well https://rust-analyzer.github.io/manual.html#configuration as well as specific diagnostics (not for everyone, but this can be a reasonable option for some). |
This comment has been minimized.
This comment has been minimized.
My two cents on the matter:
Some context: I'm chiming in because I've been seeing, in the Finally, even though I don't have much knowledge of r-a internals, if my proposal sounds legitimate / plausible, I may try to go and implement it, should the lack of person-power to drive it to completion otherwise be a deterrent w.r.t. trying it. |
Sorry but no, allowing proc macros to return results is exactly the wrong direction to take. The "emit the original input" approach is just a workaround, and not a particularly good one. All proc macro attribute do some kind of transformation to their input, which has some relevant semantic effect. The original input is not what the user actually intends to give to the compiler, and using it would be wrong. The only reason that it kind of works in a lot of cases is that we don't have a lot of checks yet that would make it more obvious. For example, take the Something similar goes for the Macros like Rocket's |
There is a syn-mid crate that worth mentioning in this thread. It doesn't parse function bodies and only allow proc macros to include bodies as-is in the result. It's enough for most of attribute macros I'm aware of, and it would solve this problem, plus saving some compile time and more benefits. It doesn't work yet for every usecase that it could. If we want to go with the fixing ecosystem route, making syn-mid perfect and suggesting it to proc macro authors is an option. |
This comment has been minimized.
This comment has been minimized.
|
It seems inherent to the problem that any solution is going to either involve imperfect heuristics (like dtolnays suggestion mentioned in the OP) or macros having annotations analyzer has to trust communicating the macro's intent (like the ticket linked in the OP for macros to expose a grammar). Does anybody think there is a third way? Only other alternative that comes to mind is really still just a heuristic -- empirically track how expanded code changes in response to changes in input, concluding things like, "when this input token changes in practice these output tokens change." In theory RA could call a proc macro many times with many different minor edits to try to understand behavior. But this seems very very hard. There's precedent in the lisp world for macro<->tool communication, e.g. in emacs lisp you can annotate your macro to specify how code passed into the macro should be indented, because even though lisp is homiconic there are different conventions for loops/branches/declarations. |
It looks like IntelliJ Rust (when completing inside an item under an attr macro) always shows completion as a union of completions inside the macro expansion and inside the item like there isn't an attr macro 🤔 |
See
So, granted, I did not think of And FWIW, the way That being said, I agree that But I do stand by the importance of featuring a palliative workaround, sooner rather than later, and regarding that, having the proc-macro server/driver re-emit the input in case of an error is the least bad option out there. |
Sorry to reply to this so late, but this is completely untrue for the current attribute macro implementation. Even the most trivial cases of these, like async-trait or route macros for various web frameworks (which don't explicitly support it, which btw is something async-trait declared it never will), just completely fail to provide any sort of integration as soon as the syntax doesn't parse. Adding an option to just treat the macro's input as the output from rust-analyzer would absolutely solve that, because that's how it worked before (as far as I understood) and it was never an issue for these cases. |
async-trait etc. don't just add around the token stream, they try to parse it and replace it by a compile error if they fail. Also, even disregarding that, async-trait does change the token stream quite a bit -- it has to, to make it actually valid Rust. For why just treating the macro input as its output is not a solution, this has already been discussed above. |
Sorry, I guess what I really want to say is: If I disable attribute macro expansion, I encounter zero issues with any of the attribute macros I'm using. Enabling this (admittedly experimental, but enabled by default) rust-analyzer feature just breaks all of them. |
@flodiebold Wait a second. When I described having a designation for macros that only add to existing code, and you said those already work fine, I assumed you still included macros that have to parse what they were passed in. If it doesn't, then we are miscommunicating. By macros that "only add code" I meant the sort of thing derive macro implementations typically do, where they iterate struct fields and generate one method for each field (e.g. generating getters or reflection methods). Iterating the struct fields still requires using something like |
@jgarvin It doesn't matter whether the macro parses the input or not, what matters is whether it passes through the input even in error cases. If it does, it will work as well as we can make any macro work (i.e. not perfectly, but very well). If it does not at least pass through the input in error cases, I would not interpret that as "not editing the token stream", but that would be quite easy to fix in the macro -- not much harder than adding an annotation. This is the easiest kind of macro to make work, so it would give us almost nothing to add an annotation to handle this case. |
Are there any examples where rust analyzer falling back to assuming tokens are passed through unedited on proc macro failure would make completion worse? Since the intention of many macros is to pass through the original tokens it still seems strictly better to have this heuristic in 1 place (analyzer) than counting on the entire ecosystem to update to an alternative that doesn't exist yet. |
It might not make completion worse, but it will lead to all kinds of other weird effects, especially when we have more diagnostics, as explained above.
This is a false dichotomy. We can implement a workaround that will actually work correctly most of the time, even if it's not the full solution: Fixing up the input nodes that we pass to attribute macros. |
@flodiebold If most users only complain about completion, you could implement the fallback only for completion, without all kinds of other weird effects like false-positive diagnostics. I.e. most of the analysis in RA see just |
11444: feat: Fix up syntax errors in attribute macro inputs to make completion work more often r=flodiebold a=flodiebold This implements the "fix up syntax nodes" workaround mentioned in #11014. It isn't much more than a proof of concept; I have only implemented a few cases, but it already helps quite a bit. Some notes: - I'm not super happy about how much the fixup procedure needs to interact with the syntax node -> token tree conversion code (e.g. needing to share the token map). This could maybe be simplified with some refactoring of that code. - It would maybe be nice to have the fixup procedure reuse or share information with the parser, though I'm not really sure how much that would actually help. Co-authored-by: Florian Diebold <[email protected]>
Is there a way to tell, inside of a proc-macro, when it is being expanded by the r-a host, versus by cargo/rustc during a normal check/build? EDIT: std::env::current_exe().unwrap().file_stem().unwrap() == "rust-analyzer" appears to work. I'm having to special-case this because some of my macros have side-effects that aren't really meant to be run on code that the user knows is incomplete. In general, it does seem useful to be able to somehow distinguish between "this may be incomplete code, don't bother me with too many errors" and "ok, it's ready now, show me some errors". |
@trevyn Can you give some examples of such side-effects and errors? In general I don't think it's feasible to distinguish between those situations, although I guess it would make some sense to distinguish between "live diagnostics in editor" and "user-triggered build". |
Sure, my #[derive(Turbosql, Default)]
struct Person {
rowid: Option<i64>
name: Option<String>
} has the side effect of updating a generated file in the end-user project that describes and utimately creates a SQLite When rust-analyzer proc-macros are enabled, apparently the macro gets called for every keystroke, so if I edit the name of the struct to With rust-analyzer proc-macros disabled, the macro would run on an explicit save/check action only, yielding the original I just updated it with the above snippet so that side-effects are suppressed when the I'm also experimenting with doing a similar thing for a code-defined RPC API in #[turbocharger::backend]
pub async fn get_person(rowid: i64) -> Result<Person, turbosql::Error> {
turbosql::select!(Person "WHERE rowid = ?", rowid)
} which has a similar side effect of collecting function signatures into a generated file for auditing. I think the problematic errors I ran into earlier and wanted to suppress were coming from race conditions that the frequent macro calls were revealing; with the side-effects suppressed things seem much better behaved. |
How accurate does the autocomplete have to be? I think most proc-macros (at least those it would be reasonable to want autocomplete for) could express a mostly superset of their valid inputs (e.g. accepts all valid inputs plus some false negatives) as a tree-sitter grammar that has access to the the rust tree-sitter parser grammar - e.g. being able to say "this part right here would be a rust expression". If someone wrote a crate that took a tree-sitter grammar and used it to emit some basic pre-flight checks on proc macros crates in a standard way, then RA could detect macros that use that crate and load up the proc macros' grammar to perform better auto-complete. TL;DR - if we give a proc-macros semi-standard way (that's friendly to RA and autocomplete) to sort of pre-validate their input, then RA could detect those and give better completion |
@Veykril now that we have the syntax fixup and it seems to actually be working pretty well, should we keep this issue open? |
Yes I think we can close this 👍 |
8986: MACRO: try to fix up rust syntax before calling an attribute proc macro r=dima74 a=vlad20012 Follows rust-lang/rust-analyzer#11014 discussion Co-authored-by: vlad20012 <[email protected]>
@Veykril , please, reopen. I can still reproduce this. Here is my code: #[tokio::main]
async fn main() {
{
} And here is Cargo.toml: [package]
name = "tok"
version = "0.1.0"
edition = "2024"
[dependencies]
tokio = { version = "1.42.0", features = ["full"] }
tracing = { version = "0.1.41", features = ["attributes"] } rustc version is In code above whole code is highlighted in red. But if I comment |
That is #18244 |
This issue is meant as a place to collect information and ideas about the current completion(IDE support rather) dilemma we have with proc-macros.
Current State of Things
When typing inside an attributed item(or proc-macro) the user will inevitably create some form of syntax error at some point which currently causes those proc-macros to just bail out, emitting a
compile_error!
invocation that r-a will happily replace the entire item with. This has the downside that when typing in such an item, all ide features will momentarily stop working resulting in incorrect or straight up missing completions and syntax highlighting flickers(when semantic highlighting is being used).First Attempt at Solving This
Our first idea to solve this was to nudge proc-macro authors towards doing more fallible expansions, meaning instead of only emitting a
compile_error!
on unexpected input, they should instead try to produce output as close as possible to what the macro would expand to in the happy case together with thecompile_error!
.This actually seems to be not as good of an idea as we have first imagined.
It goes against
syn
's ways as the crate is designed to return early on errors, and in fact, as it turns out(I wasn't aware of this fact prior) attribute and derive macros have the invariant/contract1 that they should always receiveTokenStream
s that parse to valid rust syntax.Quoting @dtolnay's comment on the matter1 which raises a lot of good points on this matter:
Possible Solutions
So with this, we have a few options at hand currently:
For attributes and derives(both of which expect rust syntax) this does seem like a bad idea after all after considering the points dtolnay has raised.
For function-like proc macros on the other hand, as there is no rust syntax being passed as well as for macros that error out on semantic problems like expecting certain identifiers, in these cases we should still nudge proc-macro authors to make their macros recoverable.
As there is nothing r-a can do in those cases.
This also has problem that we would define an implicit interface2 for r-a relying on exactly this behaviour for proc-macros, basically running the danger that if we would ever want to change our mind on this we would cause a eco system churn.
compile_error!
keep the original item. Uncertain whether this is feasable in the first place, but it sounds like a bad trade off in general as a lot of macros that change their input item will not reflect their change in this case properly due to them still failing expansion.Still violates the proc-macro contract.
Still violates the proc-macro contract.
Fixing up the input nodes seems like the best approach to me personally, to reiterate that would mean:
Now we aren't the only IDE for rust, IntelliJ seems to currently hardcode a bunch of popular attributes to special case(ignore) them3. So this obviously also weighs into the decision and we should agree on what to do here so that people do not have to write specific adjustments for each IDE(that would be a truly nightmare-ish scenario). cc @vlad20012
Assuming we go with the approach of fixing up/snipping out syntax invalid syntax nodes. The question remains how to do this reliably in a way that enables completion to work as good, if not better than with proc-macros trying to recover from everything as our initial plan was. This is the main question we would have to resolve here next.
On a side-note not relevant to fixing up syntax nodes, regarding completions with proc-macros, there is potentially interesting trick we could try to make use of to get proc-macros to help out IDEs with guiding completions outlined here4. This is not relevant to fixing up syntax nodes though.
Footnotes
https://github.com/rust-analyzer/rust-analyzer/issues/10468#issuecomment-975480478 ↩ ↩2
https://github.com/rust-analyzer/rust-analyzer/issues/10468#issuecomment-975315065 ↩
https://github.com/rust-analyzer/rust-analyzer/issues/10468#issuecomment-975436728 ↩
https://github.com/rust-analyzer/rust-analyzer/issues/7402#issuecomment-770196608 ↩
The text was updated successfully, but these errors were encountered: