Skip to content

Commit

Permalink
Rollup merge of rust-lang#94670 - xFrednet:rfc-2383-expect-impl-after…
Browse files Browse the repository at this point in the history
…-party, r=flip1995,wesleywiser

Improve `expect` impl and handle `#[expect(unfulfilled_lint_expectations)]` (RFC 2383)

This PR updates unstable `ExpectationIds` in stashed diagnostics and adds some asserts to ensure that the stored expectations are really empty in the end. Additionally, it handles the `#[expect(unfulfilled_lint_expectations)]` case.

According to the [Errors and lints docs](https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-levels) the `error` level should only be used _"when the compiler detects a problem that makes it unable to compile the program"_. As this isn't the case with `#[expect(unfulfilled_lint_expectations)]` I decided to only create a warning. To avoid adding a new lint only for this case, I simply emit a `unfulfilled_lint_expectations` diagnostic with an additional note.

---

r? `@wesleywiser` I'm requesting a review from you since you reviewed the previous PR rust-lang#87835. You are welcome to reassign it if you're busy 🙃

rfc: [RFC-2383](https://rust-lang.github.io/rfcs/2383-lint-reasons.html)

tracking issue: rust-lang#85549

cc: `@flip1995` In case you're also interested in this :)
  • Loading branch information
matthiaskrgr authored Mar 14, 2022
2 parents 774655d + be84049 commit 6548a36
Show file tree
Hide file tree
Showing 8 changed files with 167 additions and 30 deletions.
25 changes: 24 additions & 1 deletion compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ use crate::Substitution;
use crate::SubstitutionPart;
use crate::SuggestionStyle;
use crate::ToolMetadata;
use rustc_lint_defs::Applicability;
use rustc_data_structures::stable_map::FxHashMap;
use rustc_lint_defs::{Applicability, LintExpectationId};
use rustc_serialize::json::Json;
use rustc_span::edition::LATEST_STABLE_EDITION;
use rustc_span::{MultiSpan, Span, DUMMY_SP};
Expand Down Expand Up @@ -138,6 +139,28 @@ impl Diagnostic {
}
}

pub fn update_unstable_expectation_id(
&mut self,
unstable_to_stable: &FxHashMap<LintExpectationId, LintExpectationId>,
) {
if let Level::Expect(expectation_id) = &mut self.level {
if expectation_id.is_stable() {
return;
}

// The unstable to stable map only maps the unstable `AttrId` to a stable `HirId` with an attribute index.
// The lint index inside the attribute is manually transferred here.
let lint_index = expectation_id.get_lint_index();
expectation_id.set_lint_index(None);
let mut stable_id = *unstable_to_stable
.get(&expectation_id)
.expect("each unstable `LintExpectationId` must have a matching stable id");

stable_id.set_lint_index(lint_index);
*expectation_id = stable_id;
}
}

pub fn has_future_breakage(&self) -> bool {
match self.code {
Some(DiagnosticId::Lint { has_future_breakage, .. }) => has_future_breakage,
Expand Down
30 changes: 18 additions & 12 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,11 @@ impl Drop for HandlerInner {
"no warnings or errors encountered even though `delayed_good_path_bugs` issued",
);
}

assert!(
self.unstable_expect_diagnostics.is_empty(),
"all diagnostics with unstable expectations should have been converted",
);
}
}

Expand Down Expand Up @@ -942,29 +947,30 @@ impl Handler {

let mut inner = self.inner.borrow_mut();
for mut diag in diags.into_iter() {
let mut unstable_id = diag
diag.update_unstable_expectation_id(unstable_to_stable);

let stable_id = diag
.level
.get_expectation_id()
.expect("all diagnostics inside `unstable_expect_diagnostics` must have a `LintExpectationId`");

// The unstable to stable map only maps the unstable `AttrId` to a stable `HirId` with an attribute index.
// The lint index inside the attribute is manually transferred here.
let lint_index = unstable_id.get_lint_index();
unstable_id.set_lint_index(None);
let mut stable_id = *unstable_to_stable
.get(&unstable_id)
.expect("each unstable `LintExpectationId` must have a matching stable id");

stable_id.set_lint_index(lint_index);
diag.level = Level::Expect(stable_id);
inner.fulfilled_expectations.insert(stable_id);

(*TRACK_DIAGNOSTICS)(&diag);
}

inner
.stashed_diagnostics
.values_mut()
.for_each(|diag| diag.update_unstable_expectation_id(unstable_to_stable));
inner
.future_breakage_diagnostics
.iter_mut()
.for_each(|diag| diag.update_unstable_expectation_id(unstable_to_stable));
}

/// This methods steals all [`LintExpectationId`]s that are stored inside
/// [`HandlerInner`] and indicate that the linked expectation has been fulfilled.
#[must_use]
pub fn steal_fulfilled_expectation_ids(&self) -> FxHashSet<LintExpectationId> {
assert!(
self.inner.borrow().unstable_expect_diagnostics.is_empty(),
Expand Down
9 changes: 5 additions & 4 deletions compiler/rustc_lint/src/expect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ fn emit_unfulfilled_expectation_lint(
hir_id: HirId,
expectation: &LintExpectation,
) {
// FIXME: The current implementation doesn't cover cases where the
// `unfulfilled_lint_expectations` is actually expected by another lint
// expectation. This can be added here by checking the lint level and
// retrieving the `LintExpectationId` if it was expected.
tcx.struct_span_lint_hir(
builtin::UNFULFILLED_LINT_EXPECTATIONS,
hir_id,
Expand All @@ -43,6 +39,11 @@ fn emit_unfulfilled_expectation_lint(
if let Some(rationale) = expectation.reason {
diag.note(&rationale.as_str());
}

if expectation.is_unfulfilled_lint_expectations {
diag.note("the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message");
}

diag.emit();
},
);
Expand Down
36 changes: 28 additions & 8 deletions compiler/rustc_lint/src/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_middle::lint::{
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{RegisteredTools, TyCtxt};
use rustc_session::lint::{
builtin::{self, FORBIDDEN_LINT_GROUPS},
builtin::{self, FORBIDDEN_LINT_GROUPS, UNFULFILLED_LINT_EXPECTATIONS},
Level, Lint, LintExpectationId, LintId,
};
use rustc_session::parse::{add_feature_diagnostics, feature_err};
Expand Down Expand Up @@ -218,6 +218,14 @@ impl<'s> LintLevelsBuilder<'s> {
}
}
}

// The lint `unfulfilled_lint_expectations` can't be expected, as it would suppress itself.
// Handling expectations of this lint would add additional complexity with little to no
// benefit. The expect level for this lint will therefore be ignored.
if let Level::Expect(_) = level && id == LintId::of(UNFULFILLED_LINT_EXPECTATIONS) {
return;
}

if let Level::ForceWarn = old_level {
self.current_specs_mut().insert(id, (old_level, old_src));
} else {
Expand Down Expand Up @@ -350,6 +358,22 @@ impl<'s> LintLevelsBuilder<'s> {
self.store.check_lint_name(&name, tool_name, self.registered_tools);
match &lint_result {
CheckLintNameResult::Ok(ids) => {
// This checks for instances where the user writes `#[expect(unfulfilled_lint_expectations)]`
// in that case we want to avoid overriding the lint level but instead add an expectation that
// can't be fulfilled. The lint message will include an explanation, that the
// `unfulfilled_lint_expectations` lint can't be expected.
if let Level::Expect(expect_id) = level {
// The `unfulfilled_lint_expectations` lint is not part of any lint groups. Therefore. we
// only need to check the slice if it contains a single lint.
let is_unfulfilled_lint_expectations = match ids {
[lint] => *lint == LintId::of(UNFULFILLED_LINT_EXPECTATIONS),
_ => false,
};
self.lint_expectations.push((
expect_id,
LintExpectation::new(reason, sp, is_unfulfilled_lint_expectations),
));
}
let src = LintLevelSource::Node(
meta_item.path.segments.last().expect("empty lint name").ident.name,
sp,
Expand All @@ -360,10 +384,6 @@ impl<'s> LintLevelsBuilder<'s> {
self.insert_spec(id, (level, src));
}
}
if let Level::Expect(expect_id) = level {
self.lint_expectations
.push((expect_id, LintExpectation::new(reason, sp)));
}
}

CheckLintNameResult::Tool(result) => {
Expand All @@ -381,7 +401,7 @@ impl<'s> LintLevelsBuilder<'s> {
}
if let Level::Expect(expect_id) = level {
self.lint_expectations
.push((expect_id, LintExpectation::new(reason, sp)));
.push((expect_id, LintExpectation::new(reason, sp, false)));
}
}
Err((Some(ids), ref new_lint_name)) => {
Expand Down Expand Up @@ -425,7 +445,7 @@ impl<'s> LintLevelsBuilder<'s> {
}
if let Level::Expect(expect_id) = level {
self.lint_expectations
.push((expect_id, LintExpectation::new(reason, sp)));
.push((expect_id, LintExpectation::new(reason, sp, false)));
}
}
Err((None, _)) => {
Expand Down Expand Up @@ -531,7 +551,7 @@ impl<'s> LintLevelsBuilder<'s> {
}
if let Level::Expect(expect_id) = level {
self.lint_expectations
.push((expect_id, LintExpectation::new(reason, sp)));
.push((expect_id, LintExpectation::new(reason, sp, false)));
}
} else {
panic!("renamed lint does not exist: {}", new_name);
Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub enum Applicability {
/// Expected `Diagnostic`s get the lint level `Expect` which stores the `LintExpectationId`
/// to match it with the actual expectation later on.
///
/// The `LintExpectationId` has to be has stable between compilations, as diagnostic
/// The `LintExpectationId` has to be stable between compilations, as diagnostic
/// instances might be loaded from cache. Lint messages can be emitted during an
/// `EarlyLintPass` operating on the AST and during a `LateLintPass` traversing the
/// HIR tree. The AST doesn't have enough information to create a stable id. The
Expand All @@ -71,7 +71,7 @@ pub enum Applicability {
#[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash, Encodable, Decodable)]
pub enum LintExpectationId {
/// Used for lints emitted during the `EarlyLintPass`. This id is not
/// has stable and should not be cached.
/// hash stable and should not be cached.
Unstable { attr_id: AttrId, lint_index: Option<u16> },
/// The [`HirId`] that the lint expectation is attached to. This id is
/// stable and can be cached. The additional index ensures that nodes with
Expand Down Expand Up @@ -113,7 +113,9 @@ impl<HCX: rustc_hir::HashStableContext> HashStable<HCX> for LintExpectationId {
lint_index.hash_stable(hcx, hasher);
}
_ => {
unreachable!("HashStable should only be called for a filled `LintExpectationId`")
unreachable!(
"HashStable should only be called for filled and stable `LintExpectationId`"
)
}
}
}
Expand Down
12 changes: 10 additions & 2 deletions compiler/rustc_middle/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,19 @@ pub struct LintExpectation {
pub reason: Option<Symbol>,
/// The [`Span`] of the attribute that this expectation originated from.
pub emission_span: Span,
/// Lint messages for the `unfulfilled_lint_expectations` lint will be
/// adjusted to include an additional note. Therefore, we have to track if
/// the expectation is for the lint.
pub is_unfulfilled_lint_expectations: bool,
}

impl LintExpectation {
pub fn new(reason: Option<Symbol>, attr_span: Span) -> Self {
Self { reason, emission_span: attr_span }
pub fn new(
reason: Option<Symbol>,
emission_span: Span,
is_unfulfilled_lint_expectations: bool,
) -> Self {
Self { reason, emission_span, is_unfulfilled_lint_expectations }
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// check-pass
// ignore-tidy-linelength

#![feature(lint_reasons)]
#![warn(unused_mut)]

#![expect(unfulfilled_lint_expectations, reason = "idk why you would expect this")]
//~^ WARNING this lint expectation is unfulfilled
//~| NOTE `#[warn(unfulfilled_lint_expectations)]` on by default
//~| NOTE idk why you would expect this
//~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message

#[expect(unfulfilled_lint_expectations, reason = "a local: idk why you would expect this")]
//~^ WARNING this lint expectation is unfulfilled
//~| NOTE a local: idk why you would expect this
//~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
pub fn normal_test_fn() {
#[expect(unused_mut, reason = "this expectation will create a diagnostic with the default lint level")]
//~^ WARNING this lint expectation is unfulfilled
//~| NOTE this expectation will create a diagnostic with the default lint level
let mut v = vec![1, 1, 2, 3, 5];
v.sort();

// Check that lint lists including `unfulfilled_lint_expectations` are also handled correctly
#[expect(unused, unfulfilled_lint_expectations, reason = "the expectation for `unused` should be fulfilled")]
//~^ WARNING this lint expectation is unfulfilled
//~| NOTE the expectation for `unused` should be fulfilled
//~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
let value = "I'm unused";
}

#[expect(warnings, reason = "this suppresses all warnings and also suppresses itself. No warning will be issued")]
pub fn expect_warnings() {
// This lint trigger will be suppressed
#[warn(unused_mut)]
let mut v = vec![1, 1, 2, 3, 5];
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
warning: this lint expectation is unfulfilled
--> $DIR/expect_unfulfilled_expectation.rs:7:11
|
LL | #![expect(unfulfilled_lint_expectations, reason = "idk why you would expect this")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unfulfilled_lint_expectations)]` on by default
= note: idk why you would expect this
= note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message

warning: this lint expectation is unfulfilled
--> $DIR/expect_unfulfilled_expectation.rs:13:10
|
LL | #[expect(unfulfilled_lint_expectations, reason = "a local: idk why you would expect this")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: a local: idk why you would expect this
= note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message

warning: this lint expectation is unfulfilled
--> $DIR/expect_unfulfilled_expectation.rs:18:14
|
LL | #[expect(unused_mut, reason = "this expectation will create a diagnostic with the default lint level")]
| ^^^^^^^^^^
|
= note: this expectation will create a diagnostic with the default lint level

warning: this lint expectation is unfulfilled
--> $DIR/expect_unfulfilled_expectation.rs:25:22
|
LL | #[expect(unused, unfulfilled_lint_expectations, reason = "the expectation for `unused` should be fulfilled")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: the expectation for `unused` should be fulfilled
= note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message

warning: 4 warnings emitted

0 comments on commit 6548a36

Please sign in to comment.