From a002f93e51e66204e81779d7d449a50e30571c6c Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 12 Feb 2024 18:33:10 -0500 Subject: [PATCH 1/2] `expr_use_ctxt` changes: * Delay the parsing of the use node * Mark when the `SyntaxContext` changes rather than return `None` * Return a default value if the HIR tree is broken rather than `None` --- clippy_lints/src/casts/ref_as_ptr.rs | 4 +- clippy_lints/src/dereference.rs | 46 +++-- .../src/needless_borrows_for_generic_args.rs | 8 +- clippy_utils/src/lib.rs | 164 ++++++++++-------- 4 files changed, 120 insertions(+), 102 deletions(-) diff --git a/clippy_lints/src/casts/ref_as_ptr.rs b/clippy_lints/src/casts/ref_as_ptr.rs index f42bafce4dd..5f48b8bd206 100644 --- a/clippy_lints/src/casts/ref_as_ptr.rs +++ b/clippy_lints/src/casts/ref_as_ptr.rs @@ -22,9 +22,9 @@ pub(super) fn check<'tcx>( if matches!(cast_from.kind(), ty::Ref(..)) && let ty::RawPtr(_, to_mutbl) = cast_to.kind() - && let Some(use_cx) = expr_use_ctxt(cx, expr) + && let use_cx = expr_use_ctxt(cx, expr) // TODO: only block the lint if `cast_expr` is a temporary - && !matches!(use_cx.node, ExprUseNode::LetStmt(_) | ExprUseNode::ConstStatic(_)) + && !matches!(use_cx.use_node(cx), ExprUseNode::LetStmt(_) | ExprUseNode::ConstStatic(_)) { let core_or_std = if is_no_std_crate(cx) { "core" } else { "std" }; let fn_name = match to_mutbl { diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index d60320d8282..3c137b6b70f 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -260,18 +260,13 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { (None, kind) => { let expr_ty = typeck.expr_ty(expr); let use_cx = expr_use_ctxt(cx, expr); - let adjusted_ty = match &use_cx { - Some(use_cx) => match use_cx.adjustments { - [.., a] => a.target, - _ => expr_ty, - }, - _ => typeck.expr_ty_adjusted(expr), - }; + let adjusted_ty = use_cx.adjustments.last().map_or(expr_ty, |a| a.target); - match (use_cx, kind) { - (Some(use_cx), RefOp::Deref) => { + match kind { + RefOp::Deref if use_cx.same_ctxt => { + let use_node = use_cx.use_node(cx); let sub_ty = typeck.expr_ty(sub_expr); - if let ExprUseNode::FieldAccess(name) = use_cx.node + if let ExprUseNode::FieldAccess(name) = use_node && !use_cx.moved_before_use && !ty_contains_field(sub_ty, name.name) { @@ -288,9 +283,9 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { } else if sub_ty.is_ref() // Linting method receivers would require verifying that name lookup // would resolve the same way. This is complicated by trait methods. - && !use_cx.node.is_recv() - && let Some(ty) = use_cx.node.defined_ty(cx) - && TyCoercionStability::for_defined_ty(cx, ty, use_cx.node.is_return()).is_deref_stable() + && !use_node.is_recv() + && let Some(ty) = use_node.defined_ty(cx) + && TyCoercionStability::for_defined_ty(cx, ty, use_node.is_return()).is_deref_stable() { self.state = Some(( State::ExplicitDeref { mutability: None }, @@ -301,7 +296,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { )); } }, - (_, RefOp::Method { mutbl, is_ufcs }) + RefOp::Method { mutbl, is_ufcs } if !is_lint_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id) // Allow explicit deref in method chains. e.g. `foo.deref().bar()` && (is_ufcs || !in_postfix_position(cx, expr)) => @@ -319,7 +314,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }, )); }, - (Some(use_cx), RefOp::AddrOf(mutability)) => { + RefOp::AddrOf(mutability) if use_cx.same_ctxt => { // Find the number of times the borrow is auto-derefed. let mut iter = use_cx.adjustments.iter(); let mut deref_count = 0usize; @@ -338,10 +333,11 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }; }; - let stability = use_cx.node.defined_ty(cx).map_or(TyCoercionStability::None, |ty| { - TyCoercionStability::for_defined_ty(cx, ty, use_cx.node.is_return()) + let use_node = use_cx.use_node(cx); + let stability = use_node.defined_ty(cx).map_or(TyCoercionStability::None, |ty| { + TyCoercionStability::for_defined_ty(cx, ty, use_node.is_return()) }); - let can_auto_borrow = match use_cx.node { + let can_auto_borrow = match use_node { ExprUseNode::FieldAccess(_) if !use_cx.moved_before_use && matches!(sub_expr.kind, ExprKind::Field(..)) => { @@ -353,7 +349,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { // deref through `ManuallyDrop<_>` will not compile. !adjust_derefs_manually_drop(use_cx.adjustments, expr_ty) }, - ExprUseNode::Callee | ExprUseNode::FieldAccess(_) => true, + ExprUseNode::Callee | ExprUseNode::FieldAccess(_) if !use_cx.moved_before_use => true, ExprUseNode::MethodArg(hir_id, _, 0) if !use_cx.moved_before_use => { // Check for calls to trait methods where the trait is implemented // on a reference. @@ -363,9 +359,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { // priority. if let Some(fn_id) = typeck.type_dependent_def_id(hir_id) && let Some(trait_id) = cx.tcx.trait_of_item(fn_id) - && let arg_ty = cx - .tcx - .erase_regions(use_cx.adjustments.last().map_or(expr_ty, |a| a.target)) + && let arg_ty = cx.tcx.erase_regions(adjusted_ty) && let ty::Ref(_, sub_ty, _) = *arg_ty.kind() && let args = typeck.node_args_opt(hir_id).map(|args| &args[1..]).unwrap_or_default() @@ -443,7 +437,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { count: deref_count - required_refs, msg, stability, - for_field_access: if let ExprUseNode::FieldAccess(name) = use_cx.node + for_field_access: if let ExprUseNode::FieldAccess(name) = use_node && !use_cx.moved_before_use { Some(name.name) @@ -453,7 +447,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }), StateData { first_expr: expr, - adjusted_ty: use_cx.adjustments.last().map_or(expr_ty, |a| a.target), + adjusted_ty, }, )); } else if stability.is_deref_stable() @@ -465,12 +459,12 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { State::Borrow { mutability }, StateData { first_expr: expr, - adjusted_ty: use_cx.adjustments.last().map_or(expr_ty, |a| a.target), + adjusted_ty, }, )); } }, - (None, _) | (_, RefOp::Method { .. }) => (), + _ => {}, } }, ( diff --git a/clippy_lints/src/needless_borrows_for_generic_args.rs b/clippy_lints/src/needless_borrows_for_generic_args.rs index 4f99eaa40c2..9c3ead855a7 100644 --- a/clippy_lints/src/needless_borrows_for_generic_args.rs +++ b/clippy_lints/src/needless_borrows_for_generic_args.rs @@ -80,11 +80,13 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowsForGenericArgs<'tcx> { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if matches!(expr.kind, ExprKind::AddrOf(..)) && !expr.span.from_expansion() - && let Some(use_cx) = expr_use_ctxt(cx, expr) + && let use_cx = expr_use_ctxt(cx, expr) + && use_cx.same_ctxt && !use_cx.is_ty_unified - && let Some(DefinedTy::Mir(ty)) = use_cx.node.defined_ty(cx) + && let use_node = use_cx.use_node(cx) + && let Some(DefinedTy::Mir(ty)) = use_node.defined_ty(cx) && let ty::Param(ty) = *ty.value.skip_binder().kind() - && let Some((hir_id, fn_id, i)) = match use_cx.node { + && let Some((hir_id, fn_id, i)) = match use_node { ExprUseNode::MethodArg(_, _, 0) => None, ExprUseNode::MethodArg(hir_id, None, i) => cx .typeck_results() diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 7dc341ec8d7..fda870f435a 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1,6 +1,7 @@ #![feature(array_chunks)] #![feature(box_patterns)] #![feature(control_flow_enum)] +#![feature(exhaustive_patterns)] #![feature(if_let_guard)] #![feature(let_chains)] #![feature(lint_reasons)] @@ -2664,13 +2665,80 @@ pub enum DefinedTy<'tcx> { /// The context an expressions value is used in. pub struct ExprUseCtxt<'tcx> { /// The parent node which consumes the value. - pub node: ExprUseNode<'tcx>, + pub node: Node<'tcx>, + /// The child id of the node the value came from. + pub child_id: HirId, /// Any adjustments applied to the type. pub adjustments: &'tcx [Adjustment<'tcx>], - /// Whether or not the type must unify with another code path. + /// Whether the type must unify with another code path. pub is_ty_unified: bool, - /// Whether or not the value will be moved before it's used. + /// Whether the value will be moved before it's used. pub moved_before_use: bool, + /// Whether the use site has the same `SyntaxContext` as the value. + pub same_ctxt: bool, +} +impl<'tcx> ExprUseCtxt<'tcx> { + pub fn use_node(&self, cx: &LateContext<'tcx>) -> ExprUseNode<'tcx> { + match self.node { + Node::LetStmt(l) => ExprUseNode::LetStmt(l), + Node::ExprField(field) => ExprUseNode::Field(field), + + Node::Item(&Item { + kind: ItemKind::Static(..) | ItemKind::Const(..), + owner_id, + .. + }) + | Node::TraitItem(&TraitItem { + kind: TraitItemKind::Const(..), + owner_id, + .. + }) + | Node::ImplItem(&ImplItem { + kind: ImplItemKind::Const(..), + owner_id, + .. + }) => ExprUseNode::ConstStatic(owner_id), + + Node::Item(&Item { + kind: ItemKind::Fn(..), + owner_id, + .. + }) + | Node::TraitItem(&TraitItem { + kind: TraitItemKind::Fn(..), + owner_id, + .. + }) + | Node::ImplItem(&ImplItem { + kind: ImplItemKind::Fn(..), + owner_id, + .. + }) => ExprUseNode::Return(owner_id), + + Node::Expr(use_expr) => match use_expr.kind { + ExprKind::Ret(_) => ExprUseNode::Return(OwnerId { + def_id: cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap()), + }), + + ExprKind::Closure(closure) => ExprUseNode::Return(OwnerId { def_id: closure.def_id }), + ExprKind::Call(func, args) => match args.iter().position(|arg| arg.hir_id == self.child_id) { + Some(i) => ExprUseNode::FnArg(func, i), + None => ExprUseNode::Callee, + }, + ExprKind::MethodCall(name, _, args, _) => ExprUseNode::MethodArg( + use_expr.hir_id, + name.args, + args.iter() + .position(|arg| arg.hir_id == self.child_id) + .map_or(0, |i| i + 1), + ), + ExprKind::Field(_, name) => ExprUseNode::FieldAccess(name), + ExprKind::AddrOf(kind, mutbl, _) => ExprUseNode::AddrOf(kind, mutbl), + _ => ExprUseNode::Other, + }, + _ => ExprUseNode::Other, + } + } } /// The node which consumes a value. @@ -2691,7 +2759,8 @@ pub enum ExprUseNode<'tcx> { Callee, /// Access of a field. FieldAccess(Ident), - Expr, + /// Borrow expression. + AddrOf(ast::BorrowKind, Mutability), Other, } impl<'tcx> ExprUseNode<'tcx> { @@ -2768,26 +2837,25 @@ pub fn defined_ty(&self, cx: &LateContext<'tcx>) -> Option> { let sig = cx.tcx.fn_sig(id).skip_binder(); Some(DefinedTy::Mir(cx.tcx.param_env(id).and(sig.input(i)))) }, - Self::LetStmt(_) | Self::FieldAccess(..) | Self::Callee | Self::Expr | Self::Other => None, + Self::LetStmt(_) | Self::FieldAccess(..) | Self::Callee | Self::Other | Self::AddrOf(..) => None, } } } /// Gets the context an expression's value is used in. -pub fn expr_use_ctxt<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> Option> { +pub fn expr_use_ctxt<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> ExprUseCtxt<'tcx> { let mut adjustments = [].as_slice(); let mut is_ty_unified = false; let mut moved_before_use = false; + let mut same_ctxt = true; let ctxt = e.span.ctxt(); - walk_to_expr_usage(cx, e, &mut |parent_id, parent, child_id| { + let node = walk_to_expr_usage(cx, e, &mut |parent_id, parent, child_id| -> ControlFlow { if adjustments.is_empty() && let Node::Expr(e) = cx.tcx.hir_node(child_id) { adjustments = cx.typeck_results().expr_adjustments(e); } - if cx.tcx.hir().span(parent_id).ctxt() != ctxt { - return ControlFlow::Break(()); - } + same_ctxt &= cx.tcx.hir().span(parent_id).ctxt() == ctxt; if let Node::Expr(e) = parent { match e.kind { ExprKind::If(e, _, _) | ExprKind::Match(e, _, _) if e.hir_id != child_id => { @@ -2803,71 +2871,25 @@ pub fn expr_use_ctxt<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> Optio } } ControlFlow::Continue(()) - })? - .continue_value() - .map(|(use_node, child_id)| { - let node = match use_node { - Node::LetStmt(l) => ExprUseNode::LetStmt(l), - Node::ExprField(field) => ExprUseNode::Field(field), - - Node::Item(&Item { - kind: ItemKind::Static(..) | ItemKind::Const(..), - owner_id, - .. - }) - | Node::TraitItem(&TraitItem { - kind: TraitItemKind::Const(..), - owner_id, - .. - }) - | Node::ImplItem(&ImplItem { - kind: ImplItemKind::Const(..), - owner_id, - .. - }) => ExprUseNode::ConstStatic(owner_id), - - Node::Item(&Item { - kind: ItemKind::Fn(..), - owner_id, - .. - }) - | Node::TraitItem(&TraitItem { - kind: TraitItemKind::Fn(..), - owner_id, - .. - }) - | Node::ImplItem(&ImplItem { - kind: ImplItemKind::Fn(..), - owner_id, - .. - }) => ExprUseNode::Return(owner_id), - - Node::Expr(use_expr) => match use_expr.kind { - ExprKind::Ret(_) => ExprUseNode::Return(OwnerId { - def_id: cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap()), - }), - ExprKind::Closure(closure) => ExprUseNode::Return(OwnerId { def_id: closure.def_id }), - ExprKind::Call(func, args) => match args.iter().position(|arg| arg.hir_id == child_id) { - Some(i) => ExprUseNode::FnArg(func, i), - None => ExprUseNode::Callee, - }, - ExprKind::MethodCall(name, _, args, _) => ExprUseNode::MethodArg( - use_expr.hir_id, - name.args, - args.iter().position(|arg| arg.hir_id == child_id).map_or(0, |i| i + 1), - ), - ExprKind::Field(child, name) if child.hir_id == e.hir_id => ExprUseNode::FieldAccess(name), - _ => ExprUseNode::Expr, - }, - _ => ExprUseNode::Other, - }; - ExprUseCtxt { + }); + match node { + Some(ControlFlow::Continue((node, child_id))) => ExprUseCtxt { node, + child_id, adjustments, is_ty_unified, moved_before_use, - } - }) + same_ctxt, + }, + None => ExprUseCtxt { + node: Node::Crate(cx.tcx.hir().root_module()), + child_id: HirId::INVALID, + adjustments: &[], + is_ty_unified: true, + moved_before_use: true, + same_ctxt: false, + }, + } } /// Tokenizes the input while keeping the text associated with each token. From 22710f33a8ad99be51bb69f91339f8d815bd9e92 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 12 Feb 2024 18:33:22 -0500 Subject: [PATCH 2/2] Add lint `manual_inspect` --- CHANGELOG.md | 1 + clippy_config/src/msrvs.rs | 2 +- clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/manual_inspect.rs | 233 +++++++++++++++++++++ clippy_lints/src/methods/mod.rs | 24 +++ clippy_utils/src/source.rs | 27 ++- clippy_utils/src/ty.rs | 14 ++ tests/ui/copy_iterator.rs | 1 + tests/ui/copy_iterator.stderr | 2 +- tests/ui/manual_inspect.fixed | 174 +++++++++++++++ tests/ui/manual_inspect.rs | 186 ++++++++++++++++ tests/ui/manual_inspect.stderr | 182 ++++++++++++++++ 12 files changed, 844 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/methods/manual_inspect.rs create mode 100644 tests/ui/manual_inspect.fixed create mode 100644 tests/ui/manual_inspect.rs create mode 100644 tests/ui/manual_inspect.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 4de3124bd3d..3973edddb6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5521,6 +5521,7 @@ Released 2018-09-13 [`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map [`manual_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten [`manual_hash_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_hash_one +[`manual_inspect`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_inspect [`manual_instant_elapsed`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_instant_elapsed [`manual_is_ascii_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_ascii_check [`manual_is_finite`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_finite diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs index a3e7d0c3fa5..5327a125647 100644 --- a/clippy_config/src/msrvs.rs +++ b/clippy_config/src/msrvs.rs @@ -18,7 +18,7 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { 1,77,0 { C_STR_LITERALS } - 1,76,0 { PTR_FROM_REF } + 1,76,0 { PTR_FROM_REF, OPTION_RESULT_INSPECT } 1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE } 1,70,0 { OPTION_RESULT_IS_VARIANT_AND, BINARY_HEAP_RETAIN } 1,68,0 { PATH_MAIN_SEPARATOR_STR } diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 88986f9ad72..8abe5d47b65 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -403,6 +403,7 @@ crate::methods::MANUAL_C_STR_LITERALS_INFO, crate::methods::MANUAL_FILTER_MAP_INFO, crate::methods::MANUAL_FIND_MAP_INFO, + crate::methods::MANUAL_INSPECT_INFO, crate::methods::MANUAL_IS_VARIANT_AND_INFO, crate::methods::MANUAL_NEXT_BACK_INFO, crate::methods::MANUAL_OK_OR_INFO, diff --git a/clippy_lints/src/methods/manual_inspect.rs b/clippy_lints/src/methods/manual_inspect.rs new file mode 100644 index 00000000000..2f9b951c6a7 --- /dev/null +++ b/clippy_lints/src/methods/manual_inspect.rs @@ -0,0 +1,233 @@ +use clippy_config::msrvs::{self, Msrv}; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::{get_source_text, with_leading_whitespace, SpanRange}; +use clippy_utils::ty::get_field_by_name; +use clippy_utils::visitors::{for_each_expr, for_each_expr_without_closures}; +use clippy_utils::{expr_use_ctxt, is_diag_item_method, is_diag_trait_item, path_to_local_id, ExprUseNode}; +use core::ops::ControlFlow; +use rustc_errors::Applicability; +use rustc_hir::{BindingMode, BorrowKind, ByRef, ClosureKind, Expr, ExprKind, Mutability, Node, PatKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; +use rustc_span::{sym, BytePos, Span, Symbol, DUMMY_SP}; + +use super::MANUAL_INSPECT; + +#[expect(clippy::too_many_lines)] +pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name: &str, name_span: Span, msrv: &Msrv) { + if let ExprKind::Closure(c) = arg.kind + && matches!(c.kind, ClosureKind::Closure) + && let typeck = cx.typeck_results() + && let Some(fn_id) = typeck.type_dependent_def_id(expr.hir_id) + && (is_diag_trait_item(cx, fn_id, sym::Iterator) + || (msrv.meets(msrvs::OPTION_RESULT_INSPECT) + && (is_diag_item_method(cx, fn_id, sym::Option) || is_diag_item_method(cx, fn_id, sym::Result)))) + && let body = cx.tcx.hir().body(c.body) + && let [param] = body.params + && let PatKind::Binding(BindingMode(ByRef::No, Mutability::Not), arg_id, _, None) = param.pat.kind + && let arg_ty = typeck.node_type(arg_id) + && let ExprKind::Block(block, _) = body.value.kind + && let Some(final_expr) = block.expr + && !block.stmts.is_empty() + && path_to_local_id(final_expr, arg_id) + && typeck.expr_adjustments(final_expr).is_empty() + { + let mut requires_copy = false; + let mut requires_deref = false; + + // The number of unprocessed return expressions. + let mut ret_count = 0u32; + + // The uses for which processing is delayed until after the visitor. + let mut delayed = vec![]; + + let ctxt = arg.span.ctxt(); + let can_lint = for_each_expr_without_closures(block.stmts, |e| { + if let ExprKind::Closure(c) = e.kind { + // Nested closures don't need to treat returns specially. + let _: Option = for_each_expr(cx, cx.tcx.hir().body(c.body).value, |e| { + if path_to_local_id(e, arg_id) { + let (kind, same_ctxt) = check_use(cx, e); + match (kind, same_ctxt && e.span.ctxt() == ctxt) { + (_, false) | (UseKind::Deref | UseKind::Return(..), true) => { + requires_copy = true; + requires_deref = true; + }, + (UseKind::AutoBorrowed, true) => {}, + (UseKind::WillAutoDeref, true) => { + requires_copy = true; + }, + (kind, true) => delayed.push(kind), + } + } + ControlFlow::Continue(()) + }); + } else if matches!(e.kind, ExprKind::Ret(_)) { + ret_count += 1; + } else if path_to_local_id(e, arg_id) { + let (kind, same_ctxt) = check_use(cx, e); + match (kind, same_ctxt && e.span.ctxt() == ctxt) { + (UseKind::Return(..), false) => { + return ControlFlow::Break(()); + }, + (_, false) | (UseKind::Deref, true) => { + requires_copy = true; + requires_deref = true; + }, + (UseKind::AutoBorrowed, true) => {}, + (UseKind::WillAutoDeref, true) => { + requires_copy = true; + }, + (kind @ UseKind::Return(_), true) => { + ret_count -= 1; + delayed.push(kind); + }, + (kind, true) => delayed.push(kind), + } + } + ControlFlow::Continue(()) + }) + .is_none(); + + if ret_count != 0 { + // A return expression that didn't return the original value was found. + return; + } + + let mut edits = Vec::with_capacity(delayed.len() + 3); + let mut addr_of_edits = Vec::with_capacity(delayed.len()); + for x in delayed { + match x { + UseKind::Return(s) => edits.push((with_leading_whitespace(cx, s).set_span_pos(s), String::new())), + UseKind::Borrowed(s) => { + if let Some(src) = get_source_text(cx, s) + && let Some(src) = src.as_str() + && let trim_src = src.trim_start_matches([' ', '\t', '\n', '\r', '(']) + && trim_src.starts_with('&') + { + let range = s.into_range(); + #[expect(clippy::cast_possible_truncation)] + let start = BytePos(range.start.0 + (src.len() - trim_src.len()) as u32); + addr_of_edits.push(((start..BytePos(start.0 + 1)).set_span_pos(s), String::new())); + } else { + requires_copy = true; + requires_deref = true; + } + }, + UseKind::FieldAccess(name, e) => { + let Some(mut ty) = get_field_by_name(cx.tcx, arg_ty.peel_refs(), name) else { + requires_copy = true; + continue; + }; + let mut prev_expr = e; + + for (_, parent) in cx.tcx.hir().parent_iter(e.hir_id) { + if let Node::Expr(e) = parent { + match e.kind { + ExprKind::Field(_, name) + if let Some(fty) = get_field_by_name(cx.tcx, ty.peel_refs(), name.name) => + { + ty = fty; + prev_expr = e; + continue; + }, + ExprKind::AddrOf(BorrowKind::Ref, ..) => break, + _ if matches!( + typeck.expr_adjustments(prev_expr).first(), + Some(Adjustment { + kind: Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Not)) + | Adjust::Deref(_), + .. + }) + ) => + { + break; + }, + _ => {}, + } + } + requires_copy |= !ty.is_copy_modulo_regions(cx.tcx, cx.param_env); + break; + } + }, + // Already processed uses. + UseKind::AutoBorrowed | UseKind::WillAutoDeref | UseKind::Deref => {}, + } + } + + if can_lint + && (!requires_copy || arg_ty.is_copy_modulo_regions(cx.tcx, cx.param_env)) + // This case could be handled, but a fair bit of care would need to be taken. + && (!requires_deref || arg_ty.is_freeze(cx.tcx, cx.param_env)) + { + if requires_deref { + edits.push((param.span.shrink_to_lo(), "&".into())); + } else { + edits.extend(addr_of_edits); + } + edits.push(( + name_span, + String::from(match name { + "map" => "inspect", + "map_err" => "inspect_err", + _ => return, + }), + )); + edits.push(( + with_leading_whitespace(cx, final_expr.span).set_span_pos(final_expr.span), + String::new(), + )); + let app = if edits.iter().any(|(s, _)| s.from_expansion()) { + Applicability::MaybeIncorrect + } else { + Applicability::MachineApplicable + }; + span_lint_and_then(cx, MANUAL_INSPECT, name_span, "", |diag| { + diag.multipart_suggestion("try", edits, app); + }); + } + } +} + +enum UseKind<'tcx> { + AutoBorrowed, + WillAutoDeref, + Deref, + Return(Span), + Borrowed(Span), + FieldAccess(Symbol, &'tcx Expr<'tcx>), +} + +/// Checks how the value is used, and whether it was used in the same `SyntaxContext`. +fn check_use<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (UseKind<'tcx>, bool) { + let use_cx = expr_use_ctxt(cx, e); + if use_cx + .adjustments + .first() + .is_some_and(|a| matches!(a.kind, Adjust::Deref(_))) + { + return (UseKind::AutoBorrowed, use_cx.same_ctxt); + } + let res = match use_cx.use_node(cx) { + ExprUseNode::Return(_) => { + if let ExprKind::Ret(Some(e)) = use_cx.node.expect_expr().kind { + UseKind::Return(e.span) + } else { + return (UseKind::Return(DUMMY_SP), false); + } + }, + ExprUseNode::FieldAccess(name) => UseKind::FieldAccess(name.name, use_cx.node.expect_expr()), + ExprUseNode::Callee | ExprUseNode::MethodArg(_, _, 0) + if use_cx + .adjustments + .first() + .is_some_and(|a| matches!(a.kind, Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Not)))) => + { + UseKind::AutoBorrowed + }, + ExprUseNode::Callee | ExprUseNode::MethodArg(_, _, 0) => UseKind::WillAutoDeref, + ExprUseNode::AddrOf(BorrowKind::Ref, _) => UseKind::Borrowed(use_cx.node.expect_expr().span), + _ => UseKind::Deref, + }; + (res, use_cx.same_ctxt) +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6200716afbe..01438b8e8db 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -53,6 +53,7 @@ mod iterator_step_by_zero; mod join_absolute_paths; mod manual_c_str_literals; +mod manual_inspect; mod manual_is_variant_and; mod manual_next_back; mod manual_ok_or; @@ -4079,6 +4080,27 @@ "is_ascii() called on a char iterator" } +declare_clippy_lint! { + /// ### What it does + /// Checks for uses of `map` which return the original item. + /// + /// ### Why is this bad? + /// `inspect` is both clearer in intent and shorter. + /// + /// ### Example + /// ```no_run + /// let x = Some(0).map(|x| { println!("{x}"); x }); + /// ``` + /// Use instead: + /// ```no_run + /// let x = Some(0).inspect(|x| println!("{x}")); + /// ``` + #[clippy::version = "1.78.0"] + pub MANUAL_INSPECT, + complexity, + "use of `map` returning the original item" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -4244,6 +4266,7 @@ pub fn new( MANUAL_C_STR_LITERALS, UNNECESSARY_GET_THEN_CHECK, NEEDLESS_CHARACTER_ITERATION, + MANUAL_INSPECT, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -4747,6 +4770,7 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { } } map_identity::check(cx, expr, recv, m_arg, name, span); + manual_inspect::check(cx, expr, m_arg, name, span, &self.msrv); }, ("map_or", [def, map]) => { option_map_or_none::check(cx, expr, recv, def, map); diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index b7ff7ebe910..69b122cbfad 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -14,8 +14,17 @@ use std::ops::Range; /// A type which can be converted to the range portion of a `Span`. -pub trait SpanRange { +pub trait SpanRange: Sized { fn into_range(self) -> Range; + fn set_span_pos(self, sp: Span) -> Span { + let range = self.into_range(); + SpanData { + lo: range.start, + hi: range.end, + ..sp.data() + } + .span() + } } impl SpanRange for Span { fn into_range(self) -> Range { @@ -61,6 +70,22 @@ fn f(sm: &SourceMap, sp: Range) -> Option { f(cx.sess().source_map(), sp.into_range()) } +pub fn with_leading_whitespace(cx: &impl LintContext, sp: impl SpanRange) -> Range { + #[expect(clippy::needless_pass_by_value, clippy::cast_possible_truncation)] + fn f(src: SourceFileRange, sp: Range) -> Range { + let Some(text) = &src.sf.src else { + return sp; + }; + let len = src.range.start - text[..src.range.start].trim_end().len(); + BytePos(sp.start.0 - len as u32)..sp.end + } + let sp = sp.into_range(); + match get_source_text(cx, sp.clone()) { + Some(src) => f(src, sp), + None => sp, + } +} + /// Like `snippet_block`, but add braces if the expr is not an `ExprKind::Block`. pub fn expr_block( cx: &T, diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 7d4332a3d9d..3f5ed73c3ef 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -1349,3 +1349,17 @@ pub fn get_adt_inherent_method<'a>(cx: &'a LateContext<'_>, ty: Ty<'_>, method_n None } } + +/// Get's the type of a field by name. +pub fn get_field_by_name<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, name: Symbol) -> Option> { + match *ty.kind() { + ty::Adt(def, args) if def.is_union() || def.is_struct() => def + .non_enum_variant() + .fields + .iter() + .find(|f| f.name == name) + .map(|f| f.ty(tcx, args)), + ty::Tuple(args) => name.as_str().parse::().ok().and_then(|i| args.get(i).copied()), + _ => None, + } +} diff --git a/tests/ui/copy_iterator.rs b/tests/ui/copy_iterator.rs index c0e5fc3e446..ea3a4face93 100644 --- a/tests/ui/copy_iterator.rs +++ b/tests/ui/copy_iterator.rs @@ -1,4 +1,5 @@ #![warn(clippy::copy_iterator)] +#![allow(clippy::manual_inspect)] #[derive(Copy, Clone)] struct Countdown(u8); diff --git a/tests/ui/copy_iterator.stderr b/tests/ui/copy_iterator.stderr index 533bddaadb5..990b1ce628d 100644 --- a/tests/ui/copy_iterator.stderr +++ b/tests/ui/copy_iterator.stderr @@ -1,5 +1,5 @@ error: you are implementing `Iterator` on a `Copy` type - --> tests/ui/copy_iterator.rs:6:1 + --> tests/ui/copy_iterator.rs:7:1 | LL | / impl Iterator for Countdown { LL | | diff --git a/tests/ui/manual_inspect.fixed b/tests/ui/manual_inspect.fixed new file mode 100644 index 00000000000..0e1b8fe3edb --- /dev/null +++ b/tests/ui/manual_inspect.fixed @@ -0,0 +1,174 @@ +#![warn(clippy::manual_inspect)] +#![allow(clippy::no_effect, clippy::op_ref)] + +fn main() { + let _ = Some(0).inspect(|&x| { + println!("{}", x); + }); + + let _ = Some(0).inspect(|&x| { + println!("{x}"); + }); + + let _ = Some(0).inspect(|&x| { + println!("{}", x * 5 + 1); + }); + + let _ = Some(0).inspect(|&x| { + if x == 0 { + panic!(); + } + }); + + let _ = Some(0).inspect(|&x| { + if &x == &0 { + let _y = x; + panic!(); + } + }); + + let _ = Some(0).map(|x| { + let y = x + 1; + if y > 5 { + return y; + } + x + }); + + { + #[derive(PartialEq)] + struct Foo(i32); + + let _ = Some(Foo(0)).map(|x| { + if x == Foo(0) { + panic!(); + } + x + }); + + let _ = Some(Foo(0)).map(|x| { + if &x == &Foo(0) { + let _y = x; + panic!(); + } + x + }); + } + + { + macro_rules! maybe_ret { + ($e:expr) => { + if $e == 0 { + return $e; + } + }; + } + + let _ = Some(0).map(|x| { + maybe_ret!(x); + x + }); + } + + let _ = Some((String::new(), 0u32)).inspect(|x| { + if x.1 == 0 { + let _x = x.1; + panic!(); + } + }); + + let _ = Some((String::new(), 0u32)).map(|x| { + if x.1 == 0 { + let _x = x.0; + panic!(); + } + x + }); + + let _ = Some(String::new()).map(|x| { + if x.is_empty() { + let _ = || { + let _x = x; + }; + panic!(); + } + x + }); + + let _ = Some(String::new()).inspect(|x| { + if x.is_empty() { + let _ = || { + let _x = x; + }; + return; + } + println!("test"); + }); + + let _ = Some(0).inspect(|&x| { + if x == 0 { + let _ = || { + let _x = x; + }; + panic!(); + } + }); + + { + use core::cell::Cell; + #[derive(Debug)] + struct Cell2(core::cell::Cell); + + let _ = Some(Cell2(Cell::new(0u32))).inspect(|x| { + x.0.set(1); + }); + + let _ = Some(Cell2(Cell::new(0u32))).map(|x| { + let y = &x; + if x.0.get() == 0 { + y.0.set(1) + } else { + println!("{x:?}"); + } + x + }); + } + + let _: Result<_, ()> = Ok(0).inspect(|&x| { + println!("{}", x); + }); + + let _: Result<(), _> = Err(0).inspect_err(|&x| { + println!("{}", x); + }); + + let _ = [0] + .into_iter() + .inspect(|&x| { + println!("{}", x); + }) + .count(); + + { + struct S(T); + impl S { + fn map(self, f: impl FnOnce(T) -> U) -> S { + S(f(self.0)) + } + + fn map_err(self, f: impl FnOnce(T) -> U) -> S { + S(f(self.0)) + } + } + + let _ = S(0).map(|x| { + println!("{}", x); + x + }); + + let _ = S(0).map_err(|x| { + println!("{}", x); + x + }); + } +} diff --git a/tests/ui/manual_inspect.rs b/tests/ui/manual_inspect.rs new file mode 100644 index 00000000000..94cdfe39140 --- /dev/null +++ b/tests/ui/manual_inspect.rs @@ -0,0 +1,186 @@ +#![warn(clippy::manual_inspect)] +#![allow(clippy::no_effect, clippy::op_ref)] + +fn main() { + let _ = Some(0).map(|x| { + println!("{}", x); + x + }); + + let _ = Some(0).map(|x| { + println!("{x}"); + x + }); + + let _ = Some(0).map(|x| { + println!("{}", x * 5 + 1); + x + }); + + let _ = Some(0).map(|x| { + if x == 0 { + panic!(); + } + x + }); + + let _ = Some(0).map(|x| { + if &x == &0 { + let _y = x; + panic!(); + } + x + }); + + let _ = Some(0).map(|x| { + let y = x + 1; + if y > 5 { + return y; + } + x + }); + + { + #[derive(PartialEq)] + struct Foo(i32); + + let _ = Some(Foo(0)).map(|x| { + if x == Foo(0) { + panic!(); + } + x + }); + + let _ = Some(Foo(0)).map(|x| { + if &x == &Foo(0) { + let _y = x; + panic!(); + } + x + }); + } + + { + macro_rules! maybe_ret { + ($e:expr) => { + if $e == 0 { + return $e; + } + }; + } + + let _ = Some(0).map(|x| { + maybe_ret!(x); + x + }); + } + + let _ = Some((String::new(), 0u32)).map(|x| { + if x.1 == 0 { + let _x = x.1; + panic!(); + } + x + }); + + let _ = Some((String::new(), 0u32)).map(|x| { + if x.1 == 0 { + let _x = x.0; + panic!(); + } + x + }); + + let _ = Some(String::new()).map(|x| { + if x.is_empty() { + let _ = || { + let _x = x; + }; + panic!(); + } + x + }); + + let _ = Some(String::new()).map(|x| { + if x.is_empty() { + let _ = || { + let _x = &x; + }; + return x; + } + println!("test"); + x + }); + + let _ = Some(0).map(|x| { + if x == 0 { + let _ = || { + let _x = x; + }; + panic!(); + } + x + }); + + { + use core::cell::Cell; + #[derive(Debug)] + struct Cell2(core::cell::Cell); + + let _ = Some(Cell2(Cell::new(0u32))).map(|x| { + x.0.set(1); + x + }); + + let _ = Some(Cell2(Cell::new(0u32))).map(|x| { + let y = &x; + if x.0.get() == 0 { + y.0.set(1) + } else { + println!("{x:?}"); + } + x + }); + } + + let _: Result<_, ()> = Ok(0).map(|x| { + println!("{}", x); + x + }); + + let _: Result<(), _> = Err(0).map_err(|x| { + println!("{}", x); + x + }); + + let _ = [0] + .into_iter() + .map(|x| { + println!("{}", x); + x + }) + .count(); + + { + struct S(T); + impl S { + fn map(self, f: impl FnOnce(T) -> U) -> S { + S(f(self.0)) + } + + fn map_err(self, f: impl FnOnce(T) -> U) -> S { + S(f(self.0)) + } + } + + let _ = S(0).map(|x| { + println!("{}", x); + x + }); + + let _ = S(0).map_err(|x| { + println!("{}", x); + x + }); + } +} diff --git a/tests/ui/manual_inspect.stderr b/tests/ui/manual_inspect.stderr new file mode 100644 index 00000000000..efdf115b84e --- /dev/null +++ b/tests/ui/manual_inspect.stderr @@ -0,0 +1,182 @@ +error: + --> tests/ui/manual_inspect.rs:5:21 + | +LL | let _ = Some(0).map(|x| { + | ^^^ + | + = note: `-D clippy::manual-inspect` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_inspect)]` +help: try + | +LL ~ let _ = Some(0).inspect(|&x| { +LL ~ println!("{}", x); + | + +error: + --> tests/ui/manual_inspect.rs:10:21 + | +LL | let _ = Some(0).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _ = Some(0).inspect(|&x| { +LL ~ println!("{x}"); + | + +error: + --> tests/ui/manual_inspect.rs:15:21 + | +LL | let _ = Some(0).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _ = Some(0).inspect(|&x| { +LL ~ println!("{}", x * 5 + 1); + | + +error: + --> tests/ui/manual_inspect.rs:20:21 + | +LL | let _ = Some(0).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _ = Some(0).inspect(|&x| { +LL | if x == 0 { +LL | panic!(); +LL ~ } + | + +error: + --> tests/ui/manual_inspect.rs:27:21 + | +LL | let _ = Some(0).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _ = Some(0).inspect(|&x| { +LL | if &x == &0 { +LL | let _y = x; +LL | panic!(); +LL ~ } + | + +error: + --> tests/ui/manual_inspect.rs:78:41 + | +LL | let _ = Some((String::new(), 0u32)).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _ = Some((String::new(), 0u32)).inspect(|x| { +LL | if x.1 == 0 { +LL | let _x = x.1; +LL | panic!(); +LL ~ } + | + +error: + --> tests/ui/manual_inspect.rs:104:33 + | +LL | let _ = Some(String::new()).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _ = Some(String::new()).inspect(|x| { +LL | if x.is_empty() { +LL | let _ = || { +LL ~ let _x = x; +LL | }; +LL ~ return; +LL | } +LL ~ println!("test"); + | + +error: + --> tests/ui/manual_inspect.rs:115:21 + | +LL | let _ = Some(0).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _ = Some(0).inspect(|&x| { +LL | if x == 0 { + ... +LL | panic!(); +LL ~ } + | + +error: + --> tests/ui/manual_inspect.rs:130:46 + | +LL | let _ = Some(Cell2(Cell::new(0u32))).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _ = Some(Cell2(Cell::new(0u32))).inspect(|x| { +LL ~ x.0.set(1); + | + +error: + --> tests/ui/manual_inspect.rs:146:34 + | +LL | let _: Result<_, ()> = Ok(0).map(|x| { + | ^^^ + | +help: try + | +LL ~ let _: Result<_, ()> = Ok(0).inspect(|&x| { +LL ~ println!("{}", x); + | + +error: + --> tests/ui/manual_inspect.rs:151:35 + | +LL | let _: Result<(), _> = Err(0).map_err(|x| { + | ^^^^^^^ + | +help: try + | +LL ~ let _: Result<(), _> = Err(0).inspect_err(|&x| { +LL ~ println!("{}", x); + | + +error: this call to `map()` won't have an effect on the call to `count()` + --> tests/ui/manual_inspect.rs:156:13 + | +LL | let _ = [0] + | _____________^ +LL | | .into_iter() +LL | | .map(|x| { +LL | | println!("{}", x); +LL | | x +LL | | }) +LL | | .count(); + | |________________^ + | + = help: make sure you did not confuse `map` with `filter`, `for_each` or `inspect` + = note: `-D clippy::suspicious-map` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::suspicious_map)]` + +error: + --> tests/ui/manual_inspect.rs:158:10 + | +LL | .map(|x| { + | ^^^ + | +help: try + | +LL ~ .inspect(|&x| { +LL ~ println!("{}", x); + | + +error: aborting due to 13 previous errors +