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.

Sign up for GitHub

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

Jump to bottom

[rustdoc][JSON] Use exclusively externally tagged enums in the JSON representation #111427

Merged
merged 2 commits into from
May 23, 2023
Merged

[rustdoc][JSON] Use exclusively externally tagged enums in the JSON representation #111427

merged 2 commits into from
May 23, 2023

Conversation

LukeMathWalker
Copy link
Contributor

See this Zulip thread and this issue for the relevant context.

@rustbot
Copy link
Collaborator

rustbot commented May 10, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 10, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

Looks good to me. cc @aDotInTheVoid

@aDotInTheVoid
Copy link
Member

This also needs to bump FORMAT_VERSION, as it changes the serialized representation of JSON (aside: why didn't rustbot fire on this)

@LukeMathWalker
Copy link
Contributor Author

Milestone achieved: more than half of the test suite is green now, but many more to go 😭

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@LukeMathWalker LukeMathWalker marked this pull request as ready for review May 22, 2023 12:39
@rustbot
Copy link
Collaborator

rustbot commented May 22, 2023

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

obi1kenobi
obi1kenobi approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥🚀 Thanks, Luca!

aDotInTheVoid
aDotInTheVoid requested changes May 22, 2023
View reviewed changes
Copy link
Member

@aDotInTheVoid aDotInTheVoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a heroic effort, thanks so much for doing it.

In addition to the minor things above, could you rebase this to follow the no merge policy, and to squash all the test changes into a single commit

tests/rustdoc-json/enums/use_variant_foreign.rs Outdated Show resolved Hide resolved
tests/rustdoc-json/reexport/glob_collision.rs Outdated Show resolved Hide resolved
tests/rustdoc-json/reexport/rename_private.rs Show resolved Hide resolved
@aDotInTheVoid
Copy link
Member

r? @aDotInTheVoid

@aDotInTheVoid
Copy link
Member

Also, it's worth adding bincode or postcard as a dev-dep to rustdoc-json-types, so we can test that the format is (de)serializable with non-self-describing formats, even if we don't plan to ship them in rustdoc itself.

@LukeMathWalker
Serialize all enums as externally tagged to guarantee compatibility w…
a5e5101 
…ith binary formats such as bincode or postcard
@rustbot
Copy link
Collaborator

rustbot commented May 22, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@LukeMathWalker
Copy link
Contributor Author

I've added bincode to the existing roundtrip tests—let me know if that suffices.
All commits have been squashed 👌🏻

@aDotInTheVoid
Copy link
Member

r=me when CI passes

@aDotInTheVoid
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 22, 2023

📌 Commit cd7688b has been approved by aDotInTheVoid

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2023
@obi1kenobi
Copy link
Member

Thank you both for getting this in so quickly! 🙌

@aDotInTheVoid
Copy link
Member

Don't celebrate till it gets through bors, you'll jinx it 😅

@GuillaumeGomez
Copy link
Member

Just passing through to say: nice job everyone! :)

@matthiaskrgr matthiaskrgr mentioned this pull request May 23, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2023
@bors
Auto merge of rust-lang#111882 - matthiaskrgr:rollup-1xyv5mq, r=matth…
d569987 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111427 ([rustdoc][JSON] Use exclusively externally tagged enums in the JSON representation)
 - rust-lang#111486 (Pretty-print inherent projections correctly)
 - rust-lang#111722 (Document stack-protector option)
 - rust-lang#111761 (fix(resolve): not defined `extern crate shadow_name`)
 - rust-lang#111845 (Update books)
 - rust-lang#111851 (CFI: Fix encode_region: unexpected ReEarlyBound(0, 'a))
 - rust-lang#111871 (Migrate GUI colors test to original CSS color format)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d1e9910 into rust-lang:master May 23, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 23, 2023
@aDotInTheVoid aDotInTheVoid mentioned this pull request May 23, 2023
Closed
aDotInTheVoid added a commit to aDotInTheVoid/rustdoc-types that referenced this pull request May 23, 2023
@aDotInTheVoid
v0.22.0
a4e6911 
rust-lang/rust#111427
@Enselic Enselic mentioned this pull request May 24, 2023
Closed
rust-cloud-vms bot pushed a commit to aDotInTheVoid/rust that referenced this pull request Nov 9, 2023
@aDotInTheVoid
rustdoc-json: Fix test so it actuall checks things
22e1576 
After rust-lang#111427, no item has a `kind` field, so these assertions could never
fail. Instead, assert that those two items arn't present.
@aDotInTheVoid aDotInTheVoid mentioned this pull request Nov 9, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 10, 2023
@matthiaskrgr
Rollup merge of rust-lang#117751 - aDotInTheVoid:unkind, r=GuillaumeG…
186a3c8 
…omez

rustdoc-json: Fix test so it actually checks things

After rust-lang#111427, no item has a `kind` field, so these assertions could never fail. Instead, assert that those two items arn't present.

r? `@GuillaumeGomez`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2023
@rust-timer
Unrolled build for rust-lang#117751
c5e931c 
Rollup merge of rust-lang#117751 - aDotInTheVoid:unkind, r=GuillaumeGomez

rustdoc-json: Fix test so it actually checks things

After rust-lang#111427, no item has a `kind` field, so these assertions could never fail. Instead, assert that those two items arn't present.

r? `@GuillaumeGomez`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aDotInTheVoid aDotInTheVoid aDotInTheVoid requested changes

@obi1kenobi obi1kenobi obi1kenobi approved these changes

Assignees

@aDotInTheVoid aDotInTheVoid

Labels
A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.71.0
Development

Successfully merging this pull request may close these issues.
7 participants
@LukeMathWalker @rustbot @rust-log-analyzer @GuillaumeGomez @aDotInTheVoid @bors @obi1kenobi