diff --git a/CHANGELOG.md b/CHANGELOG.md index d3b2c0a7bf6e..60530675007c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5743,6 +5743,7 @@ Released 2018-09-13 [`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation [`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings +[`unnecessary_ref_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_ref_mut [`unnecessary_result_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_result_map_or_else [`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment [`unnecessary_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2b324f5f96e8..898ab0b0dcac 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -716,6 +716,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO, crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO, crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO, + crate::unnecessary_ref_mut::UNNECESSARY_REF_MUT_INFO, crate::unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS_INFO, crate::unnecessary_struct_initialization::UNNECESSARY_STRUCT_INITIALIZATION_INFO, crate::unnecessary_wraps::UNNECESSARY_WRAPS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b930175c4d89..bde77a19f048 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -351,6 +351,7 @@ mod unnamed_address; mod unnecessary_box_returns; mod unnecessary_map_on_constructor; mod unnecessary_owned_empty_strings; +mod unnecessary_ref_mut; mod unnecessary_self_imports; mod unnecessary_struct_initialization; mod unnecessary_wraps; @@ -1120,6 +1121,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(to_string_trait_impl::ToStringTraitImpl)); store.register_early_pass(|| Box::new(multiple_bound_locations::MultipleBoundLocations)); store.register_late_pass(|_| Box::new(assigning_clones::AssigningClones)); + store.register_late_pass(|_| Box::new(unnecessary_ref_mut::UnnecessaryRefMut)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unnecessary_ref_mut.rs b/clippy_lints/src/unnecessary_ref_mut.rs new file mode 100644 index 000000000000..6e6c3dc9d861 --- /dev/null +++ b/clippy_lints/src/unnecessary_ref_mut.rs @@ -0,0 +1,265 @@ +use crate::FxHashSet; +use clippy_utils::diagnostics::span_lint_and_sugg; +use rustc_ast::UnOp; +use rustc_errors::Applicability; +use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; +use rustc_hir::def_id::DefId; +use rustc_hir::intravisit::{walk_block, walk_expr, walk_local, Visitor}; +use rustc_hir::{Block, ByRef, Expr, ExprKind, HirId, Mutability, Node, PatKind, PathSegment, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; +use rustc_span::symbol::Ident; + +declare_clippy_lint! { + /// ### What it does + /// Suggests replace `ref mut` with `ref` when the reference does not need to be a mutable. + /// + /// ### Why is this bad? + /// This reference does not need to be a mutable since it will not change. + /// It can be replaced by `ref` instead. + /// + /// ### Example + /// ```no_run + /// let mut s = Some(String::new()); + /// if let Some(ref mut s_ref) = s { + /// s_ref.as_str(); + /// } + /// ``` + /// Use instead: + /// ```no_run + /// let mut s = Some(String::new()); + /// if let Some(ref s_ref) = s { + /// s_ref.as_str(); + /// } + /// ``` + #[clippy::version = "1.78.0"] + pub UNNECESSARY_REF_MUT, + restriction, + "Removing unnecessary mutable references in patterns" +} + +declare_lint_pass!(UnnecessaryRefMut => [UNNECESSARY_REF_MUT]); + +impl<'tcx> LateLintPass<'tcx> for UnnecessaryRefMut { + fn check_pat(&mut self, cx: &LateContext<'_>, pat: &'_ rustc_hir::Pat<'_>) { + if let PatKind::Binding(annotation, _, ref_mut_ident, _) = pat.kind { + if !(matches!(annotation.0, ByRef::Yes) && matches!(annotation.1, Mutability::Mut)) { + return; + } + + if let Some(block) = parent_block(cx, pat.hir_id) { + let mut v = InspectUseMutableRefVisitor::new(cx, ref_mut_ident); + walk_block(&mut v, block); + if !v.used_as_mut { + span_lint_and_sugg( + cx, + UNNECESSARY_REF_MUT, + pat.span, + "unnecessary ref mut", + "replace with", + format!("ref {ref_mut_ident}"), + Applicability::MachineApplicable, + ); + } + } + } + } +} + +fn parent_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<&'tcx Block<'tcx>> { + for (_, node) in cx.tcx.hir().parent_iter(hir_id) { + if let Node::Block(block) = node { + return Some(block); + } + } + None +} + +struct InspectUseMutableRefVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + used_as_mut: bool, + bindings: FxHashSet, +} + +impl<'a, 'tcx> Visitor<'tcx> for InspectUseMutableRefVisitor<'a, 'tcx> { + fn visit_local(&mut self, local: &'tcx rustc_hir::Local<'tcx>) -> Self::Result { + if let Some(init) = local.init + && let Some(rhs_ident) = extract_first_ident(init) + && self.bindings.contains(&rhs_ident) + && let PatKind::Binding(_, _, lhs_ident, _) = local.pat.kind + { + self.bindings.insert(lhs_ident); + } + + walk_local(self, local); + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) -> Self::Result { + match expr.kind { + ExprKind::Let(let_expr) + if let PatKind::Path(QPath::Resolved(_, path)) = let_expr.pat.kind + && let Some(lhs_segment) = path.segments.first() + && let Some(rhs_ident) = extract_first_ident(let_expr.init) + && self.bindings.contains(&rhs_ident) => + { + self.bindings.insert(lhs_segment.ident); + }, + ExprKind::Assign(lhs, rhs, _) => { + if !self.bind_assign(lhs, rhs) { + self.inspect_assign(lhs, rhs); + } + }, + ExprKind::MethodCall(_, method_expr, args, _) => { + self.inspect_method_call(method_expr, expr, args); + }, + ExprKind::Call(fn_expr, args) => { + self.inspect_call(fn_expr, args); + }, + _ => {}, + } + + if !self.used_as_mut { + walk_expr(self, expr); + } + } + + fn visit_block(&mut self, b: &'tcx Block<'tcx>) -> Self::Result { + walk_block(self, b); + } +} + +impl<'a, 'tcx> InspectUseMutableRefVisitor<'a, 'tcx> { + fn new(cx: &'a LateContext<'tcx>, ident: Ident) -> InspectUseMutableRefVisitor<'a, 'tcx> { + let mut bindings = FxHashSet::default(); + bindings.insert(ident); + Self { + cx, + used_as_mut: false, + bindings, + } + } + + fn bind_assign(&mut self, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) -> bool { + if self.set_use_as_mut_if_bind_to_static(lhs) { + return true; + } + + if let Some(rhs_ident) = extract_first_ident(rhs) + && self.bindings.contains(&rhs_ident) + && let Some(lhs_segment) = extract_first_segment(lhs) + { + self.bindings.insert(lhs_segment.ident); + true + } else { + false + } + } + + fn set_use_as_mut_if_bind_to_static(&mut self, lhs: &'tcx Expr<'tcx>) -> bool { + if let ExprKind::Path(QPath::Resolved(_, path)) = lhs.kind + && let Res::Def(DefKind::Static(Mutability::Mut), _) = path.res + { + self.used_as_mut = true; + true + } else { + false + } + } + + fn inspect_assign(&mut self, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) { + if let ExprKind::Unary(UnOp::Deref, lhs_assign) = lhs.kind + && let Some(lhs_ident) = extract_first_ident(lhs_assign) + { + if self.bindings.contains(&lhs_ident) { + self.used_as_mut = true; + return; + } + + if let Some(lhs_ident) = extract_first_ident(lhs_assign) + && self.bindings.contains(&lhs_ident) + { + self.used_as_mut = true; + return; + } + } + + if let ExprKind::Unary(UnOp::Deref, rhs_assign) = rhs.kind + && let Some(rhs_segment) = extract_first_segment(rhs_assign) + && self.bindings.contains(&rhs_segment.ident) + { + self.used_as_mut = true; + } + } + + fn inspect_method_call(&mut self, method_expr: &'tcx Expr<'tcx>, expr: &'tcx Expr<'tcx>, args: &[Expr<'tcx>]) { + let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(expr.hir_id) else { + return; + }; + + self.inspect_fn_call(method_def_id, args, true); + if self.used_as_mut { + return; + } + + if let Some(ident) = extract_first_ident(method_expr) + && self.bindings.contains(&ident) + { + let method = self.cx.tcx.fn_sig(method_def_id).instantiate_identity(); + let receiver = method.input(0).skip_binder(); + + if matches!(receiver.ref_mutability(), Some(Mutability::Mut)) { + self.used_as_mut = true; + } + } + } + + fn inspect_call(&mut self, fn_expr: &Expr<'tcx>, args: &[Expr<'tcx>]) { + let ExprKind::Path(ref path) = fn_expr.kind else { + return; + }; + let Some(fn_def_id) = self.cx.qpath_res(path, fn_expr.hir_id).opt_def_id() else { + return; + }; + self.inspect_fn_call(fn_def_id, args, false); + } + + fn position_args_index(&self, args: &[Expr<'tcx>]) -> Option { + args.iter().position(|arg| match arg.kind { + ExprKind::Path(QPath::Resolved(_, path)) => path + .segments + .iter() + .any(|segment| self.bindings.contains(&segment.ident)), + _ => false, + }) + } + + fn inspect_fn_call(&mut self, fn_def_id: DefId, args: &[Expr<'tcx>], is_method: bool) { + let Some(arg_index) = self.position_args_index(args) else { + return; + }; + + let fn_sig = self.cx.tcx.fn_sig(fn_def_id).instantiate_identity(); + let ty_kind = fn_sig.input(arg_index + usize::from(is_method)).skip_binder().kind(); + if matches!(ty_kind, rustc_middle::ty::Ref(_, _, Mutability::Mut)) { + self.used_as_mut = true; + } + } +} + +fn extract_first_segment<'tcx>(expr: &Expr<'tcx>) -> Option<&'tcx PathSegment<'tcx>> { + match expr.kind { + ExprKind::Path(QPath::Resolved(_, path)) if let Some(segment) = path.segments.first() => Some(segment), + ExprKind::Call(call, args) + if let ExprKind::Path(QPath::Resolved(_, path)) = call.kind + && let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), _) = path.res + && !args.is_empty() => + { + extract_first_segment(&args[0]) + }, + _ => None, + } +} + +fn extract_first_ident(expr: &Expr<'_>) -> Option { + extract_first_segment(expr).map(|segment| segment.ident) +} diff --git a/tests/ui/unnecessary_ref_mut.fixed b/tests/ui/unnecessary_ref_mut.fixed new file mode 100644 index 000000000000..6667ae3a73e6 --- /dev/null +++ b/tests/ui/unnecessary_ref_mut.fixed @@ -0,0 +1,213 @@ +#![warn(clippy::unnecessary_ref_mut)] +#![allow(clippy::disallowed_names, clippy::single_match)] +#![no_main] + +struct Foo; +impl Foo { + fn immutable(&self, s: &str) {} + + fn mutable(&self, s: &mut str) {} +} + +struct Config { + name: String, +} + +fn let_some() { + { + let mut s = Some(String::new()); + let Some(ref s_ref) = s else { + //~^ ERROR: unnecessary ref mut + return; + }; + + s_ref.as_bytes(); + } + + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + return; + }; + + s_ref.push('A'); + } +} + +fn if_let_some() { + { + let mut s = Some(String::new()); + if let Some(ref s_ref) = s { + //~^ ERROR: unnecessary ref mut + s_ref.as_str(); + }; + } + + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + s_ref.push('A'); + }; + } +} + +fn match_expr() { + { + let mut s = Some(String::new()); + match s { + Some(ref s_ref) => { + //~^ ERROR: unnecessary ref mut + s_ref.as_bytes(); + }, + None => {}, + } + } + + { + let mut s = Some(String::new()); + match s { + Some(ref mut s_ref) => { + s_ref.push('A'); + }, + None => {}, + } + } +} + +fn bind_split_field() { + { + let mut config = Config { + name: "name".to_string(), + }; + let Config { ref name } = config; + //~^ ERROR: unnecessary ref mut + name.to_string(); + } + + { + let mut config = Config { + name: "name".to_string(), + }; + let Config { ref mut name } = config; + name.push('A'); + } +} + +fn fn_call_args() { + { + let mut s = Some(String::new()); + let Some(ref s_ref) = s else { + //~^ ERROR: unnecessary ref mut + return; + }; + + fn call(f: &str) {} + call(s_ref); + } + + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + return; + }; + + fn call(f: &mut str) {} + call(s_ref); + } +} + +fn method_call_args() { + { + let mut s = Some(String::new()); + let Some(ref s_ref) = s else { + //~^ ERROR: unnecessary ref mut + return; + }; + + let foo = Foo; + foo.immutable(s_ref); + } + + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + return; + }; + + let foo = Foo; + foo.mutable(s_ref); + } +} + +#[allow(static_mut_refs)] +fn binding() { + { + let mut s = Some(String::new()); + if let Some(ref s_ref) = s { + //~^ ERROR: unnecessary ref mut + let s_ref2 = s_ref; + s_ref2.as_str(); + }; + } + + { + let mut s = Some(String::new()); + if let Some(ref s_ref) = s { + //~^ ERROR: unnecessary ref mut + let _ = s_ref; + }; + } + + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + let s_ref2 = s_ref; + s_ref2.push('A'); + }; + } + + { + let mut str = "".to_string(); + let mut s = Some(String::new()); + let mut s2 = &mut str; + if let Some(ref mut s_ref) = s { + s2 = s_ref; + s2.push('A'); + }; + } + + static mut STR: Option = Some(0); + static mut OUTSIDE: Option<&mut usize> = None; + + unsafe { + if let Some(ref mut s_ref) = STR { + OUTSIDE = Some(s_ref); + } + } +} + +fn binding_tuple_in_variant() { + { + let s = String::new(); + if let Some((_, ref s_ref)) = Some(((), s)) { + //~^ ERROR: unnecessary ref mut + s_ref.as_str(); + }; + } + + { + let s = String::new(); + if let Some((_, ref mut s_ref)) = Some(((), s)) { + s_ref.push('A'); + }; + } +} + +fn assign() { + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + *s_ref = "".to_string(); + }; + } +} diff --git a/tests/ui/unnecessary_ref_mut.rs b/tests/ui/unnecessary_ref_mut.rs new file mode 100644 index 000000000000..cc44ec51565d --- /dev/null +++ b/tests/ui/unnecessary_ref_mut.rs @@ -0,0 +1,213 @@ +#![warn(clippy::unnecessary_ref_mut)] +#![allow(clippy::disallowed_names, clippy::single_match)] +#![no_main] + +struct Foo; +impl Foo { + fn immutable(&self, s: &str) {} + + fn mutable(&self, s: &mut str) {} +} + +struct Config { + name: String, +} + +fn let_some() { + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + //~^ ERROR: unnecessary ref mut + return; + }; + + s_ref.as_bytes(); + } + + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + return; + }; + + s_ref.push('A'); + } +} + +fn if_let_some() { + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + //~^ ERROR: unnecessary ref mut + s_ref.as_str(); + }; + } + + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + s_ref.push('A'); + }; + } +} + +fn match_expr() { + { + let mut s = Some(String::new()); + match s { + Some(ref mut s_ref) => { + //~^ ERROR: unnecessary ref mut + s_ref.as_bytes(); + }, + None => {}, + } + } + + { + let mut s = Some(String::new()); + match s { + Some(ref mut s_ref) => { + s_ref.push('A'); + }, + None => {}, + } + } +} + +fn bind_split_field() { + { + let mut config = Config { + name: "name".to_string(), + }; + let Config { ref mut name } = config; + //~^ ERROR: unnecessary ref mut + name.to_string(); + } + + { + let mut config = Config { + name: "name".to_string(), + }; + let Config { ref mut name } = config; + name.push('A'); + } +} + +fn fn_call_args() { + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + //~^ ERROR: unnecessary ref mut + return; + }; + + fn call(f: &str) {} + call(s_ref); + } + + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + return; + }; + + fn call(f: &mut str) {} + call(s_ref); + } +} + +fn method_call_args() { + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + //~^ ERROR: unnecessary ref mut + return; + }; + + let foo = Foo; + foo.immutable(s_ref); + } + + { + let mut s = Some(String::new()); + let Some(ref mut s_ref) = s else { + return; + }; + + let foo = Foo; + foo.mutable(s_ref); + } +} + +#[allow(static_mut_refs)] +fn binding() { + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + //~^ ERROR: unnecessary ref mut + let s_ref2 = s_ref; + s_ref2.as_str(); + }; + } + + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + //~^ ERROR: unnecessary ref mut + let _ = s_ref; + }; + } + + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + let s_ref2 = s_ref; + s_ref2.push('A'); + }; + } + + { + let mut str = "".to_string(); + let mut s = Some(String::new()); + let mut s2 = &mut str; + if let Some(ref mut s_ref) = s { + s2 = s_ref; + s2.push('A'); + }; + } + + static mut STR: Option = Some(0); + static mut OUTSIDE: Option<&mut usize> = None; + + unsafe { + if let Some(ref mut s_ref) = STR { + OUTSIDE = Some(s_ref); + } + } +} + +fn binding_tuple_in_variant() { + { + let s = String::new(); + if let Some((_, ref mut s_ref)) = Some(((), s)) { + //~^ ERROR: unnecessary ref mut + s_ref.as_str(); + }; + } + + { + let s = String::new(); + if let Some((_, ref mut s_ref)) = Some(((), s)) { + s_ref.push('A'); + }; + } +} + +fn assign() { + { + let mut s = Some(String::new()); + if let Some(ref mut s_ref) = s { + *s_ref = "".to_string(); + }; + } +} diff --git a/tests/ui/unnecessary_ref_mut.stderr b/tests/ui/unnecessary_ref_mut.stderr new file mode 100644 index 000000000000..89e840103740 --- /dev/null +++ b/tests/ui/unnecessary_ref_mut.stderr @@ -0,0 +1,59 @@ +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:19:18 + | +LL | let Some(ref mut s_ref) = s else { + | ^^^^^^^^^^^^^ help: replace with: `ref s_ref` + | + = note: `-D clippy::unnecessary-ref-mut` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_ref_mut)]` + +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:40:21 + | +LL | if let Some(ref mut s_ref) = s { + | ^^^^^^^^^^^^^ help: replace with: `ref s_ref` + +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:58:18 + | +LL | Some(ref mut s_ref) => { + | ^^^^^^^^^^^^^ help: replace with: `ref s_ref` + +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:82:22 + | +LL | let Config { ref mut name } = config; + | ^^^^^^^^^^^^ help: replace with: `ref name` + +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:99:18 + | +LL | let Some(ref mut s_ref) = s else { + | ^^^^^^^^^^^^^ help: replace with: `ref s_ref` + +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:122:18 + | +LL | let Some(ref mut s_ref) = s else { + | ^^^^^^^^^^^^^ help: replace with: `ref s_ref` + +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:146:21 + | +LL | if let Some(ref mut s_ref) = s { + | ^^^^^^^^^^^^^ help: replace with: `ref s_ref` + +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:155:21 + | +LL | if let Some(ref mut s_ref) = s { + | ^^^^^^^^^^^^^ help: replace with: `ref s_ref` + +error: unnecessary ref mut + --> tests/ui/unnecessary_ref_mut.rs:192:25 + | +LL | if let Some((_, ref mut s_ref)) = Some(((), s)) { + | ^^^^^^^^^^^^^ help: replace with: `ref s_ref` + +error: aborting due to 9 previous errors +