Skip to content

[unnecessary_safety_comment] Some fixes regarding comments above attributes #15678

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
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[unnecessary_safety_comment] Some fixes regarding comments above attributes #15678

wants to merge 8 commits into from
+569 −124

Conversation

teofr
Copy link
Contributor

@teofr teofr commented Sep 15, 2025 •
edited
Loading

The way clippy checked for SAFETY comments above attributes was wrong:

  • It wasn't considered for items
  • It cause an ICE with some attributes
  • When considering comments above attributes, it didn't considered comments below them, causing some unlintable situations.

This PR tries to fix this by delegating the skipping of attributes to the function analyzing the source code itself.

changelog: [unnecessary_safety_comment]: Taking into account comments above attributes for items

Fixes #14555
Fixes #15684
Fixes #15754

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 24, 2025 •
edited
Loading

Lintcheck changes for 05d5c5d

Lint Added Removed Changed
clippy::undocumented_unsafe_blocks 0 18 1
clippy::unnecessary_safety_comment 16 0 269

This comment will be updated if you push new changes

@teofr
Copy link
Contributor Author

teofr commented Sep 24, 2025 •
edited
Loading

I checked all the removed/added lints and they all make sense to me

  • All of the removed undocumented_unsafe_block are correct, and shouldn'te have been there in the first place
  • Most of the added unnecesary_safety_comment are because a // SAFETY: comment is used in an unsafe fn. Maybe we should improve the lint message in these cases to suggest a // # Safety comment instead
  • One is a SAFETY comment on a struct, which I think is correctly linted
  • One is a bug that's been uncovered by this PR but was present already (unnecessary_safety_comment and undocumented_unsafe_blocks visit the same comment twice #15755)
  • The changes are a bit strange, basically some notes regarding why the lint is enabled are deleted or added, does anyone know what could cause this?

@teofr teofr marked this pull request as ready for review September 24, 2025 15:14
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2025

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@teofr teofr changed the title [unnecessary_safety_comment] Considering comments above attributes for items [unnecessary_safety_comment] Some fixes regarding comments above attributes Sep 26, 2025
@blyxyas
Copy link
Member

blyxyas commented Oct 2, 2025

r? blyxyas

Being that this is R4L-oriented, I'll give it priority

@rustbot rustbot assigned blyxyas and unassigned samueltardieu Oct 2, 2025
@blyxyas blyxyas added the G-Rust-for-Linux Issues related to Rust-for-Linux wants and bugfixes label Oct 2, 2025
@blyxyas
Copy link
Member

blyxyas commented Oct 2, 2025

Maybe we should improve the lint message in these cases to suggest a // # Safety comment instead

We totally should! could you add that case? (It should be simple)

blyxyas
blyxyas requested changes Oct 3, 2025
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

Great patch, but I have one request.
It's a bit hard to distinguish why you're being prompted to remove a SAFETY comment from an unsafe function without extra help. What about not linting when that SAFETY comment comes from a doc-comment, as it will be rendered as useful information for consumers?

Also, if it's not a doc-comment, we can suggest to make it into a doc comment (/// # Safety)

View changes since this review

clippy_lints/src/undocumented_unsafe_blocks.rs
Some((start + (text.len() - trimmed.len()), trimmed))
})
.filter(|(_, text)| !text.is_empty());
.filter(|(_, text)| !(text.is_empty() || (accept_comment_above_attributes && is_attribute(text))));
Copy link
Member

Choose a reason for hiding this comment

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

I really like how you managed to do a solution so elegant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I really appreciate it

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@teofr
Copy link
Contributor Author

teofr commented Oct 5, 2025

Hey, thanks for the review!

I tried to reach a (useful) middle grounds, if there's a SAFETY: comment over an unsafe function:

  • if it's a doc comment, I try to suggest the change (not very smart at the moment)
  • if it's a regular comment, I provide help indicating than a # Safety doc comment section could be better

I modified what position is used for the lint, so I don't need to find the position of SAFETY, which changes a lot (269) of lints. If this is discouraged or not worth for this fix I can bring back the original span, and just handle the suggestion cases by hand.

Anyways, I think this lint deserves a bigger refactor (I'd like to work on it, but have no time right now). #15755 and #clippy > broken lint: `undocumented_unsafe_blocks`

Let me know what you think

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Oct 5, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2025

☔ The latest upstream changes (possibly f3c020c) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blyxyas blyxyas blyxyas requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@blyxyas blyxyas

Labels
G-Rust-for-Linux Issues related to Rust-for-Linux wants and bugfixes S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
4 participants
@teofr @rustbot @blyxyas @samueltardieu