Rust: Unused function warnings should return a span of only the function name

[jackmott]

Currently a warning for an unused function returns a span of the entire function. As a consequence, RLS based IDE environments may then put green squiggles on the entire function. This makes it very annoying to work on a function, if you have not yet written the function that uses it.

There may be other unused warnings for structs/traits that could be similarly adjusted.

This issue has been assigned to @Quantumplation via this comment.

[sanxiyn]

Does it? For me warnings for unused functions only return span of function signatures, not bodies. Is that still annoying? Same for structs.

I think this is somewhat intentional as it is the item that is unused. Spanning the name usually means name resolution failure.

[jackmott]

VSCode ends up highlighting the whole function, so if you are correct this bug should be closed and moved to that repo. How can we verify?

[sanxiyn]

It would help if you could provide a test case that reproduces the bug, preferably in the following format:

$ cat test.rs
fn f() { /* not in span */ }
fn main() {}
$ rustc test.rs
warning: function is never used: `f`
 --> test.rs:1:1
  |
1 | fn f() { /* not in span */ }
  | ^^^^^^
  |
  = note: #[warn(dead_code)] on by default

[estebank]

Some further details in duplicate https://github.com/rust-lang/rust/issues/63064

[estebank]

Still reproduces.

[Quantumplation]

@rustbot claim

I'd like to pick this up as my first contribution. @jakubadamw had said (on the duplicate) that he intended to work on this, but it's been a few months, so I'm not sure if he's lost interest or not.

I started to work on this, and I figured first step would be to write a ui test for it. I notice that by now there's a few dead-code related tests. Is there a policy around moving tests to sub folders? If no one objects, I'd like to move all the dead-code related tests to a dead-code subfolder as part of this work as well.

[jackmott]

@Quantumplation I can't speak for the rust maintainers but generally it is greatly preferred to just have one thing per PR. I would do the test organization as a separate pr.

[estebank]

@Quantumplation go for it, in separate commits in the same PR.

[jakubadamw]

@Quantumplation, I didn't really lose interest. While investigating a fix I got blocked by https://github.com/rust-lang/rust/issues/63091. I think you'll hit it too, though maybe you'll come up with a good idea around it.

[Quantumplation]

@jakubadamw following @estebank's suggestion in the other issue about using ident seems to have circumvented the broken span thing. I'll get a PR up soon in case I'm missing something :) Just gotta wait for these 9207 tests to run... 😛

[Quantumplation]

@estebank Do you think I should use the ident span for other constructs as well (struct, enum, etc?) Or keep it to just functions?

[estebank]

@Quantumplation we should prefer the ident span whenever possible, but be mindful of the output, as there are some cases where pointing at just the ident might be misleading (like when talking about lifetimes or wanting to point at the whole enclosing span). I think almost all cases where we use def_span(span) can be replaced with pointing at the ident's span.

[Quantumplation]

@estebank I'm only going to focus on the dead-code case, and if you think it's worth it, i'll follow up with a more extensive PR where I evaluate other def_span cases for replacement. Want to keep my first contribution small :)