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

Lint unnecessary safety comments #9851

Merged
merged 5 commits into from
Nov 25, 2022

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Nov 15, 2022

changelog: [unnecessary_safety_comment]: Add unnecessary safety comment lint

Addresses #7954

This does not necessarily catch all occurences, as doing so would require checking all expressions in the entire source which seems rather expensive. Instead what the lint does is it checks items, statements and the tail expression of blocks for safety comments, then checks if those comments are necessary or not, then linting for the unnecessary ones.

I kept the tests in one file to check that the lints do not clash with each other.

@rust-highfive
Copy link

r? @giraffate

(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 Nov 15, 2022
@Veykril Veykril force-pushed the unnecessary-safety-comment branch 3 times, most recently from 95c1f0b to 745abda Compare November 15, 2022 10:24
@Veykril
Copy link
Member Author

Veykril commented Nov 17, 2022

Apologies for not having added tests upfront, I forgot to add any because I was expecting to potentially get a reply for my question first.

Though I think I'll just walk all statements and block tail expressions and check those, that should catch the main causes without having to check every expression.

@Veykril Veykril changed the title Lint unnecessary safety comments on items Lint unnecessary safety comments Nov 17, 2022
@Veykril
Copy link
Member Author

Veykril commented Nov 17, 2022

Okay I implemented linting on (block tail) expressions and statements as well now.

Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Sorry, I can't take the time right now, but I'll review again in a few days.

I made small comments.
The test fails because of the following:
https://github.com/rust-lang/rust-clippy/actions/runs/3489064024/jobs/5838683005#step:7:1524

error: this could be rewritten as `let...else`
   --> src/undocumented_unsafe_blocks.rs:141:9
    |
141 | /         let expr = match stmt.kind {
142 | |             hir::StmtKind::Local(&hir::Local { init: Some(expr), .. })
143 | |             | hir::StmtKind::Expr(expr)
144 | |             | hir::StmtKind::Semi(expr) => expr,
145 | |             _ => return,
146 | |         };
    | |__________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else
    = note: `-D clippy::manual-let-else` implied by `-D clippy::pedantic`
help: consider writing
    |
141 ~         let hir::StmtKind::Local(&hir::Local { init: Some(expr), .. })
142 +             | hir::StmtKind::Expr(expr)
143 +             | hir::StmtKind::Semi(expr) = stmt.kind else { return };

tests/ui/undocumented_unsafe_blocks.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks!

I didn't investigate detail, but it looks like that the test may fail because #9941 was merged. So, could you try to rebase the master branch?

@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Nov 25, 2022

📌 Commit f96dd38 has been approved by giraffate

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 25, 2022

⌛ Testing commit f96dd38 with merge efadb55...

@bors
Copy link
Contributor

bors commented Nov 25, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing efadb55 to master...

@bors bors merged commit efadb55 into rust-lang:master Nov 25, 2022
@Veykril Veykril deleted the unnecessary-safety-comment branch January 9, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants