From bd86922c4f33e2528bb87a7f693e57935970f1cf Mon Sep 17 00:00:00 2001 From: inrustwetrust Date: Mon, 1 Feb 2016 19:53:03 +0100 Subject: [PATCH] Add lint to warn for calls to `std::mem::drop` with a reference argument --- README.md | 3 +- src/drop_ref.rs | 61 ++++++++++++++++++++++++++++++++++ src/lib.rs | 3 ++ src/utils.rs | 1 + tests/compile-fail/drop_ref.rs | 43 ++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 src/drop_ref.rs create mode 100644 tests/compile-fail/drop_ref.rs diff --git a/README.md b/README.md index 9b249636c66..d24dc3de1bd 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 106 lints included in this crate: +There are 107 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -28,6 +28,7 @@ name [cyclomatic_complexity](https://github.com/Manishearth/rust-clippy/wiki#cyclomatic_complexity) | warn | finds functions that should be split up into multiple functions [deprecated_semver](https://github.com/Manishearth/rust-clippy/wiki#deprecated_semver) | warn | `Warn` on `#[deprecated(since = "x")]` where x is not semver [derive_hash_not_eq](https://github.com/Manishearth/rust-clippy/wiki#derive_hash_not_eq) | warn | deriving `Hash` but implementing `PartialEq` explicitly +[drop_ref](https://github.com/Manishearth/rust-clippy/wiki#drop_ref) | warn | call to `std::mem::drop` with a reference instead of an owned value, which will not not call the `Drop::drop` method on the underlying value [duplicate_underscore_argument](https://github.com/Manishearth/rust-clippy/wiki#duplicate_underscore_argument) | warn | Function arguments having names which only differ by an underscore [empty_loop](https://github.com/Manishearth/rust-clippy/wiki#empty_loop) | warn | empty `loop {}` detected [enum_variant_names](https://github.com/Manishearth/rust-clippy/wiki#enum_variant_names) | warn | finds enums where all variants share a prefix/postfix diff --git a/src/drop_ref.rs b/src/drop_ref.rs new file mode 100644 index 00000000000..fecb69f5c14 --- /dev/null +++ b/src/drop_ref.rs @@ -0,0 +1,61 @@ +use rustc::lint::*; +use rustc_front::hir::*; +use rustc::middle::ty; +use syntax::codemap::Span; + +use utils::DROP_PATH; +use utils::{match_def_path, span_note_and_lint}; + +/// **What it does:** This lint 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, + "call to `std::mem::drop` with a reference instead of an owned value, \ + which will not not call the `Drop::drop` method on the underlying value"); + +#[allow(missing_copy_implementations)] +pub struct DropRefPass; + +impl LintPass for DropRefPass { + fn get_lints(&self) -> LintArray { + lint_array!(DROP_REF) + } +} + +impl LateLintPass for DropRefPass { + fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { + if let ExprCall(ref path, ref args) = expr.node { + if let ExprPath(None, _) = path.node { + let def_id = cx.tcx.def_map.borrow()[&path.id].def_id(); + if match_def_path(cx, def_id, &DROP_PATH) { + 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.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/src/lib.rs b/src/lib.rs index 15d1db19d97..e2596dd58ca 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -81,6 +81,7 @@ pub mod panic; pub mod derive; pub mod print; pub mod vec; +pub mod drop_ref; mod reexport { pub use syntax::ast::{Name, NodeId}; @@ -148,6 +149,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box types::CharLitAsU8); reg.register_late_lint_pass(box print::PrintLint); reg.register_late_lint_pass(box vec::UselessVec); + reg.register_late_lint_pass(box drop_ref::DropRefPass); reg.register_lint_group("clippy_pedantic", vec![ @@ -184,6 +186,7 @@ pub fn plugin_registrar(reg: &mut Registry) { cyclomatic_complexity::CYCLOMATIC_COMPLEXITY, derive::DERIVE_HASH_NOT_EQ, derive::EXPL_IMPL_CLONE_ON_COPY, + drop_ref::DROP_REF, entry::MAP_ENTRY, enum_variants::ENUM_VARIANT_NAMES, eq_op::EQ_OP, diff --git a/src/utils.rs b/src/utils.rs index ff0c59ab290..bc6fcd44891 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -27,6 +27,7 @@ pub const CLONE_PATH: [&'static str; 3] = ["clone", "Clone", "clone"]; pub const CLONE_TRAIT_PATH: [&'static str; 2] = ["clone", "Clone"]; pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"]; pub const DEFAULT_TRAIT_PATH: [&'static str; 3] = ["core", "default", "Default"]; +pub const DROP_PATH: [&'static str; 3] = ["core", "mem", "drop"]; pub const HASHMAP_ENTRY_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "Entry"]; pub const HASHMAP_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "HashMap"]; pub const HASH_PATH: [&'static str; 2] = ["hash", "Hash"]; diff --git a/tests/compile-fail/drop_ref.rs b/tests/compile-fail/drop_ref.rs new file mode 100644 index 00000000000..3e4c0a9d8ec --- /dev/null +++ b/tests/compile-fail/drop_ref.rs @@ -0,0 +1,43 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(drop_ref)] +#![allow(toplevel_ref_arg)] + +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 +}