New cmp_null lint (fixes #1184) (#1186)

* new cmp_null lint (fixes #1184)

* adressed comments (still fails)

* fixed tests, dogfood, ran update_lints

CHANGELOG.md

@@ -180,6 +180,7 @@ All notable changes to this project will be documented in this file.
 [`clone_double_ref`]: https://github.com/Manishearth/rust-clippy/wiki#clone_double_ref
 [`clone_on_copy`]: https://github.com/Manishearth/rust-clippy/wiki#clone_on_copy
 [`cmp_nan`]: https://github.com/Manishearth/rust-clippy/wiki#cmp_nan
+[`cmp_null`]: https://github.com/Manishearth/rust-clippy/wiki#cmp_null
 [`cmp_owned`]: https://github.com/Manishearth/rust-clippy/wiki#cmp_owned
 [`collapsible_if`]: https://github.com/Manishearth/rust-clippy/wiki#collapsible_if
 [`crosspointer_transmute`]: https://github.com/Manishearth/rust-clippy/wiki#crosspointer_transmute

README.md

@@ -17,7 +17,7 @@ Table of contents:
 
 ## Lints
 
-There are 166 lints included in this crate:
+There are 167 lints included in this crate:
 
 name                                                                                                                 | default | triggers on
 ---------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -42,6 +42,7 @@ name
 [clone_double_ref](https://github.com/Manishearth/rust-clippy/wiki#clone_double_ref)                                 | warn    | using `clone` on `&&T`
 [clone_on_copy](https://github.com/Manishearth/rust-clippy/wiki#clone_on_copy)                                       | warn    | using `clone` on a `Copy` type
 [cmp_nan](https://github.com/Manishearth/rust-clippy/wiki#cmp_nan)                                                   | deny    | comparisons to NAN, which will always return false, probably not intended
+[cmp_null](https://github.com/Manishearth/rust-clippy/wiki#cmp_null)                                                 | warn    | comparing a pointer to a null pointer, suggesting to use `.is_null()` instead.
 [cmp_owned](https://github.com/Manishearth/rust-clippy/wiki#cmp_owned)                                               | warn    | creating owned instances for comparing with others, e.g. `x == "foo".to_string()`
 [collapsible_if](https://github.com/Manishearth/rust-clippy/wiki#collapsible_if)                                     | warn    | `if`s that can be collapsed (e.g. `if x { if y { ... } }` and `else { if x { ... } }`)
 [crosspointer_transmute](https://github.com/Manishearth/rust-clippy/wiki#crosspointer_transmute)                     | warn    | transmutes that have to or from types that are a pointer to the other
@@ -131,7 +132,7 @@ name
 [precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence)                                             | warn    | operations where precedence may be unclear
 [print_stdout](https://github.com/Manishearth/rust-clippy/wiki#print_stdout)                                         | allow   | printing on stdout
 [print_with_newline](https://github.com/Manishearth/rust-clippy/wiki#print_with_newline)                             | warn    | using `print!()` with a format string that ends in a newline
-[ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg)                                                   | warn    | arguments of the type `&Vec<...>` (instead of `&[...]`) or `&String` (instead of `&str`)
+[ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg)                                                   | warn    | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively
 [range_step_by_zero](https://github.com/Manishearth/rust-clippy/wiki#range_step_by_zero)                             | warn    | using `Range::step_by(0)`, which produces an infinite iterator
 [range_zip_with_len](https://github.com/Manishearth/rust-clippy/wiki#range_zip_with_len)                             | warn    | zipping iterator with a range when `enumerate()` would do
 [redundant_closure](https://github.com/Manishearth/rust-clippy/wiki#redundant_closure)                               | warn    | redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`)

clippy_lints/src/lib.rs

@@ -112,7 +112,7 @@ pub mod overflow_check_conditional;
 pub mod panic;
 pub mod precedence;
 pub mod print;
-pub mod ptr_arg;
+pub mod ptr;
 pub mod ranges;
 pub mod regex;
 pub mod returns;
@@ -182,7 +182,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box enum_glob_use::EnumGlobUse);
     reg.register_late_lint_pass(box enum_clike::UnportableVariant);
     reg.register_late_lint_pass(box bit_mask::BitMask);
-    reg.register_late_lint_pass(box ptr_arg::PtrArg);
+    reg.register_late_lint_pass(box ptr::PointerPass);
     reg.register_late_lint_pass(box needless_bool::NeedlessBool);
     reg.register_late_lint_pass(box needless_bool::BoolComparison);
     reg.register_late_lint_pass(box approx_const::Pass);
@@ -405,7 +405,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         panic::PANIC_PARAMS,
         precedence::PRECEDENCE,
         print::PRINT_WITH_NEWLINE,
-        ptr_arg::PTR_ARG,
+        ptr::CMP_NULL,
+        ptr::PTR_ARG,
         ranges::RANGE_STEP_BY_ZERO,
         ranges::RANGE_ZIP_WITH_LEN,
         regex::INVALID_REGEX,

clippy_lints/src/ptr.rs

@@ -5,14 +5,14 @@ use rustc::hir::map::NodeItem;
 use rustc::lint::*;
 use rustc::ty;
 use syntax::ast::NodeId;
-use utils::{match_type, paths, span_lint};
+use utils::{match_path, match_type, paths, span_lint};
 
-/// **What it does:** Checks for function arguments of type `&String` or `&Vec`
-/// unless the references are mutable.
+/// **What it does:** This lint checks for function arguments of type `&String` or `&Vec` unless
+/// the references are mutable.
 ///
-/// **Why is this bad?** Requiring the argument to be of the specific size makes
-/// the function less useful for no benefit; slices in the form of `&[T]` or
-/// `&str` usually suffice and can be obtained from other types, too.
+/// **Why is this bad?** Requiring the argument to be of the specific size makes the function less
+/// useful for no benefit; slices in the form of `&[T]` or `&str` usually suffice and can be
+/// obtained from other types, too.
 ///
 /// **Known problems:** None.
 ///
@@ -23,19 +23,38 @@ use utils::{match_type, paths, span_lint};
 declare_lint! {
     pub PTR_ARG,
     Warn,
-    "arguments of the type `&Vec<...>` (instead of `&[...]`) or `&String` (instead of `&str`)"
+    "fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` \
+     instead, respectively"
 }
 
+/// **What it does:** This lint checks for equality comparisons with `ptr::null`
+///
+/// **Why is this bad?** It's easier and more readable to use the inherent `.is_null()`
+/// method instead
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// if x == ptr::null { .. }
+/// ```
+declare_lint! {
+    pub CMP_NULL,
+    Warn,
+    "comparing a pointer to a null pointer, suggesting to use `.is_null()` instead."
+}
+
+
 #[derive(Copy,Clone)]
-pub struct PtrArg;
+pub struct PointerPass;
 
-impl LintPass for PtrArg {
+impl LintPass for PointerPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(PTR_ARG)
+        lint_array!(PTR_ARG, CMP_NULL)
     }
 }
 
-impl LateLintPass for PtrArg {
+impl LateLintPass for PointerPass {
     fn check_item(&mut self, cx: &LateContext, item: &Item) {
         if let ItemFn(ref decl, _, _, _, _, _) = item.node {
             check_fn(cx, decl, item.id);
@@ -58,6 +77,17 @@ impl LateLintPass for PtrArg {
             check_fn(cx, &sig.decl, item.id);
         }
     }
+    
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+        if let ExprBinary(ref op, ref l, ref r) = expr.node {
+            if (op.node == BiEq || op.node == BiNe) && (is_null_path(l) || is_null_path(r)) {
+                span_lint(cx,
+                          CMP_NULL,
+                          expr.span,
+                          "Comparing with null is better expressed by the .is_null() method");
+            }
+        }
+    }
 }
 
 fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
@@ -81,3 +111,14 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
         }
     }
 }
+
+fn is_null_path(expr: &Expr) -> bool {
+    if let ExprCall(ref pathexp, ref args) = expr.node {
+        if args.is_empty() {
+            if let ExprPath(_, ref path) = pathexp.node {
+                return match_path(path, &paths::PTR_NULL) || match_path(path, &paths::PTR_NULL_MUT)
+            }
+        }
+    }
+    false
+}

clippy_lints/src/utils/paths.rs

@@ -31,6 +31,8 @@ pub const MUTEX: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
 pub const OPEN_OPTIONS: [&'static str; 3] = ["std", "fs", "OpenOptions"];
 pub const OPS_MODULE: [&'static str; 2] = ["core", "ops"];
 pub const OPTION: [&'static str; 3] = ["core", "option", "Option"];
+pub const PTR_NULL: [&'static str; 2] = ["ptr", "null"];
+pub const PTR_NULL_MUT: [&'static str; 2] = ["ptr", "null_mut"];
 pub const RANGE: [&'static str; 3] = ["core", "ops", "Range"];
 pub const RANGE_FROM: [&'static str; 3] = ["core", "ops", "RangeFrom"];
 pub const RANGE_FROM_STD: [&'static str; 3] = ["std", "ops", "RangeFrom"];

tests/compile-fail/cmp_null.rs

@@ -0,0 +1,19 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+#![deny(cmp_null)]
+#![allow(unused_mut)]
+
+use std::ptr;
+
+fn main() {
+    let x = 0;
+    let p : *const usize = &x;
+    if p == ptr::null() { //~ERROR:  Comparing with null
+        println!("This is surprising!");
+    }
+    let mut y = 0;
+    let mut m : *mut usize = &mut y;
+    if m == ptr::null_mut() { //~ERROR:  Comparing with null
+        println!("This is surprising, too!");
+    }
+}