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

Wrong pub modifier in each function of trait documentation #81274

Closed
tesuji opened this issue Jan 22, 2021 · 15 comments · Fixed by #81288 or #84832
Closed

Wrong pub modifier in each function of trait documentation #81274

tesuji opened this issue Jan 22, 2021 · 15 comments · Fixed by #81288 or #84832
Assignees
Labels
A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate A-visibility Area: Visibility / privacy C-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@tesuji
Copy link
Contributor

tesuji commented Jan 22, 2021

The documentation of Iterator trait on 1.49.0 (current stable) doesn't contain pub modifier in function declaration of that trait.
image
However, on beta and nightly, there is pub modifier.
image

This is wrong because you cannot use pub modifier for each method/function in Trait definition.
Why is this important? People copy the method/function declaration in the documentation to their code
and see the compiler error: error[E0449]: unnecessary visibility qualifier.

Meta

rustc --version --verbose:

rustc 1.49.0 (e1884a8e3 2020-12-29)
binary: rustc
commit-hash: e1884a8e3c3e813aada8254edfa120e85bf5ffca
commit-date: 2020-12-29
host: x86_64-unknown-linux-gnu
release: 1.49.0

cc @jyn514 I think this might relate to some changes in visibility around macros ?

@tesuji tesuji added the C-bug Category: This is a bug. label Jan 22, 2021
@rustbot rustbot added regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 22, 2021
@jyn514
Copy link
Member

jyn514 commented Jan 22, 2021

Yes, this needs the same fix as #77820 (comment) (in particular

// Don't show `pub` for fields on enum variants; they are always public
Item { visibility: self.vis.clean(cx), ..what_rustc_thinks }
should work).

@jyn514 jyn514 added A-visibility Area: Visibility / privacy E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Jan 22, 2021
@camelid camelid self-assigned this Jan 22, 2021
@camelid
Copy link
Member

camelid commented Jan 22, 2021

Self-assigning because this is a beta regression.

@camelid
Copy link
Member

camelid commented Jan 22, 2021

Note that this also occurs for the impl's docs.

@bors bors closed this as completed in 64cf8c2 Jan 24, 2021
@hameerabbasi hameerabbasi removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 25, 2021
@hameerabbasi
Copy link
Contributor

Removed I-prioritize since this was fixed.

@tesuji
Copy link
Contributor Author

tesuji commented Jan 26, 2021

In nightly, trait methods still rendered as pub fn:
image

Is there something wrong to Rust release or the bug still exists ?

@jyn514
Copy link
Member

jyn514 commented Jan 26, 2021

Hmm, maybe it was only fixed for items in the local crate. @lzutao do you have time to come up with an MCVE? I expect it's related to cross-crate re-exports.

@jyn514 jyn514 reopened this Jan 26, 2021
@jyn514 jyn514 added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 26, 2021
@tesuji
Copy link
Contributor Author

tesuji commented Jan 26, 2021

Your guess sounds right. With core link (https://doc.rust-lang.org/nightly/core/iter/trait.ExactSizeIterator.html#provided-methods), things are normal.

@jyn514 jyn514 added the A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate label Jan 26, 2021
@camelid camelid removed their assignment Jan 28, 2021
@camelid
Copy link
Member

camelid commented Jan 28, 2021

I don't think I'll have time to fix this second part soon, so unassigning myself.

@camelid camelid added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 5, 2021
@camelid
Copy link
Member

camelid commented Feb 5, 2021

Assigning P-medium and removing I-prioritize as discussed in the prioritization working group.

@tesuji

This comment has been minimized.

@rustbot rustbot removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Feb 5, 2021
@jyn514
Copy link
Member

jyn514 commented Feb 5, 2021

I think

impl Clean<Item> for ty::AssocItem {
is the cross-crate case.

@jyn514 jyn514 added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Feb 5, 2021
ehuss pushed a commit to ehuss/rust that referenced this issue Feb 5, 2021
rustdoc: Fix visibility of trait and impl items

Fixes rust-lang#81274.

r? `@jyn514`
@pietroalbini
Copy link
Member

Fix backported to 1.50.0 and landed in 1.51.0, closing this.

@tesuji
Copy link
Contributor Author

tesuji commented Feb 8, 2021

Please reopen. The issue is not entirely fixed for cross-crate trait reexport case.

@jyn514 jyn514 reopened this Feb 8, 2021
@camelid
Copy link
Member

camelid commented Feb 13, 2021

The cross-crate issue is now on stable. @rustbot label: +regression-from-stable-to-stable -regression-from-stable-to-beta

@rustbot rustbot added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Feb 13, 2021
@Stupremee
Copy link
Member

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate A-visibility Area: Visibility / privacy C-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
8 participants