Skip to content

rustdoc_json: conversion cleanups #142747

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

rustdoc_json: conversion cleanups #142747

wants to merge 5 commits into from
+169 −189

Conversation

nnethercote
Copy link
Contributor

A bunch of clean-to-types conversion cleanups I found while working on perf-related stuff in rustdoc_json.

r? @aDotInTheVoid

@nnethercote
Use sym::asterisk to avoid a Symbol::intern call.
4b9a57c
@nnethercote
Remove some dead code.
d2e913e 
We currently have both `FromClean<clean::Constant> for Constant` and
`FromClean<clean::ConstantKind> for Constant` which are basically
identical, but the former is unused.
@nnethercote
Remove some code.
3f44726 
It's just replicating exactly what is done by `<Vec<GenericParamDef> as
FromClean>::into_json`
@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 Jun 19, 2025
aDotInTheVoid
aDotInTheVoid reviewed Jun 19, 2025
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.

First 3 commits look good.

4th commit broadly makes sense. I especially like loosing all the .as_ref() that was just noise, but needs to remove a few impls on tuples that are less clear than just having a function call for when there's not a 1:1 mapping from clean to json.

I think the last 2 commits make things worse and should be dropped. Item and ItemEnum are special as they're the "top level" of conversion, and there's special knowedge being shared about what things do and don't get converted. Also, with clean::Item going to Option<Item> it's not that sometimes a field is None, in the output, but it won't be present at all, which is different to every use of FromClean.

@aDotInTheVoid aDotInTheVoid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2025
@nnethercote
Use FromClean more.
d5ec4d4 
The `FromClean` trait is used a lot for converting to rustdoc-json
format. But it's not used universally; there are still some ad hoc
functions and methods for converting. This commit fixes this
inconsistency by using `FromClean` more.

The commit also introduces `FromClean` for `Box` and `Option`. This lets
a lot of `as_ref` and `map` calls be removed in favour of simple
`into_json` calls.
@nnethercote
Rename some methods.
dc8c259 
- `convert_static` -> `from_clean_static`
- `from_function` -> `from_clean_function`

To match the pre-existing `from_clean_item` and `FromClean::from_clean`.

I left `JsonRenderer::convert_item` unchanged because it's a bit
different.
@nnethercote nnethercote force-pushed the json-conversion-cleanups branch from 06f558f to dc8c259 Compare June 19, 2025 23:33
@nnethercote
Copy link
Contributor Author

nnethercote commented Jun 19, 2025 •
edited
Loading

Thanks for the fast review. I have dropped the last two commits and removed the FromClean impls using tuples. I also added one small new commit that renames a couple of conversion functions, for better naming consistency. Should be good to go now.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2025
@aDotInTheVoid
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 19, 2025

📌 Commit dc8c259 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 Jun 19, 2025
@nnethercote
Copy link
Contributor Author

nnethercote commented Jun 20, 2025 •
edited
Loading

I'm going to r- this temporarily because I really want #142502 to merge (other things I'm doing depend on it) and this might conflict a tiny bit. I will r+ again once #142502 merges.

@nnethercote
Copy link
Contributor Author

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 20, 2025
@nnethercote
Copy link
Contributor Author

I think the last 2 commits make things worse and should be dropped. Item and ItemEnum are special as they're the "top level" of conversion, and there's special knowedge being shared about what things do and don't get converted. Also, with clean::Item going to Option<Item> it's not that sometimes a field is None, in the output, but it won't be present at all, which is different to every use of FromClean.

Thinking some more: this makes sense for convert_item, which produces an Item. But from_clean_item, which produces an ItemEnum, seems much less special and a better fit for FromClean. What do you think about reinstating the commit that converts from_clean_item?

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

@aDotInTheVoid aDotInTheVoid aDotInTheVoid left review comments

Assignees

@aDotInTheVoid aDotInTheVoid

Labels
A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nnethercote @aDotInTheVoid @bors @rustbot