Merge pull request #610 from inrustwetrust/drop_ref_lint

Add lint to warn for calls to `std::mem::drop` with a reference argument

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

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));
+    }
+}

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,

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"];

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<T>(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<T>(_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
+}