• docs(ast, ast_macros, ast_tools): better documentation for Ast helper attributes. #4856 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rzvxa and the rest of your teammates on Graphite

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

CodSpeed Performance Report

Merging #4856 will not alter performance

_{Comparing 08-13-docs_ast_macros_better_documentation_for_ast_helper_attributes (47c9552) with main (90d0b2b)}

Summary

✅ 29 untouched benchmarks

The docs added here to oxc_ast_macros/src/lib.rs are excellent.

Do you think we should retain some comment at top of oxc_ast/src/ast/js.rs etc pointing readers to the docs in oxc_ast_macros?

People may understand that #[scope] attr etc are probably related to the #[ast] attr, and so will look there for docs. But if you try "jump to definition" on generate_derive, it doesn't take you anywhere, and it's not obvious that generate_derive is related to #[ast].

Oh, I didn't notice outer markers aren't pointing to the macro. My initial reasoning behind removing those was to reduce the surface area of our documentation, It is much easier to maintain the docs in one place instead of having to keep multiple places in sync.

I'll bring back the old comments and look for a way so we can have jump to the definition on those attributes as well.

@overlookmotel I've looked into having jump to definition and documentation on each attribute and found this to work:

It works similarly to Rust's built-in macros, If we can have a common crate to put some empty macros in it we should be able to document everything.

/// Hello darkness my old friend!
macro_rules! generate_derive {
    ($($trait:ident)*) => { /* `ast_tools` built-in */ }
}
pub(crate) use generate_derive;

Then in the proc-macro, I do something like this to get the compiler/RA to understand it.

                let (abs_derive, generics) = abs_trait(&derive);
                let generate_derive = // it doesn't have to be one per derive, We just need to do it once!
                    quote::quote_spanned!(attr_ident_span => generate_derive!(derive););
                quote! {{
                    // NOTE: these are wrapped in a scope to avoid the need for unique identifiers.
                    trait AssertionTrait: #abs_derive #generics {}
                    impl<T: #derive #generics> AssertionTrait for T {}
                    #generate_derive
                }}

Do you think it is worth implementing? We can place them in the oxc_ast/src/ast/macros but if we want to use it with other crates we have to rename our crates to this to follow the conventions:

oxc_ast_macros content goes into a fresh crate called oxc_ast_derive
use our current oxc_ast_macros to expose everything including the procedural derives/attributes.

Merge activity

• Aug 15, 7:31 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Aug 15, 7:31 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
• Aug 15, 7:35 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.

Do you think it is worth implementing?

I'm not sure. What do you think? I would definitely say yes if we could do it without adding an extra crate, but if it requires an extra crate, maybe it's not worth the complexity.

I am not really au fait with how proc macro spans interface with Rust Analyser, but is there any other way to achieve this? Like e.g.:

1. In proc macro, set the span of generated_derive to be span of #[ast], so at least jump to definition points you in the right direction.
2. Do the macro_rules! generate_derive trick you suggested, but put that in oxc_ast/src/macros.rs.

Basically, I like this idea, but think introducing an extra crate just for it is maybe overkill. But if you want to go for it, do it!

I'm not sure. What do you think? I would definitely say yes if we could do it without adding an extra crate, but if it requires an extra crate, maybe it's not worth the complexity.

I agree on the fact that adding it might be a bit of overkill. Let's see if there comes any other situation that such a thing can help with.

1. In proc macro, set the span of `generated_derive` to be span of `#[ast]`, so at least jump to definition points you in the right direction.

This one isn't possible due to the compiler consuming the #[ast] tokens before executing the proc-macro on the stream. Evne if it didn't I'm not sure that RA would understand it correctly.

2. Do the `macro_rules! generate_derive` trick you suggested, but put that in `oxc_ast/src/macros.rs`.

That's exactly what I did in my test. It works great but can only be used within oxc_ast, Since oxc_ast depends on both oxc_syntax and oxc_span so it isn't possible to access these, They form a cyclic dependency graph.

I'll try to think of another way to make it work.
I've already tried generating the dummy macro along with assertions for derive traits however RA doesn't respect it as it doesn't exist before expansion.