Unused function warnings should return a span of only the function name #58729
Comments
jonas-schievink
added
the
A-lint
|
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. |
|
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? |
|
It would help if you could provide a test case that reproduces the bug, preferably in the following format: |
estebank
added
A-diagnostics
|
Some further details in duplicate #63064 |
|
Still reproduces. |
estebank
added
D-papercut
|
@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 |
|
@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. |
|
@Quantumplation go for it, in separate commits in the same PR. |
|
@Quantumplation, I didn't really lose interest. While investigating a fix I got blocked by #63091. I think you'll hit it too, though maybe you'll come up with a good idea around it. |
|
@jakubadamw following @estebank's suggestion in the other issue about using |
|
@estebank Do you think I should use the ident span for other constructs as well (struct, enum, etc?) Or keep it to just functions? |
|
@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 |
|
@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 |
Use ident.span instead of def_span in dead-code pass Hello! First time contributor! :) This should fix rust-lang#58729. According to @estebank in the duplicate rust-lang#63064, def_span scans forward on the line until it finds a {, and if it can't find one, falls back to the span for the whole item. This was apparently written before the identifier span was explicitly tracked on each node. This means that if an unused function signature spans multiple lines, the entire function (potentially hundreds of lines) gets flagged as dead code. This could, for example, cause IDEs to add error squiggly's to the whole function. By using the span from the ident instead, we narrow the scope of this in most cases. In a wider sense, it's probably safe to use ident.span instead of def_span in most locations throughout the whole code base, but since this is my first contribution, I kept it small. Some interesting points that came up while I was working on this: - I reorganized the tests a bit to bring some of the dead code ones all into the same location - A few tests were for things unrelated to dead code (like the path-lookahead for parens), so I added #![allow(dead_code)] and cleaned up the stderr file to reduce noise in the future - The same fix doesn't apply to const and static declarations. I tried adding these cases to the match expression, but that created a much wider change to tests and error messages, so I left it off until I could get some code review to validate the approach.
Use ident.span instead of def_span in dead-code pass Hello! First time contributor! :) This should fix rust-lang#58729. According to @estebank in the duplicate rust-lang#63064, def_span scans forward on the line until it finds a {, and if it can't find one, falls back to the span for the whole item. This was apparently written before the identifier span was explicitly tracked on each node. This means that if an unused function signature spans multiple lines, the entire function (potentially hundreds of lines) gets flagged as dead code. This could, for example, cause IDEs to add error squiggly's to the whole function. By using the span from the ident instead, we narrow the scope of this in most cases. In a wider sense, it's probably safe to use ident.span instead of def_span in most locations throughout the whole code base, but since this is my first contribution, I kept it small. Some interesting points that came up while I was working on this: - I reorganized the tests a bit to bring some of the dead code ones all into the same location - A few tests were for things unrelated to dead code (like the path-lookahead for parens), so I added #![allow(dead_code)] and cleaned up the stderr file to reduce noise in the future - The same fix doesn't apply to const and static declarations. I tried adding these cases to the match expression, but that created a much wider change to tests and error messages, so I left it off until I could get some code review to validate the approach.
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.
The text was updated successfully, but these errors were encountered: