Add the undocumented_unsafe_blocks lint
#7557
Conversation
|
r? @flip1995 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
LeSeulArtichaut
force-pushed
the
lint-undocumented-unsafe
branch
3 times, most recently
from
9ce9984
to
d049720
Compare
There was a problem hiding this comment.
I have some concerns when it comes to performance with this lint. With the current implementation, this lint will potentially re-lex every file in the crate, if there are unsafe blocks.
I agree that this lint fits best in restriction, but I would also add a
if is_lint_allowed(...) {
return;
}to the top of the check_expr/check_block method. That and my suggestions below should deal with the performance impact.
| impl_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]); | ||
|
|
||
| impl EarlyLintPass for UndocumentedUnsafeBlocks { | ||
| fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &rustc_ast::Expr) { |
There was a problem hiding this comment.
There's also a check_block method.
| let mut pos = rustc_lexer::strip_shebang(src).unwrap_or(0); | ||
| for token in rustc_lexer::tokenize(&src[pos..]) { |
There was a problem hiding this comment.
Since we don't really care about correct and complete syntax in the file, but only finding comments immediately above the unsafe block, you should be able to optimize this. I think you can use the get_enclosing_scope in combination with the opt_span methods and then slice the to-tokenize src with the start of the enclosing scope up until the start of the block/the start of the line the unsafe block is in (for let stmts).
To do this, you would have to turn this lint into a LatePassLint, but that is a tradeoff I'm willing to take in order to not re-lex whole files just to find a comment, of which we already know where it should be.
To make sure that just slicing the src works as expected, please add a test case with unicode chars in the comment and the code above the unsafe block.
With this change, you also might be able to just directly return if the checked block has a SAFETY comment right above it, without having to do BytePos and span.hi()/.lo() gymnastics. Basically you have to track if after lexing the suggested part of the file, the last thing you saw was a comment and if it was a SAFETY comment.
|
|
||
| impl EarlyLintPass for UndocumentedUnsafeBlocks { | ||
| fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &rustc_ast::Expr) { | ||
| let (block, comments) = if_chain! { |
There was a problem hiding this comment.
I think you can add a in_external_macro check here, since this lint shouldn't be triggered for those.
| @@ -0,0 +1,74 @@ | |||
| #![warn(clippy::undocumented_unsafe_blocks)] | |||
There was a problem hiding this comment.
I'd like to see some tests of how this lint behaves in macros.
| comments.remove(i); | ||
| } | ||
| else { | ||
| span_lint(cx, UNDOCUMENTED_UNSAFE_BLOCKS, expr.span, "this `unsafe` block is missing a SAFETY comment"); |
There was a problem hiding this comment.
I think we should produce a help message here, suggesting to add a // SAFETY: ... above the unsafe block.
| let _ = | ||
| // SAFETY | ||
| unsafe {}; |
There was a problem hiding this comment.
There's a test missing for the example in the documentation:
// SAFETY: bla
let _ = unsafe {};
flip1995
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #7539) made this pull request unmergeable. Please resolve the merge conflicts. |
|
ping from triage @LeSeulArtichaut. Can you have any update on this? |
|
ping from triage @LeSeulArtichaut. According to the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this. |
giraffate
added
S-inactive-closed
changelog: add the [
undocumented_unsafe_blocks] lint.Fixes #7464.
The lint was proposed as a
stylelint, I implemented it as arestrictionlint, which is preferred?