Skip to content

Commit 190f0de

Browse files
borsbors
committed
Auto merge of #8450 - Jarcho:unsafe_blocks_8449, r=giraffate
Rework `undocumented_unsafe_blocks` fixes: #8264 fixes: #8449 One thing came up while working on this. Currently comments on the same line are supported like so: ```rust /* SAFETY: reason */ unsafe {} ``` Is this worth supporting at all? Anything other than a couple of words doesn't really fit well. edit: [zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/.60undocumented_unsafe_blocks.60.20same.20line.20comment) changelog: Don't lint `undocumented_unsafe_blocks` when the unsafe block comes from a proc-macro. changelog: Don't lint `undocumented_unsafe_blocks` when the preceding line has a safety comment and the unsafe block is a sub-expression.
2 parents 1cec8b3 + 17c8bee commit 190f0de

File tree

6 files changed

+277
-262
lines changed

6 files changed

+277
-262
lines changed

‎clippy_lints/src/lib.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// error-pattern:cargo-clippy
22

3+
#![feature(array_windows)]
34
#![feature(binary_heap_into_iter_sorted)]
45
#![feature(box_patterns)]
56
#![feature(control_flow_enum)]
@@ -849,7 +850,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
849850
enable_raw_pointer_heuristic_for_send,
850851
))
851852
});
852-
store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default()));
853+
store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks));
853854
store.register_late_pass(|| Box::new(match_str_case_mismatch::MatchStrCaseMismatch));
854855
store.register_late_pass(move || Box::new(format_args::FormatArgs));
855856
store.register_late_pass(|| Box::new(trailing_empty_array::TrailingEmptyArray));

‎clippy_lints/src/undocumented_unsafe_blocks.rs

+142-167
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,38 @@
1-
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
1+
use clippy_utils::diagnostics::span_lint_and_help;
22
use clippy_utils::is_lint_allowed;
3-
use clippy_utils::source::{indent_of, reindent_multiline, snippet};
4-
use rustc_errors::Applicability;
5-
use rustc_hir::intravisit::{walk_expr, Visitor};
6-
use rustc_hir::{Block, BlockCheckMode, Expr, ExprKind, HirId, Local, UnsafeSource};
7-
use rustc_lexer::TokenKind;
8-
use rustc_lint::{LateContext, LateLintPass};
3+
use clippy_utils::source::walk_span_to_context;
4+
use rustc_hir::{Block, BlockCheckMode, UnsafeSource};
5+
use rustc_lexer::{tokenize, TokenKind};
6+
use rustc_lint::{LateContext, LateLintPass, LintContext};
97
use rustc_middle::lint::in_external_macro;
10-
use rustc_middle::ty::TyCtxt;
11-
use rustc_session::{declare_tool_lint, impl_lint_pass};
12-
use rustc_span::{BytePos, Span};
13-
use std::borrow::Cow;
8+
use rustc_session::{declare_lint_pass, declare_tool_lint};
9+
use rustc_span::{BytePos, Pos, SyntaxContext};
10+
use std::rc::Rc;
1411

1512
declare_clippy_lint! {
1613
/// ### What it does
1714
/// Checks for `unsafe` blocks without a `// SAFETY: ` comment
1815
/// explaining why the unsafe operations performed inside
1916
/// the block are safe.
2017
///
18+
/// Note the comment must appear on the line(s) preceding the unsafe block
19+
/// with nothing appearing in between. The following is ok:
20+
/// ```ignore
21+
/// foo(
22+
/// // SAFETY:
23+
/// // This is a valid safety comment
24+
/// unsafe { *x }
25+
/// )
26+
/// ```
27+
/// But neither of these are:
28+
/// ```ignore
29+
/// // SAFETY:
30+
/// // This is not a valid safety comment
31+
/// foo(
32+
/// /* SAFETY: Neither is this */ unsafe { *x },
33+
/// );
34+
/// ```
35+
///
2136
/// ### Why is this bad?
2237
/// Undocumented unsafe blocks can make it difficult to
2338
/// read and maintain code, as well as uncover unsoundness
@@ -44,179 +59,139 @@ declare_clippy_lint! {
4459
"creating an unsafe block without explaining why it is safe"
4560
}
4661

47-
impl_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]);
48-
49-
#[derive(Default)]
50-
pub struct UndocumentedUnsafeBlocks {
51-
pub local_level: u32,
52-
pub local_span: Option<Span>,
53-
// The local was already checked for an overall safety comment
54-
// There is no need to continue checking the blocks in the local
55-
pub local_checked: bool,
56-
// Since we can only check the blocks from expanded macros
57-
// We have to omit the suggestion due to the actual definition
58-
// Not being available to us
59-
pub macro_expansion: bool,
60-
}
62+
declare_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]);
6163

6264
impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
6365
fn check_block(&mut self, cx: &LateContext<'_>, block: &'_ Block<'_>) {
64-
if_chain! {
65-
if !self.local_checked;
66-
if !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, block.hir_id);
67-
if !in_external_macro(cx.tcx.sess, block.span);
68-
if let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules;
69-
if let Some(enclosing_scope_hir_id) = cx.tcx.hir().get_enclosing_scope(block.hir_id);
70-
if self.block_has_safety_comment(cx.tcx, enclosing_scope_hir_id, block.span) == Some(false);
71-
then {
72-
let mut span = block.span;
73-
74-
if let Some(local_span) = self.local_span {
75-
span = local_span;
76-
77-
let result = self.block_has_safety_comment(cx.tcx, enclosing_scope_hir_id, span);
66+
if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided)
67+
&& !in_external_macro(cx.tcx.sess, block.span)
68+
&& !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, block.hir_id)
69+
&& !is_unsafe_from_proc_macro(cx, block)
70+
&& !block_has_safety_comment(cx, block)
71+
{
72+
let source_map = cx.tcx.sess.source_map();
73+
let span = if source_map.is_multiline(block.span) {
74+
source_map.span_until_char(block.span, '\n')
75+
} else {
76+
block.span
77+
};
7878

79-
if result.unwrap_or(true) {
80-
self.local_checked = true;
81-
return;
82-
}
83-
}
84-
85-
self.lint(cx, span);
86-
}
87-
}
88-
}
89-
90-
fn check_local(&mut self, cx: &LateContext<'_>, local: &'_ Local<'_>) {
91-
if_chain! {
92-
if !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, local.hir_id);
93-
if !in_external_macro(cx.tcx.sess, local.span);
94-
if let Some(init) = local.init;
95-
then {
96-
self.visit_expr(init);
97-
98-
if self.local_level > 0 {
99-
self.local_span = Some(local.span);
100-
}
101-
}
79+
span_lint_and_help(
80+
cx,
81+
UNDOCUMENTED_UNSAFE_BLOCKS,
82+
span,
83+
"unsafe block missing a safety comment",
84+
None,
85+
"consider adding a safety comment on the preceding line",
86+
);
10287
}
10388
}
89+
}
10490

105-
fn check_block_post(&mut self, _: &LateContext<'_>, _: &'_ Block<'_>) {
106-
self.local_level = self.local_level.saturating_sub(1);
107-
108-
if self.local_level == 0 {
109-
self.local_checked = false;
110-
self.local_span = None;
111-
}
112-
}
91+
fn is_unsafe_from_proc_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
92+
let source_map = cx.sess().source_map();
93+
let file_pos = source_map.lookup_byte_offset(block.span.lo());
94+
file_pos
95+
.sf
96+
.src
97+
.as_deref()
98+
.and_then(|src| src.get(file_pos.pos.to_usize()..))
99+
.map_or(true, |src| !src.starts_with("unsafe"))
113100
}
114101

115-
impl<'v> Visitor<'v> for UndocumentedUnsafeBlocks {
116-
fn visit_expr(&mut self, ex: &'v Expr<'v>) {
117-
match ex.kind {
118-
ExprKind::Block(_, _) => self.local_level = self.local_level.saturating_add(1),
119-
_ => walk_expr(self, ex),
102+
/// Checks if the lines immediately preceding the block contain a safety comment.
103+
fn block_has_safety_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
104+
// This intentionally ignores text before the start of a function so something like:
105+
// ```
106+
// // SAFETY: reason
107+
// fn foo() { unsafe { .. } }
108+
// ```
109+
// won't work. This is to avoid dealing with where such a comment should be place relative to
110+
// attributes and doc comments.
111+
112+
let source_map = cx.sess().source_map();
113+
let ctxt = block.span.ctxt();
114+
if ctxt != SyntaxContext::root() {
115+
// From a macro expansion. Get the text from the start of the macro declaration to start of the unsafe block.
116+
// macro_rules! foo { () => { stuff }; (x) => { unsafe { stuff } }; }
117+
// ^--------------------------------------------^
118+
if let Ok(unsafe_line) = source_map.lookup_line(block.span.lo())
119+
&& let Ok(macro_line) = source_map.lookup_line(ctxt.outer_expn_data().def_site.lo())
120+
&& Rc::ptr_eq(&unsafe_line.sf, &macro_line.sf)
121+
&& let Some(src) = unsafe_line.sf.src.as_deref()
122+
{
123+
macro_line.line < unsafe_line.line && text_has_safety_comment(
124+
src,
125+
&unsafe_line.sf.lines[macro_line.line + 1..=unsafe_line.line],
126+
unsafe_line.sf.start_pos.to_usize(),
127+
)
128+
} else {
129+
// Problem getting source text. Pretend a comment was found.
130+
true
120131
}
132+
} else if let Ok(unsafe_line) = source_map.lookup_line(block.span.lo())
133+
&& let Some(body) = cx.enclosing_body
134+
&& let Some(body_span) = walk_span_to_context(cx.tcx.hir().body(body).value.span, SyntaxContext::root())
135+
&& let Ok(body_line) = source_map.lookup_line(body_span.lo())
136+
&& Rc::ptr_eq(&unsafe_line.sf, &body_line.sf)
137+
&& let Some(src) = unsafe_line.sf.src.as_deref()
138+
{
139+
// Get the text from the start of function body to the unsafe block.
140+
// fn foo() { some_stuff; unsafe { stuff }; other_stuff; }
141+
// ^-------------^
142+
body_line.line < unsafe_line.line && text_has_safety_comment(
143+
src,
144+
&unsafe_line.sf.lines[body_line.line + 1..=unsafe_line.line],
145+
unsafe_line.sf.start_pos.to_usize(),
146+
)
147+
} else {
148+
// Problem getting source text. Pretend a comment was found.
149+
true
121150
}
122151
}
123152

124-
impl UndocumentedUnsafeBlocks {
125-
fn block_has_safety_comment(&mut self, tcx: TyCtxt<'_>, enclosing_hir_id: HirId, block_span: Span) -> Option<bool> {
126-
let map = tcx.hir();
127-
let source_map = tcx.sess.source_map();
128-
129-
let enclosing_scope_span = map.opt_span(enclosing_hir_id)?;
130-
131-
let between_span = if block_span.from_expansion() {
132-
self.macro_expansion = true;
133-
enclosing_scope_span.with_hi(block_span.hi()).source_callsite()
134-
} else {
135-
self.macro_expansion = false;
136-
enclosing_scope_span.to(block_span).source_callsite()
137-
};
138-
139-
let file_name = source_map.span_to_filename(between_span);
140-
let source_file = source_map.get_source_file(&file_name)?;
141-
142-
let lex_start = (between_span.lo().0 - source_file.start_pos.0 + 1) as usize;
143-
let lex_end = (between_span.hi().0 - source_file.start_pos.0) as usize;
144-
let src_str = source_file.src.as_ref()?[lex_start..lex_end].to_string();
145-
146-
let source_start_pos = source_file.start_pos.0 as usize + lex_start;
147-
148-
let mut pos = 0;
149-
let mut comment = false;
150-
151-
for token in rustc_lexer::tokenize(&src_str) {
152-
match token.kind {
153-
TokenKind::LineComment { doc_style: None }
154-
| TokenKind::BlockComment {
155-
doc_style: None,
156-
terminated: true,
157-
} => {
158-
let comment_str = src_str[pos + 2..pos + token.len].to_ascii_uppercase();
159-
160-
if comment_str.contains("SAFETY:") {
161-
comment = true;
162-
}
163-
},
164-
// We need to add all whitespace to `pos` before checking the comment's line number
165-
TokenKind::Whitespace => {},
166-
_ => {
167-
if comment {
168-
// Get the line number of the "comment" (really wherever the trailing whitespace ended)
169-
let comment_line_num = source_file
170-
.lookup_file_pos(BytePos((source_start_pos + pos).try_into().unwrap()))
171-
.0;
172-
// Find the block/local's line number
173-
let block_line_num = tcx.sess.source_map().lookup_char_pos(block_span.lo()).line;
174-
175-
// Check the comment is immediately followed by the block/local
176-
if block_line_num == comment_line_num + 1 || block_line_num == comment_line_num {
177-
return Some(true);
178-
}
179-
180-
comment = false;
181-
}
182-
},
153+
/// Checks if the given text has a safety comment for the immediately proceeding line.
154+
fn text_has_safety_comment(src: &str, line_starts: &[BytePos], offset: usize) -> bool {
155+
let mut lines = line_starts
156+
.array_windows::<2>()
157+
.rev()
158+
.map_while(|[start, end]| {
159+
src.get(start.to_usize() - offset..end.to_usize() - offset)
160+
.map(|text| (start.to_usize(), text.trim_start()))
161+
})
162+
.filter(|(_, text)| !text.is_empty());
163+
164+
let Some((line_start, line)) = lines.next() else {
165+
return false;
166+
};
167+
// Check for a sequence of line comments.
168+
if line.starts_with("//") {
169+
let mut line = line;
170+
loop {
171+
if line.to_ascii_uppercase().contains("SAFETY:") {
172+
return true;
173+
}
174+
match lines.next() {
175+
Some((_, x)) if x.starts_with("//") => line = x,
176+
_ => return false,
183177
}
184-
185-
pos += token.len;
186178
}
187-
188-
Some(false)
189179
}
190-
191-
fn lint(&self, cx: &LateContext<'_>, mut span: Span) {
192-
let source_map = cx.tcx.sess.source_map();
193-
194-
if source_map.is_multiline(span) {
195-
span = source_map.span_until_char(span, '\n');
180+
// No line comments; look for the start of a block comment.
181+
// This will only find them if they are at the start of a line.
182+
let (mut line_start, mut line) = (line_start, line);
183+
loop {
184+
if line.starts_with("/*") {
185+
let src = src[line_start..line_starts.last().unwrap().to_usize()].trim_start();
186+
let mut tokens = tokenize(src);
187+
return src[..tokens.next().unwrap().len]
188+
.to_ascii_uppercase()
189+
.contains("SAFETY:")
190+
&& tokens.all(|t| t.kind == TokenKind::Whitespace);
196191
}
197-
198-
if self.macro_expansion {
199-
span_lint_and_help(
200-
cx,
201-
UNDOCUMENTED_UNSAFE_BLOCKS,
202-
span,
203-
"unsafe block in macro expansion missing a safety comment",
204-
None,
205-
"consider adding a safety comment in the macro definition",
206-
);
207-
} else {
208-
let block_indent = indent_of(cx, span);
209-
let suggestion = format!("// SAFETY: ...\n{}", snippet(cx, span, ".."));
210-
211-
span_lint_and_sugg(
212-
cx,
213-
UNDOCUMENTED_UNSAFE_BLOCKS,
214-
span,
215-
"unsafe block missing a safety comment",
216-
"consider adding a safety comment",
217-
reindent_multiline(Cow::Borrowed(&suggestion), true, block_indent).to_string(),
218-
Applicability::HasPlaceholders,
219-
);
192+
match lines.next() {
193+
Some(x) => (line_start, line) = x,
194+
None => return false,
220195
}
221196
}
222197
}

‎tests/ui/auxiliary/proc_macro_unsafe.rs

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// compile-flags: --emit=link
2+
// no-prefer-dynamic
3+
4+
#![crate_type = "proc-macro"]
5+
6+
extern crate proc_macro;
7+
8+
use proc_macro::{Delimiter, Group, Ident, TokenStream, TokenTree};
9+
10+
#[proc_macro]
11+
pub fn unsafe_block(input: TokenStream) -> TokenStream {
12+
let span = input.into_iter().next().unwrap().span();
13+
TokenStream::from_iter([TokenTree::Ident(Ident::new("unsafe", span)), {
14+
let mut group = Group::new(Delimiter::Brace, TokenStream::new());
15+
group.set_span(span);
16+
TokenTree::Group(group)
17+
}])
18+
}

‎tests/ui/crashes/ice-7868.stderr

+1-5
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,7 @@ LL | unsafe { 0 };
55
| ^^^^^^^^^^^^
66
|
77
= note: `-D clippy::undocumented-unsafe-blocks` implied by `-D warnings`
8-
help: consider adding a safety comment
9-
|
10-
LL ~ // SAFETY: ...
11-
LL ~ unsafe { 0 };
12-
|
8+
= help: consider adding a safety comment on the preceding line
139

1410
error: aborting due to previous error
1511

0 commit comments

Comments
 (0)