about summary refs log tree commit diff
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2021-02-16 09:58:49 +0000
committerbors <bors@rust-lang.org>2021-02-16 09:58:49 +0000
commite2753f9a7bcfcedaad7dcf78eba6ccfe14f2a3aa (patch)
treea8eff8d034fdb788f305cfd8136667a499d3f145
parentf28c54cd08b02ba89648f80b67f512f878e9dce2 (diff)
parent9b0c1ebc183bbf0fab5fbbe5ac8b2f94e5e56b87 (diff)
downloadrust-e2753f9a7bcfcedaad7dcf78eba6ccfe14f2a3aa.tar.gz
rust-e2753f9a7bcfcedaad7dcf78eba6ccfe14f2a3aa.zip
Auto merge of #6662 - Y-Nak:default-numeric-fallback, r=flip1995
New lint: default_numeric_fallback

fixes #6064
r? `@flip1995`

As we discussed in [here](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Issue.20.236064/near/224647188) and [here](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Issue.20clippy.236064/near/224746333),   I start implementing this lint from the strictest version.
In this PR, I'll allow the below two cases to pass the lint to reduce FPs.

1. Appearances of unsuffixed numeric literals in `Local` if `Local` has a type annotation, for example:
```rust
// Good.
let x: i32 = 1;

// Also good.
let x: (i32, i32) = if cond {
   (1, 2)
} else {
   (2, 3)
};
```

2. Appearances of unsuffixed numeric literals in args of `Call` or `MethodCall`  if corresponding arguments of their signature have concrete types, for example:
```rust
fn foo_mono(x: i32) -> i32 {
    x
}

fn foo_poly<T>(t: T) -> t {
    t
}

// Good.
let x = foo_mono(13);

// Still bad.
let x: i32 = foo_poly(13);
```

changelog: Added restriction lint: `default_numeric_fallback`
-rw-r--r--CHANGELOG.md1
-rw-r--r--clippy_lints/src/default_numeric_fallback.rs237
-rw-r--r--clippy_lints/src/lib.rs4
-rw-r--r--tests/ui/default_numeric_fallback.rs135
-rw-r--r--tests/ui/default_numeric_fallback.stderr148
5 files changed, 525 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4dcf9c3529b..c59eae5d1c2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2027,6 +2027,7 @@ Released 2018-09-13
 [`debug_assert_with_mut_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#debug_assert_with_mut_call
 [`decimal_literal_representation`]: https://rust-lang.github.io/rust-clippy/master/index.html#decimal_literal_representation
 [`declare_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const
+[`default_numeric_fallback`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_numeric_fallback
 [`default_trait_access`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_trait_access
 [`deprecated_cfg_attr`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_cfg_attr
 [`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
diff --git a/clippy_lints/src/default_numeric_fallback.rs b/clippy_lints/src/default_numeric_fallback.rs
new file mode 100644
index 00000000000..6ace9aa6bdf
--- /dev/null
+++ b/clippy_lints/src/default_numeric_fallback.rs
@@ -0,0 +1,237 @@
+use rustc_ast::ast::{LitFloatType, LitIntType, LitKind};
+use rustc_errors::Applicability;
+use rustc_hir::{
+    intravisit::{walk_expr, walk_stmt, NestedVisitorMap, Visitor},
+    Body, Expr, ExprKind, HirId, Lit, Stmt, StmtKind,
+};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::{
+    hir::map::Map,
+    ty::{self, FloatTy, IntTy, PolyFnSig, Ty},
+};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+use if_chain::if_chain;
+
+use crate::utils::{snippet, span_lint_and_sugg};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of unconstrained numeric literals which may cause default numeric fallback in type
+    /// inference.
+    ///
+    /// Default numeric fallback means that if numeric types have not yet been bound to concrete
+    /// types at the end of type inference, then integer type is bound to `i32`, and similarly
+    /// floating type is bound to `f64`.
+    ///
+    /// See [RFC0212](https://github.com/rust-lang/rfcs/blob/master/text/0212-restore-int-fallback.md) for more information about the fallback.
+    ///
+    /// **Why is this bad?** For those who are very careful about types, default numeric fallback
+    /// can be a pitfall that cause unexpected runtime behavior.
+    ///
+    /// **Known problems:** This lint can only be allowed at the function level or above.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// let i = 10;
+    /// let f = 1.23;
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust
+    /// let i = 10i32;
+    /// let f = 1.23f64;
+    /// ```
+    pub DEFAULT_NUMERIC_FALLBACK,
+    restriction,
+    "usage of unconstrained numeric literals which may cause default numeric fallback."
+}
+
+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);
+    }
+}
+
+struct NumericFallbackVisitor<'a, 'tcx> {
+    /// Stack manages type bound of exprs. The top element holds current expr type.
+    ty_bounds: Vec<TyBound<'tcx>>,
+
+    cx: &'a LateContext<'tcx>,
+}
+
+impl<'a, 'tcx> NumericFallbackVisitor<'a, 'tcx> {
+    fn new(cx: &'a LateContext<'tcx>) -> Self {
+        Self {
+            ty_bounds: vec![TyBound::Nothing],
+            cx,
+        }
+    }
+
+    /// Check whether a passed literal has potential to cause fallback or not.
+    fn check_lit(&self, lit: &Lit, lit_ty: Ty<'tcx>) {
+        if_chain! {
+                if let Some(ty_bound) = self.ty_bounds.last();
+                if matches!(lit.node,
+                            LitKind::Int(_, LitIntType::Unsuffixed) | LitKind::Float(_, LitFloatType::Unsuffixed));
+                if !ty_bound.is_integral();
+                then {
+                    let suffix = match lit_ty.kind() {
+                        ty::Int(IntTy::I32) => "i32",
+                        ty::Float(FloatTy::F64) => "f64",
+                        // Default numeric fallback never results in other types.
+                        _ => return,
+                    };
+
+                    let sugg = format!("{}_{}", snippet(self.cx, lit.span, ""), suffix);
+                    span_lint_and_sugg(
+                        self.cx,
+                        DEFAULT_NUMERIC_FALLBACK,
+                        lit.span,
+                        "default numeric fallback might occur",
+                        "consider adding suffix",
+                        sugg,
+                        Applicability::MaybeIncorrect,
+                    );
+                }
+        }
+    }
+}
+
+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 let Some(fn_sig) = fn_sig_opt(self.cx, func.hir_id) {
+                    for (expr, bound) in args.iter().zip(fn_sig.skip_binder().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::Struct(qpath, fields, base) => {
+                if_chain! {
+                    if let Some(def_id) = self.cx.qpath_res(qpath, expr.hir_id).opt_def_id();
+                    let ty = self.cx.tcx.type_of(def_id);
+                    if let Some(adt_def) = ty.ty_adt_def();
+                    if adt_def.is_struct();
+                    if let Some(variant) = adt_def.variants.iter().next();
+                    then {
+                        let fields_def = &variant.fields;
+
+                        // Push field type then visit each field expr.
+                        for field in fields.iter() {
+                            let bound =
+                                fields_def
+                                    .iter()
+                                    .find_map(|f_def| {
+                                        if f_def.ident == field.ident
+                                            { Some(self.cx.tcx.type_of(f_def.did)) }
+                                        else { None }
+                                    });
+                            self.ty_bounds.push(bound.into());
+                            self.visit_expr(field.expr);
+                            self.ty_bounds.pop();
+                        }
+
+                        // Visit base with no bound.
+                        if let Some(base) = base {
+                            self.ty_bounds.push(TyBound::Nothing);
+                            self.visit_expr(base);
+                            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
+    }
+}
+
+fn fn_sig_opt<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<PolyFnSig<'tcx>> {
+    let node_ty = cx.typeck_results().node_type_opt(hir_id)?;
+    // We can't use `TyS::fn_sig` because it automatically performs substs, this may result in FNs.
+    match node_ty.kind() {
+        ty::FnDef(def_id, _) => Some(cx.tcx.fn_sig(*def_id)),
+        ty::FnPtr(fn_sig) => Some(*fn_sig),
+        _ => None,
+    }
+}
+
+#[derive(Debug, Clone, Copy)]
+enum TyBound<'tcx> {
+    Any,
+    Ty(Ty<'tcx>),
+    Nothing,
+}
+
+impl<'tcx> TyBound<'tcx> {
+    fn is_integral(self) -> bool {
+        match self {
+            TyBound::Any => true,
+            TyBound::Ty(t) => t.is_integral(),
+            TyBound::Nothing => false,
+        }
+    }
+}
+
+impl<'tcx> From<Option<Ty<'tcx>>> for TyBound<'tcx> {
+    fn from(v: Option<Ty<'tcx>>) -> Self {
+        match v {
+            Some(t) => TyBound::Ty(t),
+            None => TyBound::Nothing,
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index d96911fac1a..40727980693 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -181,6 +181,7 @@ mod copy_iterator;
 mod create_dir;
 mod dbg_macro;
 mod default;
+mod default_numeric_fallback;
 mod dereference;
 mod derive;
 mod disallowed_method;
@@ -585,6 +586,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &dbg_macro::DBG_MACRO,
         &default::DEFAULT_TRAIT_ACCESS,
         &default::FIELD_REASSIGN_WITH_DEFAULT,
+        &default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK,
         &dereference::EXPLICIT_DEREF_METHODS,
         &derive::DERIVE_HASH_XOR_EQ,
         &derive::DERIVE_ORD_XOR_PARTIAL_ORD,
@@ -1031,6 +1033,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);
 
     let msrv = conf.msrv.as_ref().and_then(|s| {
         parse_msrv(s, None, None).or_else(|| {
@@ -1265,6 +1268,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX),
         LintId::of(&create_dir::CREATE_DIR),
         LintId::of(&dbg_macro::DBG_MACRO),
+        LintId::of(&default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK),
         LintId::of(&else_if_without_else::ELSE_IF_WITHOUT_ELSE),
         LintId::of(&exhaustive_items::EXHAUSTIVE_ENUMS),
         LintId::of(&exhaustive_items::EXHAUSTIVE_STRUCTS),
diff --git a/tests/ui/default_numeric_fallback.rs b/tests/ui/default_numeric_fallback.rs
new file mode 100644
index 00000000000..0b3758952ac
--- /dev/null
+++ b/tests/ui/default_numeric_fallback.rs
@@ -0,0 +1,135 @@
+#![warn(clippy::default_numeric_fallback)]
+#![allow(unused)]
+#![allow(clippy::never_loop)]
+#![allow(clippy::no_effect)]
+#![allow(clippy::unnecessary_operation)]
+
+mod basic_expr {
+    fn test() {
+        // Should lint unsuffixed literals typed `i32`.
+        let x = 22;
+        let x = [1, 2, 3];
+        let x = if true { (1, 2) } else { (3, 4) };
+        let x = match 1 {
+            1 => 1,
+            _ => 2,
+        };
+
+        // Should lint unsuffixed literals typed `f64`.
+        let x = 0.12;
+
+        // Should NOT lint suffixed literals.
+        let x = 22_i32;
+        let x = 0.12_f64;
+
+        // Should NOT lint literals in init expr if `Local` has a type annotation.
+        let x: f64 = 0.1;
+        let x: [i32; 3] = [1, 2, 3];
+        let x: (i32, i32) = if true { (1, 2) } else { (3, 4) };
+        let x: _ = 1;
+    }
+}
+
+mod nested_local {
+    fn test() {
+        let x: _ = {
+            // Should lint this because this literal is not bound to any types.
+            let y = 1;
+
+            // Should NOT lint this because this literal is bound to `_` of outer `Local`.
+            1
+        };
+
+        let x: _ = if true {
+            // Should lint this because this literal is not bound to any types.
+            let y = 1;
+
+            // Should NOT lint this because this literal is bound to `_` of outer `Local`.
+            1
+        } else {
+            // Should lint this because this literal is not bound to any types.
+            let y = 1;
+
+            // Should NOT lint this because this literal is bound to `_` of outer `Local`.
+            2
+        };
+    }
+}
+
+mod function_def {
+    fn ret_i32() -> i32 {
+        // Even though the output type is specified,
+        // this unsuffixed literal is linted to reduce heuristics and keep codebase simple.
+        1
+    }
+
+    fn test() {
+        // Should lint this because return type is inferred to `i32` and NOT bound to a concrete
+        // type.
+        let f = || -> _ { 1 };
+
+        // Even though the output type is specified,
+        // this unsuffixed literal is linted to reduce heuristics and keep codebase simple.
+        let f = || -> i32 { 1 };
+    }
+}
+
+mod function_calls {
+    fn concrete_arg(x: i32) {}
+
+    fn generic_arg<T>(t: T) {}
+
+    fn test() {
+        // Should NOT lint this because the argument type is bound to a concrete type.
+        concrete_arg(1);
+
+        // Should lint this because the argument type is inferred to `i32` and NOT bound to a concrete type.
+        generic_arg(1);
+
+        // Should lint this because the argument type is inferred to `i32` and NOT bound to a concrete type.
+        let x: _ = generic_arg(1);
+    }
+}
+
+mod struct_ctor {
+    struct ConcreteStruct {
+        x: i32,
+    }
+
+    struct GenericStruct<T> {
+        x: T,
+    }
+
+    fn test() {
+        // Should NOT lint this because the field type is bound to a concrete type.
+        ConcreteStruct { x: 1 };
+
+        // Should lint this because the field type is inferred to `i32` and NOT bound to a concrete type.
+        GenericStruct { x: 1 };
+
+        // Should lint this because the field type is inferred to `i32` and NOT bound to a concrete type.
+        let _ = GenericStruct { x: 1 };
+    }
+}
+
+mod method_calls {
+    struct StructForMethodCallTest {}
+
+    impl StructForMethodCallTest {
+        fn concrete_arg(&self, x: i32) {}
+
+        fn generic_arg<T>(&self, t: T) {}
+    }
+
+    fn test() {
+        let s = StructForMethodCallTest {};
+
+        // Should NOT lint this because the argument type is bound to a concrete type.
+        s.concrete_arg(1);
+
+        // Should lint this because the argument type is bound to a concrete type.
+        s.generic_arg(1);
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/default_numeric_fallback.stderr b/tests/ui/default_numeric_fallback.stderr
new file mode 100644
index 00000000000..b31aa4ebcf8
--- /dev/null
+++ b/tests/ui/default_numeric_fallback.stderr
@@ -0,0 +1,148 @@
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:10:17
+   |
+LL |         let x = 22;
+   |                 ^^ help: consider adding suffix: `22_i32`
+   |
+   = note: `-D clippy::default-numeric-fallback` implied by `-D warnings`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:11:18
+   |
+LL |         let x = [1, 2, 3];
+   |                  ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:11:21
+   |
+LL |         let x = [1, 2, 3];
+   |                     ^ help: consider adding suffix: `2_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:11:24
+   |
+LL |         let x = [1, 2, 3];
+   |                        ^ help: consider adding suffix: `3_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:12:28
+   |
+LL |         let x = if true { (1, 2) } else { (3, 4) };
+   |                            ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:12:31
+   |
+LL |         let x = if true { (1, 2) } else { (3, 4) };
+   |                               ^ help: consider adding suffix: `2_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:12:44
+   |
+LL |         let x = if true { (1, 2) } else { (3, 4) };
+   |                                            ^ help: consider adding suffix: `3_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:12:47
+   |
+LL |         let x = if true { (1, 2) } else { (3, 4) };
+   |                                               ^ help: consider adding suffix: `4_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:13:23
+   |
+LL |         let x = match 1 {
+   |                       ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:14:13
+   |
+LL |             1 => 1,
+   |             ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:14:18
+   |
+LL |             1 => 1,
+   |                  ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:15:18
+   |
+LL |             _ => 2,
+   |                  ^ help: consider adding suffix: `2_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:19:17
+   |
+LL |         let x = 0.12;
+   |                 ^^^^ help: consider adding suffix: `0.12_f64`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:37:21
+   |
+LL |             let y = 1;
+   |                     ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:45:21
+   |
+LL |             let y = 1;
+   |                     ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:51:21
+   |
+LL |             let y = 1;
+   |                     ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:63:9
+   |
+LL |         1
+   |         ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:69:27
+   |
+LL |         let f = || -> _ { 1 };
+   |                           ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:73:29
+   |
+LL |         let f = || -> i32 { 1 };
+   |                             ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:87:21
+   |
+LL |         generic_arg(1);
+   |                     ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:90:32
+   |
+LL |         let x: _ = generic_arg(1);
+   |                                ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:108:28
+   |
+LL |         GenericStruct { x: 1 };
+   |                            ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:111:36
+   |
+LL |         let _ = GenericStruct { x: 1 };
+   |                                    ^ help: consider adding suffix: `1_i32`
+
+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:131:23
+   |
+LL |         s.generic_arg(1);
+   |                       ^ help: consider adding suffix: `1_i32`
+
+error: aborting due to 24 previous errors
+