diff --git a/CHANGELOG.md b/CHANGELOG.md index 45eb44a5b3d..f0e495961b0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index a9f148d058d..40112616532 100644 --- a/README.md +++ b/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`) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8c38ea7baeb..07d504f75ad 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/ptr_arg.rs b/clippy_lints/src/ptr.rs similarity index 55% rename from clippy_lints/src/ptr_arg.rs rename to clippy_lints/src/ptr.rs index f985b2a0031..dea950ac063 100644 --- a/clippy_lints/src/ptr_arg.rs +++ b/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" } -#[derive(Copy,Clone)] -pub struct PtrArg; +/// **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." +} -impl LintPass for PtrArg { + +#[derive(Copy,Clone)] +pub struct PointerPass; + +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 +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 51071aed7a9..a652d0f42f8 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/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"]; diff --git a/tests/compile-fail/cmp_null.rs b/tests/compile-fail/cmp_null.rs new file mode 100644 index 00000000000..a55ea6f5bca --- /dev/null +++ b/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!"); + } +}