Skip building subtrees for builtin derives

[Veykril]

This is a waste of resources, we go from node to subtree just to go from subtree to node in the expander impl. We can skip the subtree building and only build the tokenmap instead.

[lowr]

I'm not sure about this. When we implement cfg stripping for fields/variants (#8434), the expanders will need a way to ignore some of them after this PR. I guess we can do something like "if TokenMap doesn't have an entry for this token, this field/variant must have been censored so we ignore them" in parse_adt(), but that sounds like outside of expanders' responsibility and I'm not even sure whether that's an assumption we can make.

[Veykril]

Oh that's something we should have a test for at least (cfg usage in a builtin derive).

I guess we can do something like "if TokenMap doesn't have an entry for this token, this field/variant must have been censored so we ignore them" in parse_adt(), but that sounds like outside of expanders' responsibility and I'm not even sure whether that's an assumption we can make.

I think this is an assumption we can make, the tokenmap will exactly contain entries for uncensored tokens in the input (and if that changes, we can always easily revert this change here I think). The current censoring we have for attributes is in general kind of broken and only partially works. I would expect us to have some other form of censoring in the future and if that doesn't work with this approach here we can again just revert this (the diff here is bigger because I cleaned up something unrelated afterwards).

Another part of what drives the change here as well is my token map rewrite tbh, reducing the usages of token trees and the like makes my life a bit simpler there 😅 (though only very slightly tbf)

I just find it very unsightly that we re-parse item subtrees back into ast nodes just to fetch some things from them, given these are builtins there is no reason for us to convert them here again.

[lowr]

That's fair. One thing I'm a bit worried/don't really understand if we can make the assumption is this kind of code, unwrap_or_else() thereof specifically. I guess it shouldn't be really hit and it's "just in case" type of fallback?

reducing the usages of token trees and the like

I just find it very unsightly that we re-parse item subtrees back into ast nodes just to fetch some things from them, given these are builtins there is no reason for us to convert them here again.

Yeah I definitely understand the motivation here. It's really unfortunate that macros operate on tts, which is based on rustc internal and pretty much foreign to us so we gotta go back and forth 😢

[Veykril]

That's fair. One thing I'm a bit worried/don't really understand if we can make the assumption is [this kind of code][u], unwrap_or_else() thereof specifically. I guess it shouldn't be really hit and it's "just in case" type of fallback?

Ye that's just a safety hatch, if this is hit not much should suffer from it in theory, and it should disappear fully after my macro map rewrite (in theory, though I still have to adjust my design unfortunately)

[lowr]

That resolves all of my concerns, thanks! I'm really looking forward to the token map rewrite 🙂

[Veykril]

@bors r+

[bors]

📌 Commit f6c0909 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit f6c0909 with merge ea02f4c...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing ea02f4c to master...

[Veykril]

Turns out I missed something crucial here making this a very bad idea! We now have a dependency edge from parse_macro_expansion to parse, causing us to re-expand all builtin derive macros on every change.