Skip to content

Conversation

@overlookmotel
Copy link
Contributor

changelog: [missing_safety_doc]: accept uppercase "SAFETY"

In Oxc's codebase, we try to draw attention as clearly as possible to the safety constraints of unsafe code by including an uppercase # SAFETY doc comment.

Clippy's missing_safety_doc lint does not recognise "SAFETY" in upper case, so we also need to include #[expect(clippy::missing_safety_doc)] on every unsafe function, to avoid a false positive. Unfortunately this defeats the purpose of the lint, as if someone later removes the safety docs, the lint rule does not trigger.

I don't know how common this style of documenting unsafe functions is, but I don't imagine also supporting # SAFETY would disturb other users who prefer # Safety.

@rustbot
Copy link
Collaborator

rustbot commented Nov 18, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 18, 2024
Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

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

I'm surprised it checks case at all. I'd say it's a lot better to use to_ascii_* over this change like with undocumented_unsafe_blocks (which, to be fair, might make a bit more sense there than in headings where sentence case is standard practice)

@Manishearth Manishearth added this pull request to the merge queue Nov 18, 2024
@Manishearth
Copy link
Member

Yeah, though I don't want to incur the cost of preemptively lowercasing all docs text (probably not too bad if done right). This is fine for now.

Merged via the queue into rust-lang:master with commit 7a1e9f2 Nov 18, 2024
9 checks passed
@Centri3
Copy link
Member

Centri3 commented Nov 18, 2024

It's already only reached if it's a heading so it's about as optimized as it can be
Actually, we could just use eq_ignore_ascii_case. Always forget that exists

@overlookmotel overlookmotel deleted the uppercase-safety-comment branch November 18, 2024 20:04
@overlookmotel
Copy link
Contributor Author

Thanks for merging so swiftly!

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