Skip to content

Commit a2c13c1

Browse files
authored
refactor(ptr): split each lint into a separate module (#16001)
changelog: none
2 parents f51c555 + ccd5130 commit a2c13c1

File tree

8 files changed

+488
-432
lines changed

8 files changed

+488
-432
lines changed

clippy_lints/src/ptr/cmp_null.rs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
use super::CMP_NULL;
2+
use clippy_utils::diagnostics::span_lint_and_sugg;
3+
use clippy_utils::res::{MaybeDef, MaybeResPath};
4+
use clippy_utils::sugg::Sugg;
5+
use clippy_utils::{is_lint_allowed, sym};
6+
use rustc_errors::Applicability;
7+
use rustc_hir::{BinOpKind, Expr, ExprKind};
8+
use rustc_lint::LateContext;
9+
10+
pub(super) fn check<'tcx>(
11+
cx: &LateContext<'tcx>,
12+
expr: &'tcx Expr<'_>,
13+
op: BinOpKind,
14+
l: &Expr<'_>,
15+
r: &Expr<'_>,
16+
) -> bool {
17+
let non_null_path_snippet = match (
18+
is_lint_allowed(cx, CMP_NULL, expr.hir_id),
19+
is_null_path(cx, l),
20+
is_null_path(cx, r),
21+
) {
22+
(false, true, false) if let Some(sugg) = Sugg::hir_opt(cx, r) => sugg.maybe_paren(),
23+
(false, false, true) if let Some(sugg) = Sugg::hir_opt(cx, l) => sugg.maybe_paren(),
24+
_ => return false,
25+
};
26+
let invert = if op == BinOpKind::Eq { "" } else { "!" };
27+
28+
span_lint_and_sugg(
29+
cx,
30+
CMP_NULL,
31+
expr.span,
32+
"comparing with null is better expressed by the `.is_null()` method",
33+
"try",
34+
format!("{invert}{non_null_path_snippet}.is_null()",),
35+
Applicability::MachineApplicable,
36+
);
37+
true
38+
}
39+
40+
fn is_null_path(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
41+
if let ExprKind::Call(pathexp, []) = expr.kind {
42+
matches!(
43+
pathexp.basic_res().opt_diag_name(cx),
44+
Some(sym::ptr_null | sym::ptr_null_mut)
45+
)
46+
} else {
47+
false
48+
}
49+
}

clippy_lints/src/ptr/mod.rs

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
1+
use rustc_hir::{BinOpKind, Body, Expr, ExprKind, ImplItemKind, ItemKind, Node, TraitFn, TraitItem, TraitItemKind};
2+
use rustc_lint::{LateContext, LateLintPass};
3+
use rustc_session::declare_lint_pass;
4+
5+
mod cmp_null;
6+
mod mut_from_ref;
7+
mod ptr_arg;
8+
mod ptr_eq;
9+
10+
declare_clippy_lint! {
11+
/// ### What it does
12+
/// This lint checks for function arguments of type `&String`, `&Vec`,
13+
/// `&PathBuf`, and `Cow<_>`. It will also suggest you replace `.clone()` calls
14+
/// with the appropriate `.to_owned()`/`to_string()` calls.
15+
///
16+
/// ### Why is this bad?
17+
/// Requiring the argument to be of the specific type
18+
/// makes the function less useful for no benefit; slices in the form of `&[T]`
19+
/// or `&str` usually suffice and can be obtained from other types, too.
20+
///
21+
/// ### Known problems
22+
/// There may be `fn(&Vec)`-typed references pointing to your function.
23+
/// If you have them, you will get a compiler error after applying this lint's
24+
/// suggestions. You then have the choice to undo your changes or change the
25+
/// type of the reference.
26+
///
27+
/// Note that if the function is part of your public interface, there may be
28+
/// other crates referencing it, of which you may not be aware. Carefully
29+
/// deprecate the function before applying the lint suggestions in this case.
30+
///
31+
/// ### Example
32+
/// ```ignore
33+
/// fn foo(&Vec<u32>) { .. }
34+
/// ```
35+
///
36+
/// Use instead:
37+
/// ```ignore
38+
/// fn foo(&[u32]) { .. }
39+
/// ```
40+
#[clippy::version = "pre 1.29.0"]
41+
pub PTR_ARG,
42+
style,
43+
"fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively"
44+
}
45+
46+
declare_clippy_lint! {
47+
/// ### What it does
48+
/// This lint checks for equality comparisons with `ptr::null`
49+
///
50+
/// ### Why is this bad?
51+
/// It's easier and more readable to use the inherent
52+
/// `.is_null()`
53+
/// method instead
54+
///
55+
/// ### Example
56+
/// ```rust,ignore
57+
/// use std::ptr;
58+
///
59+
/// if x == ptr::null {
60+
/// // ..
61+
/// }
62+
/// ```
63+
///
64+
/// Use instead:
65+
/// ```rust,ignore
66+
/// if x.is_null() {
67+
/// // ..
68+
/// }
69+
/// ```
70+
#[clippy::version = "pre 1.29.0"]
71+
pub CMP_NULL,
72+
style,
73+
"comparing a pointer to a null pointer, suggesting to use `.is_null()` instead"
74+
}
75+
76+
declare_clippy_lint! {
77+
/// ### What it does
78+
/// This lint checks for functions that take immutable references and return
79+
/// mutable ones. This will not trigger if no unsafe code exists as there
80+
/// are multiple safe functions which will do this transformation
81+
///
82+
/// To be on the conservative side, if there's at least one mutable
83+
/// reference with the output lifetime, this lint will not trigger.
84+
///
85+
/// ### Why is this bad?
86+
/// Creating a mutable reference which can be repeatably derived from an
87+
/// immutable reference is unsound as it allows creating multiple live
88+
/// mutable references to the same object.
89+
///
90+
/// This [error](https://github.com/rust-lang/rust/issues/39465) actually
91+
/// lead to an interim Rust release 1.15.1.
92+
///
93+
/// ### Known problems
94+
/// This pattern is used by memory allocators to allow allocating multiple
95+
/// objects while returning mutable references to each one. So long as
96+
/// different mutable references are returned each time such a function may
97+
/// be safe.
98+
///
99+
/// ### Example
100+
/// ```ignore
101+
/// fn foo(&Foo) -> &mut Bar { .. }
102+
/// ```
103+
#[clippy::version = "pre 1.29.0"]
104+
pub MUT_FROM_REF,
105+
correctness,
106+
"fns that create mutable refs from immutable ref args"
107+
}
108+
109+
declare_clippy_lint! {
110+
/// ### What it does
111+
/// Use `std::ptr::eq` when applicable
112+
///
113+
/// ### Why is this bad?
114+
/// `ptr::eq` can be used to compare `&T` references
115+
/// (which coerce to `*const T` implicitly) by their address rather than
116+
/// comparing the values they point to.
117+
///
118+
/// ### Example
119+
/// ```no_run
120+
/// let a = &[1, 2, 3];
121+
/// let b = &[1, 2, 3];
122+
///
123+
/// assert!(a as *const _ as usize == b as *const _ as usize);
124+
/// ```
125+
/// Use instead:
126+
/// ```no_run
127+
/// let a = &[1, 2, 3];
128+
/// let b = &[1, 2, 3];
129+
///
130+
/// assert!(std::ptr::eq(a, b));
131+
/// ```
132+
#[clippy::version = "1.49.0"]
133+
pub PTR_EQ,
134+
style,
135+
"use `std::ptr::eq` when comparing raw pointers"
136+
}
137+
138+
declare_lint_pass!(Ptr => [PTR_ARG, CMP_NULL, MUT_FROM_REF, PTR_EQ]);
139+
140+
impl<'tcx> LateLintPass<'tcx> for Ptr {
141+
fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) {
142+
if let TraitItemKind::Fn(sig, trait_method) = &item.kind {
143+
if matches!(trait_method, TraitFn::Provided(_)) {
144+
// Handled by `check_body`.
145+
return;
146+
}
147+
148+
mut_from_ref::check(cx, sig, None);
149+
ptr_arg::check_trait_item(cx, item.owner_id, sig);
150+
}
151+
}
152+
153+
fn check_body(&mut self, cx: &LateContext<'tcx>, body: &Body<'tcx>) {
154+
let mut parents = cx.tcx.hir_parent_iter(body.value.hir_id);
155+
let (item_id, sig, is_trait_item) = match parents.next() {
156+
Some((_, Node::Item(i))) => {
157+
if let ItemKind::Fn { sig, .. } = &i.kind {
158+
(i.owner_id, sig, false)
159+
} else {
160+
return;
161+
}
162+
},
163+
Some((_, Node::ImplItem(i))) => {
164+
if !matches!(parents.next(),
165+
Some((_, Node::Item(i))) if matches!(&i.kind, ItemKind::Impl(i) if i.of_trait.is_none())
166+
) {
167+
return;
168+
}
169+
if let ImplItemKind::Fn(sig, _) = &i.kind {
170+
(i.owner_id, sig, false)
171+
} else {
172+
return;
173+
}
174+
},
175+
Some((_, Node::TraitItem(i))) => {
176+
if let TraitItemKind::Fn(sig, _) = &i.kind {
177+
(i.owner_id, sig, true)
178+
} else {
179+
return;
180+
}
181+
},
182+
_ => return,
183+
};
184+
185+
mut_from_ref::check(cx, sig, Some(body));
186+
ptr_arg::check_body(cx, body, item_id, sig, is_trait_item);
187+
}
188+
189+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
190+
if let ExprKind::Binary(op, l, r) = expr.kind
191+
&& (op.node == BinOpKind::Eq || op.node == BinOpKind::Ne)
192+
{
193+
#[expect(
194+
clippy::collapsible_if,
195+
reason = "the outer `if`s check the HIR, the inner ones run lints"
196+
)]
197+
if !cmp_null::check(cx, expr, op.node, l, r) {
198+
ptr_eq::check(cx, op.node, l, r, expr.span);
199+
}
200+
}
201+
}
202+
}
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
use super::MUT_FROM_REF;
2+
use clippy_utils::diagnostics::span_lint_and_then;
3+
use clippy_utils::visitors::contains_unsafe_block;
4+
use rustc_errors::MultiSpan;
5+
use rustc_hir::intravisit::Visitor;
6+
use rustc_hir::{self as hir, Body, FnRetTy, FnSig, GenericArg, Lifetime, Mutability, TyKind};
7+
use rustc_lint::LateContext;
8+
use rustc_span::Span;
9+
10+
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, sig: &FnSig<'_>, body: Option<&Body<'tcx>>) {
11+
let FnRetTy::Return(ty) = sig.decl.output else { return };
12+
for (out, mutability, out_span) in get_lifetimes(ty) {
13+
if mutability != Some(Mutability::Mut) {
14+
continue;
15+
}
16+
let out_region = cx.tcx.named_bound_var(out.hir_id);
17+
// `None` if one of the types contains `&'a mut T` or `T<'a>`.
18+
// Else, contains all the locations of `&'a T` types.
19+
let args_immut_refs: Option<Vec<Span>> = sig
20+
.decl
21+
.inputs
22+
.iter()
23+
.flat_map(get_lifetimes)
24+
.filter(|&(lt, _, _)| cx.tcx.named_bound_var(lt.hir_id) == out_region)
25+
.map(|(_, mutability, span)| (mutability == Some(Mutability::Not)).then_some(span))
26+
.collect();
27+
if let Some(args_immut_refs) = args_immut_refs
28+
&& !args_immut_refs.is_empty()
29+
&& body.is_none_or(|body| sig.header.is_unsafe() || contains_unsafe_block(cx, body.value))
30+
{
31+
span_lint_and_then(
32+
cx,
33+
MUT_FROM_REF,
34+
out_span,
35+
"mutable borrow from immutable input(s)",
36+
|diag| {
37+
let ms = MultiSpan::from_spans(args_immut_refs);
38+
diag.span_note(ms, "immutable borrow here");
39+
},
40+
);
41+
}
42+
}
43+
}
44+
45+
struct LifetimeVisitor<'tcx> {
46+
result: Vec<(&'tcx Lifetime, Option<Mutability>, Span)>,
47+
}
48+
49+
impl<'tcx> Visitor<'tcx> for LifetimeVisitor<'tcx> {
50+
fn visit_ty(&mut self, ty: &'tcx hir::Ty<'tcx, hir::AmbigArg>) {
51+
if let TyKind::Ref(lt, ref m) = ty.kind {
52+
self.result.push((lt, Some(m.mutbl), ty.span));
53+
}
54+
hir::intravisit::walk_ty(self, ty);
55+
}
56+
57+
fn visit_generic_arg(&mut self, generic_arg: &'tcx GenericArg<'tcx>) {
58+
if let GenericArg::Lifetime(lt) = generic_arg {
59+
self.result.push((lt, None, generic_arg.span()));
60+
}
61+
hir::intravisit::walk_generic_arg(self, generic_arg);
62+
}
63+
}
64+
65+
/// Visit `ty` and collect the all the lifetimes appearing in it, implicit or not.
66+
///
67+
/// The second field of the vector's elements indicate if the lifetime is attached to a
68+
/// shared reference, a mutable reference, or neither.
69+
fn get_lifetimes<'tcx>(ty: &'tcx hir::Ty<'tcx>) -> Vec<(&'tcx Lifetime, Option<Mutability>, Span)> {
70+
use hir::intravisit::VisitorExt as _;
71+
72+
let mut visitor = LifetimeVisitor { result: Vec::new() };
73+
visitor.visit_ty_unambig(ty);
74+
visitor.result
75+
}

0 commit comments

Comments
 (0)