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: Private function is rendered with a verbose visibility #80101

Closed
camelid opened this issue Dec 17, 2020 · 14 comments
Closed

rustdoc: Private function is rendered with a verbose visibility #80101

camelid opened this issue Dec 17, 2020 · 14 comments
Labels
A-visibility Area: Visibility / privacy C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@camelid
Copy link
Member

camelid commented Dec 17, 2020

If I run cargo +nightly doc --document-private-items on this code:

mod foo {
    struct S;
    impl S {
        fn new() -> S { S }
    }
}

then the output looks like this:

image

Note that the visibility is verbose – it says pub(in foo) when it should show nothing (and used to show nothing).

This bug only occurs when the function is private and --document-private-items is passed (of course, the docs are not generated without that flag).

@camelid camelid added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-visibility Area: Visibility / privacy regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. C-bug Category: This is a bug. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc labels Dec 17, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 17, 2020
@camelid
Copy link
Member Author

camelid commented Dec 17, 2020

searched nightlies: from nightly-2020-11-17 to nightly-2020-11-20
regressed nightly: nightly-2020-11-19
searched commits: from c919f49 to 8256379
regressed commit: 7d747db

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-apple-darwin
Reproduce with:

cargo bisect-rustc --preserve --prompt --start=2020-11-17 --end=2020-11-20 -- doc --document-private-items 

I'm not sure if the commit it thinks has the regression is correct though. cargo-bisect-rustc printed out this during bisection:

installing c4f836ad1aceb83507810d9499f56988fd24578d
installing 87776d7d5322422e5239e153e793b687f7f9c379
installing 7d747db0d5dd8f08f2efb073e2e77a34553465a7
testing...

and then after that it asked me again about c919f49, even though it asked me about it at the start of bisection.

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2020

the visibility is verbose – it says pub(in foo) when it should show nothing

This is probably the same cause as #79139, although the fix may be slightly different.

cargo bisect-rustc --preserve --prompt --start=2020-11-17 --end=2020-11-20 -- doc --document-private-items

The 'prompt' there makes me think you might have hit the wrong 'good/bad' option at some point.

@camelid
Copy link
Member Author

camelid commented Dec 17, 2020

I did the bisection twice though and both times it had the same result. I wish cargo-bisect-rustc could do this automatically somehow.

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2020

@camelid you could write a script that does an xpath query on the generated documentation, maybe.

@camelid
Copy link
Member Author

camelid commented Dec 17, 2020

At that point it's faster to do it manually.

@camelid
Copy link
Member Author

camelid commented Dec 17, 2020

@jyn514 Did the formatting change happen as part of #77820?

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2020

@camelid I wouldn't expect it to but I don't know without bisecting.

@camelid
Copy link
Member Author

camelid commented Dec 17, 2020

I bisected a third time, and again I got the same result. I think it might be a bug in cargo-bisect-rustc.

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2020

Well I can at least confirm that the bug is not present in beta, so this is a nightly regression.

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2020

These are the PRs merged between the 17th and 18th (the 18th also has this bug): https://github.com/rust-lang/rust/pulls?q=is%3Amerged+is%3Apr+label%3AT-rustdoc+merged%3A2020-11-17..2020-11-18. fn new() {} doesn't have the weird formatting so I suspect either #79125 or #77820.

@camelid
Copy link
Member Author

camelid commented Dec 17, 2020

Based on manually installing toolchains, I've confirmed that the issue was introduced in nightly-2020-11-19, so that's c919f49...8256379.

@camelid
Copy link
Member Author

camelid commented Dec 17, 2020

Based on yet more manual testing by installing CI toolchains, I've confirmed that the issue was introduced in c4f836a (#77820). Phew, that was a lot of manual bisection!

@camelid camelid removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Dec 17, 2020
@apiraino apiraino added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 17, 2020
@camelid camelid changed the title rustdoc: Private function is rendered with a verbose visibility and with weird formatting rustdoc: Private function is rendered with a verbose visibility Dec 23, 2020
@camelid
Copy link
Member Author

camelid commented Dec 23, 2020

I figured out that the reason it has weird formatting is that rustdoc will render functions like

fn foo(
    x: ...,
    y: ...
)

if the one-line version would be over 80 characters.

This is the code that does that conditional formatting:

let output = if declaration_len > 80 {

So, the reason the weird formatting happens only on private functions is because the visibility is verbose and thus many functions end up being over 80 characters.

We might consider increasing that limit to 100 characters since that's the Rust standard line length, but I'm not sure if that would degrade readability.

@camelid
Copy link
Member Author

camelid commented Dec 25, 2020

Closing as duplicate of #79139.

@camelid camelid closed this as completed Dec 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-visibility Area: Visibility / privacy C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants