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.

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

Adding configuration to allow safety comment above stmt containing unsafe block #10886

Merged
merged 4 commits into from
Jun 17, 2023
Merged

Adding configuration to allow safety comment above stmt containing unsafe block #10886

merged 4 commits into from
Jun 17, 2023

Conversation

lochetti
Copy link
Contributor

@lochetti lochetti commented Jun 4, 2023 •
edited by xFrednet
Loading

Adding a new configuration, accept-comment-above-statement, to allow a safety comment to be placed before the statement that has the unsafe block. It affects the undocumented_unsafe_blocks lint.

The default value for this configuration will be false. So the user has to opt-in for the change.

This PR fixes #10832

changelog: Enhancement [undocumented_unsafe_blocks]: Added accept-comment-above-statement configuration.
#10886

@rustbot
Copy link
Collaborator

rustbot commented Jun 4, 2023

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 4, 2023
@lochetti lochetti changed the title Adding configuration to allow safety comment above stmt containing un… Adding configuration to allow safety comment above stmt containing unsafe block Jun 4, 2023
@xFrednet
Copy link
Member

xFrednet commented Jun 4, 2023

Hey @Centri3, this might be an interesting and simple PR to review, if you're interested.

I can assign you to the PR, after you left a comment. That makes it easier to keep track of PRs :).

r? @xFrednet

@rustbot rustbot assigned xFrednet and unassigned Jarcho Jun 4, 2023
@Centri3
Copy link
Member

Centri3 commented Jun 4, 2023

Alright, I'll review it in a bit :D

Centri3
Centri3 reviewed Jun 4, 2023
View reviewed changes
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.

The implementation looks pretty good overall! There's just a minor typo in accept_comment_above_statement's doc comment.

clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved
tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs Outdated Show resolved Hide resolved
@lochetti
Copy link
Contributor Author

lochetti commented Jun 4, 2023

The implementation looks pretty good overall! There's just a minor typo in accept_comment_above_statement's doc comment.

Hey @Centri3 thank you very much for the review. I will address your comments in the next couple days and I let you know :)

@lochetti lochetti force-pushed the fix_10832 branch 2 times, most recently from 0b58c47 to e956267 Compare June 5, 2023 09:23
@lochetti
Copy link
Contributor Author

@Centri3 I have just rebased from master. Let me know if you have any comments about the current state of the PR :)

Centri3
Centri3 approved these changes Jun 13, 2023
View reviewed changes
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.

This looks good to me, thanks!

cc @xFrednet

xFrednet
xFrednet approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This version LGTM! I believe, you'll need to rebased on master and run cargo collect-metadata again due to some recent changes to that script. Then it should be ready to be merged :D

Thank you for the changes!

clippy_lints/src/utils/conf.rs
/// Lint: UNDOCUMENTED_UNSAFE_BLOCKS.
///
/// Whether to accept a safety comment to be placed above the statement containing the `unsafe` block
(accept_comment_above_statement: bool = false),
Copy link
Member

Choose a reason for hiding this comment

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

A recent PR adds configuration lints in the changelog file. You'll probably have to rebase on master and run cargo collect-metadata again

tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs
@@ -0,0 +1,534 @@
//@aux-build:proc_macro_unsafe.rs
Copy link
Member

Choose a reason for hiding this comment

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

A small site note @Centri3, AFAIK you requested that this file is the same as the normal test file. Generally speaking, I think it's fine to have some basic tests for the lint, some don't lint examples and then the specific case, which is influenced by the config value. Having a lot of output in the .stderr file can make it hard to distinguish the actual changes.

This is not to imply that either variant is better. This is up to the reviewer to decide and since you mainly reviewed this one, I'll follow your lead. Just so you know, it's not mandatory :)

@xFrednet
Copy link
Member

@Centri3 I'll delegate the PR to you, you can r=Centri3,xFrednet after the rebase :D.

@bors delegate=@Centri3

@bors
Copy link
Contributor

bors commented Jun 14, 2023

✌️ @Centri3, you can now approve this pull request!

If @xFrednet told you to "r=me" after making some further change, please make that change, then do @bors r=@xFrednet

@lochetti
Copy link
Contributor Author

I will take a week off, so I will only probably rebase and fix this PR after the 25th :)

@lochetti
Copy link
Contributor Author

Hey @Centri3 and @xFrednet. I have rebased and have run cargo collect-metadata. The last commit is the result of the cargo collect-metada command. Let me know if anything else is required :)

@Centri3
Copy link
Member

Centri3 commented Jun 17, 2023

Thank you!
(omg I get to merge)

@bors r=Centri3,xFrednet

@bors
Copy link
Contributor

bors commented Jun 17, 2023

📌 Commit d610201 has been approved by Centri3,xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 17, 2023

⌛ Testing commit d610201 with merge bb78d76...

@bors
Copy link
Contributor

bors commented Jun 17, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3,xFrednet
Pushing bb78d76 to master...

@bors bors merged commit bb78d76 into rust-lang:master Jun 17, 2023
@bors bors mentioned this pull request Jun 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centri3 Centri3 Centri3 approved these changes

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

@Centri3 Centri3

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

undocumented_unsafe_blocks false positive when unsafe block is on a separate line from a let
6 participants
@lochetti @rustbot @xFrednet @Centri3 @bors @Jarcho