rustdoc-json: discard non-local inherent impls for primitives #128385

its-the-shrimp · 2024-07-30T11:30:43Z

Fixes #114039
at least it should
r? @aDotInTheVoid

GuillaumeGomez · 2024-07-30T11:44:50Z

Please add a test (in tests/rustdoc-json).

aDotInTheVoid · 2024-07-30T12:17:00Z

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-json to check both backends. It looks like not having these impls collected breaks collecting methods availible via Deref
Out of interest, what does a "hello world" JSON output look like now? How much smaller is it?

@rustbot author

tests/rustdoc-json/primitives/no_primitive_impl_pollution.rs

its-the-shrimp · 2024-07-30T15:46:55Z

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

aDotInTheVoid · 2024-08-01T16:05:17Z

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.

its-the-shrimp · 2024-08-04T17:59:32Z

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

aDotInTheVoid

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.

aDotInTheVoid · 2024-08-04T18:30:56Z

tests/rustdoc-json/primitives/no_primitive_impl_pollution.rs

@@ -0,0 +1,2 @@
+//@ count "$.index[*]" 1


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.

I also renamed the test to the_smallest.rs, but I'm not sure if that's the most fitting name

src/librustdoc/formats/renderer.rs

aDotInTheVoid · 2024-08-04T18:36:07Z

src/librustdoc/formats/mod.rs

@@ -43,8 +44,7 @@ impl Impl {
    // Returns true if this is an implementation on a "local" type, meaning:
    // the type is in the current crate, or the type and the trait are both
    // re-exported by the current crate.
-    pub(crate) fn is_on_local_type(&self, cx: &Context<'_>) -> bool {
-        let cache = cx.cache();
+    pub(crate) fn is_on_local_type(&self, cache: &Cache, tcx: TyCtxt<'_>) -> bool {


Why make this change? All the call-sites seem to have a Context.

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

Could you revert this?

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

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.

src/librustdoc/formats/cache.rs

its-the-shrimp · 2024-08-05T00:04:36Z

@rustbot review

src/librustdoc/json/mod.rs

aDotInTheVoid · 2024-08-05T08:33:24Z

@rustbot author

aDotInTheVoid · 2024-08-05T11:09:02Z

@bors r+

bors · 2024-08-05T11:09:04Z

📌 Commit 7499e21 has been approved by aDotInTheVoid

It is now in the queue for this repository.

…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`

rustbot assigned aDotInTheVoid Jul 30, 2024

rustbot added 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 Jul 30, 2024

This comment has been minimized.

Sign in to view

rustbot 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 Jul 30, 2024

its-the-shrimp force-pushed the fix_114039 branch from 5274509 to cc0dc8d Compare July 30, 2024 15:16

rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Jul 30, 2024

its-the-shrimp commented Jul 30, 2024

View reviewed changes

tests/rustdoc-json/primitives/no_primitive_impl_pollution.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

its-the-shrimp force-pushed the fix_114039 branch from cc0dc8d to f138bbb Compare August 4, 2024 17:37

aDotInTheVoid requested changes Aug 4, 2024

View reviewed changes

its-the-shrimp force-pushed the fix_114039 branch 2 times, most recently from 68af4a3 to 7ec2e5b Compare August 4, 2024 19:29

its-the-shrimp changed the title ~~rustdoc-json: discard non-local inherent impls~~ rustdoc-json: discard non-local inherent impls for primitives Aug 4, 2024

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 Aug 5, 2024

aDotInTheVoid reviewed Aug 5, 2024

View reviewed changes

src/librustdoc/json/mod.rs Outdated Show resolved Hide resolved

rustbot 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 Aug 5, 2024

its-the-shrimp force-pushed the fix_114039 branch from 7ec2e5b to acaa817 Compare August 5, 2024 09:22

rustdoc-json: discard non-local inherent impls

7499e21

its-the-shrimp force-pushed the fix_114039 branch from acaa817 to 7499e21 Compare August 5, 2024 10:19

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 5, 2024

matthiaskrgr mentioned this pull request Aug 5, 2024

Rollup of 5 pull requests #128689

Merged

bors merged commit 376a6f9 into rust-lang:master Aug 5, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 5, 2024

aDotInTheVoid mentioned this pull request Aug 5, 2024

Rustdoc-Json: Large Amount of unreferenced core in index #101201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc-json: discard non-local inherent impls for primitives #128385

rustdoc-json: discard non-local inherent impls for primitives #128385

its-the-shrimp commented Jul 30, 2024

GuillaumeGomez commented Jul 30, 2024

This comment has been minimized.

aDotInTheVoid commented Jul 30, 2024

its-the-shrimp commented Jul 30, 2024

This comment has been minimized.

aDotInTheVoid commented Aug 1, 2024

its-the-shrimp commented Aug 4, 2024 •

edited

Loading

aDotInTheVoid left a comment

aDotInTheVoid Aug 4, 2024

its-the-shrimp Aug 4, 2024

aDotInTheVoid Aug 4, 2024

its-the-shrimp Aug 4, 2024 •

edited

Loading

aDotInTheVoid Aug 5, 2024

its-the-shrimp Aug 5, 2024

aDotInTheVoid Aug 5, 2024

its-the-shrimp commented Aug 5, 2024

aDotInTheVoid commented Aug 5, 2024

aDotInTheVoid commented Aug 5, 2024

bors commented Aug 5, 2024

rustdoc-json: discard non-local inherent impls for primitives #128385

rustdoc-json: discard non-local inherent impls for primitives #128385

Conversation

its-the-shrimp commented Jul 30, 2024

GuillaumeGomez commented Jul 30, 2024

This comment has been minimized.

aDotInTheVoid commented Jul 30, 2024

its-the-shrimp commented Jul 30, 2024

This comment has been minimized.

aDotInTheVoid commented Aug 1, 2024

its-the-shrimp commented Aug 4, 2024 • edited Loading

aDotInTheVoid left a comment

Choose a reason for hiding this comment

aDotInTheVoid Aug 4, 2024

Choose a reason for hiding this comment

its-the-shrimp Aug 4, 2024

Choose a reason for hiding this comment

aDotInTheVoid Aug 4, 2024

Choose a reason for hiding this comment

its-the-shrimp Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

aDotInTheVoid Aug 5, 2024

Choose a reason for hiding this comment

its-the-shrimp Aug 5, 2024

Choose a reason for hiding this comment

aDotInTheVoid Aug 5, 2024

Choose a reason for hiding this comment

its-the-shrimp commented Aug 5, 2024

aDotInTheVoid commented Aug 5, 2024

aDotInTheVoid commented Aug 5, 2024

bors commented Aug 5, 2024

its-the-shrimp commented Aug 4, 2024 •

edited

Loading

its-the-shrimp Aug 4, 2024 •

edited

Loading