From 920a2b7468db034ae5accaa584aedb42f80f3112 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Thu, 29 Dec 2016 18:28:49 -0800 Subject: [PATCH 1/5] The drop_ref test does not require implementing the Drop trait. --- tests/compile-fail/drop_ref.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/tests/compile-fail/drop_ref.rs b/tests/compile-fail/drop_ref.rs index 8454a471513..76d7d9ca21b 100644 --- a/tests/compile-fail/drop_ref.rs +++ b/tests/compile-fail/drop_ref.rs @@ -6,26 +6,25 @@ use std::mem::drop; -struct DroppableStruct; -impl Drop for DroppableStruct { fn drop(&mut self) {} } +struct SomeStruct; fn main() { - drop(&DroppableStruct); //~ERROR call to `std::mem::drop` with a reference argument + drop(&SomeStruct); //~ERROR call to `std::mem::drop` with a reference argument - let mut owned = DroppableStruct; + let mut owned = SomeStruct; drop(&owned); //~ERROR call to `std::mem::drop` with a reference argument drop(&&owned); //~ERROR call to `std::mem::drop` with a reference argument drop(&mut owned); //~ERROR call to `std::mem::drop` with a reference argument drop(owned); //OK - let reference1 = &DroppableStruct; + let reference1 = &SomeStruct; drop(reference1); //~ERROR call to `std::mem::drop` with a reference argument drop(&*reference1); //~ERROR call to `std::mem::drop` with a reference argument - let reference2 = &mut DroppableStruct; + let reference2 = &mut SomeStruct; drop(reference2); //~ERROR call to `std::mem::drop` with a reference argument - let ref reference3 = DroppableStruct; + let ref reference3 = SomeStruct; drop(reference3); //~ERROR call to `std::mem::drop` with a reference argument } @@ -38,6 +37,6 @@ fn test_generic_fn(val: T) { #[allow(dead_code)] fn test_similarly_named_function() { fn drop(_val: T) {} - drop(&DroppableStruct); //OK; call to unrelated function which happens to have the same name - std::mem::drop(&DroppableStruct); //~ERROR call to `std::mem::drop` with a reference argument + drop(&SomeStruct); //OK; call to unrelated function which happens to have the same name + std::mem::drop(&SomeStruct); //~ERROR call to `std::mem::drop` with a reference argument } From 3050d90930c54e4552f286807751356514f997ba Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Thu, 29 Dec 2016 18:45:41 -0800 Subject: [PATCH 2/5] Add forget_ref tests. Also rename drop_ref.rs to drop_forget_ref.rs in tests/compile-fail. --- tests/compile-fail/drop_forget_ref.rs | 60 +++++++++++++++++++++++++++ tests/compile-fail/drop_ref.rs | 42 ------------------- 2 files changed, 60 insertions(+), 42 deletions(-) create mode 100644 tests/compile-fail/drop_forget_ref.rs delete mode 100644 tests/compile-fail/drop_ref.rs diff --git a/tests/compile-fail/drop_forget_ref.rs b/tests/compile-fail/drop_forget_ref.rs new file mode 100644 index 00000000000..55cfe63dac4 --- /dev/null +++ b/tests/compile-fail/drop_forget_ref.rs @@ -0,0 +1,60 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(drop_ref, forget_ref)] +#![allow(toplevel_ref_arg, similar_names)] + +use std::mem::{drop, forget}; + +struct SomeStruct; + +fn main() { + drop(&SomeStruct); //~ERROR call to `std::mem::drop` with a reference argument + forget(&SomeStruct); //~ERROR call to `std::mem::forget` with a reference argument + + let mut owned1 = SomeStruct; + drop(&owned1); //~ERROR call to `std::mem::drop` with a reference argument + drop(&&owned1); //~ERROR call to `std::mem::drop` with a reference argument + drop(&mut owned1); //~ERROR call to `std::mem::drop` with a reference argument + drop(owned1); //OK + let mut owned2 = SomeStruct; + forget(&owned2); //~ERROR call to `std::mem::forget` with a reference argument + forget(&&owned2); //~ERROR call to `std::mem::forget` with a reference argument + forget(&mut owned2); //~ERROR call to `std::mem::forget` with a reference argument + forget(owned2); //OK + + let reference1 = &SomeStruct; + drop(reference1); //~ERROR call to `std::mem::drop` with a reference argument + forget(&*reference1); //~ERROR call to `std::mem::forget` with a reference argument + + let reference2 = &mut SomeStruct; + drop(reference2); //~ERROR call to `std::mem::drop` with a reference argument + let reference3 = &mut SomeStruct; + forget(reference3); //~ERROR call to `std::mem::forget` with a reference argument + + let ref reference4 = SomeStruct; + drop(reference4); //~ERROR call to `std::mem::drop` with a reference argument + forget(reference4); //~ERROR call to `std::mem::forget` with a reference argument +} + +#[allow(dead_code)] +fn test_generic_fn_drop(val: T) { + drop(&val); //~ERROR call to `std::mem::drop` with a reference argument + drop(val); //OK +} + +#[allow(dead_code)] +fn test_generic_fn_forget(val: T) { + forget(&val); //~ERROR call to `std::mem::forget` with a reference argument + forget(val); //OK +} + +#[allow(dead_code)] +fn test_similarly_named_function() { + fn drop(_val: T) {} + drop(&SomeStruct); //OK; call to unrelated function which happens to have the same name + std::mem::drop(&SomeStruct); //~ERROR call to `std::mem::drop` with a reference argument + fn forget(_val: T) {} + forget(&SomeStruct); //OK; call to unrelated function which happens to have the same name + std::mem::forget(&SomeStruct); //~ERROR call to `std::mem::forget` with a reference argument +} diff --git a/tests/compile-fail/drop_ref.rs b/tests/compile-fail/drop_ref.rs deleted file mode 100644 index 76d7d9ca21b..00000000000 --- a/tests/compile-fail/drop_ref.rs +++ /dev/null @@ -1,42 +0,0 @@ -#![feature(plugin)] -#![plugin(clippy)] - -#![deny(drop_ref)] -#![allow(toplevel_ref_arg, similar_names)] - -use std::mem::drop; - -struct SomeStruct; - -fn main() { - drop(&SomeStruct); //~ERROR call to `std::mem::drop` with a reference argument - - let mut owned = SomeStruct; - drop(&owned); //~ERROR call to `std::mem::drop` with a reference argument - drop(&&owned); //~ERROR call to `std::mem::drop` with a reference argument - drop(&mut owned); //~ERROR call to `std::mem::drop` with a reference argument - drop(owned); //OK - - let reference1 = &SomeStruct; - drop(reference1); //~ERROR call to `std::mem::drop` with a reference argument - drop(&*reference1); //~ERROR call to `std::mem::drop` with a reference argument - - let reference2 = &mut SomeStruct; - drop(reference2); //~ERROR call to `std::mem::drop` with a reference argument - - let ref reference3 = SomeStruct; - drop(reference3); //~ERROR call to `std::mem::drop` with a reference argument -} - -#[allow(dead_code)] -fn test_generic_fn(val: T) { - drop(&val); //~ERROR call to `std::mem::drop` with a reference argument - drop(val); //OK -} - -#[allow(dead_code)] -fn test_similarly_named_function() { - fn drop(_val: T) {} - drop(&SomeStruct); //OK; call to unrelated function which happens to have the same name - std::mem::drop(&SomeStruct); //~ERROR call to `std::mem::drop` with a reference argument -} From f55d20ff46ac73154b33a1e543e250fa32c32789 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Thu, 29 Dec 2016 19:22:24 -0800 Subject: [PATCH 3/5] Refactor drop_ref.rs to use the if_let_chain macro. --- clippy_lints/src/drop_ref.rs | 41 +++++++++++++++--------------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/drop_ref.rs b/clippy_lints/src/drop_ref.rs index 106e43e1b6f..bddf47d3f45 100644 --- a/clippy_lints/src/drop_ref.rs +++ b/clippy_lints/src/drop_ref.rs @@ -1,7 +1,6 @@ use rustc::lint::*; use rustc::ty; use rustc::hir::*; -use syntax::codemap::Span; use utils::{match_def_path, paths, span_note_and_lint}; /// **What it does:** Checks for calls to `std::mem::drop` with a reference @@ -37,29 +36,23 @@ impl LintPass for Pass { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { - if let ExprCall(ref path, ref args) = expr.node { - if let ExprPath(ref qpath) = path.node { - let def_id = cx.tcx.tables().qpath_def(qpath, path.id).def_id(); - if match_def_path(cx, def_id, &paths::DROP) { - if args.len() != 1 { - return; - } - check_drop_arg(cx, expr.span, &args[0]); - } + if_let_chain!{[ + let ExprCall(ref path, ref args) = expr.node, + let ExprPath(ref qpath) = path.node, + match_def_path(cx, cx.tcx.tables().qpath_def(qpath, path.id).def_id(), &paths::DROP), + args.len() == 1, + ], { + let arg = &args[0]; + let arg_ty = cx.tcx.tables().expr_ty(arg); + if let ty::TyRef(..) = arg_ty.sty { + span_note_and_lint(cx, + DROP_REF, + expr.span, + "call to `std::mem::drop` with a reference argument. \ + Dropping a reference does nothing", + arg.span, + &format!("argument has type {}", arg_ty.sty)); } - } - } -} - -fn check_drop_arg(cx: &LateContext, call_span: Span, arg: &Expr) { - let arg_ty = cx.tcx.tables().expr_ty(arg); - if let ty::TyRef(..) = arg_ty.sty { - span_note_and_lint(cx, - DROP_REF, - call_span, - "call to `std::mem::drop` with a reference argument. \ - Dropping a reference does nothing", - arg.span, - &format!("argument has type {}", arg_ty.sty)); + }} } } From e4a4d0c3610b5cb73a8768cc09ad23a166c6d4ed Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Thu, 29 Dec 2016 19:43:22 -0800 Subject: [PATCH 4/5] Implement the forget_ref lint. --- .../src/{drop_ref.rs => drop_forget_ref.rs} | 42 ++++++++++++++++--- clippy_lints/src/lib.rs | 7 ++-- 2 files changed, 41 insertions(+), 8 deletions(-) rename clippy_lints/src/{drop_ref.rs => drop_forget_ref.rs} (54%) diff --git a/clippy_lints/src/drop_ref.rs b/clippy_lints/src/drop_forget_ref.rs similarity index 54% rename from clippy_lints/src/drop_ref.rs rename to clippy_lints/src/drop_forget_ref.rs index bddf47d3f45..3950cdb6acf 100644 --- a/clippy_lints/src/drop_ref.rs +++ b/clippy_lints/src/drop_forget_ref.rs @@ -25,12 +25,32 @@ declare_lint! { "calls to `std::mem::drop` with a reference instead of an owned value" } +/// **What it does:** Checks for calls to `std::mem::forget` with a reference +/// instead of an owned value. +/// +/// **Why is this bad?** Calling `forget` on a reference will only forget the +/// reference itself, which is a no-op. It will not forget the underlying referenced +/// value, which is likely what was intended. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let x = Box::new(1); +/// std::mem::forget(&x) // Should have been forget(x), x will still be dropped +/// ``` +declare_lint! { + pub FORGET_REF, + Warn, + "calls to `std::mem::forget` with a reference instead of an owned value" +} + #[allow(missing_copy_implementations)] pub struct Pass; impl LintPass for Pass { fn get_lints(&self) -> LintArray { - lint_array!(DROP_REF) + lint_array!(DROP_REF, FORGET_REF) } } @@ -39,17 +59,29 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if_let_chain!{[ let ExprCall(ref path, ref args) = expr.node, let ExprPath(ref qpath) = path.node, - match_def_path(cx, cx.tcx.tables().qpath_def(qpath, path.id).def_id(), &paths::DROP), args.len() == 1, ], { + let def_id = cx.tcx.tables().qpath_def(qpath, path.id).def_id(); + let lint; + let msg; + if match_def_path(cx, def_id, &paths::DROP) { + lint = DROP_REF; + msg = "call to `std::mem::drop` with a reference argument. \ + Dropping a reference does nothing"; + } else if match_def_path(cx, def_id, &paths::MEM_FORGET) { + lint = FORGET_REF; + msg = "call to `std::mem::forget` with a reference argument. \ + Forgetting a reference does nothing"; + } else { + return; + } let arg = &args[0]; let arg_ty = cx.tcx.tables().expr_ty(arg); if let ty::TyRef(..) = arg_ty.sty { span_note_and_lint(cx, - DROP_REF, + lint, expr.span, - "call to `std::mem::drop` with a reference argument. \ - Dropping a reference does nothing", + msg, arg.span, &format!("argument has type {}", arg_ty.sty)); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3e0aa17f0fb..f362ff12a85 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -72,7 +72,7 @@ pub mod cyclomatic_complexity; pub mod derive; pub mod doc; pub mod double_parens; -pub mod drop_ref; +pub mod drop_forget_ref; pub mod entry; pub mod enum_clike; pub mod enum_glob_use; @@ -259,7 +259,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_early_lint_pass(box non_expressive_names::NonExpressiveNames { max_single_char_names: conf.max_single_char_names, }); - reg.register_late_lint_pass(box drop_ref::Pass); + reg.register_late_lint_pass(box drop_forget_ref::Pass); reg.register_late_lint_pass(box types::AbsurdExtremeComparisons); reg.register_late_lint_pass(box types::InvalidUpcastComparisons); reg.register_late_lint_pass(box regex::Pass::default()); @@ -360,7 +360,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { derive::EXPL_IMPL_CLONE_ON_COPY, doc::DOC_MARKDOWN, double_parens::DOUBLE_PARENS, - drop_ref::DROP_REF, + drop_forget_ref::DROP_REF, + drop_forget_ref::FORGET_REF, entry::MAP_ENTRY, enum_clike::ENUM_CLIKE_UNPORTABLE_VARIANT, enum_variants::ENUM_VARIANT_NAMES, From 4962df30d0809777b84a93e043e094c7973ac2b8 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Thu, 29 Dec 2016 19:49:56 -0800 Subject: [PATCH 5/5] Update lint documentation using util/update_lints.py --- CHANGELOG.md | 1 + README.md | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f2608089ae..3d848aebe0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -298,6 +298,7 @@ All notable changes to this project will be documented in this file. [`for_kv_map`]: https://github.com/Manishearth/rust-clippy/wiki#for_kv_map [`for_loop_over_option`]: https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option [`for_loop_over_result`]: https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result +[`forget_ref`]: https://github.com/Manishearth/rust-clippy/wiki#forget_ref [`get_unwrap`]: https://github.com/Manishearth/rust-clippy/wiki#get_unwrap [`identity_op`]: https://github.com/Manishearth/rust-clippy/wiki#identity_op [`if_let_redundant_pattern_matching`]: https://github.com/Manishearth/rust-clippy/wiki#if_let_redundant_pattern_matching diff --git a/README.md b/README.md index 85c6136b99e..f0a362295fc 100644 --- a/README.md +++ b/README.md @@ -179,7 +179,7 @@ transparently: ## Lints -There are 182 lints included in this crate: +There are 183 lints included in this crate: name | default | triggers on -----------------------------------------------------------------------------------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------- @@ -237,6 +237,7 @@ name [for_kv_map](https://github.com/Manishearth/rust-clippy/wiki#for_kv_map) | warn | looping on a map using `iter` when `keys` or `values` would do [for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option) | warn | for-looping over an `Option`, which is more clearly expressed as an `if let` [for_loop_over_result](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result) | warn | for-looping over a `Result`, which is more clearly expressed as an `if let` +[forget_ref](https://github.com/Manishearth/rust-clippy/wiki#forget_ref) | warn | calls to `std::mem::forget` with a reference instead of an owned value [get_unwrap](https://github.com/Manishearth/rust-clippy/wiki#get_unwrap) | warn | using `.get().unwrap()` or `.get_mut().unwrap()` when using `[]` would work instead [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1` [if_let_redundant_pattern_matching](https://github.com/Manishearth/rust-clippy/wiki#if_let_redundant_pattern_matching) | warn | use the proper utility function avoiding an `if let`