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

Warn on #![doc(test(...))] on items other than the crate root and use future incompatible lint #82708

Merged
merged 5 commits into from
Mar 6, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 2, 2021

Part of #82672.

This PR does multiple things:

  • Create a new INVALID_DOC_ATTRIBUTE lint which is also "future incompatible", allowing us to use it as a warning for the moment until it turns (eventually) into a hard error.
  • Use this link when #![doc(test(...))] isn't used at the crate level.
  • Make Change error about unknown attributes to a warning #82702 use this new lint as well.

r? @jyn514

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 2, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2021
@GuillaumeGomez
Copy link
Member Author

@Manishearth This error should be a warning too. Do you have suggestion for an existing lint I could use here maybe?

@Manishearth
Copy link
Member

@GuillaumeGomez Yes. I think we should create a combined INVALID_DOC_ATTRIBUTE future incompat lint in #82730

@GuillaumeGomez
Copy link
Member Author

Ok, might be worth doing it in this PR directly then. Or do you prefer in another PR?

@jyn514
Copy link
Member

jyn514 commented Mar 3, 2021

r? @Manishearth

@rust-highfive rust-highfive assigned Manishearth and unassigned jyn514 Mar 3, 2021
@Manishearth
Copy link
Member

@GuillaumeGomez Fine either way, but please use the future incompat infrastructure (it's a different kind of lint)

@GuillaumeGomez
Copy link
Member Author

Ok, going to update this PR as soon as possible then!

@jyn514
Copy link
Member

jyn514 commented Mar 3, 2021

Fixes #82672.

This does not fix #82672. That issue is for warning about unknown doc(test(xxx)) attributes. This warns about #![doc(test(...))] on an item, which is a good warning, but separate.

@GuillaumeGomez
Copy link
Member Author

Good point. I updated the PR description and I will open another PR which takes the meta item list and run the checks on them.

@jyn514 jyn514 changed the title Doc test attr check Warn on #![doc(test(...))] on items other than the crate root Mar 3, 2021
@GuillaumeGomez GuillaumeGomez force-pushed the doc-test-attr-check branch 2 times, most recently from 4c32ac1 to dbf39f3 Compare March 3, 2021 22:29
@GuillaumeGomez
Copy link
Member Author

I added the lint with the "future incompatible" as requested. It now warns by default.

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Looks good, cannot be tied to an edition yet. Also I'd like to wait for the regression fix to land so that that can also be migrated to this lint

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Manishearth
Copy link
Member

Can we also use this lint for #82702 ?

@GuillaumeGomez
Copy link
Member Author

@Manishearth Sure, do you want me to add it in this PR as well?

@Manishearth
Copy link
Member

Yes please! (and update the PR body/title)

@GuillaumeGomez GuillaumeGomez changed the title Warn on #![doc(test(...))] on items other than the crate root Warn on #![doc(test(...))] on items other than the crate root and use future incompatible lint Mar 4, 2021
@GuillaumeGomez
Copy link
Member Author

I updated the PR, its description and the title. Don't hesitate to tell me if it could be worded better!

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 4, 2021

📌 Commit 55cec90 has been approved by Manishearth

@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 4, 2021
/// Previously, there were very like checks being performed on `#[doc(..)]`
/// unlike the other attributes. It'll now catch all the issues that it
/// silently ignored previously.
pub INVALID_DOC_ATTRIBUTE,
Copy link
Member

Choose a reason for hiding this comment

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

@Manishearth
Copy link
Member

@bors r-

yep, good catch, thanks

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 5, 2021
@GuillaumeGomez
Copy link
Member Author

Fixed the lint name!

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 5, 2021

📌 Commit a11e87e has been approved by Manishearth

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 5, 2021
… r=Manishearth

Warn on `#![doc(test(...))]` on items other than the crate root and use future incompatible lint

Part of rust-lang#82672.

This PR does multiple things:
 * Create a new `INVALID_DOC_ATTRIBUTE` lint which is also "future incompatible", allowing us to use it as a warning for the moment until it turns (eventually) into a hard error.
 * Use this link when `#![doc(test(...))]` isn't used at the crate level.
 * Make rust-lang#82702 use this new lint as well.

r? `@jyn514`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2021
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#80845 (Make ItemKind::ExternCrate looks like hir::ItemKind::ExternCrate to make transition over hir::ItemKind simpler)
 - rust-lang#82708 (Warn on `#![doc(test(...))]` on items other than the crate root and use future incompatible lint)
 - rust-lang#82714 (Detect match arm body without braces)
 - rust-lang#82736 (Bump optimization from mir_opt_level 2 to 3 and 3 to 4 and make "release" be level 2 by default)
 - rust-lang#82782 (Make rustc shim's verbose output include crate_name being compiled.)
 - rust-lang#82797 (Update tests names to start with `issue-`)
 - rust-lang#82809 (rustdoc: Use substrings instead of split to grab enum variant paths)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8867f7f into rust-lang:master Mar 6, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 6, 2021
@GuillaumeGomez GuillaumeGomez deleted the doc-test-attr-check branch March 6, 2021 14:01
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.

9 participants