From 45f61ead2cf693a42025778b2315259bd95495e4 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Wed, 4 Mar 2020 16:59:16 +0900 Subject: [PATCH 1/3] Lint `if let Some` in question_mark lint --- clippy_lints/src/question_mark.rs | 55 +++++++++++++++++++++++++++++-- clippy_lints/src/types.rs | 7 +--- tests/ui/question_mark.rs | 26 +++++++++++++++ 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 f21c2985d..5ad580aa0 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 f90867605..58a909c84 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 57237c52e..77aa3976b 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> { + 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> { + 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 522501d58..dabba9842 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: aborting due to 7 previous errors +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 10 previous errors From 246709f89e0e8050ed714dca2155adc4e8d3ab80 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Wed, 4 Mar 2020 17:21:07 +0900 Subject: [PATCH 2/3] run-rustfix --- clippy_lints/src/question_mark.rs | 5 +- tests/ui/question_mark.fixed | 113 ++++++++++++++++++++++++++++++ tests/ui/question_mark.rs | 8 ++- tests/ui/question_mark.stderr | 28 ++++---- 4 files changed, 136 insertions(+), 18 deletions(-) create mode 100644 tests/ui/question_mark.fixed diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 5ad580aa0..3b030f95d 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -57,7 +57,8 @@ impl QuestionMark { if Self::is_option(cx, subject); then { - let receiver_str = &Sugg::hir(cx, subject, ".."); + let mut applicability = Applicability::MachineApplicable; + let receiver_str = snippet_with_applicability(cx, subject.span, "..", &mut applicability); let mut replacement: Option = None; if let Some(else_) = else_ { if_chain! { @@ -86,7 +87,7 @@ impl QuestionMark { expr.span, "replace it with", replacement_str, - Applicability::MaybeIncorrect, // snippet + applicability, ); } ) diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed new file mode 100644 index 000000000..2c3e4989d --- /dev/null +++ b/tests/ui/question_mark.fixed @@ -0,0 +1,113 @@ +// run-rustfix +#![allow(unreachable_code)] + +fn some_func(a: Option) -> Option { + a?; + + a +} + +fn some_other_func(a: Option) -> Option { + if a.is_none() { + return None; + } else { + return Some(0); + } + unreachable!() +} + +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 CopyStruct { + pub opt: Option, +} + +impl CopyStruct { + #[rustfmt::skip] + pub fn func(&self) -> Option { + (self.opt)?; + + self.opt?; + + let _ = Some(self.opt?); + + let _ = self.opt?; + + self.opt + } +} + +#[derive(Clone)] +pub struct MoveStruct { + pub opt: Option>, +} + +impl MoveStruct { + pub fn ref_func(&self) -> Option> { + self.opt.as_ref()?; + + self.opt.clone() + } + + pub fn mov_func_reuse(self) -> Option> { + self.opt.as_ref()?; + + self.opt + } + + pub fn mov_func_no_use(self) -> Option> { + self.opt.as_ref()?; + Some(Vec::new()) + } + + pub fn if_let_ref_func(self) -> Option> { + let v: &Vec<_> = self.opt.as_ref()?; + + Some(v.clone()) + } + + pub fn if_let_mov_func(self) -> Option> { + let v = self.opt?; + + Some(v) + } +} + +fn main() { + some_func(Some(42)); + some_func(None); + some_other_func(Some(42)); + + let copy_struct = CopyStruct { opt: Some(54) }; + copy_struct.func(); + + let move_struct = MoveStruct { + opt: Some(vec![42, 1337]), + }; + move_struct.ref_func(); + move_struct.clone().mov_func_reuse(); + move_struct.mov_func_no_use(); + + let so = SeemsOption::Some(45); + returns_something_similar_to_option(so); +} diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index 77aa3976b..24df76344 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -1,3 +1,6 @@ +// run-rustfix +#![allow(unreachable_code)] + fn some_func(a: Option) -> Option { if a.is_none() { return None; @@ -98,7 +101,7 @@ impl MoveStruct { } pub fn if_let_ref_func(self) -> Option> { - let mut v: &Vec<_> = if let Some(ref v) = self.opt { + let v: &Vec<_> = if let Some(ref v) = self.opt { v } else { return None; @@ -108,7 +111,7 @@ impl MoveStruct { } pub fn if_let_mov_func(self) -> Option> { - let mut v = if let Some(v) = self.opt { + let v = if let Some(v) = self.opt { v } else { return None; @@ -121,6 +124,7 @@ impl MoveStruct { fn main() { some_func(Some(42)); some_func(None); + some_other_func(Some(42)); let copy_struct = CopyStruct { opt: Some(54) }; copy_struct.func(); diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index dabba9842..97741069b 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -1,5 +1,5 @@ error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:2:5 + --> $DIR/question_mark.rs:5:5 | LL | / if a.is_none() { LL | | return None; @@ -9,7 +9,7 @@ LL | | } = note: `-D clippy::question-mark` implied by `-D warnings` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:47:9 + --> $DIR/question_mark.rs:50:9 | LL | / if (self.opt).is_none() { LL | | return None; @@ -17,7 +17,7 @@ LL | | } | |_________^ help: replace it with: `(self.opt)?;` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:51:9 + --> $DIR/question_mark.rs:54:9 | LL | / if self.opt.is_none() { LL | | return None @@ -25,7 +25,7 @@ LL | | } | |_________^ help: replace it with: `self.opt?;` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:55:17 + --> $DIR/question_mark.rs:58:17 | LL | let _ = if self.opt.is_none() { | _________________^ @@ -36,7 +36,7 @@ LL | | }; | |_________^ help: replace it with: `Some(self.opt?)` error: this if-let-else may be rewritten with the `?` operator - --> $DIR/question_mark.rs:61:17 + --> $DIR/question_mark.rs:64:17 | LL | let _ = if let Some(x) = self.opt { | _________________^ @@ -47,7 +47,7 @@ LL | | }; | |_________^ help: replace it with: `self.opt?` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:78:9 + --> $DIR/question_mark.rs:81:9 | LL | / if self.opt.is_none() { LL | | return None; @@ -55,7 +55,7 @@ LL | | } | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:86:9 + --> $DIR/question_mark.rs:89:9 | LL | / if self.opt.is_none() { LL | | return None; @@ -63,7 +63,7 @@ LL | | } | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:94:9 + --> $DIR/question_mark.rs:97:9 | LL | / if self.opt.is_none() { LL | | return None; @@ -71,10 +71,10 @@ LL | | } | |_________^ 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 + --> $DIR/question_mark.rs:104:26 | -LL | let mut v: &Vec<_> = if let Some(ref v) = self.opt { - | ______________________________^ +LL | let v: &Vec<_> = if let Some(ref v) = self.opt { + | __________________________^ LL | | v LL | | } else { LL | | return None; @@ -82,10 +82,10 @@ 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 + --> $DIR/question_mark.rs:114:17 | -LL | let mut v = if let Some(v) = self.opt { - | _____________________^ +LL | let v = if let Some(v) = self.opt { + | _________________^ LL | | v LL | | } else { LL | | return None; From a78a1fc97bfa78eba0d276add37aad543e0e610d Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Wed, 4 Mar 2020 12:59:58 +0000 Subject: [PATCH 3/3] Apply suggestions from code review Co-Authored-By: Philipp Krones --- clippy_lints/src/question_mark.rs | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 3b030f95d..f054c6ef6 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -8,7 +8,7 @@ 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_qpath, match_type, snippet_with_applicability, span_lint_and_then, SpanlessEq, + higher, match_def_path, match_qpath, match_type, snippet_with_applicability, span_lint_and_sugg, SpanlessEq, }; declare_clippy_lint! { @@ -58,7 +58,7 @@ impl QuestionMark { then { let mut applicability = Applicability::MachineApplicable; - let receiver_str = snippet_with_applicability(cx, subject.span, "..", &mut applicability); + let receiver_str = &Sugg::hir_with_applicability(cx, subject, "..", &mut applicability); let mut replacement: Option = None; if let Some(else_) = else_ { if_chain! { @@ -77,19 +77,14 @@ impl QuestionMark { } if let Some(replacement_str) = replacement { - span_lint_and_then( + span_lint_and_sugg( cx, QUESTION_MARK, expr.span, "this block may be rewritten with the `?` operator", - |db| { - db.span_suggestion( - expr.span, - "replace it with", - replacement_str, - applicability, - ); - } + "replace it with", + replacement_str, + applicability, ) } } @@ -124,19 +119,14 @@ impl QuestionMark { if by_ref { ".as_ref()" } else { "" }, ); - span_lint_and_then( + span_lint_and_sugg( 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, - ); - } + "replace it with", + replacement, + applicability, ) } }