-
Notifications
You must be signed in to change notification settings - Fork 13.7k
rustdoc-json: discard non-local inherent impls for primitives #128385
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
Conversation
Please add a test (in tests/rustdoc-json).
This comment has been minimized.
This comment has been minimized.
-
This needs a test in
test/rustdoc-json. Using aux build you can add a 2nd crate, and put an impl there. Ideally this test should be added in it's own commit, and then in a 2nd commit you change rustdoc, and the test. This way it's easy to verify that the test checks the change being made, but if your not comfortable doing that I can just verify it manualy. -
This seems to have broken the HTML output. When testing changes earlier in the rustdoc pipeline, use
./x test ./test/rustdoc ./tests/rustdoc-jsonto check both backends. It looks like not having these impls collected breaks collecting methods availible viaDeref -
Out of interest, what does a "hello world" JSON output look like now? How much smaller is it?
@rustbot author
5274509 to
cc0dc8d
Compare
3. Out of interest, what does a "hello world" JSON output look like now? How much smaller is it?
There's still pollution in external_crates & paths, but at least the index field now contains just 1 item - the crate's module
This comment has been minimized.
This comment has been minimized.
but at least the index field now contains just 1 item - the crate's module
Amazing! What sort of size-reduction does that give?
Also: This might be another way to test: //@ count "$.index[*]" 1 for an empty module.
cc0dc8d to
f138bbb
Compare
Amazing! What sort of size-reduction does that give?
The size of docs for 1 file containing just fn main() {} went down from 320Kb to 192Kb, i.e. every JSON docs file except for the docs of core will now be 128Kb lighter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the changes to formats/cache.rs are good, and I'd be happy to approve them, mixing cleanup and semantic changes makes it much more difficult to review (as I have to piece together what behaviour has changes, vs what's just nicer code).
In the future, I'd rather these changes were made in a seperate commit or PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if this could link back to the original issue, and also add a comment of what it's testing. At the moment, the purpose of this test won't be clear when viewed without context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also renamed the test to the_smallest.rs, but I'm not sure if that's the most fitting name
src/librustdoc/formats/mod.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make this change? All the call-sites seem to have a Context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I tried to use this function in the JsonRenderer, so I had to make it output-format-agnostic, but after I changed the approach to not require that function, I realised keeping it that way might be beneficial in the future: something that's not defined specifically for HTML output shouldn't rely on data structures defined for generating HTML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you revert this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you accept such a change in a separate PR? possibly expanded if I find other cases of output-format-agnostic functionality relying on a specific output format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. As long as this method's only used in the HTML backend, I (personally) think it's fine to rely on HTML-specific helpers.
68af4a3 to
7ec2e5b
Compare
@rustbot review
@rustbot author
7ec2e5b to
acaa817
Compare
acaa817 to
7499e21
Compare
@bors r+
...iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#128385 (rustdoc-json: discard non-local inherent impls for primitives) - rust-lang#128559 (Don't re-elaborated already elaborated caller bounds in method probe) - rust-lang#128631 (handle crates when they are not specified for std docs) - rust-lang#128664 (Add `Debug` impls to API types in `rustc_codegen_ssa`) - rust-lang#128686 (fix the invalid argument type) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#128385 - its-the-shrimp:fix_114039, r=aDotInTheVoid rustdoc-json: discard non-local inherent impls for primitives Fixes rust-lang#114039 at least it should r? `@aDotInTheVoid`
Fixes #114039
at least it should
r? @aDotInTheVoid