Skip to content

Commit 31f3769

Browse files
borsbors
committed
Auto merge of rust-lang#11031 - Centri3:needless_return, r=giraffate
New lint [`needless_return_with_try`] Closes rust-lang#10902 Rather than having a config option, this will just suggest removing the "return"; if `try_err` is used as well, then it'll be added again but without the `?`. changelog: New lint [`needless_return_with_try`]
2 parents a447725 + 0d59d1d commit 31f3769

9 files changed

+185
-17
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -5097,6 +5097,7 @@ Released 2018-09-13
50975097
[`needless_raw_string_hashes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_string_hashes
50985098
[`needless_raw_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_strings
50995099
[`needless_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
5100+
[`needless_return_with_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return_with_question_mark
51005101
[`needless_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_splitn
51015102
[`needless_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_update
51025103
[`neg_cmp_op_on_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_cmp_op_on_partial_ord

‎clippy_lints/src/declared_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
576576
crate::return_self_not_must_use::RETURN_SELF_NOT_MUST_USE_INFO,
577577
crate::returns::LET_AND_RETURN_INFO,
578578
crate::returns::NEEDLESS_RETURN_INFO,
579+
crate::returns::NEEDLESS_RETURN_WITH_QUESTION_MARK_INFO,
579580
crate::same_name_method::SAME_NAME_METHOD_INFO,
580581
crate::self_named_constructors::SELF_NAMED_CONSTRUCTORS_INFO,
581582
crate::semicolon_block::SEMICOLON_INSIDE_BLOCK_INFO,

‎clippy_lints/src/returns.rs

+72-4
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
1-
use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then};
1+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then, span_lint_hir_and_then};
22
use clippy_utils::source::{snippet_opt, snippet_with_context};
33
use clippy_utils::visitors::{for_each_expr_with_closures, Descend};
4-
use clippy_utils::{fn_def_id, path_to_local_id, span_find_starting_semi};
4+
use clippy_utils::{fn_def_id, is_from_proc_macro, path_to_local_id, span_find_starting_semi};
55
use core::ops::ControlFlow;
66
use if_chain::if_chain;
77
use rustc_errors::Applicability;
88
use rustc_hir::intravisit::FnKind;
9-
use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, LangItem, MatchSource, PatKind, QPath, StmtKind};
9+
use rustc_hir::{
10+
Block, Body, Expr, ExprKind, FnDecl, ItemKind, LangItem, MatchSource, OwnerNode, PatKind, QPath, Stmt, StmtKind,
11+
};
1012
use rustc_lint::{LateContext, LateLintPass, LintContext};
1113
use rustc_middle::lint::in_external_macro;
1214
use rustc_middle::ty::subst::GenericArgKind;
@@ -77,6 +79,46 @@ declare_clippy_lint! {
7779
"using a return statement like `return expr;` where an expression would suffice"
7880
}
7981

82+
declare_clippy_lint! {
83+
/// ### What it does
84+
/// Checks for return statements on `Err` paired with the `?` operator.
85+
///
86+
/// ### Why is this bad?
87+
/// The `return` is unnecessary.
88+
///
89+
/// ### Example
90+
/// ```rust,ignore
91+
/// fn foo(x: usize) -> Result<(), Box<dyn Error>> {
92+
/// if x == 0 {
93+
/// return Err(...)?;
94+
/// }
95+
/// Ok(())
96+
/// }
97+
/// ```
98+
/// simplify to
99+
/// ```rust,ignore
100+
/// fn foo(x: usize) -> Result<(), Box<dyn Error>> {
101+
/// if x == 0 {
102+
/// Err(...)?;
103+
/// }
104+
/// Ok(())
105+
/// }
106+
/// ```
107+
/// if paired with `try_err`, use instead:
108+
/// ```rust,ignore
109+
/// fn foo(x: usize) -> Result<(), Box<dyn Error>> {
110+
/// if x == 0 {
111+
/// return Err(...);
112+
/// }
113+
/// Ok(())
114+
/// }
115+
/// ```
116+
#[clippy::version = "1.73.0"]
117+
pub NEEDLESS_RETURN_WITH_QUESTION_MARK,
118+
style,
119+
"using a return statement like `return Err(expr)?;` where removing it would suffice"
120+
}
121+
80122
#[derive(PartialEq, Eq)]
81123
enum RetReplacement<'tcx> {
82124
Empty,
@@ -116,9 +158,35 @@ impl<'tcx> ToString for RetReplacement<'tcx> {
116158
}
117159
}
118160

119-
declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN]);
161+
declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN, NEEDLESS_RETURN_WITH_QUESTION_MARK]);
120162

121163
impl<'tcx> LateLintPass<'tcx> for Return {
164+
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
165+
if !in_external_macro(cx.sess(), stmt.span)
166+
&& let StmtKind::Semi(expr) = stmt.kind
167+
&& let ExprKind::Ret(Some(ret)) = expr.kind
168+
&& let ExprKind::Match(.., MatchSource::TryDesugar) = ret.kind
169+
// Ensure this is not the final stmt, otherwise removing it would cause a compile error
170+
&& let OwnerNode::Item(item) = cx.tcx.hir().owner(cx.tcx.hir().get_parent_item(expr.hir_id))
171+
&& let ItemKind::Fn(_, _, body) = item.kind
172+
&& let block = cx.tcx.hir().body(body).value
173+
&& let ExprKind::Block(block, _) = block.kind
174+
&& let [.., final_stmt] = block.stmts
175+
&& final_stmt.hir_id != stmt.hir_id
176+
&& !is_from_proc_macro(cx, expr)
177+
{
178+
span_lint_and_sugg(
179+
cx,
180+
NEEDLESS_RETURN_WITH_QUESTION_MARK,
181+
expr.span.until(ret.span),
182+
"unneeded `return` statement with `?` operator",
183+
"remove it",
184+
String::new(),
185+
Applicability::MachineApplicable,
186+
);
187+
}
188+
}
189+
122190
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) {
123191
// we need both a let-binding stmt and an expr
124192
if_chain! {

‎tests/ui/needless_return_with_question_mark.fixed

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
//@run-rustfix
2+
//@aux-build:proc_macros.rs:proc-macro
3+
#![allow(
4+
clippy::needless_return,
5+
clippy::no_effect,
6+
clippy::unit_arg,
7+
clippy::useless_conversion,
8+
unused
9+
)]
10+
11+
#[macro_use]
12+
extern crate proc_macros;
13+
14+
fn a() -> u32 {
15+
return 0;
16+
}
17+
18+
fn b() -> Result<u32, u32> {
19+
return Err(0);
20+
}
21+
22+
// Do not lint
23+
fn c() -> Option<()> {
24+
return None?;
25+
}
26+
27+
fn main() -> Result<(), ()> {
28+
Err(())?;
29+
return Ok::<(), ()>(());
30+
Err(())?;
31+
Ok::<(), ()>(());
32+
return Err(().into());
33+
external! {
34+
return Err(())?;
35+
}
36+
with_span! {
37+
return Err(())?;
38+
}
39+
Err(())
40+
}

‎tests/ui/needless_return_with_question_mark.rs

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
//@run-rustfix
2+
//@aux-build:proc_macros.rs:proc-macro
3+
#![allow(
4+
clippy::needless_return,
5+
clippy::no_effect,
6+
clippy::unit_arg,
7+
clippy::useless_conversion,
8+
unused
9+
)]
10+
11+
#[macro_use]
12+
extern crate proc_macros;
13+
14+
fn a() -> u32 {
15+
return 0;
16+
}
17+
18+
fn b() -> Result<u32, u32> {
19+
return Err(0);
20+
}
21+
22+
// Do not lint
23+
fn c() -> Option<()> {
24+
return None?;
25+
}
26+
27+
fn main() -> Result<(), ()> {
28+
return Err(())?;
29+
return Ok::<(), ()>(());
30+
Err(())?;
31+
Ok::<(), ()>(());
32+
return Err(().into());
33+
external! {
34+
return Err(())?;
35+
}
36+
with_span! {
37+
return Err(())?;
38+
}
39+
Err(())
40+
}

‎tests/ui/needless_return_with_question_mark.stderr

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
error: unneeded `return` statement with `?` operator
2+
--> $DIR/needless_return_with_question_mark.rs:28:5
3+
|
4+
LL | return Err(())?;
5+
| ^^^^^^^ help: remove it
6+
|
7+
= note: `-D clippy::needless-return-with-question-mark` implied by `-D warnings`
8+
9+
error: aborting due to previous error
10+

‎tests/ui/try_err.fixed

+5-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22
//@aux-build:proc_macros.rs:proc-macro
33

44
#![deny(clippy::try_err)]
5-
#![allow(clippy::unnecessary_wraps, clippy::needless_question_mark)]
5+
#![allow(
6+
clippy::unnecessary_wraps,
7+
clippy::needless_question_mark,
8+
clippy::needless_return_with_question_mark
9+
)]
610

711
extern crate proc_macros;
812
use proc_macros::{external, inline_macros};

‎tests/ui/try_err.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22
//@aux-build:proc_macros.rs:proc-macro
33

44
#![deny(clippy::try_err)]
5-
#![allow(clippy::unnecessary_wraps, clippy::needless_question_mark)]
5+
#![allow(
6+
clippy::unnecessary_wraps,
7+
clippy::needless_question_mark,
8+
clippy::needless_return_with_question_mark
9+
)]
610

711
extern crate proc_macros;
812
use proc_macros::{external, inline_macros};

‎tests/ui/try_err.stderr

+11-11
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: returning an `Err(_)` with the `?` operator
2-
--> $DIR/try_err.rs:19:9
2+
--> $DIR/try_err.rs:23:9
33
|
44
LL | Err(err)?;
55
| ^^^^^^^^^ help: try: `return Err(err)`
@@ -11,65 +11,65 @@ LL | #![deny(clippy::try_err)]
1111
| ^^^^^^^^^^^^^^^
1212

1313
error: returning an `Err(_)` with the `?` operator
14-
--> $DIR/try_err.rs:29:9
14+
--> $DIR/try_err.rs:33:9
1515
|
1616
LL | Err(err)?;
1717
| ^^^^^^^^^ help: try: `return Err(err.into())`
1818

1919
error: returning an `Err(_)` with the `?` operator
20-
--> $DIR/try_err.rs:49:17
20+
--> $DIR/try_err.rs:53:17
2121
|
2222
LL | Err(err)?;
2323
| ^^^^^^^^^ help: try: `return Err(err)`
2424

2525
error: returning an `Err(_)` with the `?` operator
26-
--> $DIR/try_err.rs:68:17
26+
--> $DIR/try_err.rs:72:17
2727
|
2828
LL | Err(err)?;
2929
| ^^^^^^^^^ help: try: `return Err(err.into())`
3030

3131
error: returning an `Err(_)` with the `?` operator
32-
--> $DIR/try_err.rs:88:23
32+
--> $DIR/try_err.rs:92:23
3333
|
3434
LL | Err(_) => Err(1)?,
3535
| ^^^^^^^ help: try: `return Err(1)`
3636
|
3737
= note: this error originates in the macro `__inline_mac_fn_calling_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
3838

3939
error: returning an `Err(_)` with the `?` operator
40-
--> $DIR/try_err.rs:95:23
40+
--> $DIR/try_err.rs:99:23
4141
|
4242
LL | Err(_) => Err(inline!(1))?,
4343
| ^^^^^^^^^^^^^^^^ help: try: `return Err(inline!(1))`
4444
|
4545
= note: this error originates in the macro `__inline_mac_fn_calling_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
4646

4747
error: returning an `Err(_)` with the `?` operator
48-
--> $DIR/try_err.rs:122:9
48+
--> $DIR/try_err.rs:126:9
4949
|
5050
LL | Err(inline!(inline!(String::from("aasdfasdfasdfa"))))?;
5151
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `return Err(inline!(inline!(String::from("aasdfasdfasdfa"))))`
5252

5353
error: returning an `Err(_)` with the `?` operator
54-
--> $DIR/try_err.rs:129:9
54+
--> $DIR/try_err.rs:133:9
5555
|
5656
LL | Err(io::ErrorKind::WriteZero)?
5757
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `return Poll::Ready(Err(io::ErrorKind::WriteZero.into()))`
5858

5959
error: returning an `Err(_)` with the `?` operator
60-
--> $DIR/try_err.rs:131:9
60+
--> $DIR/try_err.rs:135:9
6161
|
6262
LL | Err(io::Error::new(io::ErrorKind::InvalidInput, "error"))?
6363
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `return Poll::Ready(Err(io::Error::new(io::ErrorKind::InvalidInput, "error")))`
6464

6565
error: returning an `Err(_)` with the `?` operator
66-
--> $DIR/try_err.rs:139:9
66+
--> $DIR/try_err.rs:143:9
6767
|
6868
LL | Err(io::ErrorKind::NotFound)?
6969
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `return Poll::Ready(Some(Err(io::ErrorKind::NotFound.into())))`
7070

7171
error: returning an `Err(_)` with the `?` operator
72-
--> $DIR/try_err.rs:148:16
72+
--> $DIR/try_err.rs:152:16
7373
|
7474
LL | return Err(42)?;
7575
| ^^^^^^^^ help: try: `Err(42)`

0 commit comments

Comments
 (0)