diff options
| -rw-r--r-- | clippy_lints/src/question_mark.rs | 55 | ||||
| -rw-r--r-- | clippy_lints/src/types.rs | 7 | ||||
| -rw-r--r-- | tests/ui/question_mark.rs | 26 | ||||
| -rw-r--r-- | tests/ui/question_mark.stderr | 55 |
4 files changed, 123 insertions, 20 deletions
diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index f21c2985d19..5ad580aa052 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -1,13 +1,15 @@ use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; -use rustc_hir::{def, Block, Expr, ExprKind, StmtKind}; +use rustc_hir::{def, BindingAnnotation, Block, Expr, ExprKind, MatchSource, PatKind, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use crate::utils::paths::{OPTION, OPTION_NONE}; use crate::utils::sugg::Sugg; -use crate::utils::{higher, match_def_path, match_type, span_lint_and_then, SpanlessEq}; +use crate::utils::{ + higher, match_def_path, match_qpath, match_type, snippet_with_applicability, span_lint_and_then, SpanlessEq, +}; declare_clippy_lint! { /// **What it does:** Checks for expressions that could be replaced by the question mark operator. @@ -82,7 +84,7 @@ impl QuestionMark { |db| { db.span_suggestion( expr.span, - "replace_it_with", + "replace it with", replacement_str, Applicability::MaybeIncorrect, // snippet ); @@ -93,6 +95,52 @@ impl QuestionMark { } } + fn check_if_let_some_and_early_return_none(cx: &LateContext<'_, '_>, expr: &Expr<'_>) { + if_chain! { + if let ExprKind::Match(subject, arms, source) = &expr.kind; + if *source == MatchSource::IfLetDesugar { contains_else_clause: true }; + if Self::is_option(cx, subject); + + if let PatKind::TupleStruct(path1, fields, None) = &arms[0].pat.kind; + if match_qpath(path1, &["Some"]); + if let PatKind::Binding(annot, _, bind, _) = &fields[0].kind; + let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut); + + if let ExprKind::Block(block, None) = &arms[0].body.kind; + if block.stmts.is_empty(); + if let Some(trailing_expr) = &block.expr; + if let ExprKind::Path(path) = &trailing_expr.kind; + if match_qpath(path, &[&bind.as_str()]); + + if let PatKind::Wild = arms[1].pat.kind; + if Self::expression_returns_none(cx, arms[1].body); + then { + let mut applicability = Applicability::MachineApplicable; + let receiver_str = snippet_with_applicability(cx, subject.span, "..", &mut applicability); + let replacement = format!( + "{}{}?", + receiver_str, + if by_ref { ".as_ref()" } else { "" }, + ); + + span_lint_and_then( + cx, + QUESTION_MARK, + expr.span, + "this if-let-else may be rewritten with the `?` operator", + |db| { + db.span_suggestion( + expr.span, + "replace it with", + replacement, + applicability, + ); + } + ) + } + } + } + fn moves_by_default(cx: &LateContext<'_, '_>, expression: &Expr<'_>) -> bool { let expr_ty = cx.tables.expr_ty(expression); @@ -158,5 +206,6 @@ impl QuestionMark { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for QuestionMark { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { Self::check_is_none_and_early_return_none(cx, expr); + Self::check_if_let_some_and_early_return_none(cx, expr); } } diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index f90867605aa..58a909c845d 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1698,12 +1698,7 @@ fn detect_absurd_comparison<'a, 'tcx>( return None; } - let normalized = normalize_comparison(op, lhs, rhs); - let (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized { - val - } else { - return None; - }; + let (rel, normalized_lhs, normalized_rhs) = normalize_comparison(op, lhs, rhs)?; let lx = detect_extreme_expr(cx, normalized_lhs); let rx = detect_extreme_expr(cx, normalized_rhs); diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index 57237c52e8c..77aa3976b79 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -58,6 +58,12 @@ impl CopyStruct { self.opt }; + let _ = if let Some(x) = self.opt { + x + } else { + return None; + }; + self.opt } } @@ -90,6 +96,26 @@ impl MoveStruct { } Some(Vec::new()) } + + pub fn if_let_ref_func(self) -> Option<Vec<u32>> { + let mut v: &Vec<_> = if let Some(ref v) = self.opt { + v + } else { + return None; + }; + + Some(v.clone()) + } + + pub fn if_let_mov_func(self) -> Option<Vec<u32>> { + let mut v = if let Some(v) = self.opt { + v + } else { + return None; + }; + + Some(v) + } } fn main() { diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 522501d58c6..dabba9842e4 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -4,7 +4,7 @@ error: this block may be rewritten with the `?` operator LL | / if a.is_none() { LL | | return None; LL | | } - | |_____^ help: replace_it_with: `a?;` + | |_____^ help: replace it with: `a?;` | = note: `-D clippy::question-mark` implied by `-D warnings` @@ -14,7 +14,7 @@ error: this block may be rewritten with the `?` operator LL | / if (self.opt).is_none() { LL | | return None; LL | | } - | |_________^ help: replace_it_with: `(self.opt)?;` + | |_________^ help: replace it with: `(self.opt)?;` error: this block may be rewritten with the `?` operator --> $DIR/question_mark.rs:51:9 @@ -22,7 +22,7 @@ error: this block may be rewritten with the `?` operator LL | / if self.opt.is_none() { LL | | return None LL | | } - | |_________^ help: replace_it_with: `self.opt?;` + | |_________^ help: replace it with: `self.opt?;` error: this block may be rewritten with the `?` operator --> $DIR/question_mark.rs:55:17 @@ -33,31 +33,64 @@ LL | | return None; LL | | } else { LL | | self.opt LL | | }; - | |_________^ help: replace_it_with: `Some(self.opt?)` + | |_________^ help: replace it with: `Some(self.opt?)` + +error: this if-let-else may be rewritten with the `?` operator + --> $DIR/question_mark.rs:61:17 + | +LL | let _ = if let Some(x) = self.opt { + | _________________^ +LL | | x +LL | | } else { +LL | | return None; +LL | | }; + | |_________^ help: replace it with: `self.opt?` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:72:9 + --> $DIR/question_mark.rs:78:9 | LL | / if self.opt.is_none() { LL | | return None; LL | | } - | |_________^ help: replace_it_with: `self.opt.as_ref()?;` + | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:80:9 + --> $DIR/question_mark.rs:86:9 | LL | / if self.opt.is_none() { LL | | return None; LL | | } - | |_________^ help: replace_it_with: `self.opt.as_ref()?;` + | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:88:9 + --> $DIR/question_mark.rs:94:9 | LL | / if self.opt.is_none() { LL | | return None; LL | | } - | |_________^ help: replace_it_with: `self.opt.as_ref()?;` + | |_________^ help: replace it with: `self.opt.as_ref()?;` + +error: this if-let-else may be rewritten with the `?` operator + --> $DIR/question_mark.rs:101:30 + | +LL | let mut v: &Vec<_> = if let Some(ref v) = self.opt { + | ______________________________^ +LL | | v +LL | | } else { +LL | | return None; +LL | | }; + | |_________^ help: replace it with: `self.opt.as_ref()?` + +error: this if-let-else may be rewritten with the `?` operator + --> $DIR/question_mark.rs:111:21 + | +LL | let mut v = if let Some(v) = self.opt { + | _____________________^ +LL | | v +LL | | } else { +LL | | return None; +LL | | }; + | |_________^ help: replace it with: `self.opt?` -error: aborting due to 7 previous errors +error: aborting due to 10 previous errors |