Add some restrictions to default_numeric_fallback to avoid FNs

Y-Nak · Y-Nak · commit 93796b2346a1 · 2021-02-15T14:43:37.000+09:00
diff --git a/clippy_lints/src/default_numeric_fallback.rs b/clippy_lints/src/default_numeric_fallback.rs
@@ -1,9 +1,14 @@
-use rustc_ast::{LitFloatType, LitIntType, LitKind};
-use rustc_data_structures::fx::FxHashSet;
-use rustc_hir::{Expr, ExprKind, HirId, Stmt, StmtKind};
+use rustc_ast::ast::{LitFloatType, LitIntType, LitKind};
+use rustc_hir::{
+    intravisit::{walk_expr, walk_stmt, NestedVisitorMap, Visitor},
+    Body, Expr, ExprKind, Lit, Stmt, StmtKind,
+};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::{self, FloatTy, IntTy};
-use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_middle::{
+    hir::map::Map,
+    ty::{self, FloatTy, IntTy, Ty},
+};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 use if_chain::if_chain;
 
@@ -33,53 +38,141 @@ declare_clippy_lint! {
     /// Use instead:
     /// ```rust
     /// let i = 10i32;
-    /// let f: f64 = 1.23;
+    /// let f = 1.23f64;
     /// ```
     pub DEFAULT_NUMERIC_FALLBACK,
     restriction,
     "usage of unconstrained numeric literals which may cause default numeric fallback."
 }
 
-#[derive(Default)]
-pub struct DefaultNumericFallback {
-    /// Hold `init` in `Local` if `Local` has a type annotation.
-    bounded_inits: FxHashSet<HirId>,
+declare_lint_pass!(DefaultNumericFallback => [DEFAULT_NUMERIC_FALLBACK]);
+
+impl LateLintPass<'_> for DefaultNumericFallback {
+    fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) {
+        let mut visitor = NumericFallbackVisitor::new(cx);
+        visitor.visit_body(body);
+    }
 }
 
-impl_lint_pass!(DefaultNumericFallback => [DEFAULT_NUMERIC_FALLBACK]);
+struct NumericFallbackVisitor<'a, 'tcx> {
+    /// Stack manages type bound of exprs. The top element holds current expr type.
+    ty_bounds: Vec<TyBound<'tcx>>,
 
-impl LateLintPass<'_> for DefaultNumericFallback {
-    fn check_stmt(&mut self, _: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
-        if_chain! {
-            if let StmtKind::Local(local) = stmt.kind;
-            if local.ty.is_some();
-            if let Some(init) = local.init;
-            then {
-                self.bounded_inits.insert(init.hir_id);
-            }
+    cx: &'a LateContext<'tcx>,
+}
+
+impl<'a, 'tcx> NumericFallbackVisitor<'a, 'tcx> {
+    fn new(cx: &'a LateContext<'tcx>) -> Self {
+        Self {
+            ty_bounds: vec![TyBound::Nothing],
+            cx,
         }
     }
 
-    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        let expr_ty = cx.typeck_results().expr_ty(expr);
-        let hir_id = expr.hir_id;
+    /// Check whether a passed literal has potential to cause fallback or not.
+    fn check_lit(&self, lit: &Lit, lit_ty: Ty<'tcx>) {
+        let ty_bound = self.ty_bounds.last().unwrap();
         if_chain! {
-            if let ExprKind::Lit(ref lit) = expr.kind;
-            if matches!(lit.node,
-                        LitKind::Int(_, LitIntType::Unsuffixed) | LitKind::Float(_, LitFloatType::Unsuffixed));
-            if matches!(expr_ty.kind(), ty::Int(IntTy::I32) | ty::Float(FloatTy::F64));
-            if !self.bounded_inits.contains(&hir_id);
-            if !cx.tcx.hir().parent_iter(hir_id).any(|(ref hir_id, _)| self.bounded_inits.contains(hir_id));
-            then {
-                 span_lint_and_help(
-                    cx,
-                    DEFAULT_NUMERIC_FALLBACK,
-                    lit.span,
-                    "default numeric fallback might occur",
-                    None,
-                    "consider adding suffix to avoid default numeric fallback",
-                 )
-            }
+                if matches!(lit.node,
+                            LitKind::Int(_, LitIntType::Unsuffixed) | LitKind::Float(_, LitFloatType::Unsuffixed));
+                if matches!(lit_ty.kind(), ty::Int(IntTy::I32) | ty::Float(FloatTy::F64));
+                if !ty_bound.is_integral();
+                then {
+                    span_lint_and_help(
+                        self.cx,
+                        DEFAULT_NUMERIC_FALLBACK,
+                        lit.span,
+                        "default numeric fallback might occur",
+                        None,
+                        "consider adding suffix to avoid default numeric fallback",
+                    );
+                }
+        }
+    }
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for NumericFallbackVisitor<'a, 'tcx> {
+    type Map = Map<'tcx>;
+
+    #[allow(clippy::too_many_lines)]
+    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+        match &expr.kind {
+            ExprKind::Call(func, args) => {
+                if_chain! {
+                    if let ExprKind::Path(ref func_path) = func.kind;
+                    if let Some(def_id) = self.cx.qpath_res(func_path, func.hir_id).opt_def_id();
+                    then {
+                        let fn_sig = self.cx.tcx.fn_sig(def_id).skip_binder();
+                        for (expr, bound) in args.iter().zip(fn_sig.inputs().iter()) {
+                            // Push found arg type, then visit arg.
+                            self.ty_bounds.push(TyBound::Ty(bound));
+                            self.visit_expr(expr);
+                            self.ty_bounds.pop();
+                        }
+                        return;
+                    }
+                }
+            },
+
+            ExprKind::MethodCall(_, _, args, _) => {
+                if let Some(def_id) = self.cx.typeck_results().type_dependent_def_id(expr.hir_id) {
+                    let fn_sig = self.cx.tcx.fn_sig(def_id).skip_binder();
+                    for (expr, bound) in args.iter().zip(fn_sig.inputs().iter()) {
+                        self.ty_bounds.push(TyBound::Ty(bound));
+                        self.visit_expr(expr);
+                        self.ty_bounds.pop();
+                    }
+                    return;
+                }
+            },
+
+            ExprKind::Lit(lit) => {
+                let ty = self.cx.typeck_results().expr_ty(expr);
+                self.check_lit(lit, ty);
+                return;
+            },
+
+            _ => {},
+        }
+
+        walk_expr(self, expr);
+    }
+
+    fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) {
+        match stmt.kind {
+            StmtKind::Local(local) => {
+                if local.ty.is_some() {
+                    self.ty_bounds.push(TyBound::Any)
+                } else {
+                    self.ty_bounds.push(TyBound::Nothing)
+                }
+            },
+
+            _ => self.ty_bounds.push(TyBound::Nothing),
+        }
+
+        walk_stmt(self, stmt);
+        self.ty_bounds.pop();
+    }
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::None
+    }
+}
+
+#[derive(Debug, Clone, Copy)]
+enum TyBound<'ctx> {
+    Any,
+    Ty(Ty<'ctx>),
+    Nothing,
+}
+
+impl<'ctx> TyBound<'ctx> {
+    fn is_integral(self) -> bool {
+        match self {
+            TyBound::Any => true,
+            TyBound::Ty(t) => t.is_integral(),
+            TyBound::Nothing => false,
         }
     }
 }
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -1028,7 +1028,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box strings::StringAdd);
     store.register_late_pass(|| box implicit_return::ImplicitReturn);
     store.register_late_pass(|| box implicit_saturating_sub::ImplicitSaturatingSub);
-    store.register_late_pass(|| box default_numeric_fallback::DefaultNumericFallback::default());
+    store.register_late_pass(|| box default_numeric_fallback::DefaultNumericFallback);
 
     let msrv = conf.msrv.as_ref().and_then(|s| {
         parse_msrv(s, None, None).or_else(|| {
diff --git a/tests/ui/default_numeric_fallback.rs b/tests/ui/default_numeric_fallback.rs
@@ -1,18 +1,58 @@
 #![warn(clippy::default_numeric_fallback)]
 #![allow(unused)]
+#![allow(clippy::never_loop)]
+#![allow(clippy::no_effect)]
+#![allow(clippy::unnecessary_operation)]
+
+fn concrete_arg(x: i32) {}
+
+fn generic_arg<T>(t: T) {}
+
+struct ConcreteStruct {
+    x: i32,
+}
+
+struct StructForMethodCallTest {
+    x: i32,
+}
+
+impl StructForMethodCallTest {
+    fn concrete_arg(&self, x: i32) {}
+
+    fn generic_arg<T>(&self, t: T) {}
+}
 
 fn main() {
+    let s = StructForMethodCallTest { x: 10_i32 };
+
     // Bad.
     let x = 1;
     let x = 0.1;
+
     let x = if true { 1 } else { 2 };
 
+    let x: _ = {
+        let y = 1;
+        1
+    };
+
+    generic_arg(10);
+    s.generic_arg(10);
+    let x: _ = generic_arg(10);
+    let x: _ = s.generic_arg(10);
+
     // Good.
     let x = 1_i32;
     let x: i32 = 1;
     let x: _ = 1;
     let x = 0.1_f64;
     let x: f64 = 0.1;
     let x: _ = 0.1;
+
     let x: _ = if true { 1 } else { 2 };
+
+    concrete_arg(10);
+    s.concrete_arg(10);
+    let x = concrete_arg(10);
+    let x = s.concrete_arg(10);
 }
diff --git a/tests/ui/default_numeric_fallback.stderr b/tests/ui/default_numeric_fallback.stderr
@@ -1,5 +1,5 @@
 error: default numeric fallback might occur
-  --> $DIR/default_numeric_fallback.rs:6:13
+  --> $DIR/default_numeric_fallback.rs:29:13
    |
 LL |     let x = 1;
    |             ^
@@ -8,28 +8,68 @@ LL |     let x = 1;
    = help: consider adding suffix to avoid default numeric fallback
 
 error: default numeric fallback might occur
-  --> $DIR/default_numeric_fallback.rs:7:13
+  --> $DIR/default_numeric_fallback.rs:30:13
    |
 LL |     let x = 0.1;
    |             ^^^
    |
    = help: consider adding suffix to avoid default numeric fallback
 
 error: default numeric fallback might occur
-  --> $DIR/default_numeric_fallback.rs:8:23
+  --> $DIR/default_numeric_fallback.rs:32:23
    |
 LL |     let x = if true { 1 } else { 2 };
    |                       ^
    |
    = help: consider adding suffix to avoid default numeric fallback
 
 error: default numeric fallback might occur
-  --> $DIR/default_numeric_fallback.rs:8:34
+  --> $DIR/default_numeric_fallback.rs:32:34
    |
 LL |     let x = if true { 1 } else { 2 };
    |                                  ^
    |
    = help: consider adding suffix to avoid default numeric fallback
 
-error: aborting due to 4 previous errors
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:35:17
+   |
+LL |         let y = 1;
+   |                 ^
+   |
+   = help: consider adding suffix to avoid default numeric fallback
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:39:17
+   |
+LL |     generic_arg(10);
+   |                 ^^
+   |
+   = help: consider adding suffix to avoid default numeric fallback
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:40:19
+   |
+LL |     s.generic_arg(10);
+   |                   ^^
+   |
+   = help: consider adding suffix to avoid default numeric fallback
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:41:28
+   |
+LL |     let x: _ = generic_arg(10);
+   |                            ^^
+   |
+   = help: consider adding suffix to avoid default numeric fallback
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:42:30
+   |
+LL |     let x: _ = s.generic_arg(10);
+   |                              ^^
+   |
+   = help: consider adding suffix to avoid default numeric fallback
+
+error: aborting due to 9 previous errors
 
