-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
base: master
Are you sure you want to change the base?
Conversation
ffddadf
to
73a19a0
Compare
@@ -448,6 +455,7 @@ fn check_final_expr<'tcx>( | |||
|
|||
fn emit_return_lint( | |||
cx: &LateContext<'_>, | |||
lint_span: Span, |
There was a problem hiding this comment.
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!
@@ -84,14 +84,14 @@ fn test_macro_call() -> i32 { | |||
} | |||
|
|||
fn test_void_fun() { | |||
//~^^ needless_return | |||
//~^ needless_return |
There was a problem hiding this comment.
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
| | ||
LL | fn test_void_fun() { | ||
| _____________________^ | ||
LL | | return; |
There was a problem hiding this comment.
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?
@@ -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; |
There was a problem hiding this comment.
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
Still have some tests to fix - but I'll get to those once there's alignment on approach |
Fixes #14750 by reducing the lint span for needless_return.
changelog: [
needless_return
]: Lint span no longer wraps to previous line