From b8cdefb6cfab11087943bb41824bb955c655daf1 Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Tue, 29 Sep 2015 13:11:19 +0200 Subject: [PATCH 1/8] Add unnecessary mut passed lint --- README.md | 3 ++- src/lib.rs | 3 +++ src/mut_reference.rs | 53 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 src/mut_reference.rs diff --git a/README.md b/README.md index e15ed7e8fce..427f4181dea 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 58 lints included in this crate: +There are 59 lints included in this crate: name | default | meaning -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -63,6 +63,7 @@ name [type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity) | warn | usage of very complex types; recommends factoring out parts into `type` definitions [unicode_not_nfc](https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc) | allow | using a unicode literal not in NFC normal form (see http://www.unicode.org/reports/tr15/ for further information) [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively) +[unnecessary_mut_passed](https://github.com/Manishearth/rust-clippy/wiki#unnecessary_mut_passed) | warn | an argument is passed as a mutable reference although the function only demands an immutable reference [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect) | warn | `collect()`ing an iterator without using the result; this is usually better written as a for loop [while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop) | warn | `loop { if let { ... } else break }` can be written as a `while let` loop [wrong_pub_self_convention](https://github.com/Manishearth/rust-clippy/wiki#wrong_pub_self_convention) | allow | defining a public method named with an established prefix (like "into_") that takes `self` with the wrong convention diff --git a/src/lib.rs b/src/lib.rs index 3c2d870aee2..2f71d8cc9df 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -33,6 +33,7 @@ pub mod eta_reduction; pub mod identity_op; pub mod minmax; pub mod mut_mut; +pub mod mut_reference; pub mod len_zero; pub mod attrs; pub mod collapsible_if; @@ -66,6 +67,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box eta_reduction::EtaPass); reg.register_late_lint_pass(box identity_op::IdentityOp); reg.register_late_lint_pass(box mut_mut::MutMut); + reg.register_late_lint_pass(box mut_reference::UnnecessaryMutPassed); reg.register_late_lint_pass(box len_zero::LenZero); reg.register_late_lint_pass(box misc::CmpOwned); reg.register_late_lint_pass(box attrs::AttrPass); @@ -138,6 +140,7 @@ pub fn plugin_registrar(reg: &mut Registry) { misc::MODULO_ONE, misc::REDUNDANT_PATTERN, misc::TOPLEVEL_REF_ARG, + mut_reference::UNNECESSARY_MUT_PASSED, needless_bool::NEEDLESS_BOOL, precedence::PRECEDENCE, ranges::RANGE_STEP_BY_ZERO, diff --git a/src/mut_reference.rs b/src/mut_reference.rs new file mode 100644 index 00000000000..13cf1e1301e --- /dev/null +++ b/src/mut_reference.rs @@ -0,0 +1,53 @@ +use rustc::lint::*; +use rustc_front::hir::*; +use utils::span_lint; +use rustc::middle::ty::{TypeAndMut, TypeVariants}; + +declare_lint! { + pub UNNECESSARY_MUT_PASSED, + Warn, + "an argument is passed as a mutable reference although the function only demands an \ + immutable reference" +} + + +#[derive(Copy,Clone)] +pub struct UnnecessaryMutPassed; + +impl LintPass for UnnecessaryMutPassed { + fn get_lints(&self) -> LintArray { + lint_array!(UNNECESSARY_MUT_PASSED) + } +} + +impl LateLintPass for UnnecessaryMutPassed { + fn check_expr(&mut self, cx: &LateContext, e: &Expr) { + if let &ExprCall(ref fn_expr, ref arguments) = &e.node { + let borrowed_table = cx.tcx.tables.borrow(); + let funtion_type = match borrowed_table.node_types.get(&fn_expr.id) { + Some(funtion_type) => funtion_type, + None => unreachable!(), // A function with unknown type is called. + // If this happened the compiler would have aborted the + // compilation long ago. + }; + if let TypeVariants::TyBareFn(_, ref b) = funtion_type.sty { + let parameters = b.sig.skip_binder().inputs.clone(); + for (argument, parameter) in arguments.iter().zip(parameters.iter()) { + match parameter.sty { + TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | + TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { + if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { + if let ExprPath(_, path) = fn_expr.node.clone() { + span_lint(cx, UNNECESSARY_MUT_PASSED, + argument.span, &format!("This argument of the \ + function \"{}\" doesn't need to be mutable", path)); + } + } + }, + _ => {} + } + } + } + } + } +} From 40e180d8c777a7689b20033b9c9eba982a9cebda Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Tue, 29 Sep 2015 13:16:53 +0200 Subject: [PATCH 2/8] Replace tabs by spaces --- src/mut_reference.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/mut_reference.rs b/src/mut_reference.rs index 13cf1e1301e..96e78256f4e 100644 --- a/src/mut_reference.rs +++ b/src/mut_reference.rs @@ -23,28 +23,28 @@ impl LintPass for UnnecessaryMutPassed { impl LateLintPass for UnnecessaryMutPassed { fn check_expr(&mut self, cx: &LateContext, e: &Expr) { if let &ExprCall(ref fn_expr, ref arguments) = &e.node { - let borrowed_table = cx.tcx.tables.borrow(); - let funtion_type = match borrowed_table.node_types.get(&fn_expr.id) { - Some(funtion_type) => funtion_type, - None => unreachable!(), // A function with unknown type is called. - // If this happened the compiler would have aborted the - // compilation long ago. - }; + let borrowed_table = cx.tcx.tables.borrow(); + let funtion_type = match borrowed_table.node_types.get(&fn_expr.id) { + Some(funtion_type) => funtion_type, + None => unreachable!(), // A function with unknown type is called. + // If this happened the compiler would have aborted the + // compilation long ago. + }; if let TypeVariants::TyBareFn(_, ref b) = funtion_type.sty { let parameters = b.sig.skip_binder().inputs.clone(); for (argument, parameter) in arguments.iter().zip(parameters.iter()) { match parameter.sty { - TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | - TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { - if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { - if let ExprPath(_, path) = fn_expr.node.clone() { - span_lint(cx, UNNECESSARY_MUT_PASSED, + TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | + TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { + if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { + if let ExprPath(_, path) = fn_expr.node.clone() { + span_lint(cx, UNNECESSARY_MUT_PASSED, argument.span, &format!("This argument of the \ function \"{}\" doesn't need to be mutable", path)); - } - } - }, - _ => {} + } + } + }, + _ => {} } } } From e2a6c9e375c1d160133aadb202a948245e73e750 Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Tue, 29 Sep 2015 18:43:38 +0200 Subject: [PATCH 3/8] Add unnecessary mut passed lint for methods --- src/mut_reference.rs | 80 ++++++++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 25 deletions(-) diff --git a/src/mut_reference.rs b/src/mut_reference.rs index 96e78256f4e..16973ea987a 100644 --- a/src/mut_reference.rs +++ b/src/mut_reference.rs @@ -1,12 +1,12 @@ use rustc::lint::*; use rustc_front::hir::*; use utils::span_lint; -use rustc::middle::ty::{TypeAndMut, TypeVariants}; +use rustc::middle::ty::{TypeAndMut, TypeVariants, MethodCall}; declare_lint! { pub UNNECESSARY_MUT_PASSED, Warn, - "an argument is passed as a mutable reference although the function only demands an \ + "an argument is passed as a mutable reference although the function/method only demands an \ immutable reference" } @@ -22,32 +22,62 @@ impl LintPass for UnnecessaryMutPassed { impl LateLintPass for UnnecessaryMutPassed { fn check_expr(&mut self, cx: &LateContext, e: &Expr) { - if let &ExprCall(ref fn_expr, ref arguments) = &e.node { - let borrowed_table = cx.tcx.tables.borrow(); - let funtion_type = match borrowed_table.node_types.get(&fn_expr.id) { - Some(funtion_type) => funtion_type, - None => unreachable!(), // A function with unknown type is called. - // If this happened the compiler would have aborted the - // compilation long ago. - }; - if let TypeVariants::TyBareFn(_, ref b) = funtion_type.sty { - let parameters = b.sig.skip_binder().inputs.clone(); - for (argument, parameter) in arguments.iter().zip(parameters.iter()) { - match parameter.sty { - TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | - TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { - if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { - if let ExprPath(_, path) = fn_expr.node.clone() { - span_lint(cx, UNNECESSARY_MUT_PASSED, - argument.span, &format!("This argument of the \ - function \"{}\" doesn't need to be mutable", path)); + match e.node { + ExprCall(ref fn_expr, ref arguments) => { + let borrowed_table = cx.tcx.tables.borrow(); + let funtion_type = match borrowed_table.node_types.get(&fn_expr.id) { + Some(funtion_type) => funtion_type, + None => unreachable!(), // A function with unknown type is called. + // If this happened the compiler would have aborted the + // compilation long ago. + }; + if let TypeVariants::TyBareFn(_, ref b) = funtion_type.sty { + let parameters = b.sig.skip_binder().inputs.clone(); + for (argument, parameter) in arguments.iter().zip(parameters.iter()) { + match parameter.sty { + TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | + TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { + if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { + if let ExprPath(_, path) = fn_expr.node.clone() { + span_lint(cx, UNNECESSARY_MUT_PASSED, + argument.span, &format!("This argument of the \ + function \"{}\" doesn't need to be mutable", path)); + } } - } - }, - _ => {} + }, + _ => {} + } } } - } + }, + ExprMethodCall(ref name, _, ref arguments) => { + let method_call = MethodCall::expr(e.id); + let borrowed_table = cx.tcx.tables.borrow(); + let method_type = match borrowed_table.method_map.get(&method_call) { + Some(method_type) => method_type, + None => unreachable!(), // Just like above, this should never happen. + }; + if let TypeVariants::TyBareFn(_, ref b) = method_type.ty.sty { + let parameters = b.sig.skip_binder().inputs.iter().clone(); + for (argument, parameter) in arguments.iter().zip(parameters).skip(1) { + // Skip the first argument and the first parameter because it is the + // struct the function is called on. + match parameter.sty { + TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | + TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { + if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { + span_lint(cx, UNNECESSARY_MUT_PASSED, + argument.span, &format!("This argument of the \ + method \"{}\" doesn't need to be mutable", + name.node.as_str())); + } + }, + _ => {} + } + } + } + }, + _ => {} } } } From e42f00e470b1f61b02f325f1ba0c7795c0725bfc Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Tue, 29 Sep 2015 18:52:19 +0200 Subject: [PATCH 4/8] Change description of unnecessary mut passed lint --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 427f4181dea..5d65143f366 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ name [type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity) | warn | usage of very complex types; recommends factoring out parts into `type` definitions [unicode_not_nfc](https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc) | allow | using a unicode literal not in NFC normal form (see http://www.unicode.org/reports/tr15/ for further information) [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively) -[unnecessary_mut_passed](https://github.com/Manishearth/rust-clippy/wiki#unnecessary_mut_passed) | warn | an argument is passed as a mutable reference although the function only demands an immutable reference +[unnecessary_mut_passed](https://github.com/Manishearth/rust-clippy/wiki#unnecessary_mut_passed) | warn | an argument is passed as a mutable reference although the function/method only demands an immutable reference [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect) | warn | `collect()`ing an iterator without using the result; this is usually better written as a for loop [while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop) | warn | `loop { if let { ... } else break }` can be written as a `while let` loop [wrong_pub_self_convention](https://github.com/Manishearth/rust-clippy/wiki#wrong_pub_self_convention) | allow | defining a public method named with an established prefix (like "into_") that takes `self` with the wrong convention From 33a0799fa9777a4e2735faa8c4d6595db5cb6179 Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Wed, 30 Sep 2015 13:08:29 +0200 Subject: [PATCH 5/8] Remove unnecessary clones and add helper function --- src/mut_reference.rs | 79 ++++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 44 deletions(-) diff --git a/src/mut_reference.rs b/src/mut_reference.rs index 16973ea987a..1cc04e096ba 100644 --- a/src/mut_reference.rs +++ b/src/mut_reference.rs @@ -1,7 +1,8 @@ use rustc::lint::*; use rustc_front::hir::*; use utils::span_lint; -use rustc::middle::ty::{TypeAndMut, TypeVariants, MethodCall}; +use rustc::middle::ty::{TypeAndMut, TypeVariants, MethodCall, TyS}; +use syntax::ptr::P; declare_lint! { pub UNNECESSARY_MUT_PASSED, @@ -22,62 +23,52 @@ impl LintPass for UnnecessaryMutPassed { impl LateLintPass for UnnecessaryMutPassed { fn check_expr(&mut self, cx: &LateContext, e: &Expr) { + let borrowed_table = cx.tcx.tables.borrow(); match e.node { ExprCall(ref fn_expr, ref arguments) => { - let borrowed_table = cx.tcx.tables.borrow(); - let funtion_type = match borrowed_table.node_types.get(&fn_expr.id) { - Some(funtion_type) => funtion_type, + match borrowed_table.node_types.get(&fn_expr.id) { + Some(function_type) => { + if let ExprPath(_, ref path) = fn_expr.node { + check_arguments(cx, &arguments, function_type, + &format!("{}", path)); + } + }, None => unreachable!(), // A function with unknown type is called. // If this happened the compiler would have aborted the // compilation long ago. }; - if let TypeVariants::TyBareFn(_, ref b) = funtion_type.sty { - let parameters = b.sig.skip_binder().inputs.clone(); - for (argument, parameter) in arguments.iter().zip(parameters.iter()) { - match parameter.sty { - TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | - TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { - if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { - if let ExprPath(_, path) = fn_expr.node.clone() { - span_lint(cx, UNNECESSARY_MUT_PASSED, - argument.span, &format!("This argument of the \ - function \"{}\" doesn't need to be mutable", path)); - } - } - }, - _ => {} - } - } - } + + }, ExprMethodCall(ref name, _, ref arguments) => { let method_call = MethodCall::expr(e.id); - let borrowed_table = cx.tcx.tables.borrow(); - let method_type = match borrowed_table.method_map.get(&method_call) { - Some(method_type) => method_type, + match borrowed_table.method_map.get(&method_call) { + Some(method_type) => check_arguments(cx, &arguments, method_type.ty, + &format!("{}", name.node.as_str())), None => unreachable!(), // Just like above, this should never happen. }; - if let TypeVariants::TyBareFn(_, ref b) = method_type.ty.sty { - let parameters = b.sig.skip_binder().inputs.iter().clone(); - for (argument, parameter) in arguments.iter().zip(parameters).skip(1) { - // Skip the first argument and the first parameter because it is the - // struct the function is called on. - match parameter.sty { - TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | - TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { - if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { - span_lint(cx, UNNECESSARY_MUT_PASSED, - argument.span, &format!("This argument of the \ - method \"{}\" doesn't need to be mutable", - name.node.as_str())); - } - }, - _ => {} - } - } - } }, _ => {} } } } + +fn check_arguments(cx: &LateContext, arguments: &[P], type_definition: &TyS, name: &str) { + if let TypeVariants::TyBareFn(_, ref fn_type) = type_definition.sty { + let parameters = &fn_type.sig.skip_binder().inputs; + for (argument, parameter) in arguments.iter().zip(parameters.iter()) { + match parameter.sty { + TypeVariants::TyRef(_, TypeAndMut {ty: _, mutbl: MutImmutable}) | + TypeVariants::TyRawPtr(TypeAndMut {ty: _, mutbl: MutImmutable}) => { + if let Expr_::ExprAddrOf(MutMutable, _) = argument.node { + span_lint(cx, UNNECESSARY_MUT_PASSED, + argument.span, &format!("The function/method \"{}\" \ + doesn't need a mutable reference", + name)); + } + }, + _ => {} + } + } + } +} From 52aee99f6dcb2d7bb217c066d9b79d5fd61d1b45 Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Wed, 30 Sep 2015 13:27:09 +0200 Subject: [PATCH 6/8] Add test for unnecessary mut passed lint --- tests/compile-fail/mut_reference.rs | 43 +++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 tests/compile-fail/mut_reference.rs diff --git a/tests/compile-fail/mut_reference.rs b/tests/compile-fail/mut_reference.rs new file mode 100644 index 00000000000..8d3723b1752 --- /dev/null +++ b/tests/compile-fail/mut_reference.rs @@ -0,0 +1,43 @@ +#![feature(plugin)] +#![plugin(clippy)] + +fn takes_an_immutable_reference(a: &i32) { +} + +fn takes_a_mutable_reference(a: &mut i32) { +} + +struct MyStruct; + +impl MyStruct { + fn takes_an_immutable_reference(&self, a: &i32) { + } + + fn takes_a_mutable_reference(&self, a: &mut i32) { + } +} + +#[deny(unnecessary_mut_passed)] +fn main() { + // Functions + takes_an_immutable_reference(&mut 42); //~ERROR The function/method "takes_an_immutable_reference" doesn't need a mutable reference + + // Methods + let my_struct = MyStruct; + my_struct.takes_an_immutable_reference(&mut 42); //~ERROR The function/method "takes_an_immutable_reference" doesn't need a mutable reference + + + // No error + + // Functions + takes_an_immutable_reference(&42); + takes_a_mutable_reference(&mut 42); + let mut a = &mut 42; + takes_an_immutable_reference(a); + + // Methods + my_struct.takes_an_immutable_reference(&42); + my_struct.takes_a_mutable_reference(&mut 42); + my_struct.takes_an_immutable_reference(a); + +} From c5ab8d62e304e546ae685880272edd0ab949e69d Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Wed, 30 Sep 2015 18:00:14 +0200 Subject: [PATCH 7/8] Fix tests --- tests/compile-fail/for_loop.rs | 2 +- tests/compile-fail/mut_reference.rs | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index d6d73db3c18..11810242a88 100755 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -16,7 +16,7 @@ impl Unrelated { #[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)] #[deny(unused_collect)] -#[allow(linkedlist,shadow_unrelated)] +#[allow(linkedlist,shadow_unrelated,unnecessary_mut_passed)] fn main() { let mut vec = vec![1, 2, 3, 4]; let vec2 = vec![1, 2, 3, 4]; diff --git a/tests/compile-fail/mut_reference.rs b/tests/compile-fail/mut_reference.rs index 8d3723b1752..59c0aaf0c32 100644 --- a/tests/compile-fail/mut_reference.rs +++ b/tests/compile-fail/mut_reference.rs @@ -1,9 +1,12 @@ #![feature(plugin)] #![plugin(clippy)] +#![allow(unused_variable)] + fn takes_an_immutable_reference(a: &i32) { } + fn takes_a_mutable_reference(a: &mut i32) { } @@ -32,7 +35,7 @@ fn main() { // Functions takes_an_immutable_reference(&42); takes_a_mutable_reference(&mut 42); - let mut a = &mut 42; + let a = &mut 42; takes_an_immutable_reference(a); // Methods From 390168cc0f8eae3f30a49decd690f667ff211773 Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Wed, 30 Sep 2015 18:17:55 +0200 Subject: [PATCH 8/8] Well, fix them again --- tests/compile-fail/mut_reference.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/compile-fail/mut_reference.rs b/tests/compile-fail/mut_reference.rs index 59c0aaf0c32..7480add8e68 100644 --- a/tests/compile-fail/mut_reference.rs +++ b/tests/compile-fail/mut_reference.rs @@ -1,7 +1,7 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(unused_variable)] +#![allow(unused_variables)] fn takes_an_immutable_reference(a: &i32) { }