Merge pull request #1610 from Manishearth/no_const_warnings

Don't lint `nan_cmp` and `zero_ptr` in constants

Author: mcarton

clippy_lints/src/lib.rs

@@ -445,14 +445,14 @@ 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,
         misc_early::MIXED_CASE_HEX_LITERALS,
         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,

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);
-                }
-                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;
+        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 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;
                     }
+                    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");
                 }
-                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");
-            }
+            },
+            _ => {}
         }
         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);
+    }}
+}

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

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| {

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