Make lint span smaller for needless return #14790
Conversation
rustbot
added
the
S-waiting-on-review
barun511
force-pushed
the
parruck/fix-lint-span
branch
from
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.
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!
There was a problem hiding this comment.
Ideally the suggestion span would be created inside this function, but that doesn't look like it will be trivial.
| @@ -84,14 +84,14 @@ fn test_macro_call() -> i32 { | |||
| } | |||
|
|
|||
| fn test_void_fun() { | |||
| //~^^ needless_return | |||
| //~^ needless_return | |||
There was a problem hiding this comment.
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.
I'm not entirely convinced that we shouldn't extend this span upto at least the full line?
There was a problem hiding this comment.
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" 🤔
There was a problem hiding this comment.
The semicolon spans are handled separately in the suggestion since there may be multiple of them.
| @@ -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.
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
There was a problem hiding this comment.
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.
|
Still have some tests to fix - but I'll get to those once there's alignment on approach |
|
Thank you! I won't be able to get to it until Tuesday, probably, but will fix the tests then |
Fixes #14750 by reducing the lint span for needless_return.
changelog: [
needless_return]: Lint span no longer wraps to previous line