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: skip // some variants omitted if enum is #[non_exhaustive] #109007

Merged
merged 1 commit into from
Mar 26, 2023

Conversation

Ezrashaw
Copy link
Contributor

Fixes #108925

Never touched rustdoc before so probably not the best code.

cc @dtolnay

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2023

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

@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 Mar 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2023

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@Ezrashaw Ezrashaw marked this pull request as draft March 11, 2023 05:31
@Ezrashaw Ezrashaw force-pushed the tweak-some-variants-omitted branch from 8b118d3 to db038b2 Compare March 11, 2023 09:09
@Ezrashaw
Copy link
Contributor Author

@jsha I've added the lint and implemented your advice in the linked issue.

As per the linked issue, Enum1 warns with the new lint. Enum2 and Enum3 have identical output, just the #[non_exhaustive] attribute.

@rustbot ready

@Ezrashaw Ezrashaw force-pushed the tweak-some-variants-omitted branch from db038b2 to 496e77e Compare March 11, 2023 09:15
@Ezrashaw Ezrashaw marked this pull request as ready for review March 11, 2023 09:15
@Ezrashaw Ezrashaw force-pushed the tweak-some-variants-omitted branch from 496e77e to ea89853 Compare March 12, 2023 08:20
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

I think this lint may need a crater run to make sure the warning doesn't cause too much breakage. I'm also not familiar with the motivation for this change, but in some ways, I think the lint may be a better fit for clippy. The reason is I'm a bit concerned it could be triggered by valid code.

src/doc/rustdoc/src/lints.md Outdated Show resolved Hide resolved
src/doc/rustdoc/src/lints.md Outdated Show resolved Hide resolved
@Ezrashaw
Copy link
Contributor Author

@camelid I'm not convinced by this change now, it breaks libcore which obviously has -D warnings. While it's impossible to merge this PR, I do think that libcore and co have bad code in this regard. For example, take this snippet from libbacktrace:

/// The styles of printing that we can print
#[derive(Copy, Clone, Eq, PartialEq)]
pub enum PrintFmt {
    /// Prints a terser backtrace which ideally only contains relevant information
    Short,
    /// Prints a backtrace that contains all possible information
    Full,
    #[doc(hidden)]
    __Nonexhaustive,
}

It's just wrong and the exact thing this lint is for.

Not sure where you want to go from here.

@camelid
Copy link
Member

camelid commented Mar 14, 2023

@Ezrashaw Thanks for looking into the breakage! I do think this lint could potentially be a good fit for clippy (though they'd have to be consulted of course).

I'd also like to hear what @jsha's thoughts are on this since he proposed the lint: Do you think moving it to clippy would make sense?

@jsha
Copy link
Contributor

jsha commented Mar 15, 2023

I have to admit I don't know the factors that would favor a rustdoc lint or a clippy lint. Above you mention that it could be triggered by valid code. Are rustdoc lints only for input that is definitely invalid?

@Ezrashaw
Copy link
Contributor Author

@jsha Not sure if someone else was going to answer this; nobody else has so here goes: I think that possibly valid code should never be triggered by (warn-by-default) rustdoc lints, period. I think that the question here is really: is code valid if it contains #[doc(hidden)] variants whilst not being #[non_exhaustive]. I am of the opinion that such code is invalid, but libcore contains code which triggers the lint (+ -D warnings so we can't compile the standard library).

The bottom line is: there is nothing we can feasibly do while this lint exists to get it to pass CI. I'll remove the lint and maybe we can land the one-line change which is the other part of this PR..

@Ezrashaw Ezrashaw force-pushed the tweak-some-variants-omitted branch from ea89853 to cdc9cc0 Compare March 18, 2023 10:15
@notriddle
Copy link
Contributor

I think this lint makes more sense in Clippy than in Rustdoc.

@Ezrashaw
Copy link
Contributor Author

@notriddle Agreed, I've removed that part of the PR, now there is just the one liner fix which the linked issue was primarily concerned with.

@Ezrashaw
Copy link
Contributor Author

@jsha Can we land this now?

@camelid camelid changed the title rustdoc: replace // some variants omitted with #[non_exhaustive] rustdoc: skip // some variants omitted if enum is #[non_exhaustive] Mar 22, 2023
@camelid
Copy link
Member

camelid commented Mar 22, 2023

I updated the title since I believe the content of this PR has changed.

@GuillaumeGomez
Copy link
Member

Please add a test for it and then I'll approve it. Also, if not done already, I think adding this as a clippy lint would be nice. Please at least open an issue there so it's not forgotten.

Don't display `// some variants omitted` if enum is marked
`#[non_exhaustive]`
@Ezrashaw Ezrashaw force-pushed the tweak-some-variants-omitted branch from cdc9cc0 to e0ec9c0 Compare March 26, 2023 05:06
@notriddle
Copy link
Contributor

@bors r=notriddle,GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Mar 26, 2023

📌 Commit e0ec9c0 has been approved by notriddle,GuillaumeGomez

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 26, 2023

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

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

Rollup of 6 pull requests

Successful merges:

 - rust-lang#109007 (rustdoc: skip `// some variants omitted` if enum is `#[non_exhaustive]`)
 - rust-lang#109593 (Rustdoc Book refer to rustdoc::missing_doc_code_examples. Fixes rust-lang#109592.)
 - rust-lang#109595 (Improve "Auto-hide trait implementation documentation" GUI test)
 - rust-lang#109619 (Still-further-specializable projections are ambiguous in new solver)
 - rust-lang#109620 (Correct typo (`back_box` -> `black_box`))
 - rust-lang#109621 (Refactor: `VariantIdx::from_u32(0)` -> `FIRST_VARIANT`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit df25f15 into rust-lang:master Mar 26, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 26, 2023
@Ezrashaw Ezrashaw deleted the tweak-some-variants-omitted branch April 7, 2023 02:35
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.

Consider omitting "some variants omitted" in enum documentation if enum is non_exhaustive
7 participants