From 40d50fe8b22c3926e802c480724ad42e72aad903 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 7 Mar 2017 12:58:07 +0100 Subject: [PATCH] Don't lint `nan_cmp` and `zero_ptr` in constants --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/misc.rs | 125 ++++++++++++++++++++++---------- clippy_lints/src/misc_early.rs | 39 +--------- clippy_lints/src/utils/mod.rs | 12 +++ tests/run-pass/mut_mut_macro.rs | 12 ++- 5 files changed, 110 insertions(+), 80 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 31b2e23422f..881d210a28d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -445,6 +445,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { misc::REDUNDANT_PATTERN, misc::SHORT_CIRCUIT_STATEMENT, misc::TOPLEVEL_REF_ARG, + misc::ZERO_PTR, misc_early::BUILTIN_TYPE_SHADOW, misc_early::DOUBLE_NEG, misc_early::DUPLICATE_UNDERSCORE_ARGUMENT, @@ -452,7 +453,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { misc_early::REDUNDANT_CLOSURE_CALL, misc_early::UNNEEDED_FIELD_PATTERN, misc_early::ZERO_PREFIXED_LITERAL, - misc_early::ZERO_PTR, mut_reference::UNNECESSARY_MUT_PASSED, mutex_atomic::MUTEX_ATOMIC, needless_bool::BOOL_COMPARISON, diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index b4e79a1401d..885d10d2fd9 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -8,8 +8,9 @@ use rustc_const_eval::ConstContext; use rustc_const_math::ConstFloat; use syntax::codemap::{Span, Spanned, ExpnFormat}; use utils::{get_item_name, get_parent_expr, implements_trait, in_macro, is_integer_literal, match_path, snippet, - span_lint, span_lint_and_then, walk_ptrs_ty, last_path_segment, iter_input_pats}; + span_lint, span_lint_and_then, walk_ptrs_ty, last_path_segment, iter_input_pats, in_constant}; use utils::sugg::Sugg; +use syntax::ast::LitKind; /// **What it does:** Checks for function arguments and let bindings denoted as `ref`. /// @@ -172,6 +173,24 @@ declare_lint! { "using a short circuit boolean condition as a statement" } +/// **What it does:** Catch casts from `0` to some pointer type +/// +/// **Why is this bad?** This generally means `null` and is better expressed as +/// {`std`, `core`}`::ptr::`{`null`, `null_mut`}. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// +/// ```rust +/// 0 as *const u32 +/// ``` +declare_lint! { + pub ZERO_PTR, + Warn, + "using 0 as *{const, mut} T" +} + #[derive(Copy, Clone)] pub struct Pass; @@ -184,7 +203,8 @@ impl LintPass for Pass { MODULO_ONE, REDUNDANT_PATTERN, USED_UNDERSCORE_BINDING, - SHORT_CIRCUIT_STATEMENT) + SHORT_CIRCUIT_STATEMENT, + ZERO_PTR) } } @@ -263,41 +283,48 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { - if let ExprBinary(ref cmp, ref left, ref right) = expr.node { - let op = cmp.node; - if op.is_comparison() { - if let ExprPath(QPath::Resolved(_, ref path)) = left.node { - check_nan(cx, path, expr.span); + match expr.node { + ExprCast(ref e, ref ty) => { + check_cast(cx, expr.span, e, ty); + return; + }, + ExprBinary(ref cmp, ref left, ref right) => { + let op = cmp.node; + if op.is_comparison() { + if let ExprPath(QPath::Resolved(_, ref path)) = left.node { + check_nan(cx, path, expr); + } + if let ExprPath(QPath::Resolved(_, ref path)) = right.node { + check_nan(cx, path, expr); + } + check_to_owned(cx, left, right, true, cmp.span); + check_to_owned(cx, right, left, false, cmp.span) } - if let ExprPath(QPath::Resolved(_, ref path)) = right.node { - check_nan(cx, path, expr.span); - } - check_to_owned(cx, left, right, true, cmp.span); - check_to_owned(cx, right, left, false, cmp.span) - } - if (op == BiEq || op == BiNe) && (is_float(cx, left) || is_float(cx, right)) { - if is_allowed(cx, left) || is_allowed(cx, right) { - return; - } - if let Some(name) = get_item_name(cx, expr) { - let name = &*name.as_str(); - if name == "eq" || name == "ne" || name == "is_nan" || name.starts_with("eq_") || - name.ends_with("_eq") { + if (op == BiEq || op == BiNe) && (is_float(cx, left) || is_float(cx, right)) { + if is_allowed(cx, left) || is_allowed(cx, right) { return; } - } - span_lint_and_then(cx, FLOAT_CMP, expr.span, "strict comparison of f32 or f64", |db| { - let lhs = Sugg::hir(cx, left, ".."); - let rhs = Sugg::hir(cx, right, ".."); + if let Some(name) = get_item_name(cx, expr) { + let name = &*name.as_str(); + if name == "eq" || name == "ne" || name == "is_nan" || name.starts_with("eq_") || + name.ends_with("_eq") { + return; + } + } + span_lint_and_then(cx, FLOAT_CMP, expr.span, "strict comparison of f32 or f64", |db| { + let lhs = Sugg::hir(cx, left, ".."); + let rhs = Sugg::hir(cx, right, ".."); - db.span_suggestion(expr.span, - "consider comparing them within some error", - format!("({}).abs() < error", lhs - rhs)); - db.span_note(expr.span, "std::f32::EPSILON and std::f64::EPSILON are available."); - }); - } else if op == BiRem && is_integer_literal(right, 1) { - span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0"); - } + db.span_suggestion(expr.span, + "consider comparing them within some error", + format!("({}).abs() < error", lhs - rhs)); + db.span_note(expr.span, "std::f32::EPSILON and std::f64::EPSILON are available."); + }); + } else if op == BiRem && is_integer_literal(right, 1) { + span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0"); + } + }, + _ => {} } if in_attributes_expansion(cx, expr) { // Don't lint things expanded by #[derive(...)], etc @@ -349,13 +376,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } -fn check_nan(cx: &LateContext, path: &Path, span: Span) { - path.segments.last().map(|seg| if &*seg.name.as_str() == "NAN" { - span_lint(cx, - CMP_NAN, - span, - "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead"); - }); +fn check_nan(cx: &LateContext, path: &Path, expr: &Expr) { + if !in_constant(cx, expr.id) { + path.segments.last().map(|seg| if &*seg.name.as_str() == "NAN" { + span_lint(cx, + CMP_NAN, + expr.span, + "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead"); + }); + } } fn is_allowed(cx: &LateContext, expr: &Expr) -> bool { @@ -489,3 +518,19 @@ fn non_macro_local(cx: &LateContext, def: &def::Def) -> bool { _ => false, } } + +fn check_cast(cx: &LateContext, span: Span, e: &Expr, ty: &Ty) { + if_let_chain! {[ + let TyPtr(MutTy { mutbl, .. }) = ty.node, + let ExprLit(ref lit) = e.node, + let LitKind::Int(value, ..) = lit.node, + value == 0, + !in_constant(cx, e.id) + ], { + let msg = match mutbl { + Mutability::MutMutable => "`0 as *mut _` detected. Consider using `ptr::null_mut()`", + Mutability::MutImmutable => "`0 as *const _` detected. Consider using `ptr::null()`", + }; + span_lint(cx, ZERO_PTR, span, msg); + }} +} diff --git a/clippy_lints/src/misc_early.rs b/clippy_lints/src/misc_early.rs index f9de6a011a1..57c0c637f41 100644 --- a/clippy_lints/src/misc_early.rs +++ b/clippy_lints/src/misc_early.rs @@ -162,24 +162,6 @@ declare_lint! { "shadowing a builtin type" } -/// **What it does:** Catch casts from `0` to some pointer type -/// -/// **Why is this bad?** This generally means `null` and is better expressed as -/// {`std`, `core`}`::ptr::`{`null`, `null_mut`}. -/// -/// **Known problems:** None. -/// -/// **Example:** -/// -/// ```rust -/// 0 as *const u32 -/// ``` -declare_lint! { - pub ZERO_PTR, - Warn, - "using 0 as *{const, mut} T" -} - #[derive(Copy, Clone)] pub struct MiscEarly; @@ -192,8 +174,7 @@ impl LintPass for MiscEarly { MIXED_CASE_HEX_LITERALS, UNSEPARATED_LITERAL_SUFFIX, ZERO_PREFIXED_LITERAL, - BUILTIN_TYPE_SHADOW, - ZERO_PTR) + BUILTIN_TYPE_SHADOW) } } @@ -381,9 +362,6 @@ impl EarlyLintPass for MiscEarly { } }} }, - ExprKind::Cast(ref e, ref ty) => { - check_cast(cx, expr.span, e, ty); - }, _ => (), } } @@ -412,18 +390,3 @@ impl EarlyLintPass for MiscEarly { } } } - -fn check_cast(cx: &EarlyContext, span: Span, e: &Expr, ty: &Ty) { - if_let_chain! {[ - let TyKind::Ptr(MutTy { mutbl, .. }) = ty.node, - let ExprKind::Lit(ref lit) = e.node, - let LitKind::Int(value, ..) = lit.node, - value == 0 - ], { - let msg = match mutbl { - Mutability::Mutable => "`0 as *mut _` detected. Consider using `ptr::null_mut()`", - Mutability::Immutable => "`0 as *const _` detected. Consider using `ptr::null()`", - }; - span_lint(cx, ZERO_PTR, span, msg); - }} -} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 693b14d1b99..58b416b8325 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -10,6 +10,7 @@ use rustc::traits; use rustc::ty::subst::Subst; use rustc::ty; use rustc::ty::layout::TargetDataLayout; +use rustc::mir::transform::MirSource; use rustc_errors; use std::borrow::Cow; use std::env; @@ -98,6 +99,17 @@ pub mod higher; pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool { rhs.expn_id != lhs.expn_id } + +pub fn in_constant(cx: &LateContext, id: NodeId) -> bool { + let parent_id = cx.tcx.hir.get_parent(id); + match MirSource::from_node(cx.tcx, parent_id) { + MirSource::Fn(_) => false, + MirSource::Const(_) | + MirSource::Static(..) | + MirSource::Promoted(..) => true, + } +} + /// Returns true if this `expn_info` was expanded by any macro. pub fn in_macro<'a, T: LintContext<'a>>(cx: &T, span: Span) -> bool { cx.sess().codemap().with_expn_info(span.expn_id, |info| { diff --git a/tests/run-pass/mut_mut_macro.rs b/tests/run-pass/mut_mut_macro.rs index e652862c4ff..9f63a6b2d73 100644 --- a/tests/run-pass/mut_mut_macro.rs +++ b/tests/run-pass/mut_mut_macro.rs @@ -1,12 +1,20 @@ #![feature(plugin)] #![plugin(clippy)] +#![deny(mut_mut, zero_ptr, cmp_nan)] +#![allow(dead_code)] #[macro_use] extern crate lazy_static; use std::collections::HashMap; -#[deny(mut_mut)] +// ensure that we don't suggest `is_nan` and `is_null` inside constants +// FIXME: once const fn is stable, suggest these functions again in constants +const BAA: *const i32 = 0 as *const i32; +static mut BAR: *const i32 = BAA; +static mut FOO: *const i32 = 0 as *const i32; +static mut BUH: bool = 42.0 < std::f32::NAN; + #[allow(unused_variables, unused_mut)] fn main() { lazy_static! { @@ -19,4 +27,6 @@ fn main() { static ref MUT_COUNT : usize = MUT_MAP.len(); } assert!(*MUT_COUNT == 1); + // FIXME: don't lint in array length, requires `check_body` + //let _ = [""; (42.0 < std::f32::NAN) as usize]; }