Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (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 (
|
rustbot
added
the
S-waiting-on-review
|
Should we be suppressing the lint here, or just emitting the correct suggestion? |
|
I'd be fine with both. |
There was a problem hiding this comment.
Well... from what I see 👀, skipping the lint completely when unsafe block is involved doesn't really fits the lint's description, where it says:
for_eachmay be used after applying iterator transformers like
filterfor better readability and performance. It may also be used to fit a simple
operation on one line.
But when none of these apply, a simpleforloop is more idiomatic.
It might be a good idea to specify it in the doc as well, of why it's skipping the unsafe.
Also, make sure you put a fixes: with the issue number which you're trying to fix (I believe it's #12514 correct?) in your PR summary, therefore when this got merged, that issue will be closed automatically by bot.
| && let ExprKind::Block(..) = body.value.kind | ||
| // Skip the lint if the body is not safe, so as not to suggest `for … in … unsafe {}` | ||
| // and suggesting `for … in … { unsafe { } }` is a little ugly. | ||
| && let ExprKind::Block(Block { rules: BlockCheckMode::DefaultBlock, .. }, ..) = body.value.kind |
There was a problem hiding this comment.
In case you ever want to support the ugly suggestion 😂, you may want to take rules out, then offer the suggestion accordingly, such as:
if let BlockCheckMode::UnsafeBlock(_) = rules {
Sugg::hir_with_applicability(cx, body.value, "..", &mut applicability).blockify()
} else {
Sugg::hir_with_applicability(cx, body.value, "..", &mut applicability)
}at line 102.
Which gives the correct suggestion
huh, i just found the issue on my own, that person seems to have discovered it at nearly the exact same time as me. |
Well... I saw both that one issue and your PR were the first on the list, and that issue was posted a few hours prior, so I thought you were trying to fix that issue, oops 😅 |
|
@bend-n I see a warning of: Can you fix that? It's probably some sort of wide space in some comment. Also I'd like to see a "Known Issues" section in the docs that tells people they should expect the lint not to trigger for unsafe blocks. |
|
please lookup |
|
Thank you both for the clarification and the fix. @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes #12514
changelog: [
needless_for_each]: unsafe block in for loop body suggestion