Skip to content

Emit errors/warns on some wrong uses of rustdoc attributes - #80300

Merged
bors merged 4 commits into
rust-lang:masterfrom
LeSeulArtichaut:80275-doc-inline
May 11, 2021
Merged

Emit errors/warns on some wrong uses of rustdoc attributes#80300
bors merged 4 commits into
rust-lang:masterfrom
LeSeulArtichaut:80275-doc-inline

Conversation

@LeSeulArtichaut

@LeSeulArtichautLeSeulArtichaut commented Dec 22, 2020•
edited by GuillaumeGomez
Loading

Copy link
Copy Markdown
Contributor

This PR adds a few diagnostics:

  • error if conflicting #[doc(inline)]/#[doc(no_inline)] are found
  • introduce the invalid_doc_attributes lint (warn-by-default) which triggers:
    • if a crate-level attribute is used on a non-crate item
    • if #[doc(inline)]/#[doc(no_inline)] is used on a non-use item

The code could probably be improved but I wanted to get feedback first. Also, some of those changes could be considered breaking changes, so I don't know what the procedure would be? And finally, for the warnings, they are currently hard warnings, maybe it would be better to introduce a lint? (EDIT: introduced the invalid_doc_attributes lint)

Closes#80275.
r? @jyn514

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2020
@jyn514

jyn514 commented Dec 22, 2020

Copy link
Copy Markdown
Member

I think this should probably go through FCP - I'll take a look at the code first so the review isn't intermingled with the policy change.

jyn514
jyn514 reviewed Dec 22, 2020
View reviewed changes

@jyn514jyn514 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a few comments, and I think it would be helpful for @estebank to take a look if you have time.

Comment threadsrc/librustdoc/visit_ast.rs Outdated
Comment threadsrc/librustdoc/visit_ast.rs Outdated
Comment threadsrc/librustdoc/visit_ast.rs Outdated
Comment threadsrc/test/rustdoc-ui/check-doc-attrs.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
pickfire
pickfire reviewed Dec 22, 2020
View reviewed changes
Comment threadsrc/test/rustdoc-ui/check-doc-attrs.rs Outdated
@LeSeulArtichaut
LeSeulArtichautforce-pushed the 80275-doc-inline branch 2 times, most recently from b790ab6 to febf1d5CompareDecember 22, 2020 17:33
@rust-log-analyzer

This comment has been minimized.

Sign in to view
GuillaumeGomez
GuillaumeGomez reviewed Dec 22, 2020
View reviewed changes
Comment threadsrc/librustdoc/visit_ast.rs Outdated
@GuillaumeGomez

GuillaumeGomez commented Dec 22, 2020

Copy link
Copy Markdown
Member

Also, it could be interesting to add a macro like https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/macro.struct_span_err.html for warnings (but without error code).

@jyn514

jyn514 commented Dec 26, 2020

Copy link
Copy Markdown
Member

Also, it could be interesting to add a macro like https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/macro.struct_span_err.html for warnings (but without error code).

I think that's https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.struct_span_lint_hir.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
jyn514
jyn514 reviewed Dec 26, 2020
View reviewed changes
Comment threadsrc/librustdoc/visit_ast.rs Outdated
Comment threadsrc/test/rustdoc-ui/check-doc-attrs.stderr Outdated
Comment threadsrc/test/rustdoc-ui/check-doc-attrs.stderr Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@LeSeulArtichaut

This comment has been minimized.

Sign in to view
@jyn514jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-attributes Area: Attributes (`#[…]`, `#![…]`) labels Dec 27, 2020
@LeSeulArtichautLeSeulArtichaut added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 27, 2020
jyn514
jyn514 reviewed Dec 27, 2020
View reviewed changes
Comment threadsrc/librustdoc/visit_ast.rs Outdated
@LeSeulArtichautLeSeulArtichaut added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 8, 2021
@jyn514

jyn514 commented May 9, 2021

Copy link
Copy Markdown
Member

I don't have time to look at this.

r? @Manishearth

Manishearth
Manishearth approved these changes May 10, 2021
View reviewed changes

@ManishearthManishearth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall looks great, would love to have more doc comments

Comment threadcompiler/rustc_passes/src/check_attr.rs
@LeSeulArtichaut

LeSeulArtichaut commented May 10, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@Manishearth Added a few doc comments in a separate commit. Let me know if you'd want anything else documented.

@Manishearth

Manishearth commented May 10, 2021

Copy link
Copy Markdown
Member

@bors r+

thanks!

@bors

bors commented May 10, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 6e8d0db has been approved by Manishearth

@borsbors 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 May 10, 2021
@bors

bors commented May 10, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 6e8d0db with merge ac17b18a35f83579e5f75a8da8a7756f4abc9b17...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

bors commented May 10, 2021

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@borsbors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2021
@LeSeulArtichaut

LeSeulArtichaut commented May 10, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

This is the only case of invalid #[doc(inline)]/#[doc(no_inline)] I found in std::{os,sys} so we should be good now

@Manishearth

Manishearth commented May 11, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented May 11, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 804ab9f has been approved by Manishearth

@borsbors 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 May 11, 2021
@bors

bors commented May 11, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 804ab9f with merge fe62c6e...

@bors

bors commented May 11, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: Manishearth
Pushing fe62c6e to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label May 11, 2021
@bors
bors merged commit fe62c6e into rust-lang:masterMay 11, 2021
@rustbotrustbot added this to the 1.54.0 milestone May 11, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estebankestebankestebank left review comments
@ManishearthManishearthManishearth approved these changes
@GuillaumeGomezGuillaumeGomezGuillaumeGomez approved these changes
+2 more reviewers
@pickfirepickfirepickfire left review comments
@jyn514jyn514jyn514 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@ManishearthManishearth

Labels

A-attributesArea: Attributes (`#[…]`, `#![…]`)A-diagnosticsArea: Messages for errors, warnings, and lintsdisposition-mergeThis issue / PR is in PFCP or FCP with a disposition to merge it.finished-final-comment-periodThe final comment period is finished for this PR / Issue.merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.54.0

Development

Successfully merging this pull request may close these issues.

Rustdoc should give an error if you set both doc(inline) and doc(no_inline)

14 participants

@LeSeulArtichaut@jyn514@rust-log-analyzer@GuillaumeGomez@rfcbot@bors@Manishearth@camelid@estebank@pickfire@JohnCSimon@apiraino@rust-highfive@rustbot