Stop using higher-order macros to declare arenas

[Zalathar]

The arenas used by rustc_hir and rustc_middle are declared via macros. But instead of invoking rustc_arena::declare_arena! directly, those crates declare a higher-order macro containing a list of types, and then invoke that macro with declare_arena! as an argument.

From what I can tell, the only reason for this arrangement is so that the list of types can also contain [decode] modifiers that produce a corresponding implementation of RefDecodable that decodes into the arena. But the number of types involved is small enough that it's easy to list them separately, and the advantage of doing so is that we can remove a layer of macro indirection, making it easier to navigate to the real code.

There should be no change to compiler behaviour.

[rustbot]

r? @camelid

rustbot has assigned @camelid.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 74 candidates
• Random selection from 16 candidates

[Zalathar]

This will have textual conflicts with #159893.

[mejrs]

You should be able to get around the indirection and still keep the [decode] syntax like this

#[macro_export]
macro_rules! eat {
    (decode) => {};
}

#[rustc_macro_transparency = "semiopaque"]
pub macro declare_arena([$([$($a:ident)?] $name:ident: $ty:ty,)*]) {
    $(
        $(
            $crate::eat!($a); // or ${ignore($a)}
            impl<'tcx, D: TyDecoder<'tcx>> RefDecodable<'tcx, D> for $ty {
                #[inline]
                fn decode(decoder: &mut D) -> &'tcx Self {
                    todo!()
                }
            }

            impl<'tcx, D: TyDecoder<'tcx>> rRefDecodable<'tcx, D> for [$ty] {
                #[inline]
                fn decode(decoder: &mut D) -> &'tcx Self {
                    todo!()
                }
            }
        )?
    )*
   // stuff
}

That said, I haven't yet looked in-depth at how comfy either design would be if someone down the line has to add a[decode] in the rustc_hir arena.

Also another thought; maybe we should make the syntax a bit more rust-y?

rustc_arena::declare_arena! {
    pub struct Arena<'tcx> {
        asm_template: rustc_ast::InlineAsmTemplatePiece,
        #[decode]
        attribute: rustc_hir::Attribute,
        macro_def: rustc_ast::MacroDef,
    }
}

(then it can also be an attribute macro, with macro_attr)

[Zalathar]

You should be able to get around the indirection and still keep the [decode] syntax like this
...
That said, I haven't yet looked in-depth at how comfy either design would be if someone down the line has to add a [decode] in the rustc_hir arena.

I think the value of being able to write [decode] inline is pretty low. We already need a separate list of types for implementing RefDecodable via arena on Copy types (since they don't need to be listed in the arena), and the number of needs-drop types that end up needing to be written out twice is very small.

I also have a strong preference for avoiding complex macros whenever reasonably possible. In this case, I think anything beyond a very simple macro is a high price to pay, and the value of [decode] doesn't justify that price.

Also another thought; maybe we should make the syntax a bit more rust-y?

IMO it's a good thing that the declare_arena! syntax doesn't resemble “real Rust” too closely.

E.g. if I saw something like that in some unfamiliar code, I would end up being pretty annoyed that the code was “lying” to me by resembling real code that is meaningfully different from the actual expansion.

[mejrs]

r? me

IMO it's a good thing that the declare_arena! syntax doesn't resemble “real Rust” too closely.

Fair enough. 👍

View changes since this review

[mejrs]

Can you update these docs as well? Including a pointer towards impl_ref_decodable_into_arena!?

[Zalathar]

I'm a bit confused by this request, because I don't understand what should be added.

To me, impl_ref_decodable_into_arena! is an implementation detail of rustc_middle that isn't directly relevant to the arena itself, so I can't think of what would be worth documenting here.

[mejrs]

I'm a bit confused by this request, because I don't understand what should be added.

The entire "cases of interest" paragraph can probably be removed?

I'd guess that when these docs were written there were more arguments than decode, but this pr removes the ability to pass arguments altogether. The documentation still talks about "passing arguments".

To me, impl_ref_decodable_into_arena! is an implementation detail of rustc_middle that isn't directly relevant to the arena itself,

It's relevant to people making changes to what is or isn't allocated in arena. Currently if you add something to arena and get compile errors about RefDecodable not being implemented, these docs will send you on a goose chase.

[Zalathar]

Hmm, one thing I notice is that my new comments about copy vs needs-drop types are not quite accurate, and I need to adjust them to be more in line with what this existing comment is saying.

[Zalathar]

I'd guess that when these docs were written there were more arguments than decode, but this pr removes the ability to pass arguments altogether. The documentation still talks about "passing arguments".

Ah, I think you're misunderstanding “arguments” here. The docs aren't referring to the things inside []; they're referring to the list of [] field_name: Type, entries, which are the arguments to the macro.

It's relevant to people making changes to what is or isn't allocated in arena. Currently if you add something to arena and get compile errors about RefDecodable not being implemented, these docs will send you on a goose chase.

I don't understand this scenario. If someone adds a new [] field_name: Type, entry to the declaration, that by itself isn't going to start causing RefDecodable errors.

[mejrs]

that by itself isn't going to start causing RefDecodable errors.

Of course not, but it might if they then try to actually allocate things in the arena.

The docs aren't referring to the things inside []; they're referring to the list of [] field_name: Type, entries, which are the arguments to the macro.

Then explain it to me as if I'm fresh and I'm trying to put things on arena. I look at the macro syntax and how it's used by other arena types. I figure I need to add [] name: type, which is a bit weird but whatever and it happens to work so I don't care.

Then I look at the docs on declare_arena, and my type isn't Copy which according to the docs means something "must be specified in the arguments".

Please explain to me what I need to additionally specify and where.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Zalathar]

I overhauled some comments, and also dropped the [] to see how that looks (diff).