Use ident.span instead of def_span in dead-code pass #65830
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hmm, more failures... I'm not sure if these tests ran when I ran tests locally... Fixing now! |
|
I got a seemingly unrelated test failure when running locally. given the error message, it sounds like it might just be a test that's not intended to run on WSL? I'm not sure... This gets thrown here: https://github.com/rust-lang/rust/blob/master/src/libstd/net/tcp.rs#L1257 I'm going to push my changes and hope the build passes when running on the linux build server 😅 |
There was a problem hiding this comment.
This looks good to me. If you don't mind, I'd prefer the last three commits (84ecd026184e4979beb43d1aaf1a4e83c44bacc1, 0e00a56746d868c81bad604bca107364ef775b12 and 9b5ca693fdf731beaf88332b813f90bf3f0031ca) be squashed into 1277650b5fed98c19ee56c2bcf68fdc9bacaf7b5.
|
Sure thing! |
Quantumplation
force-pushed
the
master
branch
from
84ecd02
to
94890d2
Compare
|
Thanks @Quantumplation! @bors r+ |
|
📌 Commit 94890d2 has been approved by |
bors
added
S-waiting-on-bors
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.
|
At @estebank's request, rebasing this onto master and re-blessing the tests |
This test was actually about the unreachable_code flag, not dead_code, so I renamed it for clarity (to prepare for the next commit, where I plan to move a bunch of the dead_code tests to a single folder)
This helps organize the tests better. I also renamed several of the tests to remove redundant dead-code in the path, and better match what they're testing
According to @estebank, def_span scans forward on the line until it finds a {, and if it can't find one, fallse 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.
|
I wasn't able to run tests to completion (same bind_address issue I mentioned above), but it got pretty far and no tests were updated. I'm not sure whether to push the rebase anyway. |
Quantumplation
force-pushed
the
master
branch
from
94890d2
to
7985510
Compare
|
@bors r+ |
|
📌 Commit 7985510 has been approved by |
|
Possibly caused failure in #65854 (comment), @bors rollup=never |
|
@Centril what is your recommended path to resolution on this? |
|
@Quantumplation Check if something in your PR could have caused this, otherwise CI will tell us eventually. |
@Quantumplation Rust's CI merges with master before it runs any tests. If a new test was added in master that your patch would have affected, then it won't have an updated stderr (because your patch doesn't know about it). When @Centril attempted a rollup (combining lots of patches to merge all at once) yesterday, the failure looked like it might have been because of what I've just described happening with your patch. That's probably why @estebank requested you rebase and re-bless. Since you did that, so long as more new tests haven't been added that would fail in the same way, your patch should be good to merge, which is why I re-approved it. But given that your patch seems susceptible to this sort of failure (as evidenced by it having happened), @Centril marked it as tl;dr you probably don't need to do anything right now, by rebasing and re-blessing yesterday you've probably fixed the problem. |
|
@davidtwco Ah, right, that makes sense. For some reason I thought this failed even after I rebased, but it didn't make it's way into a roll up, so everything makes sense again heh. |
|
⌛ Testing commit 7985510 with merge 8ca4a7f8acf67dabd6d9cf9e60f8b282a297da99... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
After some digging, I think the test failure and the error message regression reported by @bjorn3 are caused by the same thing, i.e. a change in the macro expansion. Specifically, see this line: https://github.com/rust-lang/rust/blob/master/src/librustc/lint/mod.rs#L681 Here, it uses the reported span alone to detect if the error is in an external macro. Previously, (I guess, I still don't understand well enough to say definitively),
One thing that's unclear to me is why Until someone can weigh in on how to address this more thoroughly, I'll just revert to the old def_span call if https://github.com/rust-lang/rust/blob/master/src/librustc/lint/mod.rs#L898 |
I believe the later |
If item.span is part of a macro invocation, this has several downstream implications. To name two that were found while working on this: - The dead-code error gets annotated with a "in this macro invocation" - Some errors get canceled if they refer to remote crates Ideally, we should annotate item.ident.span with the same macro info, but this is a larger change (see: rust-lang#66095), so for now we just fall back to the old behavior if this item was generated by a macro. I use span.macro_backtrace().len() to detect if it's part of a macro, because that (among other things) is what is used by the code which adds the "in this macro invocation" annotations mentioned above.
Quantumplation
force-pushed
the
master
branch
from
c6cd395
to
15ccad3
Compare
|
Sorry, had a bit of a mixup with my commits, forgot to configure user.name and user.email on a new setup so I had to force push :) |
|
The hacky solution is ok by me. |
macro_backtrace() allocates a vector, whereas source_callee() doesn't but indicates the same thing. Suggested by @estebank
|
@bors r=davidtwco,estebank |
|
📌 Commit 6186ede has been approved by |
bors
added
S-waiting-on-bors
Use ident.span instead of def_span in dead-code pass Hello! First time contributor! :) This should fix #58729. According to @estebank in the duplicate #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.
|
☀️ Test successful - checks-azure |
Hello! First time contributor! :)
This should fix #58729.
According to @estebank in the duplicate #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:
into the same location
path-lookahead for parens), so I added #![allow(dead_code)] and
cleaned up the stderr file to reduce noise in the future
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.