diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 25fb171720f..287595a6e8a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -89,6 +89,7 @@ macro_rules! declare_restriction_lint { pub mod double_comparison; pub mod double_parens; pub mod drop_forget_ref; +pub mod question_mark; pub mod else_if_without_else; pub mod empty_enum; pub mod entry; @@ -371,6 +372,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box replace_consts::ReplaceConsts); reg.register_late_lint_pass(box types::UnitArg); reg.register_late_lint_pass(box double_comparison::DoubleComparisonPass); + reg.register_late_lint_pass(box question_mark::QuestionMarkPass); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs new file mode 100644 index 00000000000..4c01899936b --- /dev/null +++ b/clippy_lints/src/question_mark.rs @@ -0,0 +1,138 @@ +use rustc::lint::*; +use rustc::hir::*; +use rustc::hir::def::Def; +use utils::sugg::Sugg; +use syntax::ptr::P; + +use utils::{match_def_path, match_type, span_lint_and_then}; +use utils::paths::*; + +/// **What it does:** Checks for expressions that could be replaced by the question mark operator +/// +/// **Why is this bad?** Question mark usage is more idiomatic +/// +/// **Known problems:** None +/// +/// **Example:** +/// ```rust +/// if option.is_none() { +/// return None; +/// } +/// ``` +/// +/// Could be written: +/// +/// ```rust +/// option?; +/// ``` +declare_lint!{ + pub QUESTION_MARK, + Warn, + "checks for expressions that could be replaced by the question mark operator" +} + +#[derive(Copy, Clone)] +pub struct QuestionMarkPass; + +impl LintPass for QuestionMarkPass { + fn get_lints(&self) -> LintArray { + lint_array!(QUESTION_MARK) + } +} + +impl QuestionMarkPass { + /// Check if the given expression on the given context matches the following structure: + /// + /// ``` + /// if option.is_none() { + /// return None; + /// } + /// ``` + /// + /// If it matches, it will suggest to use the question mark operator instead + fn check_is_none_and_early_return_none(cx: &LateContext, expr: &Expr) { + if_chain! { + if let ExprIf(ref if_expr, ref body, _) = expr.node; + if let ExprMethodCall(ref segment, _, ref args) = if_expr.node; + if segment.name == "is_none"; + if Self::expression_returns_none(cx, &body); + if let Some(subject) = args.get(0); + if Self::is_option(cx, subject); + + then { + span_lint_and_then( + cx, + QUESTION_MARK, + expr.span, + &format!("this block may be rewritten with the `?` operator"), + |db| { + let receiver_str = &Sugg::hir(cx, subject, ".."); + + db.span_suggestion( + expr.span, + "replace_it_with", + format!("{}?;", receiver_str), + ); + } + ) + } + } + } + + fn is_option(cx: &LateContext, expression: &Expr) -> bool { + let expr_ty = cx.tables.expr_ty(expression); + + return match_type(cx, expr_ty, &OPTION); + } + + fn expression_returns_none(cx: &LateContext, expression: &Expr) -> bool { + match expression.node { + ExprBlock(ref block) => { + if let Some(return_expression) = Self::return_expression(block) { + return Self::expression_returns_none(cx, &return_expression); + } + + false + }, + ExprRet(Some(ref expr)) => { + Self::expression_returns_none(cx, expr) + }, + ExprPath(ref qp) => { + if let Def::VariantCtor(def_id, _) = cx.tables.qpath_def(qp, expression.hir_id) { + return match_def_path(cx.tcx, def_id, &OPTION_NONE); + } + + false + }, + _ => false + } + } + + fn return_expression(block: &Block) -> Option> { + // Check if last expression is a return statement. Then, return the expression + if_chain! { + if block.stmts.len() == 1; + if let Some(expr) = block.stmts.iter().last(); + if let StmtSemi(ref expr, _) = expr.node; + if let ExprRet(ref ret_expr) = expr.node; + if let &Some(ref ret_expr) = ret_expr; + + then { + return Some(ret_expr.clone()); + } + } + + // Check if the block has an implicit return expression + if let Some(ref ret_expr) = block.expr { + return Some(ret_expr.clone()); + } + + return None; + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for QuestionMarkPass { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + Self::check_is_none_and_early_return_none(cx, expr); + } +} diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs new file mode 100644 index 00000000000..369b868a50d --- /dev/null +++ b/tests/ui/question_mark.rs @@ -0,0 +1,54 @@ +fn some_func(a: Option) -> Option { + if a.is_none() { + return None + } + + a +} + +pub enum SeemsOption { + Some(T), + None +} + +impl SeemsOption { + pub fn is_none(&self) -> bool { + match *self { + SeemsOption::None => true, + SeemsOption::Some(_) => false, + } + } +} + +fn returns_something_similar_to_option(a: SeemsOption) -> SeemsOption { + if a.is_none() { + return SeemsOption::None; + } + + a +} + +pub struct SomeStruct { + pub opt: Option, +} + +impl SomeStruct { + pub fn func(&self) -> Option { + if (self.opt).is_none() { + return None; + } + + self.opt + } +} + +fn main() { + some_func(Some(42)); + some_func(None); + + let some_struct = SomeStruct { opt: Some(54) }; + some_struct.func(); + + let so = SeemsOption::Some(45); + returns_something_similar_to_option(so); +} diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr new file mode 100644 index 00000000000..e97b1869824 --- /dev/null +++ b/tests/ui/question_mark.stderr @@ -0,0 +1,22 @@ +error: this block may be rewritten with the `?` operator + --> $DIR/question_mark.rs:2:2 + | +2 | if a.is_none() { + | _____^ +3 | | return None +4 | | } + | |_____^ help: replace_it_with: `a?;` + | + = note: `-D question-mark` implied by `-D warnings` + +error: this block may be rewritten with the `?` operator + --> $DIR/question_mark.rs:37:3 + | +37 | if (self.opt).is_none() { + | _________^ +38 | | return None; +39 | | } + | |_________^ help: replace_it_with: `(self.opt)?;` + +error: aborting due to 2 previous errors +