[rustdoc] Don't document stripped items in JSON renderer. by aDotInTheVoid · Pull Request #83055 · rust-lang/rust

aDotInTheVoid · 2021-03-12T15:32:51Z

Fixes#80664, see my comment there for why

Note that we already do something similar in convert_item:

Lines 28 to 31 in bb4cdf8

let inner = match*kind {
 clean::StrippedItem(_) => returnNone,
 x => from_clean_item_kind(x,self.tcx,&name),
};

@rustbot modify labels: +T-rustdoc +A-rustdoc-json

r? @jyn514
cc @CraftSpider

aDotInTheVoid · 2021-03-19T17:59:04Z

This line

rust/src/librustdoc/json/conversions.rs

Line 182 in b97fd3e

StrippedItem(inner) => from_clean_item_kind(*inner, tcx, name),

Should be changed to unreachable I think, as we should never call from_clean_item_kind on a StrippedItem
(or make from_clean_item_kind return an Option)

jyn514

Looks good assuming my comment in #80664 (comment) is accurate that JSON doesn't care about inlining.

jyn514 · 2021-03-22T04:07:30Z

This line
rust/src/librustdoc/json/conversions.rs
Line 182 in b97fd3e
StrippedItem(inner) => from_clean_item_kind(*inner, tcx, name),
Should be changed to unreachable I think, as we should never call from_clean_item_kind on a StrippedItem
(or make from_clean_item_kind return an Option)

Sounds good - could you change that to unreachable! as part of this PR?

jyn514

It looks like you're still making changes, but when you're ready for review - what's going on in bdf6786 ?

aDotInTheVoid · 2021-03-22T19:57:34Z

It looks like you're still making changes, but when you're ready for review - what's going on in bdf6786 ?

It turns out that we also don't need to call item even for no striped modules. And this fixes the Duplication, at least with the current test suite. I'm trying it with tests for #83057 now, and it looks less promizing.

I'll let you know when this is ready for review

aDotInTheVoid · 2021-03-22T22:23:25Z

This PR is a bit of a mess, but now does 3 things

Avoids recursing in run_format, controlled by a const in FormatRenderer. This fixes ICE on rustdoc-json with re-export due to ID collision #80664. Also add a test for that
Adds tests for where the index still has collisions, even with change 1
Improvements to jsondocck to make the first 2 changes possible

The git history is a mess, and I'd be willing to clean it up and/or split this into 3 pr's if you want

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

jyn514 · 2021-03-24T02:38:52Z

It turns out that we also don't need to call item even for non stripped modules. And this fixes the Duplication, at least with the current test suite. I'm trying it with tests for #83057 now, and it looks less promizing.

Can you post what tests fail when you disable this for the HTML backend too? I'd like to keep it in sync with the JSON backend where possible.

jyn514 · 2021-03-24T02:39:47Z

The git history is a mess, and I'd be willing to clean it up and/or split this into 3 pr's if you want

Splitting this into 2 commits - one changing jsondocck, the other fixing the bug - seems fine.

jyn514 · 2021-03-24T02:40:49Z

Adds tests for where the index still has collisions, even with change 1

It wasn't clear to me by reading: which test is this? Where is the collision? Could you add it as a comment?

aDotInTheVoid · 2021-03-24T16:14:02Z

Can you post what tests fail when you disable this for the HTML backend too? I'd like to keep it in sync with the JSON backend where possible.

Like every single one. log. If instead we instrad call for non-striped, but dont for non striped the following tests fail

 [rustdoc] rustdoc/inline_local/glob-extern.rs
[rustdoc] rustdoc/inline_local/glob-private.rs
[rustdoc] rustdoc/redirect-const.rs
[rustdoc] rustdoc/redirect-rename.rs
[rustdoc] rustdoc/redirect.rs

(accurate 2 weeks ago, I can rerun now if it realy matters) log

It wasn't clear to me by reading: which test is this? Where is the collision? Could you add it as a comment?

Its the in_root_and_mod ones. Theirs a comment by the assert, because the tests will still be good, even once this is gone.

Splitting this into 2 commits - one changing jsondocck, the other fixing the bug - seems fine.

Done and force pushed

@jyn514 this is now waiting on review, you forgot to set it to waiting on author.

aDotInTheVoid · 2021-03-24T16:39:13Z

Welp, I guess not. ~~Cant reproduce the error (./x.py test src/test/rustdoc-json/reexport/in_root_and_mod_pub.rs) passes~~

@rustbot modify labels: -S-waiting-on-review +S-waiting-on-author

Closesrust-lang#80664

aDotInTheVoid · 2021-03-24T19:31:13Z

Rebased onto master, can reproduce

Caused by rust-lang@b0659f9#diff-ede26372490522288745c5b3df2b6b2a1cc913dcd09b29af3a49935afe00c7e6L464-R464

aDotInTheVoid · 2021-03-24T19:47:41Z

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

jyn514 · 2021-03-25T18:25:55Z

@bors r+

bors · 2021-03-25T18:25:57Z

📌 Commit d9e2d8d has been approved by jyn514

…doc, r=jyn514 [rustdoc] Don't document stripped items in JSON renderer. Fixesrust-lang#80664, see [my comment there](rust-lang#80664 (comment)) for why Note that we already do something similar in `convert_item`: https://github.com/rust-lang/rust/blob/bb4cdf8ec034dca5c056ec9295f38062e5b7e871/src/librustdoc/json/conversions.rs#L28-L31 `@rustbot` modify labels: +T-rustdoc +A-rustdoc-json r? `@jyn514` cc `@CraftSpider`

Rollup of 8 pull requests Successful merges: - rust-lang#83055 ([rustdoc] Don't document stripped items in JSON renderer.) - rust-lang#83437 (Refactor rust-lang#82270 as lint instead of an error) - rust-lang#83444 (Fix bootstrap tests on beta) - rust-lang#83456 (Add docs for Vec::from functions) - rust-lang#83463 (ExitStatusExt: Fix missing word in two docs messages) - rust-lang#83470 (Fix patch note about rust-lang#80653 not mentioning nested nor recursive) - rust-lang#83485 (Mark asm tests as requiring LLVM 10.0.1) - rust-lang#83486 (Don't ICE when using `#[global_alloc]` on a non-item statement) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustdoc: Turn `JsonRenderer::mod_item_in` into `unreachable!()` The JSON renderer no longer gets called on modules (since rust-lang#83055). r? `@aDotInTheVoid`

rustdoc: Turn `JsonRenderer::mod_item_in` into `unreachable!()` The JSON renderer no longer gets called on modules (since rust-lang#83055). r? ``@aDotInTheVoid``

rustbot added A-rustdoc-json Area: Rustdoc JSON backend T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 12, 2021

rust-highfive assigned jyn514 Mar 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 12, 2021

jyn514 approved these changes Mar 22, 2021
View reviewed changes

Comment threadsrc/librustdoc/formats/renderer.rs Outdated
Comment threadsrc/librustdoc/formats/renderer.rs Outdated

jyn514 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 Mar 22, 2021

jyn514 changed the title ~~[rustdoc] Only document items in striped modules in html renderer.~~[rustdoc] Only document items in stripped modules in html renderer.Mar 22, 2021

jyn514 changed the title ~~[rustdoc] Only document items in stripped modules in html renderer.~~[rustdoc] Don't document stripped items in JSON renderer.Mar 22, 2021

aDotInTheVoid force-pushed the selective-strip-item-doc branch from 7d9ac84 to a72ef29Compare March 22, 2021 17:12

jyn514 reviewed Mar 22, 2021
View reviewed changes

Comment threadsrc/librustdoc/json/mod.rs Outdated

This comment has been minimized.
Sign in to view

aDotInTheVoid force-pushed the selective-strip-item-doc branch from 9cf63eb to fc82df4Compare March 22, 2021 22:18

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 Mar 22, 2021

aDotInTheVoid force-pushed the selective-strip-item-doc branch from fc82df4 to eb72f7eCompare March 24, 2021 15:57

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 Mar 24, 2021

aDotInTheVoid added 2 commits March 24, 2021 16:54

Better errors in jsondocck
ce21447

Don't call item on modules for json renderer
9ba9297
Closesrust-lang#80664

aDotInTheVoid force-pushed the selective-strip-item-doc branch from eb72f7e to 9ba9297Compare March 24, 2021 16:54

This comment has been minimized.
Sign in to view

Rename span to source
d9e2d8d
Caused by rust-lang@b0659f9#diff-ede26372490522288745c5b3df2b6b2a1cc913dcd09b29af3a49935afe00c7e6L464-R464

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 Mar 24, 2021

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 Mar 25, 2021

Dylan-DPC-zz mentioned this pull request Mar 25, 2021
Rollup of 9 pull requests #83497
Closed

Dylan-DPC-zz mentioned this pull request Mar 26, 2021
Rollup of 8 pull requests #83503
Merged

bors merged commit 0502815 into rust-lang:masterMar 26, 2021

rustbot added this to the 1.53.0 milestone Mar 26, 2021

jyn514 mentioned this pull request Apr 22, 2021
rustdoc: Turn JsonRenderer::mod_item_in into unreachable!()#84459
Merged

aDotInTheVoid deleted the selective-strip-item-doc branch December 2, 2024 19:03

	let inner = match*kind {
	clean::StrippedItem(_) => returnNone,
	x => from_clean_item_kind(x,self.tcx,&name),
	};

Uh oh!

Uh oh!

Conversation

aDotInTheVoid commented Mar 12, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aDotInTheVoid commented Mar 19, 2021

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jyn514 commented Mar 22, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aDotInTheVoid commented Mar 22, 2021

Uh oh!

This comment has been minimized.

aDotInTheVoid commented Mar 22, 2021

Uh oh!

jyn514 commented Mar 24, 2021

Uh oh!

jyn514 commented Mar 24, 2021

Uh oh!

jyn514 commented Mar 24, 2021

Uh oh!

aDotInTheVoid commented Mar 24, 2021

Uh oh!

This comment has been minimized.

aDotInTheVoid commented Mar 24, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

aDotInTheVoid commented Mar 24, 2021

Uh oh!

aDotInTheVoid commented Mar 24, 2021

Uh oh!

jyn514 commented Mar 25, 2021

Uh oh!

bors commented Mar 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aDotInTheVoid commented Mar 12, 2021•
edited
Loading

jyn514 commented Mar 22, 2021•
edited
Loading

aDotInTheVoid commented Mar 24, 2021•
edited
Loading