-
Notifications
You must be signed in to change notification settings - Fork 1.8k
refactor(missing_asserts_for_indexing): overhaul diagnostics #16120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This is a general advice, and so shouldn't be repeated
This allows printing the labels in the same diagnostic. Note: if we were to _only_ replaces notes by labels, but still pointed the diagnostic at `full_span`, the resulting diagnostic would end up rather ugly: ``` error: indexing into a slice multiple times with an `assert` that does not cover the highest index --> tests/ui/missing_asserts_for_indexing.rs:30:5 | LL | v[0] + v[1] + v[2] + v[3] + v[4] | ----^^^----^^^----^^^----^^^---- | | | | | | | | | | | slice indexed here | | | | slice indexed here | | | slice indexed here | | slice indexed here | slice indexed here | ```
|
Lintcheck changes for bc76aef
This comment will be updated if you push new changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the labels need to be removed. Looks good otherwise.
| diag.span_note(*span, "slice indexed here"); | ||
| } | ||
| diag.note("asserting the length before indexing will elide bounds checks"); | ||
| diag.span_labels(index_spans, "slice indexed here"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This label adds more noise than anything.
| | ^^^^ ^^^^ ^^^^ ^^^^ ^^^^ slice indexed here | ||
| | | | | | | ||
| | | | | slice indexed here | ||
| | | | slice indexed here | ||
| | | slice indexed here | ||
| | slice indexed here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the previous. This is several lines cluttering the diagnostic while not adding new information.
|
Reminder, once the PR becomes ready for a review, use |
changelog: [
missing_asserts_for_indexing]: overhaul diagnostics