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` diff --git a/clippy_lints/src/drop_forget_ref.rs b/clippy_lints/src/drop_forget_ref.rs new file mode 100644 index 00000000000..3950cdb6acf --- /dev/null +++ b/clippy_lints/src/drop_forget_ref.rs @@ -0,0 +1,90 @@ +use rustc::lint::*; +use rustc::ty; +use rustc::hir::*; +use utils::{match_def_path, paths, span_note_and_lint}; + +/// **What it does:** Checks for calls to `std::mem::drop` with a reference +/// instead of an owned value. +/// +/// **Why is this bad?** Calling `drop` on a reference will only drop the +/// reference itself, which is a no-op. It will not call the `drop` method (from +/// the `Drop` trait implementation) on the underlying referenced value, which +/// is likely what was intended. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let mut lock_guard = mutex.lock(); +/// std::mem::drop(&lock_guard) // Should have been drop(lock_guard), mutex still locked +/// operation_that_requires_mutex_to_be_unlocked(); +/// ``` +declare_lint! { + pub DROP_REF, + Warn, + "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, FORGET_REF) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + if_let_chain!{[ + let ExprCall(ref path, ref args) = expr.node, + let ExprPath(ref qpath) = path.node, + 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, + lint, + expr.span, + msg, + arg.span, + &format!("argument has type {}", arg_ty.sty)); + } + }} + } +} diff --git a/clippy_lints/src/drop_ref.rs b/clippy_lints/src/drop_ref.rs deleted file mode 100644 index 106e43e1b6f..00000000000 --- a/clippy_lints/src/drop_ref.rs +++ /dev/null @@ -1,65 +0,0 @@ -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 -/// instead of an owned value. -/// -/// **Why is this bad?** Calling `drop` on a reference will only drop the -/// reference itself, which is a no-op. It will not call the `drop` method (from -/// the `Drop` trait implementation) on the underlying referenced value, which -/// is likely what was intended. -/// -/// **Known problems:** None. -/// -/// **Example:** -/// ```rust -/// let mut lock_guard = mutex.lock(); -/// std::mem::drop(&lock_guard) // Should have been drop(lock_guard), mutex still locked -/// operation_that_requires_mutex_to_be_unlocked(); -/// ``` -declare_lint! { - pub DROP_REF, - Warn, - "calls to `std::mem::drop` 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) - } -} - -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]); - } - } - } - } -} - -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)); - } -} 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, 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 8454a471513..00000000000 --- a/tests/compile-fail/drop_ref.rs +++ /dev/null @@ -1,43 +0,0 @@ -#![feature(plugin)] -#![plugin(clippy)] - -#![deny(drop_ref)] -#![allow(toplevel_ref_arg, similar_names)] - -use std::mem::drop; - -struct DroppableStruct; -impl Drop for DroppableStruct { fn drop(&mut self) {} } - -fn main() { - drop(&DroppableStruct); //~ERROR call to `std::mem::drop` with a reference argument - - let mut owned = DroppableStruct; - 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; - 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; - drop(reference2); //~ERROR call to `std::mem::drop` with a reference argument - - let ref reference3 = DroppableStruct; - 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(&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 -}