diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a7463ba0..4ace142aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -330,6 +330,7 @@ All notable changes to this project will be documented in this file. [`doc_markdown`]: https://github.com/Manishearth/rust-clippy/wiki#doc_markdown [`double_neg`]: https://github.com/Manishearth/rust-clippy/wiki#double_neg [`double_parens`]: https://github.com/Manishearth/rust-clippy/wiki#double_parens +[`drop_copy`]: https://github.com/Manishearth/rust-clippy/wiki#drop_copy [`drop_ref`]: https://github.com/Manishearth/rust-clippy/wiki#drop_ref [`duplicate_underscore_argument`]: https://github.com/Manishearth/rust-clippy/wiki#duplicate_underscore_argument [`empty_enum`]: https://github.com/Manishearth/rust-clippy/wiki#empty_enum @@ -351,6 +352,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_copy`]: https://github.com/Manishearth/rust-clippy/wiki#forget_copy [`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 diff --git a/README.md b/README.md index f0e933ee8..1cfc1fafe 100644 --- a/README.md +++ b/README.md @@ -180,7 +180,7 @@ transparently: ## Lints -There are 194 lints included in this crate: +There are 196 lints included in this crate: name | default | triggers on -----------------------------------------------------------------------------------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------- @@ -218,6 +218,7 @@ name [doc_markdown](https://github.com/Manishearth/rust-clippy/wiki#doc_markdown) | warn | presence of `_`, `::` or camel-case outside backticks in documentation [double_neg](https://github.com/Manishearth/rust-clippy/wiki#double_neg) | warn | `--x`, which is a double negation of `x` and not a pre-decrement as in C/C++ [double_parens](https://github.com/Manishearth/rust-clippy/wiki#double_parens) | warn | Warn on unnecessary double parentheses +[drop_copy](https://github.com/Manishearth/rust-clippy/wiki#drop_copy) | warn | calls to `std::mem::drop` with a value that implements Copy [drop_ref](https://github.com/Manishearth/rust-clippy/wiki#drop_ref) | warn | calls to `std::mem::drop` with a reference instead of an owned 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_enum](https://github.com/Manishearth/rust-clippy/wiki#empty_enum) | allow | enum with no variants @@ -238,6 +239,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_copy](https://github.com/Manishearth/rust-clippy/wiki#forget_copy) | warn | calls to `std::mem::forget` with a value that implements Copy [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` diff --git a/clippy_lints/src/drop_forget_ref.rs b/clippy_lints/src/drop_forget_ref.rs index c0c9a7e68..688f1cc9e 100644 --- a/clippy_lints/src/drop_forget_ref.rs +++ b/clippy_lints/src/drop_forget_ref.rs @@ -1,7 +1,7 @@ use rustc::lint::*; use rustc::ty; use rustc::hir::*; -use utils::{match_def_path, paths, span_note_and_lint}; +use utils::{match_def_path, paths, span_note_and_lint, is_copy}; /// **What it does:** Checks for calls to `std::mem::drop` with a reference /// instead of an owned value. @@ -45,12 +45,65 @@ declare_lint! { "calls to `std::mem::forget` with a reference instead of an owned value" } +/// **What it does:** Checks for calls to `std::mem::drop` with a value +/// that derives the Copy trait +/// +/// **Why is this bad?** Calling `std::mem::drop` [does nothing for types that +/// implement Copy](https://doc.rust-lang.org/std/mem/fn.drop.html), since the +/// value will be copied and moved into the function on invocation. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let x:i32 = 42; // i32 implements Copy +/// std::mem::drop(x) // A copy of x is passed to the function, leaving the original unaffected +/// ``` +declare_lint! { + pub DROP_COPY, + Warn, + "calls to `std::mem::drop` with a value that implements Copy" +} + +/// **What it does:** Checks for calls to `std::mem::forget` with a value that +/// derives the Copy trait +/// +/// **Why is this bad?** Calling `std::mem::forget` [does nothing for types that +/// implement Copy](https://doc.rust-lang.org/std/mem/fn.drop.html) since the +/// value will be copied and moved into the function on invocation. +/// +/// An alternative, but also valid, explanation is that Copy types do not implement +/// the Drop trait, which means they have no destructors. Without a destructor, there +/// is nothing for `std::mem::forget` to ignore. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let x:i32 = 42; // i32 implements Copy +/// std::mem::forget(x) // A copy of x is passed to the function, leaving the original unaffected +/// ``` +declare_lint! { + pub FORGET_COPY, + Warn, + "calls to `std::mem::forget` with a value that implements Copy" +} + +const DROP_REF_SUMMARY: &str = "calls to `std::mem::drop` with a reference instead of an owned value. \ + Dropping a reference does nothing."; +const FORGET_REF_SUMMARY: &str = "calls to `std::mem::forget` with a reference instead of an owned value. \ + Forgetting a reference does nothing."; +const DROP_COPY_SUMMARY: &str = "calls to `std::mem::drop` with a value that implements Copy. \ + Dropping a copy leaves the original intact."; +const FORGET_COPY_SUMMARY: &str = "calls to `std::mem::forget` with a value that implements Copy. \ + Forgetting a copy leaves the original intact."; + #[allow(missing_copy_implementations)] pub struct Pass; impl LintPass for Pass { fn get_lints(&self) -> LintArray { - lint_array!(DROP_REF, FORGET_REF) + lint_array!(DROP_REF, FORGET_REF, DROP_COPY, FORGET_COPY) } } @@ -64,24 +117,39 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { let def_id = cx.tables.qpath_def(qpath, path.id).def_id(); let lint; let msg; - if match_def_path(cx.tcx, 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.tcx, 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.tables.expr_ty(arg); + if let ty::TyRef(..) = arg_ty.sty { + if match_def_path(cx.tcx, def_id, &paths::DROP) { + lint = DROP_REF; + msg = DROP_REF_SUMMARY.to_string(); + } else if match_def_path(cx.tcx, def_id, &paths::MEM_FORGET) { + lint = FORGET_REF; + msg = FORGET_REF_SUMMARY.to_string(); + } else { + return; + } span_note_and_lint(cx, lint, expr.span, - msg, + &msg, + arg.span, + &format!("argument has type {}", arg_ty.sty)); + } else if is_copy(cx, arg_ty, cx.tcx.hir.get_parent(arg.id)) { + if match_def_path(cx.tcx, def_id, &paths::DROP) { + lint = DROP_COPY; + msg = DROP_COPY_SUMMARY.to_string(); + } else if match_def_path(cx.tcx, def_id, &paths::MEM_FORGET) { + lint = FORGET_COPY; + msg = FORGET_COPY_SUMMARY.to_string(); + } else { + return; + } + span_note_and_lint(cx, + lint, + expr.span, + &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 c78804240..068bc5d2e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -374,7 +374,9 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { derive::EXPL_IMPL_CLONE_ON_COPY, doc::DOC_MARKDOWN, double_parens::DOUBLE_PARENS, + drop_forget_ref::DROP_COPY, drop_forget_ref::DROP_REF, + drop_forget_ref::FORGET_COPY, drop_forget_ref::FORGET_REF, entry::MAP_ENTRY, enum_clike::ENUM_CLIKE_UNPORTABLE_VARIANT, diff --git a/tests/ui/drop_forget_copy.rs b/tests/ui/drop_forget_copy.rs new file mode 100644 index 000000000..d92c05347 --- /dev/null +++ b/tests/ui/drop_forget_copy.rs @@ -0,0 +1,62 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(drop_copy, forget_copy)] +#![allow(toplevel_ref_arg, drop_ref, forget_ref, unused_mut)] + +use std::mem::{drop, forget}; +use std::vec::Vec; + +#[derive(Copy, Clone)] +struct SomeStruct { +} + +struct AnotherStruct { + x: u8, + y: u8, + z: Vec +} + +impl Clone for AnotherStruct { + fn clone(& self) -> AnotherStruct { + AnotherStruct{x: self.x, y: self.y, z: self.z.clone()} + } +} + +fn main() { + let s1 = SomeStruct {}; + let s2 = s1; + let s3 = &s1; + let mut s4 = s1; + let ref s5 = s1; + + drop(s1); + drop(s2); + drop(s3); + drop(s4); + drop(s5); + + forget(s1); + forget(s2); + forget(s3); + forget(s4); + forget(s5); + + let a1 = AnotherStruct {x: 255, y: 0, z: vec![1, 2, 3]}; + let a2 = &a1; + let mut a3 = a1.clone(); + let ref a4 = a1; + let a5 = a1.clone(); + + drop(a2); + drop(a3); + drop(a4); + drop(a5); + + forget(a2); + let a3 = &a1; + forget(a3); + forget(a4); + let a5 = a1.clone(); + forget(a5); +} diff --git a/tests/ui/drop_forget_copy.stderr b/tests/ui/drop_forget_copy.stderr new file mode 100644 index 000000000..5d95e9f86 --- /dev/null +++ b/tests/ui/drop_forget_copy.stderr @@ -0,0 +1,84 @@ +error: calls to `std::mem::drop` with a value that implements Copy. Dropping a copy leaves the original intact. + --> $DIR/drop_forget_copy.rs:33:5 + | +33 | drop(s1); + | ^^^^^^^^ + | +note: lint level defined here + --> $DIR/drop_forget_copy.rs:4:9 + | +4 | #![deny(drop_copy, forget_copy)] + | ^^^^^^^^^ +note: argument has type SomeStruct + --> $DIR/drop_forget_copy.rs:33:10 + | +33 | drop(s1); + | ^^ + +error: calls to `std::mem::drop` with a value that implements Copy. Dropping a copy leaves the original intact. + --> $DIR/drop_forget_copy.rs:34:5 + | +34 | drop(s2); + | ^^^^^^^^ + | +note: argument has type SomeStruct + --> $DIR/drop_forget_copy.rs:34:10 + | +34 | drop(s2); + | ^^ + +error: calls to `std::mem::drop` with a value that implements Copy. Dropping a copy leaves the original intact. + --> $DIR/drop_forget_copy.rs:36:5 + | +36 | drop(s4); + | ^^^^^^^^ + | +note: argument has type SomeStruct + --> $DIR/drop_forget_copy.rs:36:10 + | +36 | drop(s4); + | ^^ + +error: calls to `std::mem::forget` with a value that implements Copy. Forgetting a copy leaves the original intact. + --> $DIR/drop_forget_copy.rs:39:5 + | +39 | forget(s1); + | ^^^^^^^^^^ + | +note: lint level defined here + --> $DIR/drop_forget_copy.rs:4:20 + | +4 | #![deny(drop_copy, forget_copy)] + | ^^^^^^^^^^^ +note: argument has type SomeStruct + --> $DIR/drop_forget_copy.rs:39:12 + | +39 | forget(s1); + | ^^ + +error: calls to `std::mem::forget` with a value that implements Copy. Forgetting a copy leaves the original intact. + --> $DIR/drop_forget_copy.rs:40:5 + | +40 | forget(s2); + | ^^^^^^^^^^ + | +note: argument has type SomeStruct + --> $DIR/drop_forget_copy.rs:40:12 + | +40 | forget(s2); + | ^^ + +error: calls to `std::mem::forget` with a value that implements Copy. Forgetting a copy leaves the original intact. + --> $DIR/drop_forget_copy.rs:42:5 + | +42 | forget(s4); + | ^^^^^^^^^^ + | +note: argument has type SomeStruct + --> $DIR/drop_forget_copy.rs:42:12 + | +42 | forget(s4); + | ^^ + +error: aborting due to 6 previous errors + diff --git a/tests/ui/drop_forget_ref.stderr b/tests/ui/drop_forget_ref.stderr index 80c724c19..b1d674604 100644 --- a/tests/ui/drop_forget_ref.stderr +++ b/tests/ui/drop_forget_ref.stderr @@ -1,4 +1,4 @@ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:12:5 | 12 | drop(&SomeStruct); @@ -15,7 +15,7 @@ note: argument has type &SomeStruct 12 | drop(&SomeStruct); | ^^^^^^^^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:13:5 | 13 | forget(&SomeStruct); @@ -32,7 +32,7 @@ note: argument has type &SomeStruct 13 | forget(&SomeStruct); | ^^^^^^^^^^^ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:16:5 | 16 | drop(&owned1); @@ -44,7 +44,7 @@ note: argument has type &SomeStruct 16 | drop(&owned1); | ^^^^^^^ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:17:5 | 17 | drop(&&owned1); @@ -56,7 +56,7 @@ note: argument has type &&SomeStruct 17 | drop(&&owned1); | ^^^^^^^^ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:18:5 | 18 | drop(&mut owned1); @@ -68,7 +68,7 @@ note: argument has type &mut SomeStruct 18 | drop(&mut owned1); | ^^^^^^^^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:21:5 | 21 | forget(&owned2); @@ -80,7 +80,7 @@ note: argument has type &SomeStruct 21 | forget(&owned2); | ^^^^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:22:5 | 22 | forget(&&owned2); @@ -92,7 +92,7 @@ note: argument has type &&SomeStruct 22 | forget(&&owned2); | ^^^^^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:23:5 | 23 | forget(&mut owned2); @@ -104,7 +104,7 @@ note: argument has type &mut SomeStruct 23 | forget(&mut owned2); | ^^^^^^^^^^^ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:27:5 | 27 | drop(reference1); @@ -116,7 +116,7 @@ note: argument has type &SomeStruct 27 | drop(reference1); | ^^^^^^^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:28:5 | 28 | forget(&*reference1); @@ -128,7 +128,7 @@ note: argument has type &SomeStruct 28 | forget(&*reference1); | ^^^^^^^^^^^^ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:31:5 | 31 | drop(reference2); @@ -140,7 +140,7 @@ note: argument has type &mut SomeStruct 31 | drop(reference2); | ^^^^^^^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:33:5 | 33 | forget(reference3); @@ -152,7 +152,7 @@ note: argument has type &mut SomeStruct 33 | forget(reference3); | ^^^^^^^^^^ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:36:5 | 36 | drop(reference4); @@ -164,7 +164,7 @@ note: argument has type &SomeStruct 36 | drop(reference4); | ^^^^^^^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:37:5 | 37 | forget(reference4); @@ -176,7 +176,7 @@ note: argument has type &SomeStruct 37 | forget(reference4); | ^^^^^^^^^^ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:42:5 | 42 | drop(&val); @@ -188,7 +188,7 @@ note: argument has type &T 42 | drop(&val); | ^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:48:5 | 48 | forget(&val); @@ -200,7 +200,7 @@ note: argument has type &T 48 | forget(&val); | ^^^^ -error: call to `std::mem::drop` with a reference argument. Dropping a reference does nothing +error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. --> $DIR/drop_forget_ref.rs:56:5 | 56 | std::mem::drop(&SomeStruct); @@ -212,7 +212,7 @@ note: argument has type &SomeStruct 56 | std::mem::drop(&SomeStruct); | ^^^^^^^^^^^ -error: call to `std::mem::forget` with a reference argument. Forgetting a reference does nothing +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. --> $DIR/drop_forget_ref.rs:59:5 | 59 | std::mem::forget(&SomeStruct); diff --git a/tests/ui/mem_forget.rs b/tests/ui/mem_forget.rs index 9832bd992..e553afcf0 100644 --- a/tests/ui/mem_forget.rs +++ b/tests/ui/mem_forget.rs @@ -1,6 +1,7 @@ #![feature(plugin)] #![plugin(clippy)] + use std::sync::Arc; use std::rc::Rc; @@ -8,6 +9,7 @@ use std::mem::forget as forgetSomething; use std::mem as memstuff; #[deny(mem_forget)] +#[allow(forget_copy)] fn main() { let five: i32 = 5; forgetSomething(five); diff --git a/tests/ui/mem_forget.stderr b/tests/ui/mem_forget.stderr index 47c61adfb..21cb65f82 100644 --- a/tests/ui/mem_forget.stderr +++ b/tests/ui/mem_forget.stderr @@ -1,25 +1,25 @@ error: usage of mem::forget on Drop type - --> $DIR/mem_forget.rs:16:5 + --> $DIR/mem_forget.rs:18:5 | -16 | memstuff::forget(six); +18 | memstuff::forget(six); | ^^^^^^^^^^^^^^^^^^^^^ | note: lint level defined here - --> $DIR/mem_forget.rs:10:8 + --> $DIR/mem_forget.rs:11:8 | -10 | #[deny(mem_forget)] +11 | #[deny(mem_forget)] | ^^^^^^^^^^ error: usage of mem::forget on Drop type - --> $DIR/mem_forget.rs:20:5 + --> $DIR/mem_forget.rs:22:5 | -20 | std::mem::forget(seven); +22 | std::mem::forget(seven); | ^^^^^^^^^^^^^^^^^^^^^^^ error: usage of mem::forget on Drop type - --> $DIR/mem_forget.rs:24:5 + --> $DIR/mem_forget.rs:26:5 | -24 | forgetSomething(eight); +26 | forgetSomething(eight); | ^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to 3 previous errors