From 8b0ea2288590b7e0cf7f276467ed1851208902fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20F=C3=BCrstenau?= Date: Tue, 18 Dec 2018 11:25:13 +0100 Subject: [PATCH 1/6] Deduplicate some code? --- clippy_lints/src/question_mark.rs | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 057b4850e4f..95f79219105 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -72,6 +72,8 @@ impl Pass { if Self::is_option(cx, subject); then { + let receiver_str = &Sugg::hir(cx, subject, ".."); + let mut replacement_str = String::new(); if let Some(else_) = else_ { if_chain! { if let ExprKind::Block(block, None) = &else_.node; @@ -79,37 +81,22 @@ impl Pass { if let Some(block_expr) = &block.expr; if SpanlessEq::new(cx).ignore_fn().eq_expr(subject, block_expr); then { - span_lint_and_then( - cx, - QUESTION_MARK, - expr.span, - "this block may be rewritten with the `?` operator", - |db| { - db.span_suggestion_with_applicability( - expr.span, - "replace_it_with", - format!("Some({}?)", Sugg::hir(cx, subject, "..")), - Applicability::MaybeIncorrect, // snippet - ); - } - ) + replacement_str = format!("Some({}?)", receiver_str); } } - return; + } else { + replacement_str = format!("{}?;", receiver_str); } - span_lint_and_then( cx, QUESTION_MARK, expr.span, "this block may be rewritten with the `?` operator", |db| { - let receiver_str = &Sugg::hir(cx, subject, ".."); - db.span_suggestion_with_applicability( expr.span, "replace_it_with", - format!("{}?;", receiver_str), + replacement_str, Applicability::MaybeIncorrect, // snippet ); } @@ -132,7 +119,7 @@ impl Pass { } false - }, + } ExprKind::Ret(Some(ref expr)) => Self::expression_returns_none(cx, expr), ExprKind::Path(ref qp) => { if let Def::VariantCtor(def_id, _) = cx.tables.qpath_def(qp, expression.hir_id) { @@ -140,7 +127,7 @@ impl Pass { } false - }, + } _ => false, } } From ee0856cbeb4ec600399b01fcca58e1804478c3a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20F=C3=BCrstenau?= Date: Tue, 18 Dec 2018 13:55:04 +0100 Subject: [PATCH 2/6] Recomend `.as_ref()?` in certain situations --- clippy_lints/src/question_mark.rs | 8 ++++++ tests/ui/question_mark.rs | 45 ++++++++++++++++++++++++++++--- tests/ui/question_mark.stderr | 26 +++++++++++++++++- 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 95f79219105..c7501ccf2d7 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -84,6 +84,8 @@ impl Pass { replacement_str = format!("Some({}?)", receiver_str); } } + } else if Self::moves_by_default(cx, subject) { + replacement_str = format!("{}.as_ref()?;", receiver_str); } else { replacement_str = format!("{}?;", receiver_str); } @@ -105,6 +107,12 @@ impl Pass { } } + fn moves_by_default(cx: &LateContext<'_, '_>, expression: &Expr) -> bool { + let expr_ty = cx.tables.expr_ty(expression); + + expr_ty.moves_by_default(cx.tcx, cx.param_env, expression.span) + } + fn is_option(cx: &LateContext<'_, '_>, expression: &Expr) -> bool { let expr_ty = cx.tables.expr_ty(expression); diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index b1edec32eee..c6726ee567c 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -37,11 +37,11 @@ fn returns_something_similar_to_option(a: SeemsOption) -> SeemsOption a } -pub struct SomeStruct { +pub struct CopyStruct { pub opt: Option, } -impl SomeStruct { +impl CopyStruct { #[rustfmt::skip] pub fn func(&self) -> Option { if (self.opt).is_none() { @@ -62,12 +62,49 @@ impl SomeStruct { } } +#[derive(Clone)] +pub struct MoveStruct { + pub opt: Option>, +} + +impl MoveStruct { + pub fn ref_func(&self) -> Option> { + if self.opt.is_none() { + return None; + } + + self.opt.clone() + } + + pub fn mov_func_reuse(self) -> Option> { + if self.opt.is_none() { + return None; + } + + self.opt + } + + pub fn mov_func_no_use(self) -> Option> { + if self.opt.is_none() { + return None; + } + Some(Vec::new()) + } +} + fn main() { some_func(Some(42)); some_func(None); - let some_struct = SomeStruct { opt: Some(54) }; - some_struct.func(); + 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.clone().mov_func_no_use(); 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 index c9d5538f36f..4f3b2c65de5 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -35,5 +35,29 @@ error: this block may be rewritten with the `?` operator 59 | | }; | |_________^ help: replace_it_with: `Some(self.opt?)` -error: aborting due to 4 previous errors +error: this block may be rewritten with the `?` operator + --> $DIR/question_mark.rs:72:9 + | +72 | / if self.opt.is_none() { +73 | | return None; +74 | | } + | |_________^ help: replace_it_with: `self.opt.as_ref()?;` + +error: this block may be rewritten with the `?` operator + --> $DIR/question_mark.rs:80:9 + | +80 | / if self.opt.is_none() { +81 | | return None; +82 | | } + | |_________^ help: replace_it_with: `self.opt.as_ref()?;` + +error: this block may be rewritten with the `?` operator + --> $DIR/question_mark.rs:88:9 + | +88 | / if self.opt.is_none() { +89 | | return None; +90 | | } + | |_________^ help: replace_it_with: `self.opt.as_ref()?;` + +error: aborting due to 7 previous errors From e722b1338e22c46bbe74a8e7a82bc29916570105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20F=C3=BCrstenau?= Date: Wed, 19 Dec 2018 20:31:08 +0100 Subject: [PATCH 3/6] Reinserted commata --- clippy_lints/src/question_mark.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index c7501ccf2d7..98aeb3a8e10 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -127,7 +127,7 @@ impl Pass { } false - } + }, ExprKind::Ret(Some(ref expr)) => Self::expression_returns_none(cx, expr), ExprKind::Path(ref qp) => { if let Def::VariantCtor(def_id, _) = cx.tables.qpath_def(qp, expression.hir_id) { @@ -135,7 +135,7 @@ impl Pass { } false - } + }, _ => false, } } From 18584698eebfe1f0e51349de352a53179735cf52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20F=C3=BCrstenau?= Date: Wed, 19 Dec 2018 20:46:12 +0100 Subject: [PATCH 4/6] Add failing test --- tests/ui/question_mark.rs | 9 ++++++ tests/ui/question_mark.stderr | 52 +++++++++++++++++------------------ 2 files changed, 35 insertions(+), 26 deletions(-) diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index c6726ee567c..880c163e833 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -15,6 +15,15 @@ fn some_func(a: Option) -> Option { 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, diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 4f3b2c65de5..f55a83d43c8 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -9,54 +9,54 @@ error: this block may be rewritten with the `?` operator = 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:56:9 | -47 | / if (self.opt).is_none() { -48 | | return None; -49 | | } +56 | / if (self.opt).is_none() { +57 | | return None; +58 | | } | |_________^ 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:60:9 | -51 | / if self.opt.is_none() { -52 | | return None -53 | | } +60 | / if self.opt.is_none() { +61 | | return None +62 | | } | |_________^ 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:64:17 | -55 | let _ = if self.opt.is_none() { +64 | let _ = if self.opt.is_none() { | _________________^ -56 | | return None; -57 | | } else { -58 | | self.opt -59 | | }; +65 | | return None; +66 | | } else { +67 | | self.opt +68 | | }; | |_________^ help: replace_it_with: `Some(self.opt?)` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:72:9 + --> $DIR/question_mark.rs:81:9 | -72 | / if self.opt.is_none() { -73 | | return None; -74 | | } +81 | / if self.opt.is_none() { +82 | | return None; +83 | | } | |_________^ 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:89:9 | -80 | / if self.opt.is_none() { -81 | | return None; -82 | | } +89 | / if self.opt.is_none() { +90 | | return None; +91 | | } | |_________^ 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:97:9 | -88 | / if self.opt.is_none() { -89 | | return None; -90 | | } +97 | / if self.opt.is_none() { +98 | | return None; +99 | | } | |_________^ help: replace_it_with: `self.opt.as_ref()?;` error: aborting due to 7 previous errors From 65c35333a4269a987350272339f3a078fd64f2b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20F=C3=BCrstenau?= Date: Wed, 19 Dec 2018 20:55:01 +0100 Subject: [PATCH 5/6] Only print out question_mark lint when it actually triggered --- clippy_lints/src/question_mark.rs | 39 +++++++++++++++++-------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 98aeb3a8e10..76fb6350681 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -73,7 +73,7 @@ impl Pass { then { let receiver_str = &Sugg::hir(cx, subject, ".."); - let mut replacement_str = String::new(); + let mut replacement: Option = None; if let Some(else_) = else_ { if_chain! { if let ExprKind::Block(block, None) = &else_.node; @@ -81,28 +81,31 @@ impl Pass { if let Some(block_expr) = &block.expr; if SpanlessEq::new(cx).ignore_fn().eq_expr(subject, block_expr); then { - replacement_str = format!("Some({}?)", receiver_str); + replacement = Some(format!("Some({}?)", receiver_str)); } } } else if Self::moves_by_default(cx, subject) { - replacement_str = format!("{}.as_ref()?;", receiver_str); + replacement = Some(format!("{}.as_ref()?;", receiver_str)); } else { - replacement_str = format!("{}?;", receiver_str); + replacement = Some(format!("{}?;", receiver_str)); } - span_lint_and_then( - cx, - QUESTION_MARK, - expr.span, - "this block may be rewritten with the `?` operator", - |db| { - db.span_suggestion_with_applicability( - expr.span, - "replace_it_with", - replacement_str, - Applicability::MaybeIncorrect, // snippet - ); - } - ) + + if let Some(replacement_str) = replacement { + span_lint_and_then( + cx, + QUESTION_MARK, + expr.span, + "this block may be rewritten with the `?` operator", + |db| { + db.span_suggestion_with_applicability( + expr.span, + "replace_it_with", + replacement_str, + Applicability::MaybeIncorrect, // snippet + ); + } + ) + } } } } From 8be7050b740c0e48a921e01446d5ec0e9a35881d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20F=C3=BCrstenau?= Date: Fri, 28 Dec 2018 20:52:46 +0100 Subject: [PATCH 6/6] Fix formatting --- tests/ui/question_mark.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index 880c163e833..7e749d164ca 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -19,7 +19,7 @@ fn some_other_func(a: Option) -> Option { if a.is_none() { return None; } else { - return Some(0); + return Some(0); } unreachable!() }