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.

Sign up for GitHub

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

Jump to bottom

Apply unused_doc_comments lint to inner items #78367

Closed
wants to merge 2 commits into from
Closed

Apply unused_doc_comments lint to inner items #78367

wants to merge 2 commits into from

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Oct 25, 2020 •
edited
Loading

Split out from #78306

The unused_doc_comments lint fires on comments that are not rendered by rustdoc. Currently, this fires on all non-item statements:

fn main() {
    /// Doc comment // warning: unused doc comment
    let _ = 1; 
    
    /// Other doc comment // warning: unused doc comment
    ()
}

However, we do not currently lint item statements:

fn main() {
	/// Doc comment, no warning
	fn inner() {}

	/// Another doc comment, no warning
	struct Foo {}
}

None of these doc comments will get rendered by rustdoc. However, as commit d12ea6e (#78367) shows, these kind of doc comments occur in many places throughout rustc. It"s possibile this pattern is used in the wider ecosystem.

We have two options here, depending on what we think the meaning of doc comments should be:

  1. Doc comments are an instruction to rustdoc to render something. We should lint doc comments on inner items - users should write regular comments instead, since they will not be rendered by rustdoc.
  2. Doc comments are information for the user of an item (e.g. the caller of a function), which may get rendered as a result of running rustdoc. We should not lint doc comments on inner items, as they serve as a visual indicator to people reading the code. This is opposed to regular comments, which are primarily intended for people modifying a particular piece of code.

I don"t have a strong preference as to which interpretation is correct. However, I think we should decide on one of them, and document the decision somewhere.

The current behavior of not linting item statements appears to have come about by accident. There was code in the lint that explicitly checked for StmtKind::Item, but it was never run due to a bug (fixed in #78326) with how we handled attributes on statements.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2020
@Aaron1011 Aaron1011 mentioned this pull request Oct 25, 2020
Closed
@Aaron1011 Aaron1011 added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2020
@bors
Copy link
Contributor

bors commented Oct 29, 2020

☔ The latest upstream changes (presumably #78430) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@Aaron1011 Aaron1011 added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 30, 2020
@matthewjasper
Copy link
Contributor

cc @petrochenkov who reviewed #78306

@nikomatsakis
Copy link
Contributor

Doc comments are information for the user of an item (e.g. the caller of a function), which may get rendered as a result of running rustdoc. We should not lint doc comments on inner items, as they serve as a visual indicator to people reading the code. This is opposed to regular comments, which are primarily intended for people modifying a particular piece of code.

This is how I feel. I would be quite surprised to see warnings here, the same as I would be surprised to see warnings on private or doc(hidden) items, even though those things may not be rendered in rustdoc.

I think I would want the property in particular that anything which is legal on a struct Foo definition at top-level is also legal on such a definition within a function.

@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 20, 2020
@nikomatsakis
Copy link
Contributor

@rfcbot fcp close

We discussed this in recent @rust-lang/lang meetings and decided that we felt that we did not want to warn for doc comments attached to inner items, just as we do not warn for doc comments on private functions or other things that may or may not appear in rustdoc output. Therefore, I"m moving to close the issue.

@rfcbot
Copy link

rfcbot commented Dec 1, 2020 •
edited by cramertj
Loading

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn"t been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Dec 1, 2020
@nikomatsakis
Copy link
Contributor

I checked the names of folks who were present in the meeting.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 1, 2020
@rfcbot
Copy link

rfcbot commented Dec 1, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 11, 2020
@rfcbot
Copy link

rfcbot commented Dec 11, 2020

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Dec 11, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Dec 17, 2020
@Aaron1011 Aaron1011 closed this Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@matthewjasper matthewjasper

Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Aaron1011 @rust-highfive @bors @matthewjasper @nikomatsakis @rfcbot @spastorino @crlf0710 @Mark-Simulacrum