Skip to content
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

Load proc macro metadata in the correct order. #64528

Merged
merged 1 commit into from Sep 18, 2019

Conversation

@Aaron1011
Copy link
Contributor

commented Sep 16, 2019

Serialized proc macro metadata is assumed to have a one-to-one
correspondence with the entries in static array generated by proc_macro_harness.
However, we were previously serializing proc macro metadata in a
different order than proc macros were laied out in the static array.
This lead to us associating the wrong data with a proc macro when
generating documentation, causing Rustdoc to generate incorrect docs for
proc macros.

This commit keeps track of the order in which we insert proc macros into
the generated static array. We use this same order when serializing proc
macro metadata, ensuring that we can later associate the metadata for a
proc macro with its entry in the static array.

Fixes #64251

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 16, 2019

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

@petrochenkov petrochenkov self-assigned this Sep 16, 2019
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

I'm not super familiar with this but from my perspective it seems like a sort of odd field to add to the AST, it seems like a better solution might be to ensure that the raw proc macro harnesses are in the same order as the AST, or ensuring that they're encoded in the same order that the harnesses show up (which appears to basically be sorted by type)

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

it seems like a better solution might be to ensure that the raw proc macro harnesses are in the same order as the AST

How is this different from your second suggestion?

or ensuring that they're encoded in the same order that the harnesses show up (which appears to basically be sorted by type)

I considered that - however, it seems very fragile to me. The harness code runs very early on, and depends on the AST order. When we (currently) serialize proc macro data, we rely on the HIR order, which AFAIK is not guaranteed to be equivalent to the AST order in any way.

We could sort the proc macros before generating harnesses, but this would require us to ensure that the sorting stays in sync between the harness code and the metadata serialization. It seems much simpler to just record the actual order used by the harness code, and not worry about what that order ends up being.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

we rely on the HIR order, which AFAIK is not guaranteed to be equivalent to the AST order in any way.

HIR visiting order needs to follow AST visiting order when it matters (e.g. any parameter/argument order, or statement order) and don't have many reasons to deviate from it even when it technically could.
Proc macros is just one more place where the order matters.

I'd change the harness to visit macros in the AST visiting order, without grouping into derives/attributes/etc, that's a pretty easy order to keep in sync.

This ensures that we match the order used by proc macro metadata
serialization.

Fixes #64251
@Aaron1011 Aaron1011 force-pushed the Aaron1011:fix/proc-macro-type branch from 9b4cf01 to 3daa8bd Sep 17, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@petrochenkov @alexcrichton : I've modified the harness code to generate macro definitions in AST order.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

📌 Commit 3daa8bd has been approved by alexcrichton

tmandry added a commit to tmandry/rust that referenced this pull request Sep 18, 2019
…excrichton

Load proc macro metadata in the correct order.

Serialized proc macro metadata is assumed to have a one-to-one
correspondence with the entries in static array generated by proc_macro_harness.
However, we were previously serializing proc macro metadata in a
different order than proc macros were laied out in the static array.
This lead to us associating the wrong data with a proc macro when
generating documentation, causing Rustdoc to generate incorrect docs for
proc macros.

This commit keeps track of the order in which we insert proc macros into
the generated static array. We use this same order when serializing proc
macro metadata, ensuring that we can later associate the metadata for a
proc macro with its entry in the static array.

Fixes rust-lang#64251
bors added a commit that referenced this pull request Sep 18, 2019
Rollup of 4 pull requests

Successful merges:

 - #64486 (Print out more information for `-Zunpretty=expanded,hygiene`)
 - #64516 (update Nomicon and Reference)
 - #64528 (Load proc macro metadata in the correct order.)
 - #64536 (Update Cargo)

Failed merges:

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Sep 18, 2019
…excrichton

Load proc macro metadata in the correct order.

Serialized proc macro metadata is assumed to have a one-to-one
correspondence with the entries in static array generated by proc_macro_harness.
However, we were previously serializing proc macro metadata in a
different order than proc macros were laied out in the static array.
This lead to us associating the wrong data with a proc macro when
generating documentation, causing Rustdoc to generate incorrect docs for
proc macros.

This commit keeps track of the order in which we insert proc macros into
the generated static array. We use this same order when serializing proc
macro metadata, ensuring that we can later associate the metadata for a
proc macro with its entry in the static array.

Fixes rust-lang#64251
tmandry added a commit to tmandry/rust that referenced this pull request Sep 18, 2019
…excrichton

Load proc macro metadata in the correct order.

Serialized proc macro metadata is assumed to have a one-to-one
correspondence with the entries in static array generated by proc_macro_harness.
However, we were previously serializing proc macro metadata in a
different order than proc macros were laied out in the static array.
This lead to us associating the wrong data with a proc macro when
generating documentation, causing Rustdoc to generate incorrect docs for
proc macros.

This commit keeps track of the order in which we insert proc macros into
the generated static array. We use this same order when serializing proc
macro metadata, ensuring that we can later associate the metadata for a
proc macro with its entry in the static array.

Fixes rust-lang#64251
bors added a commit that referenced this pull request Sep 18, 2019
Rollup of 4 pull requests

Successful merges:

 - #64486 (Print out more information for `-Zunpretty=expanded,hygiene`)
 - #64503 (rename Allocation::retag -> with_tags_and_extra)
 - #64516 (update Nomicon and Reference)
 - #64528 (Load proc macro metadata in the correct order.)

Failed merges:

r? @ghost
@bors bors merged commit 3daa8bd into rust-lang:master Sep 18, 2019
4 checks passed
4 checks passed
pr Build #20190917.69 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.