Skip to content

Commit

Permalink
Auto merge of #13027 - xFrednet:12998-expect-returns, r=Jarcho
Browse files Browse the repository at this point in the history 
`needless_return`: Support `#[expect]` on the return statement

A fix for #9361 suppresses `clippy::needless_return` if there are any attributes on the `return` statement. This leads to some unexpected behavior, as described in #12998, where adding `#[expect(clippy::needless_return)]` suppresses the lint, but doesn't fulfill the expectation.

I now decided to manually fulfill any expectations, if they are before the attribute check.

---

Closes: #12998

changelog: none
  • Loading branch information
@bors
bors committed Jul 3, 2024
2 parents 0f4035f + 903874d commit a4bdab3
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 29 deletions.
47 changes: 38 additions & 9 deletions clippy_lints/src/returns.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,21 @@ use clippy_utils::{
path_to_local_id, span_contains_cfg, span_find_starting_semi,
};
use core::ops::ControlFlow;
use rustc_ast::NestedMetaItem;
use rustc_errors::Applicability;
use rustc_hir::intravisit::FnKind;
use rustc_hir::LangItem::ResultErr;
use rustc_hir::{
Block, Body, Expr, ExprKind, FnDecl, HirId, ItemKind, LangItem, MatchSource, Node, OwnerNode, PatKind, QPath, Stmt,
StmtKind,
};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_lint::{LateContext, LateLintPass, Level, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::adjustment::Adjust;
use rustc_middle::ty::{self, GenericArgKind, Ty};
use rustc_session::declare_lint_pass;
use rustc_span::def_id::LocalDefId;
use rustc_span::{BytePos, Pos, Span};
use rustc_span::{sym, BytePos, Pos, Span};
use std::borrow::Cow;
use std::fmt::Display;

Expand Down Expand Up @@ -80,6 +81,9 @@ declare_clippy_lint! {
/// ```
#[clippy::version = "pre 1.29.0"]
pub NEEDLESS_RETURN,
// This lint requires some special handling in `check_final_expr` for `#[expect]`.
// This handling needs to be updated if the group gets changed. This should also
// be caught by tests.
style,
"using a return statement like `return expr;` where an expression would suffice"
}
Expand All @@ -91,6 +95,9 @@ declare_clippy_lint! {
/// ### Why is this bad?
/// The `return` is unnecessary.
///
/// Returns may be used to add attributes to the return expression. Return
/// statements with attributes are therefore be accepted by this lint.
///
/// ### Example
/// ```rust,ignore
/// fn foo(x: usize) -> Result<(), Box<dyn Error>> {
Expand Down Expand Up @@ -377,13 +384,39 @@ fn check_final_expr<'tcx>(
}
};

if !cx.tcx.hir().attrs(expr.hir_id).is_empty() {
return;
}
let borrows = inner.map_or(false, |inner| last_statement_borrows(cx, inner));
if borrows {
return;
}
if ret_span.from_expansion() {
return;
}

// Returns may be used to turn an expression into a statement in rustc's AST.
// This allows the addition of attributes, like `#[allow]` (See: clippy#9361)
// `#[expect(clippy::needless_return)]` needs to be handled separatly to
// actually fullfil the expectation (clippy::#12998)
match cx.tcx.hir().attrs(expr.hir_id) {
[] => {},
[attr] => {
if matches!(Level::from_attr(attr), Some(Level::Expect(_)))
&& let metas = attr.meta_item_list()
&& let Some(lst) = metas
&& let [NestedMetaItem::MetaItem(meta_item)] = lst.as_slice()
&& let [tool, lint_name] = meta_item.path.segments.as_slice()
&& tool.ident.name == sym::clippy
&& matches!(
lint_name.ident.name.as_str(),
"needless_return" | "style" | "all" | "warnings"
)
{
// This is an expectation of the `needless_return` lint
} else {
return;
}
},
_ => return,
}

emit_return_lint(cx, ret_span, semi_spans, &replacement, expr.hir_id);
},
Expand Down Expand Up @@ -415,10 +448,6 @@ fn emit_return_lint(
replacement: &RetReplacement<'_>,
at: HirId,
) {
if ret_span.from_expansion() {
return;
}

span_lint_hir_and_then(
cx,
NEEDLESS_RETURN,
Expand Down
35 changes: 32 additions & 3 deletions tests/ui/needless_return.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,41 @@ fn needless_return_macro() -> String {
format!("Hello {}", "world!")
}

fn issue_9361() -> i32 {
let n = 1;
#[allow(clippy::arithmetic_side_effects)]
fn issue_9361(n: i32) -> i32 {
#[expect(clippy::arithmetic_side_effects)]
return n + n;
}

mod issue_12998 {
fn expect_lint() -> i32 {
let x = 1;

#[expect(clippy::needless_return)]
return x;
}

fn expect_group() -> i32 {
let x = 1;

#[expect(clippy::style)]
return x;
}

fn expect_all() -> i32 {
let x = 1;

#[expect(clippy::all)]
return x;
}

fn expect_warnings() -> i32 {
let x = 1;

#[expect(warnings)]
return x;
}
}

fn issue8336(x: i32) -> bool {
if x > 0 {
println!("something");
Expand Down
35 changes: 32 additions & 3 deletions tests/ui/needless_return.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,41 @@ fn needless_return_macro() -> String {
return format!("Hello {}", "world!");
}

fn issue_9361() -> i32 {
let n = 1;
#[allow(clippy::arithmetic_side_effects)]
fn issue_9361(n: i32) -> i32 {
#[expect(clippy::arithmetic_side_effects)]
return n + n;
}

mod issue_12998 {
fn expect_lint() -> i32 {
let x = 1;

#[expect(clippy::needless_return)]
return x;
}

fn expect_group() -> i32 {
let x = 1;

#[expect(clippy::style)]
return x;
}

fn expect_all() -> i32 {
let x = 1;

#[expect(clippy::all)]
return x;
}

fn expect_warnings() -> i32 {
let x = 1;

#[expect(warnings)]
return x;
}
}

fn issue8336(x: i32) -> bool {
if x > 0 {
println!("something");
Expand Down
28 changes: 14 additions & 14 deletions tests/ui/needless_return.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ LL + format!("Hello {}", "world!")
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:248:9
--> tests/ui/needless_return.rs:277:9
|
LL | return true;
| ^^^^^^^^^^^
Expand All @@ -497,7 +497,7 @@ LL ~ }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:250:9
--> tests/ui/needless_return.rs:279:9
|
LL | return false;
| ^^^^^^^^^^^^
Expand All @@ -509,7 +509,7 @@ LL ~ }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:257:13
--> tests/ui/needless_return.rs:286:13
|
LL | return 10;
| ^^^^^^^^^
Expand All @@ -524,7 +524,7 @@ LL ~ }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:260:13
--> tests/ui/needless_return.rs:289:13
|
LL | return 100;
| ^^^^^^^^^^
Expand All @@ -537,7 +537,7 @@ LL ~ }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:268:9
--> tests/ui/needless_return.rs:297:9
|
LL | return 0;
| ^^^^^^^^
Expand All @@ -549,7 +549,7 @@ LL ~ }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:275:13
--> tests/ui/needless_return.rs:304:13
|
LL | return *(x as *const isize);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -564,7 +564,7 @@ LL ~ }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:277:13
--> tests/ui/needless_return.rs:306:13
|
LL | return !*(x as *const isize);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -577,7 +577,7 @@ LL ~ }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:284:20
--> tests/ui/needless_return.rs:313:20
|
LL | let _ = 42;
| ____________________^
Expand All @@ -594,7 +594,7 @@ LL + let _ = 42;
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:291:20
--> tests/ui/needless_return.rs:320:20
|
LL | let _ = 42; return;
| ^^^^^^^
Expand All @@ -606,7 +606,7 @@ LL + let _ = 42;
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:303:9
--> tests/ui/needless_return.rs:332:9
|
LL | return Ok(format!("ok!"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -618,7 +618,7 @@ LL + Ok(format!("ok!"))
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:305:9
--> tests/ui/needless_return.rs:334:9
|
LL | return Err(format!("err!"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -630,7 +630,7 @@ LL + Err(format!("err!"))
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:311:9
--> tests/ui/needless_return.rs:340:9
|
LL | return if true { 1 } else { 2 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -642,7 +642,7 @@ LL + if true { 1 } else { 2 }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:315:9
--> tests/ui/needless_return.rs:344:9
|
LL | return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -654,7 +654,7 @@ LL + (if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:336:5
--> tests/ui/needless_return.rs:365:5
|
LL | return { "a".to_string() } + "b" + { "c" };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down

0 comments on commit a4bdab3

Please sign in to comment.