Skip to content

Commit eba8a3a

Browse files
committed
Add new lint macros_hiding_unsafe_code
This lint only triggers if the `unsafe_op_in_unsafe_fn` lint is enabled. This lint is placed in the restriction group, since it suggests code, that requires to allow the `unused_unsafe` lint locally. Instead of allowing this lint, the unsafe block in the macro could be removed.
1 parent 54a20a0 commit eba8a3a

7 files changed

+324
-0
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -2613,6 +2613,7 @@ Released 2018-09-13
26132613
[`logic_bug`]: https://rust-lang.github.io/rust-clippy/master/index.html#logic_bug
26142614
[`lossy_float_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#lossy_float_literal
26152615
[`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports
2616+
[`macros_hiding_unsafe_code`]: https://rust-lang.github.io/rust-clippy/master/index.html#macros_hiding_unsafe_code
26162617
[`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
26172618
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
26182619
[`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map

clippy_lints/src/lib.rs

+4
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,7 @@ mod lifetimes;
252252
mod literal_representation;
253253
mod loops;
254254
mod macro_use;
255+
mod macros_hiding_unsafe_code;
255256
mod main_recursion;
256257
mod manual_async_fn;
257258
mod manual_map;
@@ -707,6 +708,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
707708
loops::WHILE_LET_LOOP,
708709
loops::WHILE_LET_ON_ITERATOR,
709710
macro_use::MACRO_USE_IMPORTS,
711+
macros_hiding_unsafe_code::MACROS_HIDING_UNSAFE_CODE,
710712
main_recursion::MAIN_RECURSION,
711713
manual_async_fn::MANUAL_ASYNC_FN,
712714
manual_map::MANUAL_MAP,
@@ -1018,6 +1020,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10181020
LintId::of(integer_division::INTEGER_DIVISION),
10191021
LintId::of(let_underscore::LET_UNDERSCORE_MUST_USE),
10201022
LintId::of(literal_representation::DECIMAL_LITERAL_REPRESENTATION),
1023+
LintId::of(macros_hiding_unsafe_code::MACROS_HIDING_UNSAFE_CODE),
10211024
LintId::of(map_err_ignore::MAP_ERR_IGNORE),
10221025
LintId::of(matches::REST_PAT_IN_FULLY_BOUND_STRUCTS),
10231026
LintId::of(matches::WILDCARD_ENUM_MATCH_ARM),
@@ -2101,6 +2104,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
21012104
let scripts = conf.allowed_scripts.clone();
21022105
store.register_early_pass(move || box disallowed_script_idents::DisallowedScriptIdents::new(&scripts));
21032106
store.register_late_pass(|| box strlen_on_c_strings::StrlenOnCStrings);
2107+
store.register_late_pass(|| box macros_hiding_unsafe_code::MacrosHidingUnsafeCode::default());
21042108
}
21052109

21062110
#[rustfmt::skip]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
use clippy_utils::{diagnostics::span_lint_and_sugg, in_macro, is_lint_allowed, source::snippet_with_applicability};
2+
use rustc_errors::Applicability;
3+
use rustc_hir::{intravisit::FnKind, Block, BlockCheckMode, Body, FnDecl, FnSig, HirId, UnsafeSource, Unsafety};
4+
use rustc_lint::{
5+
builtin::{UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE},
6+
LateContext, LateLintPass, LintContext,
7+
};
8+
use rustc_middle::lint::in_external_macro;
9+
use rustc_session::{declare_tool_lint, impl_lint_pass};
10+
use rustc_span::Span;
11+
12+
declare_clippy_lint! {
13+
/// **What it does:** Checks for macro calls inside an `unsafe` function which expansion
14+
/// contains an `unsafe` block, while the macro call is not wrapped in an `unsafe` block
15+
/// itself. This lint only triggers in functions where the `unsafe_op_in_unsafe_fn` lint is
16+
/// enabled.
17+
///
18+
/// **Why is this bad?** This hides an unsafe operation inside a macro call. This is against
19+
/// the intention of the `unsafe_op_in_unsafe_fn` lint, which is meant to make unsafe code more
20+
/// visible by requiring `unsafe` blocks also in `unsafe` functions.
21+
///
22+
/// **Known problems:**
23+
/// - In some cases the intention of the macro is to actually hide the unsafety, because the
24+
/// macro itself ensures the safety of the `unsafe` operation.
25+
/// - For local macros, either an `#[allow(unused_unsafe)]` has to be added to the new unsafe
26+
/// block or the unsafe block inside the macro has to be removed.
27+
///
28+
/// **Example:**
29+
///
30+
/// ```rust
31+
/// macro_rules! unsafe_macro {
32+
/// ($e:expr) => {
33+
/// unsafe { $e };
34+
/// };
35+
/// }
36+
///
37+
/// #[warn(unsafe_op_in_unsafe_fn)]
38+
/// unsafe fn foo(x: *const usize) {
39+
/// unsafe_macro!(std::ptr::read(x));
40+
/// }
41+
/// ```
42+
/// Use instead:
43+
/// ```rust
44+
/// # macro_rules! unsafe_macro {
45+
/// # ($e:expr) => {
46+
/// # unsafe { $e };
47+
/// # };
48+
/// # }
49+
/// #[warn(unsafe_op_in_unsafe_fn)]
50+
/// unsafe fn foo(x: *const usize) {
51+
/// #[allow(unused_unsafe)] unsafe { unsafe_macro!(std::ptr::read(x)) };
52+
/// }
53+
/// ```
54+
pub MACROS_HIDING_UNSAFE_CODE,
55+
restriction,
56+
"macros hiding unsafe code, while `unsafe_op_in_unsafe_fn` is enabled"
57+
}
58+
59+
#[derive(Clone, Copy, Default)]
60+
pub struct MacrosHidingUnsafeCode {
61+
in_unsafe_fn: bool,
62+
in_unsafe_block: usize,
63+
}
64+
65+
impl_lint_pass!(MacrosHidingUnsafeCode => [MACROS_HIDING_UNSAFE_CODE]);
66+
67+
impl LateLintPass<'_> for MacrosHidingUnsafeCode {
68+
fn check_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>) {
69+
if let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules {
70+
if in_macro(block.span) {
71+
if self.in_unsafe_fn
72+
&& self.in_unsafe_block == 0
73+
&& !is_lint_allowed(cx, UNSAFE_OP_IN_UNSAFE_FN, block.hir_id)
74+
{
75+
let macro_call_span = block.span.source_callsite();
76+
let unused_unsafe_sugg = if !in_external_macro(cx.sess(), block.span)
77+
&& !is_lint_allowed(cx, UNUSED_UNSAFE, block.hir_id)
78+
{
79+
"#[allow(unused_unsafe)] "
80+
} else {
81+
""
82+
};
83+
let mut applicability = Applicability::MaybeIncorrect;
84+
span_lint_and_sugg(
85+
cx,
86+
MACROS_HIDING_UNSAFE_CODE,
87+
macro_call_span,
88+
"this macro call hides unsafe code",
89+
"wrap it in an `unsafe` block",
90+
format!(
91+
"{}unsafe {{ {} }}",
92+
unused_unsafe_sugg,
93+
snippet_with_applicability(cx, macro_call_span, "...", &mut applicability),
94+
),
95+
applicability,
96+
);
97+
}
98+
} else {
99+
self.in_unsafe_block = self.in_unsafe_block.saturating_add(1);
100+
}
101+
}
102+
}
103+
104+
fn check_block_post(&mut self, _: &LateContext<'_>, block: &Block<'_>) {
105+
if let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules {
106+
if !in_macro(block.span) {
107+
self.in_unsafe_block = self.in_unsafe_block.saturating_sub(1);
108+
}
109+
}
110+
}
111+
112+
fn check_fn(&mut self, _: &LateContext<'_>, fn_kind: FnKind<'_>, _: &FnDecl<'_>, _: &Body<'_>, _: Span, _: HirId) {
113+
if is_unsafe_fn(fn_kind) {
114+
self.in_unsafe_fn = true;
115+
}
116+
}
117+
118+
fn check_fn_post(
119+
&mut self,
120+
_: &LateContext<'_>,
121+
fn_kind: FnKind<'_>,
122+
_: &FnDecl<'_>,
123+
_: &Body<'_>,
124+
_: Span,
125+
_: HirId,
126+
) {
127+
if is_unsafe_fn(fn_kind) {
128+
self.in_unsafe_fn = false;
129+
}
130+
}
131+
}
132+
133+
fn is_unsafe_fn(fn_kind: FnKind<'_>) -> bool {
134+
match fn_kind {
135+
FnKind::ItemFn(_, _, header, _) | FnKind::Method(_, &FnSig { header, .. }, _) => {
136+
matches!(header.unsafety, Unsafety::Unsafe)
137+
},
138+
FnKind::Closure => false,
139+
}
140+
}

tests/ui/auxiliary/macro_rules.rs

+7
Original file line numberDiff line numberDiff line change
@@ -113,3 +113,10 @@ macro_rules! default_numeric_fallback {
113113
let x = 22;
114114
};
115115
}
116+
117+
#[macro_export]
118+
macro_rules! unsafe_external_macro {
119+
($e:expr) => {
120+
unsafe { $e }
121+
};
122+
}
+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// run-rustfix
2+
// aux-build: macro_rules.rs
3+
4+
#![warn(clippy::macros_hiding_unsafe_code)]
5+
// compiletest allows unused code by default. But we want to explicitely test
6+
// for this lint here.
7+
#![warn(unused_unsafe)]
8+
9+
extern crate macro_rules;
10+
11+
use macro_rules::unsafe_external_macro;
12+
13+
macro_rules! unsafe_macro {
14+
($e:expr) => {
15+
unsafe { $e }
16+
};
17+
(NESTED $e:expr) => {
18+
unsafe { unsafe_function($e) }
19+
};
20+
}
21+
22+
unsafe fn unsafe_function(x: *const usize) {
23+
let _ = std::ptr::read(x);
24+
}
25+
26+
mod no_unsafe_op_in_unsafe_fn {
27+
use super::*;
28+
29+
#[allow(unused_unsafe)]
30+
pub unsafe fn ok(x: *const usize) {
31+
unsafe_macro!(std::ptr::read(x));
32+
unsafe_external_macro!(std::ptr::read(x));
33+
}
34+
}
35+
36+
mod unsafe_op_in_unsafe_fn {
37+
#![warn(unsafe_op_in_unsafe_fn)]
38+
39+
use super::*;
40+
41+
pub unsafe fn bad(x: *const usize) {
42+
#[allow(unused_unsafe)] unsafe { unsafe_macro!(std::ptr::read(x)); }
43+
#[allow(unused_unsafe)] unsafe { unsafe_macro!(NESTED std::ptr::read(&x)); }
44+
unsafe { unsafe_external_macro!(std::ptr::read(x)); }
45+
}
46+
47+
pub unsafe fn ok(x: *const usize) {
48+
#[allow(unused_unsafe)]
49+
unsafe {
50+
unsafe_macro!(std::ptr::read(x));
51+
unsafe_external_macro!(std::ptr::read(x));
52+
}
53+
}
54+
55+
#[allow(unused_unsafe)]
56+
pub unsafe fn unused_unsafe_disabled(x: *const usize) {
57+
unsafe { unsafe_macro!(std::ptr::read(x)); }
58+
unsafe { unsafe_external_macro!(std::ptr::read(x)); }
59+
}
60+
}
61+
62+
fn main() {
63+
unsafe {
64+
no_unsafe_op_in_unsafe_fn::ok(std::ptr::null());
65+
unsafe_op_in_unsafe_fn::bad(std::ptr::null());
66+
unsafe_op_in_unsafe_fn::ok(std::ptr::null());
67+
unsafe_op_in_unsafe_fn::unused_unsafe_disabled(std::ptr::null());
68+
}
69+
}

tests/ui/macros_hiding_unsafe_code.rs

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// run-rustfix
2+
// aux-build: macro_rules.rs
3+
4+
#![warn(clippy::macros_hiding_unsafe_code)]
5+
// compiletest allows unused code by default. But we want to explicitely test
6+
// for this lint here.
7+
#![warn(unused_unsafe)]
8+
9+
extern crate macro_rules;
10+
11+
use macro_rules::unsafe_external_macro;
12+
13+
macro_rules! unsafe_macro {
14+
($e:expr) => {
15+
unsafe { $e }
16+
};
17+
(NESTED $e:expr) => {
18+
unsafe { unsafe_function($e) }
19+
};
20+
}
21+
22+
unsafe fn unsafe_function(x: *const usize) {
23+
let _ = std::ptr::read(x);
24+
}
25+
26+
mod no_unsafe_op_in_unsafe_fn {
27+
use super::*;
28+
29+
#[allow(unused_unsafe)]
30+
pub unsafe fn ok(x: *const usize) {
31+
unsafe_macro!(std::ptr::read(x));
32+
unsafe_external_macro!(std::ptr::read(x));
33+
}
34+
}
35+
36+
mod unsafe_op_in_unsafe_fn {
37+
#![warn(unsafe_op_in_unsafe_fn)]
38+
39+
use super::*;
40+
41+
pub unsafe fn bad(x: *const usize) {
42+
unsafe_macro!(std::ptr::read(x));
43+
unsafe_macro!(NESTED std::ptr::read(&x));
44+
unsafe_external_macro!(std::ptr::read(x));
45+
}
46+
47+
pub unsafe fn ok(x: *const usize) {
48+
#[allow(unused_unsafe)]
49+
unsafe {
50+
unsafe_macro!(std::ptr::read(x));
51+
unsafe_external_macro!(std::ptr::read(x));
52+
}
53+
}
54+
55+
#[allow(unused_unsafe)]
56+
pub unsafe fn unused_unsafe_disabled(x: *const usize) {
57+
unsafe_macro!(std::ptr::read(x));
58+
unsafe_external_macro!(std::ptr::read(x));
59+
}
60+
}
61+
62+
fn main() {
63+
unsafe {
64+
no_unsafe_op_in_unsafe_fn::ok(std::ptr::null());
65+
unsafe_op_in_unsafe_fn::bad(std::ptr::null());
66+
unsafe_op_in_unsafe_fn::ok(std::ptr::null());
67+
unsafe_op_in_unsafe_fn::unused_unsafe_disabled(std::ptr::null());
68+
}
69+
}
+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
error: this macro call hides unsafe code
2+
--> $DIR/macros_hiding_unsafe_code.rs:42:9
3+
|
4+
LL | unsafe_macro!(std::ptr::read(x));
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: wrap it in an `unsafe` block: `#[allow(unused_unsafe)] unsafe { unsafe_macro!(std::ptr::read(x)); }`
6+
|
7+
= note: `-D clippy::macros-hiding-unsafe-code` implied by `-D warnings`
8+
9+
error: this macro call hides unsafe code
10+
--> $DIR/macros_hiding_unsafe_code.rs:43:9
11+
|
12+
LL | unsafe_macro!(NESTED std::ptr::read(&x));
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: wrap it in an `unsafe` block: `#[allow(unused_unsafe)] unsafe { unsafe_macro!(NESTED std::ptr::read(&x)); }`
14+
15+
error: this macro call hides unsafe code
16+
--> $DIR/macros_hiding_unsafe_code.rs:44:9
17+
|
18+
LL | unsafe_external_macro!(std::ptr::read(x));
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: wrap it in an `unsafe` block: `unsafe { unsafe_external_macro!(std::ptr::read(x)); }`
20+
21+
error: this macro call hides unsafe code
22+
--> $DIR/macros_hiding_unsafe_code.rs:57:9
23+
|
24+
LL | unsafe_macro!(std::ptr::read(x));
25+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: wrap it in an `unsafe` block: `unsafe { unsafe_macro!(std::ptr::read(x)); }`
26+
27+
error: this macro call hides unsafe code
28+
--> $DIR/macros_hiding_unsafe_code.rs:58:9
29+
|
30+
LL | unsafe_external_macro!(std::ptr::read(x));
31+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: wrap it in an `unsafe` block: `unsafe { unsafe_external_macro!(std::ptr::read(x)); }`
32+
33+
error: aborting due to 5 previous errors
34+

0 commit comments

Comments
 (0)