Skip to content

Commit 404a6de

Browse files
JarchoJarcho
committed
Add lint manual_inspect 
1 parent a8c5746 commit 404a6de

12 files changed

+844
-3
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -5462,6 +5462,7 @@ Released 2018-09-13
54625462
[`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map
54635463
[`manual_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten
54645464
[`manual_hash_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_hash_one
5465+
[`manual_inspect`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_inspect
54655466
[`manual_instant_elapsed`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_instant_elapsed
54665467
[`manual_is_ascii_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_ascii_check
54675468
[`manual_is_finite`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_finite

‎clippy_config/src/msrvs.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ macro_rules! msrv_aliases {
1818
// names may refer to stabilized feature flags or library items
1919
msrv_aliases! {
2020
1,77,0 { C_STR_LITERALS }
21-
1,76,0 { PTR_FROM_REF }
21+
1,76,0 { PTR_FROM_REF, OPTION_RESULT_INSPECT }
2222
1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE }
2323
1,70,0 { OPTION_RESULT_IS_VARIANT_AND, BINARY_HEAP_RETAIN }
2424
1,68,0 { PATH_MAIN_SEPARATOR_STR }

‎clippy_lints/src/declared_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
402402
crate::methods::MANUAL_C_STR_LITERALS_INFO,
403403
crate::methods::MANUAL_FILTER_MAP_INFO,
404404
crate::methods::MANUAL_FIND_MAP_INFO,
405+
crate::methods::MANUAL_INSPECT_INFO,
405406
crate::methods::MANUAL_IS_VARIANT_AND_INFO,
406407
crate::methods::MANUAL_NEXT_BACK_INFO,
407408
crate::methods::MANUAL_OK_OR_INFO,

‎clippy_lints/src/methods/manual_inspect.rs

+233
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,233 @@
1+
use clippy_config::msrvs::{self, Msrv};
2+
use clippy_utils::diagnostics::span_lint_and_then;
3+
use clippy_utils::source::{get_source_text, with_leading_whitespace, SpanRange};
4+
use clippy_utils::ty::get_field_by_name;
5+
use clippy_utils::visitors::{for_each_expr, for_each_expr_with_closures};
6+
use clippy_utils::{expr_use_ctxt, is_diag_item_method, is_diag_trait_item, path_to_local_id, ExprUseNode};
7+
use core::ops::ControlFlow;
8+
use rustc_errors::Applicability;
9+
use rustc_hir::{BindingMode, BorrowKind, ByRef, ClosureKind, Expr, ExprKind, Mutability, Node, PatKind};
10+
use rustc_lint::LateContext;
11+
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
12+
use rustc_span::{sym, BytePos, Span, Symbol, DUMMY_SP};
13+
14+
use super::MANUAL_INSPECT;
15+
16+
#[expect(clippy::too_many_lines)]
17+
pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name: &str, name_span: Span, msrv: &Msrv) {
18+
if let ExprKind::Closure(c) = arg.kind
19+
&& matches!(c.kind, ClosureKind::Closure)
20+
&& let typeck = cx.typeck_results()
21+
&& let Some(fn_id) = typeck.type_dependent_def_id(expr.hir_id)
22+
&& (is_diag_trait_item(cx, fn_id, sym::Iterator)
23+
|| (msrv.meets(msrvs::OPTION_RESULT_INSPECT)
24+
&& (is_diag_item_method(cx, fn_id, sym::Option) || is_diag_item_method(cx, fn_id, sym::Result))))
25+
&& let body = cx.tcx.hir().body(c.body)
26+
&& let [param] = body.params
27+
&& let PatKind::Binding(BindingMode(ByRef::No, Mutability::Not), arg_id, _, None) = param.pat.kind
28+
&& let arg_ty = typeck.node_type(arg_id)
29+
&& let ExprKind::Block(block, _) = body.value.kind
30+
&& let Some(final_expr) = block.expr
31+
&& !block.stmts.is_empty()
32+
&& path_to_local_id(final_expr, arg_id)
33+
&& typeck.expr_adjustments(final_expr).is_empty()
34+
{
35+
let mut requires_copy = false;
36+
let mut requires_deref = false;
37+
38+
// The number of unprocessed return expressions.
39+
let mut ret_count = 0u32;
40+
41+
// The uses for which processing is delayed until after the visitor.
42+
let mut delayed = vec![];
43+
44+
let ctxt = arg.span.ctxt();
45+
let can_lint = for_each_expr(block.stmts, |e| {
46+
if let ExprKind::Closure(c) = e.kind {
47+
// Nested closures don't need to treat returns specially.
48+
let _: Option<!> = for_each_expr_with_closures(cx, cx.tcx.hir().body(c.body).value, |e| {
49+
if path_to_local_id(e, arg_id) {
50+
let (kind, same_ctxt) = check_use(cx, e);
51+
match (kind, same_ctxt && e.span.ctxt() == ctxt) {
52+
(_, false) | (UseKind::Deref | UseKind::Return(..), true) => {
53+
requires_copy = true;
54+
requires_deref = true;
55+
},
56+
(UseKind::AutoBorrowed, true) => {},
57+
(UseKind::WillAutoDeref, true) => {
58+
requires_copy = true;
59+
},
60+
(kind, true) => delayed.push(kind),
61+
}
62+
}
63+
ControlFlow::Continue(())
64+
});
65+
} else if matches!(e.kind, ExprKind::Ret(_)) {
66+
ret_count += 1;
67+
} else if path_to_local_id(e, arg_id) {
68+
let (kind, same_ctxt) = check_use(cx, e);
69+
match (kind, same_ctxt && e.span.ctxt() == ctxt) {
70+
(UseKind::Return(..), false) => {
71+
return ControlFlow::Break(());
72+
},
73+
(_, false) | (UseKind::Deref, true) => {
74+
requires_copy = true;
75+
requires_deref = true;
76+
},
77+
(UseKind::AutoBorrowed, true) => {},
78+
(UseKind::WillAutoDeref, true) => {
79+
requires_copy = true;
80+
},
81+
(kind @ UseKind::Return(_), true) => {
82+
ret_count -= 1;
83+
delayed.push(kind);
84+
},
85+
(kind, true) => delayed.push(kind),
86+
}
87+
}
88+
ControlFlow::Continue(())
89+
})
90+
.is_none();
91+
92+
if ret_count != 0 {
93+
// A return expression that didn't return the original value was found.
94+
return;
95+
}
96+
97+
let mut edits = Vec::with_capacity(delayed.len() + 3);
98+
let mut addr_of_edits = Vec::with_capacity(delayed.len());
99+
for x in delayed {
100+
match x {
101+
UseKind::Return(s) => edits.push((with_leading_whitespace(cx, s).set_span_pos(s), String::new())),
102+
UseKind::Borrowed(s) => {
103+
if let Some(src) = get_source_text(cx, s)
104+
&& let Some(src) = src.as_str()
105+
&& let trim_src = src.trim_start_matches(|x| matches!(x, ' ' | '\t' | '\n' | '\r' | '('))
106+
&& trim_src.starts_with('&')
107+
{
108+
let range = s.into_range();
109+
#[expect(clippy::cast_possible_truncation)]
110+
let start = BytePos(range.start.0 + (src.len() - trim_src.len()) as u32);
111+
addr_of_edits.push(((start..BytePos(start.0 + 1)).set_span_pos(s), String::new()));
112+
} else {
113+
requires_copy = true;
114+
requires_deref = true;
115+
}
116+
},
117+
UseKind::FieldAccess(name, e) => {
118+
let Some(mut ty) = get_field_by_name(cx.tcx, arg_ty.peel_refs(), name) else {
119+
requires_copy = true;
120+
continue;
121+
};
122+
let mut prev_expr = e;
123+
124+
for (_, parent) in cx.tcx.hir().parent_iter(e.hir_id) {
125+
if let Node::Expr(e) = parent {
126+
match e.kind {
127+
ExprKind::Field(_, name)
128+
if let Some(fty) = get_field_by_name(cx.tcx, ty.peel_refs(), name.name) =>
129+
{
130+
ty = fty;
131+
prev_expr = e;
132+
continue;
133+
},
134+
ExprKind::AddrOf(BorrowKind::Ref, ..) => break,
135+
_ if matches!(
136+
typeck.expr_adjustments(prev_expr).first(),
137+
Some(Adjustment {
138+
kind: Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Not))
139+
| Adjust::Deref(_),
140+
..
141+
})
142+
) =>
143+
{
144+
break;
145+
},
146+
_ => {},
147+
}
148+
}
149+
requires_copy |= !ty.is_copy_modulo_regions(cx.tcx, cx.param_env);
150+
break;
151+
}
152+
},
153+
// Already processed uses.
154+
UseKind::AutoBorrowed | UseKind::WillAutoDeref | UseKind::Deref => {},
155+
}
156+
}
157+
158+
if can_lint
159+
&& (!requires_copy || arg_ty.is_copy_modulo_regions(cx.tcx, cx.param_env))
160+
// This case could be handled, but a fair bit of care would need to be taken.
161+
&& (!requires_deref || arg_ty.is_freeze(cx.tcx, cx.param_env))
162+
{
163+
if requires_deref {
164+
edits.push((param.span.shrink_to_lo(), "&".into()));
165+
} else {
166+
edits.extend(addr_of_edits);
167+
}
168+
edits.push((
169+
name_span,
170+
String::from(match name {
171+
"map" => "inspect",
172+
"map_err" => "inspect_err",
173+
_ => return,
174+
}),
175+
));
176+
edits.push((
177+
with_leading_whitespace(cx, final_expr.span).set_span_pos(final_expr.span),
178+
String::new(),
179+
));
180+
let app = if edits.iter().any(|(s, _)| s.from_expansion()) {
181+
Applicability::MaybeIncorrect
182+
} else {
183+
Applicability::MachineApplicable
184+
};
185+
span_lint_and_then(cx, MANUAL_INSPECT, name_span, "", |diag| {
186+
diag.multipart_suggestion("try", edits, app);
187+
});
188+
}
189+
}
190+
}
191+
192+
enum UseKind<'tcx> {
193+
AutoBorrowed,
194+
WillAutoDeref,
195+
Deref,
196+
Return(Span),
197+
Borrowed(Span),
198+
FieldAccess(Symbol, &'tcx Expr<'tcx>),
199+
}
200+
201+
/// Checks how the value is used, and whether it was used in the same `SyntaxContext`.
202+
fn check_use<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (UseKind<'tcx>, bool) {
203+
let use_cx = expr_use_ctxt(cx, e);
204+
if use_cx
205+
.adjustments
206+
.first()
207+
.is_some_and(|a| matches!(a.kind, Adjust::Deref(_)))
208+
{
209+
return (UseKind::AutoBorrowed, use_cx.same_ctxt);
210+
}
211+
let res = match use_cx.use_node(cx) {
212+
ExprUseNode::Return(_) => {
213+
if let ExprKind::Ret(Some(e)) = use_cx.node.expect_expr().kind {
214+
UseKind::Return(e.span)
215+
} else {
216+
return (UseKind::Return(DUMMY_SP), false);
217+
}
218+
},
219+
ExprUseNode::FieldAccess(name) => UseKind::FieldAccess(name.name, use_cx.node.expect_expr()),
220+
ExprUseNode::Callee | ExprUseNode::MethodArg(_, _, 0)
221+
if use_cx
222+
.adjustments
223+
.first()
224+
.is_some_and(|a| matches!(a.kind, Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Not)))) =>
225+
{
226+
UseKind::AutoBorrowed
227+
},
228+
ExprUseNode::Callee | ExprUseNode::MethodArg(_, _, 0) => UseKind::WillAutoDeref,
229+
ExprUseNode::AddrOf(BorrowKind::Ref, _) => UseKind::Borrowed(use_cx.node.expect_expr().span),
230+
_ => UseKind::Deref,
231+
};
232+
(res, use_cx.same_ctxt)
233+
}

‎clippy_lints/src/methods/mod.rs

+24
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ mod iter_with_drain;
5353
mod iterator_step_by_zero;
5454
mod join_absolute_paths;
5555
mod manual_c_str_literals;
56+
mod manual_inspect;
5657
mod manual_is_variant_and;
5758
mod manual_next_back;
5859
mod manual_ok_or;
@@ -4112,6 +4113,27 @@ declare_clippy_lint! {
41124113
"is_ascii() called on a char iterator"
41134114
}
41144115

4116+
declare_clippy_lint! {
4117+
/// ### What it does
4118+
/// Checks for uses of `map` which return the original item.
4119+
///
4120+
/// ### Why is this bad?
4121+
/// `inspect` is both clearer in intent and shorter.
4122+
///
4123+
/// ### Example
4124+
/// ```no_run
4125+
/// let x = Some(0).map(|x| { println!("{x}"); x });
4126+
/// ```
4127+
/// Use instead:
4128+
/// ```no_run
4129+
/// let x = Some(0).inspect(|x| println!("{x}"));
4130+
/// ```
4131+
#[clippy::version = "1.78.0"]
4132+
pub MANUAL_INSPECT,
4133+
complexity,
4134+
"use of `map` returning the original item"
4135+
}
4136+
41154137
pub struct Methods {
41164138
avoid_breaking_exported_api: bool,
41174139
msrv: Msrv,
@@ -4278,6 +4300,7 @@ impl_lint_pass!(Methods => [
42784300
MANUAL_C_STR_LITERALS,
42794301
UNNECESSARY_GET_THEN_CHECK,
42804302
NEEDLESS_CHARACTER_ITERATION,
4303+
MANUAL_INSPECT,
42814304
]);
42824305

42834306
/// Extracts a method call name, args, and `Span` of the method name.
@@ -4782,6 +4805,7 @@ impl Methods {
47824805
}
47834806
}
47844807
map_identity::check(cx, expr, recv, m_arg, name, span);
4808+
manual_inspect::check(cx, expr, m_arg, name, span, &self.msrv);
47854809
},
47864810
("map_or", [def, map]) => {
47874811
option_map_or_none::check(cx, expr, recv, def, map);

‎clippy_utils/src/source.rs

+26-1
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,17 @@ use std::borrow::Cow;
1313
use std::ops::Range;
1414

1515
/// A type which can be converted to the range portion of a `Span`.
16-
pub trait SpanRange {
16+
pub trait SpanRange: Sized {
1717
fn into_range(self) -> Range<BytePos>;
18+
fn set_span_pos(self, sp: Span) -> Span {
19+
let range = self.into_range();
20+
SpanData {
21+
lo: range.start,
22+
hi: range.end,
23+
..sp.data()
24+
}
25+
.span()
26+
}
1827
}
1928
impl SpanRange for Span {
2029
fn into_range(self) -> Range<BytePos> {
@@ -60,6 +69,22 @@ pub fn get_source_text(cx: &impl LintContext, sp: impl SpanRange) -> Option<Sour
6069
f(cx.sess().source_map(), sp.into_range())
6170
}
6271

72+
pub fn with_leading_whitespace(cx: &impl LintContext, sp: impl SpanRange) -> Range<BytePos> {
73+
#[expect(clippy::needless_pass_by_value, clippy::cast_possible_truncation)]
74+
fn f(src: SourceFileRange, sp: Range<BytePos>) -> Range<BytePos> {
75+
let Some(text) = &src.sf.src else {
76+
return sp;
77+
};
78+
let len = src.range.start - text[..src.range.start].trim_end().len();
79+
BytePos(sp.start.0 - len as u32)..sp.end
80+
}
81+
let sp = sp.into_range();
82+
match get_source_text(cx, sp.clone()) {
83+
Some(src) => f(src, sp),
84+
None => sp,
85+
}
86+
}
87+
6388
/// Like `snippet_block`, but add braces if the expr is not an `ExprKind::Block`.
6489
pub fn expr_block<T: LintContext>(
6590
cx: &T,

‎clippy_utils/src/ty.rs

+14
Original file line numberDiff line numberDiff line change
@@ -1364,3 +1364,17 @@ pub fn get_adt_inherent_method<'a>(cx: &'a LateContext<'_>, ty: Ty<'_>, method_n
13641364
None
13651365
}
13661366
}
1367+
1368+
/// Get's the type of a field by name.
1369+
pub fn get_field_by_name<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, name: Symbol) -> Option<Ty<'tcx>> {
1370+
match *ty.kind() {
1371+
ty::Adt(def, args) if def.is_union() || def.is_struct() => def
1372+
.non_enum_variant()
1373+
.fields
1374+
.iter()
1375+
.find(|f| f.name == name)
1376+
.map(|f| f.ty(tcx, args)),
1377+
ty::Tuple(args) => name.as_str().parse::<usize>().ok().and_then(|i| args.get(i).copied()),
1378+
_ => None,
1379+
}
1380+
}

‎tests/ui/copy_iterator.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#![warn(clippy::copy_iterator)]
2+
#![allow(clippy::manual_inspect)]
23

34
#[derive(Copy, Clone)]
45
struct Countdown(u8);

‎tests/ui/copy_iterator.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: you are implementing `Iterator` on a `Copy` type
2-
--> tests/ui/copy_iterator.rs:6:1
2+
--> tests/ui/copy_iterator.rs:7:1
33
|
44
LL | / impl Iterator for Countdown {
55
LL | |

0 commit comments

Comments
 (0)