Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: default_numeric_fallback #6662

Merged
merged 8 commits into from
Feb 16, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 132 additions & 39 deletions clippy_lints/src/default_numeric_fallback.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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! {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let ty_bound = self.ty_bounds.last().unwrap();
if_chain! {
if_chain! {
if let Some(ty_bound) = self.ty_bounds.last();

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,
}
}
}
2 changes: 1 addition & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(|| {
Expand Down
40 changes: 40 additions & 0 deletions tests/ui/default_numeric_fallback.rs
Original file line number Diff line number Diff line change
@@ -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);
}
50 changes: 45 additions & 5 deletions tests/ui/default_numeric_fallback.stderr
Original file line number Diff line number Diff line change
@@ -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;
| ^
Expand All @@ -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