Skip to content

Make lint span smaller for needless return #14790

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 20, 2025
Merged

Make lint span smaller for needless return #14790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73a19a0
Make lint span smaller for needless return
barun511 May 12, 2025
e0fd814
Fix tests
barun511 May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions clippy_lints/src/returns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,14 @@ fn check_final_expr<'tcx>(
_ => return,
}

emit_return_lint(cx, ret_span, semi_spans, &replacement, expr.hir_id);
emit_return_lint(
cx,
peeled_drop_expr.span,
ret_span,
semi_spans,
&replacement,
expr.hir_id,
);
},
ExprKind::If(_, then, else_clause_opt) => {
check_block_return(cx, &then.kind, peeled_drop_expr.span, semi_spans.clone());
Expand All @@ -448,6 +455,7 @@ fn check_final_expr<'tcx>(

fn emit_return_lint(
cx: &LateContext<'_>,
lint_span: Span,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super confident that this is the way we want to go - but it seemed like the lint_span and the suggestion_span had to diverge here. Happy to take suggestions!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the suggestion span would be created inside this function, but that doesn't look like it will be trivial.

ret_span: Span,
semi_spans: Vec<Span>,
replacement: &RetReplacement<'_>,
Expand All @@ -457,7 +465,7 @@ fn emit_return_lint(
cx,
NEEDLESS_RETURN,
at,
ret_span,
lint_span,
"unneeded `return` statement",
|diag| {
let suggestions = std::iter::once((ret_span, replacement.to_string()))
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/crashes/ice-12491.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@
fn main() {
if (true) {
// anything一些中文
//~^^ needless_return
//~^ needless_return
}
}
2 changes: 1 addition & 1 deletion tests/ui/crashes/ice-12491.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ fn main() {
if (true) {
// anything一些中文
return;
//~^^ needless_return
//~^ needless_return
}
}
8 changes: 3 additions & 5 deletions tests/ui/crashes/ice-12491.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
error: unneeded `return` statement
--> tests/ui/crashes/ice-12491.rs:5:24
--> tests/ui/crashes/ice-12491.rs:6:9
|
LL | // anything一些中文
| ____________________________^
LL | | return;
| |______________^
LL | return;
| ^^^^^^
|
= note: `-D clippy::needless-return` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::needless_return)]`
Expand Down
18 changes: 9 additions & 9 deletions tests/ui/needless_return.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,14 @@ fn test_macro_call() -> i32 {
}

fn test_void_fun() {
//~^^ needless_return
//~^ needless_return
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to change these because the lint span now starts on the immediate previous line, not the one before

}

fn test_void_if_fun(b: bool) {
if b {
//~^^ needless_return
//~^ needless_return
} else {
//~^^ needless_return
//~^ needless_return
}
}

Expand All @@ -108,7 +108,7 @@ fn test_nested_match(x: u32) {
0 => (),
1 => {
let _ = 42;
//~^^ needless_return
//~^ needless_return
},
_ => (),
//~^ needless_return
Expand Down Expand Up @@ -156,7 +156,7 @@ mod issue6501 {

fn test_closure() {
let _ = || {
//~^^ needless_return
//~^ needless_return
};
let _ = || {};
//~^ needless_return
Expand Down Expand Up @@ -220,14 +220,14 @@ async fn async_test_macro_call() -> i32 {
}

async fn async_test_void_fun() {
//~^^ needless_return
//~^ needless_return
}

async fn async_test_void_if_fun(b: bool) {
if b {
//~^^ needless_return
//~^ needless_return
} else {
//~^^ needless_return
//~^ needless_return
}
}

Expand Down Expand Up @@ -354,7 +354,7 @@ fn issue9503(x: usize) -> isize {
mod issue9416 {
pub fn with_newline() {
let _ = 42;
//~^^ needless_return
//~^ needless_return
}

#[rustfmt::skip]
Expand Down
18 changes: 9 additions & 9 deletions tests/ui/needless_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,16 @@ fn test_macro_call() -> i32 {

fn test_void_fun() {
return;
//~^^ needless_return
//~^ needless_return
}

fn test_void_if_fun(b: bool) {
if b {
return;
//~^^ needless_return
//~^ needless_return
} else {
return;
//~^^ needless_return
//~^ needless_return
}
}

Expand All @@ -112,7 +112,7 @@ fn test_nested_match(x: u32) {
1 => {
let _ = 42;
return;
//~^^ needless_return
//~^ needless_return
},
_ => return,
//~^ needless_return
Expand Down Expand Up @@ -161,7 +161,7 @@ mod issue6501 {
fn test_closure() {
let _ = || {
return;
//~^^ needless_return
//~^ needless_return
};
let _ = || return;
//~^ needless_return
Expand Down Expand Up @@ -226,16 +226,16 @@ async fn async_test_macro_call() -> i32 {

async fn async_test_void_fun() {
return;
//~^^ needless_return
//~^ needless_return
}

async fn async_test_void_if_fun(b: bool) {
if b {
return;
//~^^ needless_return
//~^ needless_return
} else {
return;
//~^^ needless_return
//~^ needless_return
}
}

Expand Down Expand Up @@ -363,7 +363,7 @@ mod issue9416 {
pub fn with_newline() {
let _ = 42;
return;
//~^^ needless_return
//~^ needless_return
}

#[rustfmt::skip]
Expand Down
76 changes: 29 additions & 47 deletions tests/ui/needless_return.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,10 @@ LL + the_answer!()
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:86:21
--> tests/ui/needless_return.rs:87:5
|
LL | fn test_void_fun() {
| _____________________^
LL | | return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely convinced that we shouldn't extend this span upto at least the full line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extend the span to the ;. Not sure why it wasn't included in the previous span though. There might've been a reason.

Might need some logic like "if parent statement is a StmtKind::Semi, extend the span to the statement" 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semicolon spans are handled separately in the suggestion since there may be multiple of them.

| |__________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand All @@ -148,12 +146,10 @@ LL + fn test_void_fun() {
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:92:11
--> tests/ui/needless_return.rs:93:9
|
LL | if b {
| ___________^
LL | | return;
| |______________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand All @@ -163,12 +159,10 @@ LL + if b {
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:95:13
--> tests/ui/needless_return.rs:96:9
|
LL | } else {
| _____________^
LL | | return;
| |______________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand All @@ -190,12 +184,10 @@ LL + _ => (),
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:113:24
--> tests/ui/needless_return.rs:114:13
|
LL | let _ = 42;
| ________________________^
LL | | return;
| |__________________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand Down Expand Up @@ -253,12 +245,10 @@ LL + bar.unwrap_or_else(|_| {})
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:162:21
--> tests/ui/needless_return.rs:163:13
|
LL | let _ = || {
| _____________________^
LL | | return;
| |__________________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand Down Expand Up @@ -400,12 +390,10 @@ LL + the_answer!()
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:227:33
--> tests/ui/needless_return.rs:228:5
|
LL | async fn async_test_void_fun() {
| _________________________________^
LL | | return;
| |__________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand All @@ -415,12 +403,10 @@ LL + async fn async_test_void_fun() {
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:233:11
--> tests/ui/needless_return.rs:234:9
|
LL | if b {
| ___________^
LL | | return;
| |______________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand All @@ -430,12 +416,10 @@ LL + if b {
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:236:13
--> tests/ui/needless_return.rs:237:9
|
LL | } else {
| _____________^
LL | | return;
| |______________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand Down Expand Up @@ -593,12 +577,10 @@ LL ~ }
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:364:20
--> tests/ui/needless_return.rs:365:9
|
LL | let _ = 42;
| ____________________^
LL | | return;
| |______________^
LL | return;
| ^^^^^^
|
help: remove `return`
|
Expand All @@ -608,10 +590,10 @@ LL + let _ = 42;
|

error: unneeded `return` statement
--> tests/ui/needless_return.rs:371:20
--> tests/ui/needless_return.rs:371:21
|
LL | let _ = 42; return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example in this instance I think the previous span was slightly better. But as I don't have high conviction, leaving as is for now and we can address

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely think the new span is better. Linting on a return but pointing to whitespace is just weird and doesn't match how basically every other lint/error uses spans. It's also just extra work to extend the span.

| ^^^^^^^
| ^^^^^^
|
help: remove `return`
|
Expand Down