Skip to content

Commit 10913c0

Browse files
committed
Auto merge of #87835 - xFrednet:rfc-2383-expect-attribute-with-ids, r=wesleywiser
Implementation of the `expect` attribute (RFC 2383) This is an implementation of the `expect` attribute as described in [RFC-2383](https://rust-lang.github.io/rfcs/2383-lint-reasons.html). The attribute allows the suppression of lint message by expecting them. Unfulfilled lint expectations (meaning no expected lint was caught) will emit the `unfulfilled_lint_expectations` lint at the `expect` attribute. ### Example #### input ```rs // required feature flag #![feature(lint_reasons)] #[expect(unused_mut)] // Will warn about an unfulfilled expectation #[expect(unused_variables)] // Will be fulfilled by x fn main() { let x = 0; } ``` #### output ```txt warning: this lint expectation is unfulfilled --> $DIR/trigger_lint.rs:3:1 | LL | #[expect(unused_mut)] // Will warn about an unfulfilled expectation | ^^^^^^^^^^ | = note: `#[warn(unfulfilled_lint_expectations)]` on by default ``` ### Implementation This implementation introduces `Expect` as a new lint level for diagnostics, which have been expected. All lint expectations marked via the `expect` attribute are collected in the [`LintLevelsBuilder`] and assigned an ID that is stored in the new lint level. The `LintLevelsBuilder` stores all found expectations and the data needed to emit the `unfulfilled_lint_expectations` in the [`LintLevelsMap`] which is the result of the [`lint_levels()`] query. The [`rustc_errors::HandlerInner`] is the central error handler in rustc and handles the emission of all diagnostics. Lint message with the level `Expect` are suppressed during this emission, while the expectation ID is stored in a set which marks them as fulfilled. The last step is then so simply check if all expectations collected by the [`LintLevelsBuilder`] in the [`LintLevelsMap`] have been marked as fulfilled in the [`rustc_errors::HandlerInner`]. Otherwise, a new lint message will be emitted. The implementation of the `LintExpectationId` required some special handling to make it stable between sessions. Lints can be emitted during [`EarlyLintPass`]es. At this stage, it's not possible to create a stable identifier. The level instead stores an unstable identifier, which is later converted to a stable `LintExpectationId`. ### Followup TO-DOs All open TO-DOs have been marked with `FIXME` comments in the code. This is the combined list of them: * [ ] The current implementation doesn't cover cases where the `unfulfilled_lint_expectations` lint is actually expected by another `expect` attribute. * This should be easily possible, but I wanted to get some feedback before putting more work into this. * This could also be done in a new PR to not add to much more code to this one * [ ] Update unstable documentation to reflect this change. * [ ] Update unstable expectation ids in [`HandlerInner::stashed_diagnostics`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.HandlerInner.html#structfield.stashed_diagnostics) ### Open questions I also have a few open questions where I would like to get feedback on: 1. The RFC discussion included a suggestion to change the `expect` attribute to something else. (Initiated by `@Ixrec` [here](rust-lang/rfcs#2383 (comment)), suggestion from `@scottmcm` to use `#[should_lint(...)]` [here](rust-lang/rfcs#2383 (comment))). No real conclusion was drawn on that point from my understanding. Is this still open for discussion, or was this discarded with the merge of the RFC? 2. How should the expect attribute deal with the new `force-warn` lint level? --- This approach was inspired by a discussion with `@LeSeulArtichaut.` RFC tracking issue: #54503 Mentoring/Implementation issue: #85549 [`LintLevelsBuilder`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/levels/struct.LintLevelsBuilder.html [`LintLevelsMap`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/lint/struct.LintLevelMap.html [`lint_levels()`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.lint_levels [`rustc_errors::HandlerInner`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.HandlerInner.html [`EarlyLintPass`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html
2 parents 32cbc76 + 5275d02 commit 10913c0

37 files changed

+1040
-26
lines changed

Cargo.lock

+1
Original file line numberDiff line numberDiff line change
@@ -3882,6 +3882,7 @@ version = "0.0.0"
38823882
dependencies = [
38833883
"rustc_ast",
38843884
"rustc_data_structures",
3885+
"rustc_hir",
38853886
"rustc_macros",
38863887
"rustc_serialize",
38873888
"rustc_span",

compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs

+1
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ fn annotation_type_for_level(level: Level) -> AnnotationType {
7575
// FIXME(#59346): Not sure how to map this level
7676
Level::FailureNote => AnnotationType::Error,
7777
Level::Allow => panic!("Should not call with Allow"),
78+
Level::Expect(_) => panic!("Should not call with Expect"),
7879
}
7980
}
8081

compiler/rustc_errors/src/diagnostic.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ impl Diagnostic {
133133
| Level::Error { .. }
134134
| Level::FailureNote => true,
135135

136-
Level::Warning | Level::Note | Level::Help | Level::Allow => false,
136+
Level::Warning | Level::Note | Level::Help | Level::Allow | Level::Expect(_) => false,
137137
}
138138
}
139139

compiler/rustc_errors/src/lib.rs

+90-3
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,12 @@ extern crate tracing;
2020

2121
pub use emitter::ColorConfig;
2222

23+
use rustc_lint_defs::LintExpectationId;
2324
use Level::*;
2425

2526
use emitter::{is_case_difference, Emitter, EmitterWriter};
2627
use registry::Registry;
27-
use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
28+
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap};
2829
use rustc_data_structures::stable_hasher::StableHasher;
2930
use rustc_data_structures::sync::{self, Lock, Lrc};
3031
use rustc_data_structures::AtomicRef;
@@ -450,6 +451,22 @@ struct HandlerInner {
450451
deduplicated_warn_count: usize,
451452

452453
future_breakage_diagnostics: Vec<Diagnostic>,
454+
455+
/// Expected [`Diagnostic`]s store a [`LintExpectationId`] as part of
456+
/// the lint level. [`LintExpectationId`]s created early during the compilation
457+
/// (before `HirId`s have been defined) are not stable and can therefore not be
458+
/// stored on disk. This buffer stores these diagnostics until the ID has been
459+
/// replaced by a stable [`LintExpectationId`]. The [`Diagnostic`]s are the
460+
/// submitted for storage and added to the list of fulfilled expectations.
461+
unstable_expect_diagnostics: Vec<Diagnostic>,
462+
463+
/// expected diagnostic will have the level `Expect` which additionally
464+
/// carries the [`LintExpectationId`] of the expectation that can be
465+
/// marked as fulfilled. This is a collection of all [`LintExpectationId`]s
466+
/// that have been marked as fulfilled this way.
467+
///
468+
/// [RFC-2383]: https://rust-lang.github.io/rfcs/2383-lint-reasons.html
469+
fulfilled_expectations: FxHashSet<LintExpectationId>,
453470
}
454471

455472
/// A key denoting where from a diagnostic was stashed.
@@ -570,6 +587,8 @@ impl Handler {
570587
emitted_diagnostics: Default::default(),
571588
stashed_diagnostics: Default::default(),
572589
future_breakage_diagnostics: Vec::new(),
590+
unstable_expect_diagnostics: Vec::new(),
591+
fulfilled_expectations: Default::default(),
573592
}),
574593
}
575594
}
@@ -677,6 +696,11 @@ impl Handler {
677696
DiagnosticBuilder::new(self, Level::Allow, msg)
678697
}
679698

699+
/// Construct a builder at the `Expect` level with the `msg`.
700+
pub fn struct_expect(&self, msg: &str, id: LintExpectationId) -> DiagnosticBuilder<'_, ()> {
701+
DiagnosticBuilder::new(self, Level::Expect(id), msg)
702+
}
703+
680704
/// Construct a builder at the `Error` level at the given `span` and with the `msg`.
681705
pub fn struct_span_err(
682706
&self,
@@ -906,6 +930,48 @@ impl Handler {
906930
pub fn emit_unused_externs(&self, lint_level: &str, unused_externs: &[&str]) {
907931
self.inner.borrow_mut().emit_unused_externs(lint_level, unused_externs)
908932
}
933+
934+
pub fn update_unstable_expectation_id(
935+
&self,
936+
unstable_to_stable: &FxHashMap<LintExpectationId, LintExpectationId>,
937+
) {
938+
let diags = std::mem::take(&mut self.inner.borrow_mut().unstable_expect_diagnostics);
939+
if diags.is_empty() {
940+
return;
941+
}
942+
943+
let mut inner = self.inner.borrow_mut();
944+
for mut diag in diags.into_iter() {
945+
let mut unstable_id = diag
946+
.level
947+
.get_expectation_id()
948+
.expect("all diagnostics inside `unstable_expect_diagnostics` must have a `LintExpectationId`");
949+
950+
// The unstable to stable map only maps the unstable `AttrId` to a stable `HirId` with an attribute index.
951+
// The lint index inside the attribute is manually transferred here.
952+
let lint_index = unstable_id.get_lint_index();
953+
unstable_id.set_lint_index(None);
954+
let mut stable_id = *unstable_to_stable
955+
.get(&unstable_id)
956+
.expect("each unstable `LintExpectationId` must have a matching stable id");
957+
958+
stable_id.set_lint_index(lint_index);
959+
diag.level = Level::Expect(stable_id);
960+
inner.fulfilled_expectations.insert(stable_id);
961+
962+
(*TRACK_DIAGNOSTICS)(&diag);
963+
}
964+
}
965+
966+
/// This methods steals all [`LintExpectationId`]s that are stored inside
967+
/// [`HandlerInner`] and indicate that the linked expectation has been fulfilled.
968+
pub fn steal_fulfilled_expectation_ids(&self) -> FxHashSet<LintExpectationId> {
969+
assert!(
970+
self.inner.borrow().unstable_expect_diagnostics.is_empty(),
971+
"`HandlerInner::unstable_expect_diagnostics` should be empty at this point",
972+
);
973+
std::mem::take(&mut self.inner.borrow_mut().fulfilled_expectations)
974+
}
909975
}
910976

911977
impl HandlerInner {
@@ -951,9 +1017,21 @@ impl HandlerInner {
9511017
return;
9521018
}
9531019

1020+
// The `LintExpectationId` can be stable or unstable depending on when it was created.
1021+
// Diagnostics created before the definition of `HirId`s are unstable and can not yet
1022+
// be stored. Instead, they are buffered until the `LintExpectationId` is replaced by
1023+
// a stable one by the `LintLevelsBuilder`.
1024+
if let Level::Expect(LintExpectationId::Unstable { .. }) = diagnostic.level {
1025+
self.unstable_expect_diagnostics.push(diagnostic.clone());
1026+
return;
1027+
}
1028+
9541029
(*TRACK_DIAGNOSTICS)(diagnostic);
9551030

956-
if diagnostic.level == Allow {
1031+
if let Level::Expect(expectation_id) = diagnostic.level {
1032+
self.fulfilled_expectations.insert(expectation_id);
1033+
return;
1034+
} else if diagnostic.level == Allow {
9571035
return;
9581036
}
9591037

@@ -1250,6 +1328,7 @@ pub enum Level {
12501328
Help,
12511329
FailureNote,
12521330
Allow,
1331+
Expect(LintExpectationId),
12531332
}
12541333

12551334
impl fmt::Display for Level {
@@ -1275,7 +1354,7 @@ impl Level {
12751354
spec.set_fg(Some(Color::Cyan)).set_intense(true);
12761355
}
12771356
FailureNote => {}
1278-
Allow => unreachable!(),
1357+
Allow | Expect(_) => unreachable!(),
12791358
}
12801359
spec
12811360
}
@@ -1289,12 +1368,20 @@ impl Level {
12891368
Help => "help",
12901369
FailureNote => "failure-note",
12911370
Allow => panic!("Shouldn't call on allowed error"),
1371+
Expect(_) => panic!("Shouldn't call on expected error"),
12921372
}
12931373
}
12941374

12951375
pub fn is_failure_note(&self) -> bool {
12961376
matches!(*self, FailureNote)
12971377
}
1378+
1379+
pub fn get_expectation_id(&self) -> Option<LintExpectationId> {
1380+
match self {
1381+
Level::Expect(id) => Some(*id),
1382+
_ => None,
1383+
}
1384+
}
12981385
}
12991386

13001387
// FIXME(eddyb) this doesn't belong here AFAICT, should be moved to callsite.

compiler/rustc_feature/src/builtin_attrs.rs

+4
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,10 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
282282
ungated!(
283283
allow, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#), DuplicatesOk
284284
),
285+
gated!(
286+
expect, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#), DuplicatesOk,
287+
lint_reasons, experimental!(expect)
288+
),
285289
ungated!(
286290
forbid, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#), DuplicatesOk
287291
),

compiler/rustc_lint/src/context.rs

+4
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ struct LintGroup {
109109
depr: Option<LintAlias>,
110110
}
111111

112+
#[derive(Debug)]
112113
pub enum CheckLintNameResult<'a> {
113114
Ok(&'a [LintId]),
114115
/// Lint doesn't exist. Potentially contains a suggestion for a correct lint name.
@@ -377,6 +378,9 @@ impl LintStore {
377378
Level::ForceWarn => "--force-warn",
378379
Level::Deny => "-D",
379380
Level::Forbid => "-F",
381+
Level::Expect(_) => {
382+
unreachable!("lints with the level of `expect` should not run this code");
383+
}
380384
},
381385
lint_name
382386
);

compiler/rustc_lint/src/early.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ impl<'a, T: EarlyLintPass> EarlyContextAndPass<'a, T> {
5959
F: FnOnce(&mut Self),
6060
{
6161
let is_crate_node = id == ast::CRATE_NODE_ID;
62-
let push = self.context.builder.push(attrs, is_crate_node);
62+
let push = self.context.builder.push(attrs, is_crate_node, None);
63+
6364
self.check_id(id);
6465
self.enter_attrs(attrs);
6566
f(self);

compiler/rustc_lint/src/expect.rs

+49
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
use crate::builtin;
2+
use rustc_hir::HirId;
3+
use rustc_middle::{lint::LintExpectation, ty::TyCtxt};
4+
use rustc_session::lint::LintExpectationId;
5+
use rustc_span::symbol::sym;
6+
7+
pub fn check_expectations(tcx: TyCtxt<'_>) {
8+
if !tcx.sess.features_untracked().enabled(sym::lint_reasons) {
9+
return;
10+
}
11+
12+
let fulfilled_expectations = tcx.sess.diagnostic().steal_fulfilled_expectation_ids();
13+
let lint_expectations = &tcx.lint_levels(()).lint_expectations;
14+
15+
for (id, expectation) in lint_expectations {
16+
if !fulfilled_expectations.contains(id) {
17+
// This check will always be true, since `lint_expectations` only
18+
// holds stable ids
19+
if let LintExpectationId::Stable { hir_id, .. } = id {
20+
emit_unfulfilled_expectation_lint(tcx, *hir_id, expectation);
21+
} else {
22+
unreachable!("at this stage all `LintExpectationId`s are stable");
23+
}
24+
}
25+
}
26+
}
27+
28+
fn emit_unfulfilled_expectation_lint(
29+
tcx: TyCtxt<'_>,
30+
hir_id: HirId,
31+
expectation: &LintExpectation,
32+
) {
33+
// FIXME: The current implementation doesn't cover cases where the
34+
// `unfulfilled_lint_expectations` is actually expected by another lint
35+
// expectation. This can be added here by checking the lint level and
36+
// retrieving the `LintExpectationId` if it was expected.
37+
tcx.struct_span_lint_hir(
38+
builtin::UNFULFILLED_LINT_EXPECTATIONS,
39+
hir_id,
40+
expectation.emission_span,
41+
|diag| {
42+
let mut diag = diag.build("this lint expectation is unfulfilled");
43+
if let Some(rationale) = expectation.reason {
44+
diag.note(&rationale.as_str());
45+
}
46+
diag.emit();
47+
},
48+
);
49+
}

compiler/rustc_lint/src/late.rs

+3
Original file line numberDiff line numberDiff line change
@@ -503,4 +503,7 @@ pub fn check_crate<'tcx, T: LateLintPass<'tcx>>(
503503
});
504504
},
505505
);
506+
507+
// This check has to be run after all lints are done processing for this crate
508+
tcx.sess.time("check_lint_expectations", || crate::expect::check_expectations(tcx));
506509
}

0 commit comments

Comments
 (0)