-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
introing one-time diagnostics: only emit "lint level defined here" once #37191
introing one-time diagnostics: only emit "lint level defined here" once #37191
Conversation
We introduce a new `one_time_diagnostics` field on `rustc::session::Session` to hold a hashset of diagnostic messages we've set once but don't want to see again (as uniquified by span and message text), "lint level defined here" being the motivating example dealt with here. This is in the matter of rust-lang#24690.
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.
The changes look good -- my main question is whether we want one message per span or one per lint-kind.
let span_message = (span, message.to_owned()); | ||
let already_noted: bool = self.one_time_diagnostics.borrow() | ||
.contains(&span_message); | ||
if !already_noted { |
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.
Nit: you can write
let fresh = self.one_time_diagnostics.borrow_mut().insert(&span_message);
if fresh {
diag_builder.span_note(span, &message);
}
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.
@@ -452,8 +452,7 @@ pub fn raw_struct_lint<'a>(sess: &'a Session, | |||
} | |||
|
|||
if let Some(span) = def { | |||
let explanation = "lint level defined here"; | |||
err.span_note(span, &explanation); | |||
sess.diag_span_note_once(&mut err, span, "lint level defined 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.
Hmm. So the key is the span+message, but it's possible that the level for many lints is set at one point (e.g., #[allow(warnings)]
). Do we want to issue one note per kind of lint, or just one per span? I'm actually not sure =)
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.
I'm leaning towards one-per-lint (ef6a072).
@zackmdavis another point, raised by @jonathandturner, is that if we are emitting in json mode, we probably want to include the duplicates |
Thanks to Niko Matsakis's review for the suggestion.
Jonathan D. Turner pointed out that we don't want to dedup in JSON mode. Since the compile-test runner uses JSON output, we regrettably need to revert the edits to existing tests; one imagines that testing for one-time diagnosticity for humans will have to be added as a UI test. This remains in the matter of rust-lang#24690.
Some lint-level attributes (like `bad-style`, or, more dramatically, `warnings`) can affect more than one lint; it seems fairer to point out the attribute once for each distinct lint affected. Also, a UI test is added. This remains in the matter of rust-lang#24690.
@nikomatsakis Not-deduping in JSON mode is implemented in 8d1da84; unfortunately, this means the change in behavior can no longer be detected by the compile-fail tests, but ef6a072 adds a UI test to compensate. |
@zackmdavis looks great! thanks =) |
@bors r+ |
📌 Commit ef6a072 has been approved by |
⌛ Testing commit ef6a072 with merge 349394e... |
@bors: retry force clean
|
… r=nikomatsakis introing one-time diagnostics: only emit "lint level defined here" once This is a revised resubmission of PR #34084 (which was closed due to inactivity on account of time constraints on the author's part). --- We introduce a new `one_time_diagnostics` field on `rustc::session::Session` to hold a hashset of diagnostic messages we've set once but don't want to see again (as uniquified by span and message text), "lint level defined here" being the motivating example dealt with here. This is in the matter of #24690. --- r? @nikomatsakis
This is a revised resubmission of PR #34084 (which was closed due to inactivity on account of time constraints on the author's part).
We introduce a new
one_time_diagnostics
field onrustc::session::Session
to hold a hashset of diagnostic messages we'veset once but don't want to see again (as uniquified by span and message
text), "lint level defined here" being the motivating example dealt with
here.
This is in the matter of #24690.
r? @nikomatsakis