also point to value in "value assigned is never read" lint #49197
Conversation
In this commit, liveness-dead and liveness-unused are made into UI tests (cf. rust-lang#44844) rather than compile-fail tests, because then if some future commit were to, say, change some of the spans in these lints, that commit's diff would clearly show exactly what changed. If. Hypothetically.
Thanks to Yemi Bedu for suggesting that we put a span on both sides of the assignment. (Some might argue that we should just put the single span on the right-hand side, but as the change to liveness-dead.rs highlights, it's also nice to have a span on the left when the left-hand side has multiple patterns being assigned.) Resolves rust-lang#49171.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -52,7 +52,7 @@ warning: value assigned to `x` is never read | |||
| --> $DIR/liveness-unused.rs:42:5 | |||
| | | |||
| LL | x += 4; | |||
| | ^ | |||
| | ^ ^ | |||
There was a problem hiding this comment.
This seems a bit surprising to me. I feel like highlighting multiple things makes sense when they are a homogeneous list, but here they are two distinct things, so I would expect labels. I can't however come up with obviously good labels. Probably label 4 with something like "value" and x with ... I don't know. There is also a risk here when the expression on the RHS spans multiple lines, in which case the underline looks pretty bad usually:
let x = foo(
1,
2,
);
cc @estebank -- thoughts?
There was a problem hiding this comment.
Hello,
An expression or statement boundary check to see if it spans more than one line would be needed for multiple line layouts. In this isolated case, we are looking for the semi-colon and if the backtrack has lines greater than one, use another warning marker layout.
An argument could be made to run that snippet through a format code cycle just to provide more streamlined feedback. Your example would collapse to let x = foo(1, 2); and it would lead to less visual edge cases for providing warnings. Not sure about how to carve it out or the nature of retaining original line numbering. The flattening could just be for display purposes maybe.
For labeling the pieces, you may just want to use a different character to point out the LHS from the RHS. so <^^ on LHS and >^^ on RHS to distinguish binding from value. No sure if all this is better in the original ticket, I will transplant if so. Thank you. Good day.
| --> $DIR/liveness-dead.rs:21:10 | ||
| | | ||
| LL | let (mut x, y) = (3, 4); //~ WARN: value assigned to `x` is never read | ||
| | ^^^^^ ^^^^^^ |
There was a problem hiding this comment.
-
The second span isn't correct, as it should be pointing only to
3, not the entire tuple -
It doesn't seem to be as useful in this case, I feel that pointing to the reassignment of
xin line 22 would be better instead:
warning: value assigned to `x` is never read
--> $DIR/liveness-dead.rs:21:10
|
LL | let (mut x, y) = (3, 4);
| ^^^^^ - this value is never read...
LL | x = 4;
| - ...because its binding is reassigned here before being used
- I would prefer if the second span where a secondary span (blue), instead of both spans being primary spans (red).
There was a problem hiding this comment.
It's going to be fairly hard (and not always possible) to highlight the value in a case like that. Consider let (x, y) = pair; But I do like the second label! Right now I don't think we have that information though. (Grr, we should rewrite liveness in terms of MIR)
There was a problem hiding this comment.
The language value from here is never read rather than this value... may make up for our inability to get a variable-specific span on the right hand side. (Ordinarily I'd prefer to avoid "here" language for the same reason it should be avoided in hyperlinks, but "value from assignment" feels less clear?)
|
Ping from triage, @zackmdavis ! It's been over a week since we've heard from you; will you have time to address the review comments in the near future? |
|
@shepmaster it might be a couple weeks; #49258 and some AtheMathmo/rusty-machine maintainership are ahead in my open-source work queue |
|
@zackmdavis I'm not sure yet what we think these messages should look like. To me, just underlining the value alone doesn't feel like a net win, though I imagine adding labels or other info might be helpful. |
|
Ping from triage @zackmdavis! Are you planning to work on this in the near future? |
|
yes |
|
Weekly ping from triage @zackmdavis! This PR still need some action from you. |
|
OK, I get it |
|
(intending to revise and resubmit later but can't promise a date because I'm pretty busy; obviously I have no objections if someone has their own PR to address #49171) |
|
By the way, if you'll have time in the future to work on this again, please reopen this PR instead of creating a new one. Thanks |
The first commit ports some compile-fail tests to UI so that the output diff is easy to read in the second commit 60e2af0.
Thanks to @pr-yemibedu for suggesting that we put a span on both sides of
the assignment. (Some might argue that we should just put the single
span on the right-hand side, but as the change to liveness-dead.rs
highlights, it's also nice to have a span on the left when the left-hand
side has multiple patterns being assigned.)
Resolves #49171.