Skip to content

Commit ac04157

Browse files
JarchoJarcho
committed
Rework undocumented_unsafe_blocks 
1 parent dc5423a commit ac04157

File tree

5 files changed

+208
-262
lines changed

5 files changed

+208
-262
lines changed

‎clippy_lints/src/lib.rs

Lines changed: 2 additions & 1 deletion
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)]
@@ -846,7 +847,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
846847
enable_raw_pointer_heuristic_for_send,
847848
))
848849
});
849-
store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default()));
850+
store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks));
850851
store.register_late_pass(|| Box::new(match_str_case_mismatch::MatchStrCaseMismatch));
851852
store.register_late_pass(move || Box::new(format_args::FormatArgs));
852853
store.register_late_pass(|| Box::new(trailing_empty_array::TrailingEmptyArray));

‎clippy_lints/src/undocumented_unsafe_blocks.rs

Lines changed: 113 additions & 168 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,13 @@
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
@@ -44,179 +41,127 @@ declare_clippy_lint! {
4441
"creating an unsafe block without explaining why it is safe"
4542
}
4643

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-
}
44+
declare_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]);
6145

6246
impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
6347
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);
78-
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-
}
102-
}
103-
}
48+
if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided)
49+
&& !in_external_macro(cx.tcx.sess, block.span)
50+
&& !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, block.hir_id)
51+
&& !block_has_safety_comment(cx, block)
52+
{
53+
let source_map = cx.tcx.sess.source_map();
54+
let span = if source_map.is_multiline(block.span) {
55+
source_map.span_until_char(block.span, '\n')
56+
} else {
57+
block.span
58+
};
10459

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;
60+
span_lint_and_help(
61+
cx,
62+
UNDOCUMENTED_UNSAFE_BLOCKS,
63+
span,
64+
"unsafe block missing a safety comment",
65+
None,
66+
"consider adding a safety comment on the preceding line",
67+
);
11168
}
11269
}
11370
}
11471

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),
72+
/// Checks if the lines immediately preceding the block contain a safety comment.
73+
fn block_has_safety_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
74+
// This intentionally ignores text before the start of a function so something like:
75+
// ```
76+
// // SAFETY: reason
77+
// fn foo() { unsafe { .. } }
78+
// ```
79+
// won't work. This is to avoid dealing with where such a comment should be place relative to
80+
// attributes and doc comments.
81+
82+
let source_map = cx.sess().source_map();
83+
let ctxt = block.span.ctxt();
84+
if ctxt != SyntaxContext::root() {
85+
// From a macro expansion. Get the text from the start of the macro declaration to start of the unsafe block.
86+
// macro_rules! foo { () => { stuff }; (x) => { unsafe { stuff } }; }
87+
// ^--------------------------------------------^
88+
if let Ok(unsafe_line) = source_map.lookup_line(block.span.lo())
89+
&& let Ok(macro_line) = source_map.lookup_line(ctxt.outer_expn_data().def_site.lo())
90+
&& Rc::ptr_eq(&unsafe_line.sf, &macro_line.sf)
91+
&& let Some(src) = unsafe_line.sf.src.as_deref()
92+
{
93+
macro_line.line < unsafe_line.line && text_has_safety_comment(
94+
src,
95+
&unsafe_line.sf.lines[macro_line.line + 1..unsafe_line.line + 1],
96+
unsafe_line.sf.start_pos.to_usize(),
97+
)
98+
} else {
99+
// Problem getting source text. Pretend a comment was found.
100+
true
120101
}
102+
} else if let Ok(unsafe_line) = source_map.lookup_line(block.span.lo())
103+
&& let Some(body) = cx.enclosing_body
104+
&& let Some(body_span) = walk_span_to_context(cx.tcx.hir().body(body).value.span, SyntaxContext::root())
105+
&& let Ok(body_line) = source_map.lookup_line(body_span.lo())
106+
&& Rc::ptr_eq(&unsafe_line.sf, &body_line.sf)
107+
&& let Some(src) = unsafe_line.sf.src.as_deref()
108+
{
109+
// Get the text from the start of function body to the unsafe block.
110+
// fn foo() { some_stuff; unsafe { stuff }; other_stuff; }
111+
// ^-------------^
112+
body_line.line < unsafe_line.line && text_has_safety_comment(
113+
src,
114+
&unsafe_line.sf.lines[body_line.line + 1..unsafe_line.line + 1],
115+
unsafe_line.sf.start_pos.to_usize(),
116+
)
117+
} else {
118+
// Problem getting source text. Pretend a comment was found.
119+
true
121120
}
122121
}
123122

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-
},
123+
/// Checks if the given text has a safety comment for the immediately proceeding line.
124+
fn text_has_safety_comment(src: &str, line_starts: &[BytePos], offset: usize) -> bool {
125+
let mut lines = line_starts
126+
.array_windows::<2>()
127+
.rev()
128+
.map_while(|[start, end]| {
129+
src.get(start.to_usize() - offset..end.to_usize() - offset)
130+
.map(|text| (start.to_usize(), text.trim_start()))
131+
})
132+
.filter(|(_, text)| !text.is_empty());
133+
134+
let Some((line_start, line)) = lines.next() else {
135+
return false;
136+
};
137+
// Check for a sequence of line comments.
138+
if line.starts_with("//") {
139+
let mut line = line;
140+
loop {
141+
if line.to_ascii_uppercase().contains("SAFETY:") {
142+
return true;
143+
}
144+
match lines.next() {
145+
Some((_, x)) if x.starts_with("//") => line = x,
146+
_ => return false,
183147
}
184-
185-
pos += token.len;
186148
}
187-
188-
Some(false)
189149
}
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');
150+
// No line comments; look for the start of a block comment.
151+
// This will only find them if they are at the start of a line.
152+
let (mut line_start, mut line) = (line_start, line);
153+
loop {
154+
if line.starts_with("/*") {
155+
let src = src[line_start..line_starts.last().unwrap().to_usize()].trim_start();
156+
let mut tokens = tokenize(src);
157+
return src[..tokens.next().unwrap().len]
158+
.to_ascii_uppercase()
159+
.contains("SAFETY:")
160+
&& tokens.all(|t| t.kind == TokenKind::Whitespace);
196161
}
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-
);
162+
match lines.next() {
163+
Some(x) => (line_start, line) = x,
164+
None => return false,
220165
}
221166
}
222167
}

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

Lines changed: 1 addition & 5 deletions
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

‎tests/ui/undocumented_unsafe_blocks.rs

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,6 @@ fn block_comment_newlines() {
8989
unsafe {}
9090
}
9191

92-
#[rustfmt::skip]
93-
fn inline_block_comment() {
94-
/* Safety: */unsafe {}
95-
}
96-
9792
fn block_comment_with_extras() {
9893
/* This is a description
9994
* SAFETY:
@@ -209,6 +204,43 @@ fn local_nest() {
209204
let _ = [(42, unsafe {}, unsafe {}), (52, unsafe {}, unsafe {})];
210205
}
211206

207+
fn in_fn_call(x: *const u32) {
208+
fn f(x: u32) {}
209+
210+
// Safety: reason
211+
f(unsafe { *x });
212+
}
213+
214+
fn multi_in_fn_call(x: *const u32) {
215+
fn f(x: u32, y: u32) {}
216+
217+
// Safety: reason
218+
f(unsafe { *x }, unsafe { *x });
219+
}
220+
221+
fn in_multiline_fn_call(x: *const u32) {
222+
fn f(x: u32, y: u32) {}
223+
224+
f(
225+
// Safety: reason
226+
unsafe { *x },
227+
0,
228+
);
229+
}
230+
231+
fn in_macro_call(x: *const u32) {
232+
// Safety: reason
233+
println!("{}", unsafe { *x });
234+
}
235+
236+
fn in_multiline_macro_call(x: *const u32) {
237+
println!(
238+
"{}",
239+
// Safety: reason
240+
unsafe { *x },
241+
);
242+
}
243+
212244
// Invalid comments
213245

214246
fn no_comment() {

0 commit comments

Comments
 (0)