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

fix: undocumented_unsafe_blocks FP on trait/impl items #13888

Merged
merged 4 commits into from
Mar 1, 2025

Conversation

profetia
Copy link
Contributor

@profetia profetia commented Dec 28, 2024

fixes #11709

Continuation of #12672. r? @Alexendoo if you don't mind?

changelog: [undocumented_unsafe_blocks] fix FP on trait/impl items

@rustbot
Copy link
Collaborator

rustbot commented Dec 28, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Alexendoo (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 Dec 28, 2024
@profetia
Copy link
Contributor Author

profetia commented Feb 4, 2025

r? clippy

@rustbot rustbot assigned Manishearth and unassigned Alexendoo Feb 4, 2025
@Manishearth
Copy link
Member

r? clippy

on a work trip and my notifications are packed

@rustbot rustbot assigned xFrednet and unassigned Manishearth Feb 5, 2025
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the update (And sorry for the delay with the review. 😅 )


Roses are red,
Violets are blue,
Safety comments,
For unsafe code

@xFrednet xFrednet enabled auto-merge February 16, 2025 13:50
@xFrednet xFrednet added this pull request to the merge queue Feb 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 17, 2025
@profetia profetia force-pushed the issue11709 branch 2 times, most recently from 322323a to fe44ed7 Compare February 17, 2025 06:56
@profetia
Copy link
Contributor Author

Looks like it is conflicted with the recent update on ui toml. Should be ok now, you may need to re-press the button though @xFrednet.

@profetia profetia requested a review from xFrednet February 17, 2025 07:18
@xFrednet
Copy link
Member

Thanks for the rebase!

I've checked the CI lintcheck output: https://github.com/rust-lang/rust-clippy/actions/runs/13364592312?pr=13888 and actually noticed a regression here: https://docs.rs/time/0.3.36/src/time/offset_date_time.rs.html#35. The linked code would now be linted while it wasn't before. Could you take a look at it and add a test?

@profetia profetia force-pushed the issue11709 branch 2 times, most recently from 3879429 to 644122a Compare February 17, 2025 16:18
@profetia
Copy link
Contributor Author

@xFrednet Can you check it now? I fixed that one and we also got 9 cases REMOVED. How wiered that this one in time-rs does not exist initially.

@xFrednet
Copy link
Member

xFrednet commented Mar 1, 2025

@xFrednet Can you check it now? I fixed that one and we also got 9 cases REMOVED. How wiered that this one in time-rs does not exist initially.

This is super nice, all of them have been FPs that you fixed now :D

It might be that the lint is still incorrectly emitted. The CI only shows the difference. But I don't think we should block on this. This PR has already been open for quite some time :)

@xFrednet
Copy link
Member

xFrednet commented Mar 1, 2025

This version looks good to me, could you rebase one last time?

Thank you for your patience while waiting on my review 😅

@profetia
Copy link
Contributor Author

profetia commented Mar 1, 2025

This version looks good to me, could you rebase one last time?

Thank you for your patience while waiting on my review 😅

Rebased

@xFrednet
Copy link
Member

xFrednet commented Mar 1, 2025

// Safety: The following poem is not complete
unsafe {
    Rose are red,
    Violets are blue
}

@xFrednet xFrednet added this pull request to the merge queue Mar 1, 2025
Merged via the queue into rust-lang:master with commit 62f34f2 Mar 1, 2025
11 checks passed
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.

undocumented_unsafe_blocks doesn't detect safety comment above associated constant
6 participants