From d4196d029314c494d84aed97043b2eb84ecdea5b Mon Sep 17 00:00:00 2001 From: dswij Date: Fri, 22 Oct 2021 13:36:13 +0800 Subject: [PATCH 1/5] Add #7859 FP test case for `question_mark` --- tests/ui/question_mark.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index ca3722371..fc68a42e4 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -2,6 +2,9 @@ #![allow(unreachable_code)] #![allow(clippy::unnecessary_wraps)] +use std::env; +use std::path::PathBuf; + fn some_func(a: Option) -> Option { if a.is_none() { return None; @@ -134,7 +137,11 @@ fn func() -> Option { Some(0) } -fn result_func(x: Result) -> Result { +fn func_returning_result() -> Result { + Ok(1) +} + +fn result_func(x: Result) -> Result { let _ = if let Ok(x) = x { x } else { return x }; if x.is_err() { @@ -145,9 +152,21 @@ fn result_func(x: Result) -> Result { let y = if let Ok(x) = x { x } else { - return Err("some error"); + return Err("some error".to_string()); }; + // issue #7859 + // no warning + let _ = if let Ok(x) = func_returning_result() { + x + } else { + return Err("some error".to_string()); + }; + + if func_returning_result().is_err() { + return func_returning_result(); + } + Ok(y) } From a1a399d16830ac84c1ce18c2a0e8a77a13edf265 Mon Sep 17 00:00:00 2001 From: dswij Date: Fri, 22 Oct 2021 13:41:46 +0800 Subject: [PATCH 2/5] Fix `question_mark` FP on calls --- clippy_lints/src/question_mark.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 4d616e26b..01fa52330 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -172,23 +172,17 @@ impl QuestionMark { } } - fn expression_returns_unmodified_err( - cx: &LateContext<'_>, - expression: &Expr<'_>, - origin_hir_id: &Expr<'_>, - ) -> bool { + fn expression_returns_unmodified_err(cx: &LateContext<'_>, expression: &Expr<'_>, cond_expr: &Expr<'_>) -> bool { match expression.kind { ExprKind::Block(block, _) => { if let Some(return_expression) = Self::return_expression(block) { - return Self::expression_returns_unmodified_err(cx, return_expression, origin_hir_id); + return Self::expression_returns_unmodified_err(cx, return_expression, cond_expr); } false }, - ExprKind::Ret(Some(expr)) | ExprKind::Call(expr, _) => { - Self::expression_returns_unmodified_err(cx, expr, origin_hir_id) - }, - ExprKind::Path(_) => path_to_local(expression) == path_to_local(origin_hir_id), + ExprKind::Ret(Some(expr)) => Self::expression_returns_unmodified_err(cx, expr, cond_expr), + ExprKind::Path(_) => path_to_local(expression) == path_to_local(cond_expr), _ => false, } } From a91ec594601cc1b3e11cf110a403c54ae826044b Mon Sep 17 00:00:00 2001 From: dswij Date: Fri, 22 Oct 2021 13:43:21 +0800 Subject: [PATCH 3/5] Update `question_mark` expected test output --- tests/ui/question_mark.fixed | 25 ++++++++++++++++++++++--- tests/ui/question_mark.stderr | 28 ++++++++++++++-------------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index ccb2e5a30..060ea8c8c 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -2,6 +2,9 @@ #![allow(unreachable_code)] #![allow(clippy::unnecessary_wraps)] +use std::env; +use std::path::PathBuf; + fn some_func(a: Option) -> Option { a?; @@ -104,18 +107,34 @@ fn func() -> Option { Some(0) } -fn result_func(x: Result) -> Result { +fn func_returning_result() -> Result { + Ok(1) +} + +fn result_func(x: Result) -> Result { let _ = x?; - x?; + x.as_ref()?; // No warning let y = if let Ok(x) = x { x } else { - return Err("some error"); + return Err("some error".to_string()); }; + // issue #7859 + // no warning + let _ = if let Ok(x) = func_returning_result() { + x + } else { + return Err("some error".to_string()); + }; + + if func_returning_result().is_err() { + return func_returning_result(); + } + Ok(y) } diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 161588cb7..8eee7f8cb 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:6:5 + --> $DIR/question_mark.rs:9: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:51:9 + --> $DIR/question_mark.rs:54: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:55:9 + --> $DIR/question_mark.rs:58: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:59:17 + --> $DIR/question_mark.rs:62: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:65:17 + --> $DIR/question_mark.rs:68: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:82:9 + --> $DIR/question_mark.rs:85: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:90:9 + --> $DIR/question_mark.rs:93: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:98:9 + --> $DIR/question_mark.rs:101:9 | LL | / if self.opt.is_none() { LL | | return None; @@ -71,7 +71,7 @@ LL | | } | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this if-let-else may be rewritten with the `?` operator - --> $DIR/question_mark.rs:105:26 + --> $DIR/question_mark.rs:108:26 | LL | let v: &Vec<_> = if let Some(ref v) = self.opt { | __________________________^ @@ -82,7 +82,7 @@ LL | | }; | |_________^ help: replace it with: `self.opt.as_ref()?` error: this if-let-else may be rewritten with the `?` operator - --> $DIR/question_mark.rs:115:17 + --> $DIR/question_mark.rs:118:17 | LL | let v = if let Some(v) = self.opt { | _________________^ @@ -93,7 +93,7 @@ LL | | }; | |_________^ help: replace it with: `self.opt?` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:130:5 + --> $DIR/question_mark.rs:133:5 | LL | / if f().is_none() { LL | | return None; @@ -101,18 +101,18 @@ LL | | } | |_____^ help: replace it with: `f()?;` error: this if-let-else may be rewritten with the `?` operator - --> $DIR/question_mark.rs:138:13 + --> $DIR/question_mark.rs:145:13 | LL | let _ = if let Ok(x) = x { x } else { return x }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x?` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:140:5 + --> $DIR/question_mark.rs:147:5 | LL | / if x.is_err() { LL | | return x; LL | | } - | |_____^ help: replace it with: `x?;` + | |_____^ help: replace it with: `x.as_ref()?;` error: aborting due to 13 previous errors From 2fd168285a77c0b39941c9b418a6c8418d75d45a Mon Sep 17 00:00:00 2001 From: dswij Date: Fri, 22 Oct 2021 14:11:04 +0800 Subject: [PATCH 4/5] Update `question_mark` test to behave better --- tests/ui/question_mark.fixed | 14 ++++++-------- tests/ui/question_mark.rs | 12 +++++------- tests/ui/question_mark.stderr | 28 ++++++++++++++-------------- 3 files changed, 25 insertions(+), 29 deletions(-) diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index 060ea8c8c..e93469e5f 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -2,9 +2,6 @@ #![allow(unreachable_code)] #![allow(clippy::unnecessary_wraps)] -use std::env; -use std::path::PathBuf; - fn some_func(a: Option) -> Option { a?; @@ -107,20 +104,20 @@ fn func() -> Option { Some(0) } -fn func_returning_result() -> Result { +fn func_returning_result() -> Result { Ok(1) } -fn result_func(x: Result) -> Result { +fn result_func(x: Result) -> Result { let _ = x?; - x.as_ref()?; + x?; // No warning let y = if let Ok(x) = x { x } else { - return Err("some error".to_string()); + return Err(0); }; // issue #7859 @@ -128,9 +125,10 @@ fn result_func(x: Result) -> Result { let _ = if let Ok(x) = func_returning_result() { x } else { - return Err("some error".to_string()); + return Err(0); }; + // no warning if func_returning_result().is_err() { return func_returning_result(); } diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index fc68a42e4..dd179e9be 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -2,9 +2,6 @@ #![allow(unreachable_code)] #![allow(clippy::unnecessary_wraps)] -use std::env; -use std::path::PathBuf; - fn some_func(a: Option) -> Option { if a.is_none() { return None; @@ -137,11 +134,11 @@ fn func() -> Option { Some(0) } -fn func_returning_result() -> Result { +fn func_returning_result() -> Result { Ok(1) } -fn result_func(x: Result) -> Result { +fn result_func(x: Result) -> Result { let _ = if let Ok(x) = x { x } else { return x }; if x.is_err() { @@ -152,7 +149,7 @@ fn result_func(x: Result) -> Result { let y = if let Ok(x) = x { x } else { - return Err("some error".to_string()); + return Err(0); }; // issue #7859 @@ -160,9 +157,10 @@ fn result_func(x: Result) -> Result { let _ = if let Ok(x) = func_returning_result() { x } else { - return Err("some error".to_string()); + return Err(0); }; + // no warning if func_returning_result().is_err() { return func_returning_result(); } diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 8eee7f8cb..8d782b71d 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:9:5 + --> $DIR/question_mark.rs:6: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:54:9 + --> $DIR/question_mark.rs:51: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:58:9 + --> $DIR/question_mark.rs:55: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:62:17 + --> $DIR/question_mark.rs:59: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:68:17 + --> $DIR/question_mark.rs:65: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:85:9 + --> $DIR/question_mark.rs:82: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:93:9 + --> $DIR/question_mark.rs:90: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:101:9 + --> $DIR/question_mark.rs:98:9 | LL | / if self.opt.is_none() { LL | | return None; @@ -71,7 +71,7 @@ LL | | } | |_________^ help: replace it with: `self.opt.as_ref()?;` error: this if-let-else may be rewritten with the `?` operator - --> $DIR/question_mark.rs:108:26 + --> $DIR/question_mark.rs:105:26 | LL | let v: &Vec<_> = if let Some(ref v) = self.opt { | __________________________^ @@ -82,7 +82,7 @@ LL | | }; | |_________^ help: replace it with: `self.opt.as_ref()?` error: this if-let-else may be rewritten with the `?` operator - --> $DIR/question_mark.rs:118:17 + --> $DIR/question_mark.rs:115:17 | LL | let v = if let Some(v) = self.opt { | _________________^ @@ -93,7 +93,7 @@ LL | | }; | |_________^ help: replace it with: `self.opt?` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:133:5 + --> $DIR/question_mark.rs:130:5 | LL | / if f().is_none() { LL | | return None; @@ -101,18 +101,18 @@ LL | | } | |_____^ help: replace it with: `f()?;` error: this if-let-else may be rewritten with the `?` operator - --> $DIR/question_mark.rs:145:13 + --> $DIR/question_mark.rs:142:13 | LL | let _ = if let Ok(x) = x { x } else { return x }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x?` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:147:5 + --> $DIR/question_mark.rs:144:5 | LL | / if x.is_err() { LL | | return x; LL | | } - | |_____^ help: replace it with: `x.as_ref()?;` + | |_____^ help: replace it with: `x?;` error: aborting due to 13 previous errors From fb0fbad5caa97da949cb2c6dbc56a1185bb53af9 Mon Sep 17 00:00:00 2001 From: Dharma Saputra Wijaya Date: Tue, 26 Oct 2021 21:22:22 +0800 Subject: [PATCH 5/5] Rename variable name in `question_mark` --- clippy_lints/src/question_mark.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 01fa52330..f63ef163b 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -172,8 +172,8 @@ impl QuestionMark { } } - fn expression_returns_unmodified_err(cx: &LateContext<'_>, expression: &Expr<'_>, cond_expr: &Expr<'_>) -> bool { - match expression.kind { + fn expression_returns_unmodified_err(cx: &LateContext<'_>, expr: &Expr<'_>, cond_expr: &Expr<'_>) -> bool { + match expr.kind { ExprKind::Block(block, _) => { if let Some(return_expression) = Self::return_expression(block) { return Self::expression_returns_unmodified_err(cx, return_expression, cond_expr); @@ -181,8 +181,8 @@ impl QuestionMark { false }, - ExprKind::Ret(Some(expr)) => Self::expression_returns_unmodified_err(cx, expr, cond_expr), - ExprKind::Path(_) => path_to_local(expression) == path_to_local(cond_expr), + ExprKind::Ret(Some(ret_expr)) => Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr), + ExprKind::Path(_) => path_to_local(expr) == path_to_local(cond_expr), _ => false, } }