[rustdoc] Don't document stripped items in JSON renderer. #83055

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(_) => return None, 
        
               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.

src/librustdoc/formats/renderer.rs

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 ?

src/librustdoc/json/mod.rs

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

Closes rust-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. Fixes rust-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

src/librustdoc/formats/renderer.rs Outdated Show resolved Hide resolved

src/librustdoc/formats/renderer.rs Outdated Show resolved Hide resolved

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 a72ef29 Compare March 22, 2021 17:12

jyn514 reviewed Mar 22, 2021

View reviewed changes

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

This comment has been minimized.

Sign in to view

aDotInTheVoid force-pushed the selective-strip-item-doc branch from 9cf63eb to fc82df4 Compare 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 eb72f7e Compare 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

Closes rust-lang#80664

aDotInTheVoid force-pushed the selective-strip-item-doc branch from eb72f7e to 9ba9297 Compare 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:master Mar 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rustdoc] Don't document stripped items in JSON renderer. #83055

[rustdoc] Don't document stripped items in JSON renderer. #83055

aDotInTheVoid commented Mar 12, 2021 •

edited

Loading

aDotInTheVoid commented Mar 19, 2021

jyn514 left a comment

jyn514 commented Mar 22, 2021 •

edited

Loading

jyn514 left a comment

aDotInTheVoid commented Mar 22, 2021

This comment has been minimized.

aDotInTheVoid commented Mar 22, 2021

jyn514 commented Mar 24, 2021

jyn514 commented Mar 24, 2021

jyn514 commented Mar 24, 2021

aDotInTheVoid commented Mar 24, 2021

This comment has been minimized.

aDotInTheVoid commented Mar 24, 2021 •

edited

Loading

This comment has been minimized.

aDotInTheVoid commented Mar 24, 2021

aDotInTheVoid commented Mar 24, 2021

jyn514 commented Mar 25, 2021

bors commented Mar 25, 2021

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

[rustdoc] Don't document stripped items in JSON renderer. #83055

[rustdoc] Don't document stripped items in JSON renderer. #83055

Conversation

aDotInTheVoid commented Mar 12, 2021 • edited Loading

aDotInTheVoid commented Mar 19, 2021

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 commented Mar 22, 2021 • edited Loading

jyn514 left a comment

Choose a reason for hiding this comment

aDotInTheVoid commented Mar 22, 2021

This comment has been minimized.

aDotInTheVoid commented Mar 22, 2021

jyn514 commented Mar 24, 2021

jyn514 commented Mar 24, 2021

jyn514 commented Mar 24, 2021

aDotInTheVoid commented Mar 24, 2021

This comment has been minimized.

aDotInTheVoid commented Mar 24, 2021 • edited Loading

This comment has been minimized.

aDotInTheVoid commented Mar 24, 2021

aDotInTheVoid commented Mar 24, 2021

jyn514 commented Mar 25, 2021

bors commented Mar 25, 2021

aDotInTheVoid commented Mar 12, 2021 •

edited

Loading

jyn514 commented Mar 22, 2021 •

edited

Loading

aDotInTheVoid commented Mar 24, 2021 •

edited

Loading