Skip to content
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

rustdoc: Fix invalid handling of nested items with --document-private-items #110450

Merged
merged 3 commits into from Apr 18, 2023

Conversation

GuillaumeGomez
Copy link
Member

Fixes #110422.

The problem is that only impl block and re-exported macro_rules! items are "visible" as nested items. This PR adds the missing checks to handle this correctly.

cc @compiler-errors
r? @notriddle

@rustbot 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 Apr 17, 2023
@@ -1,6 +1,7 @@
pub enum E { //~ ERROR
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to move this nested enum outside because since it's not "processed" anymore, the failure doesn't appear anymore either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this test case was first added in #75127, it seems like it was specifically meant to check for an ICE that only triggered when an item was declared inside of a function.

You can add a second enum if you want to keep the test triggering the error, but don't remove the one inside the function's body.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't triggered anymore. Hence why I moved it. Should I just remove the test then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, the test doesn't seem to exist to check the actual error (AFAICT, @jyn514 would know better). It seems to exist as a regression test for an ICE. infinite-recursive-type.rs already exists to test the case where an infinite recursive type is outside of a function.

If it's not generating an error, I think (if I'm wrong, please chime in @jyn514) this should be changed to a // check-pass test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to check-pass seems ok (moving the item to the top-level was definitely incorrect, thank you @notriddle for catching it). tbh I was hesitant to add this test originally, given that there are many other similar ICEs that we haven't bothered fixing in 3 years, but if in the future something changes we can revisit the decision.

While you're looking at this, could you please rename the test to tests/rustdoc-ui/error-in-impl-trait/infinite-recursive-type.rs so it's easier to recover the context? It sounds like this change was because of --document-private-items, so if you could also add a revision of the test where f and E are both public, that would be helpful too.

@GuillaumeGomez GuillaumeGomez force-pushed the fix-nested-items-on-private-doc branch from 4b04c9d to c456e15 Compare April 17, 2023 18:27
@GuillaumeGomez
Copy link
Member Author

I changed the ui-test to check that it passes (since it's supposed to check there is no ICE).

@notriddle
Copy link
Contributor

@bors r=notriddle,jyn514 rollup

@bors
Copy link
Contributor

bors commented Apr 17, 2023

📌 Commit c456e15 has been approved by notriddle,jyn514

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 Apr 17, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 18, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#109981 (Set commit information environment variables when building tools)
 - rust-lang#110348 (Add list of supported disambiguators and suffixes for intra-doc links in the rustdoc book)
 - rust-lang#110409 (Don't use `serde_json` to serialize a simple JSON object)
 - rust-lang#110442 (Avoid including dry run steps in the build metrics)
 - rust-lang#110450 (rustdoc: Fix invalid handling of nested items with `--document-private-items`)
 - rust-lang#110461 (Use `Item::expect_*` and `ImplItem::expect_*` more)
 - rust-lang#110465 (Assure everyone that `has_type_flags` is fast)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d646891 into rust-lang:master Apr 18, 2023
11 checks passed
@rustbot rustbot added this to the 1.71.0 milestone Apr 18, 2023
@GuillaumeGomez GuillaumeGomez deleted the fix-nested-items-on-private-doc branch April 18, 2023 08:22
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 18, 2023
…sive-type, r=jyn514

Add a failing rustdoc-ui test for public infinite recursive type

As suggested in rust-lang#110450 (comment).

r? `@jyn514`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: nested items should not be documented with --document-private-items
5 participants