Point at signature on unused lint #44847
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
@bors r+ |
|
📌 Commit bc7554a has been approved by |
estebank
force-pushed
the
unused-signature
branch
from
bc7554a to
9c3fa4d
Compare
|
Fix missed test. @bors r=nikomatsakis |
|
📌 Commit 9c3fa4d has been approved by |
estebank
added
the
S-waiting-on-bors
| 35 | | -> usize | ||
| 36 | | { | ||
| 37 | | 3 | ||
| 38 | | } |
There was a problem hiding this comment.
Why is this span including the body? Just because it's multiline?
There was a problem hiding this comment.
@oli-obk it was originally proposed that given that in cases where a multiline span was already being shown, we might as well point at the entire body. This decision can be revised if needed.
There was a problem hiding this comment.
For RLS this is a major annoyance.
There was a problem hiding this comment.
@oli-obk For RLS indeed it'd be very handy to only point at the symbol name, but I think from the CLI ergonomics point of view, displaying multiline diagnostics might still be superior? (gives more context, CLIs are often run out-of-the-editor etc.)
There was a problem hiding this comment.
It would still be multiline, it just wouldn't show the function body. I don't see much use in showing function bodies at all for such diagnostics
There was a problem hiding this comment.
If that would work, I guess we could. However, aren't there other def_span users who still rely on the full multiline when a signature is also spanning across multiple lines? I'm not very well versed in rustc itself, sorry.
There was a problem hiding this comment.
@Xanewok there're are only two users of def_span at the moment, including this one, and neither rely on it showing the full span when multiline.
There was a problem hiding this comment.
@estebank ah, my bad. Must've looked up wrong def_span, sorry. The one from the codemap seems to only be used for lints (if I'm not mistaken?) so I think it'd be great to go ahead and limit the multiline spans only to signatures, without bodies, in lints.
There was a problem hiding this comment.
@estebank @oli-obk with the change now also ui\span\multiline-span-E0072.rs fails as follows:
after change:
error[E0072]: recursive type `ListNode` has infinite size
--> $DIR/multiline-span-E0072.rs:12:1
|
12 | / struct
13 | | ListNode
| |________^ recursive type has infinite size
...
16 | tail: Option<ListNode>,
| ---------------------- recursive without indirection
|
= help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `ListNode` representable
expected:
error[E0072]: recursive type `ListNode` has infinite size
--> $DIR/multiline-span-E0072.rs:12:1
|
12 | / struct
13 | | ListNode
14 | | {
15 | | head: u8,
16 | | tail: Option<ListNode>,
| | ---------------------- recursive without indirection
17 | | }
| |_^ recursive type has infinite size
|
= help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `ListNode` representable
Is this desired? On one hand it's handy to highlight the faulty (recursive here) bit within the whole span, on the other highlighting entire definition may be an overkill, especially for RLS (iirc right now RLS can't even highlight primary/secondary spans properly per a single diagnostic, only the primary one).
cc @GuillaumeGomez
There was a problem hiding this comment.
@Xanewok I also prefer the current output, but understand that we're bound by the limitations of VSCode/RLS here.
@nikomatsakis would you have a comment? I'm inclined to move ahead with the change.
Point at signature on unused lint
```
warning: struct is never used: `Struct`
--> $DIR/unused-warning-point-at-signature.rs:22:1
|
22 | struct Struct {
| ^^^^^^^^^^^^^
```
Fix #33961.
|
☀️ Test successful - status-appveyor, status-travis |
Fix #33961.