Expand derive invocations in left-to-right order #84023
Conversation
|
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
compiler/rustc_expand/src/expand.rs
Outdated
| let (fragment, collected_invocations) = | ||
| self.collect_invocations(fragment, &derive_placeholders); | ||
| // Any derive invocations associated with this macro invocation should be | ||
| // expanded *before* any macro invocations collected from the output fragment |
There was a problem hiding this comment.
I don't understand this.
The order of entries in the top level invocations vector is non-significant, it's a job queue - entries can return to it if they are not ready to be resolved or expanded so the exact expansion order is unspecified in general.
So this PR changes one unpredictable order to another?
Once #[derive(Trait1, Trait2)] ITEM produced ITEM, placeholder(Trait1) and placeholder(Trait2) these three items part their ways from each other and can be expanded independently in any order (you don't need any specific order to expand them correctly).
The existing code chooses the tentative order ITEM -> placeholder(Trait2) -> placeholder(Trait1), but it may change later if invocations in ITEM are not yet ready.
The new code chooses the tentative order placeholder(Trait1) -> placeholder(Trait2) -> ITEM (which may also change, but only inside ITEM because the two placeholders here are always ready by construction).
Am I missing anything?
There was a problem hiding this comment.
My reasoning was that we should be treating #[derive] like a normal macro as much as possible. While the special delayed-resolution behavior of multiple #[derive] attributes (compared to normal attributes) means that we can't guarantee this, I think it makes sense to expand derives left-to-right when possible.
By doing so, we'll make the order of messages printed by derive macros more comprehensible in some cases. Even though the order is unspecified, I don't think we should intentionally pick an order (when everything can be resolved immediately) that's inconsistent with how users normally expect attribute macros to work.
There was a problem hiding this comment.
You can see the "FIXME: Consider using the derive resolutions ..." near the changed code.
I've just realized that if it's fully implemented, then we'll stop enqueuing derives entirely, just expand them immediately in place (we have all the data necessary to do that).
In that case it'll indeed match the new order implemented in this PR.
I also agree the new order is more intuitive even if it's not specified, so it seems fine to change it.
There was a problem hiding this comment.
Could you change the "should be expanded before" comment to something less prescriptive? (The new order is a choice, not necessity.)
There was a problem hiding this comment.
#82907 was merged so addressing the FIXME is actually pretty close to the top of my work queue.
petrochenkov
added
S-waiting-on-author
|
r=me with the comment tweaked (#84023 (comment)). |
While derives were being collected in left-to-order order, the corresponding `Invocation`s were being pushed in the wrong order.
Aaron1011
force-pushed
the
derive-invoc-order
branch
from
30e5df9 to
21e6cc1
Compare
|
@bors r=petrochenkov |
|
📌 Commit 21e6cc1 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
While derives were being collected in left-to-order order, the
corresponding
Invocations were being pushed in the wrong order.