Skip to content
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

Rewrite lint_expectations in a single pass. #127313

Merged
merged 4 commits into from
Sep 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use rustc_lint_defs::{Applicability, LintExpectationId};
use rustc_macros::{Decodable, Encodable};
use rustc_span::source_map::Spanned;
use rustc_span::symbol::Symbol;
use rustc_span::{Span, DUMMY_SP};
use rustc_span::{AttrId, Span, DUMMY_SP};
use tracing::debug;

use crate::snippet::Style;
Expand Down Expand Up @@ -356,24 +356,19 @@ impl DiagInner {

pub(crate) fn update_unstable_expectation_id(
&mut self,
unstable_to_stable: &FxIndexMap<LintExpectationId, LintExpectationId>,
unstable_to_stable: &FxIndexMap<AttrId, LintExpectationId>,
) {
if let Level::Expect(expectation_id) | Level::ForceWarning(Some(expectation_id)) =
&mut self.level
&& let LintExpectationId::Unstable { attr_id, lint_index } = *expectation_id
{
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")
.normalize();
let Some(stable_id) = unstable_to_stable.get(&attr_id) else {
panic!("{expectation_id:?} must have a matching stable id")
};

let mut stable_id = *stable_id;
stable_id.set_lint_index(lint_index);
*expectation_id = stable_id;
}
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ use rustc_macros::{Decodable, Encodable};
pub use rustc_span::fatal_error::{FatalError, FatalErrorMarker};
use rustc_span::source_map::SourceMap;
pub use rustc_span::ErrorGuaranteed;
use rustc_span::{Loc, Span, DUMMY_SP};
use rustc_span::{AttrId, Loc, Span, DUMMY_SP};
pub use snippet::Style;
// Used by external projects such as `rust-gpu`.
// See https://github.com/rust-lang/rust/pull/115393.
Expand Down Expand Up @@ -1096,7 +1096,7 @@ impl<'a> DiagCtxtHandle<'a> {

pub fn update_unstable_expectation_id(
&self,
unstable_to_stable: &FxIndexMap<LintExpectationId, LintExpectationId>,
unstable_to_stable: FxIndexMap<AttrId, LintExpectationId>,
) {
let mut inner = self.inner.borrow_mut();
let diags = std::mem::take(&mut inner.unstable_expect_diagnostics);
Expand All @@ -1105,7 +1105,7 @@ impl<'a> DiagCtxtHandle<'a> {
if !diags.is_empty() {
inner.suppressed_expected_diag = true;
for mut diag in diags.into_iter() {
diag.update_unstable_expectation_id(unstable_to_stable);
diag.update_unstable_expectation_id(&unstable_to_stable);

// Here the diagnostic is given back to `emit_diagnostic` where it was first
// intercepted. Now it should be processed as usual, since the unstable expectation
Expand All @@ -1117,11 +1117,11 @@ impl<'a> DiagCtxtHandle<'a> {
inner
.stashed_diagnostics
.values_mut()
.for_each(|(diag, _guar)| diag.update_unstable_expectation_id(unstable_to_stable));
.for_each(|(diag, _guar)| 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));
.for_each(|diag| diag.update_unstable_expectation_id(&unstable_to_stable));
}

/// This methods steals all [`LintExpectationId`]s that are stored inside
Expand Down Expand Up @@ -1567,7 +1567,7 @@ impl DiagCtxtInner {
if let LintExpectationId::Unstable { .. } = expect_id {
unreachable!(); // this case was handled at the top of this function
}
self.fulfilled_expectations.insert(expect_id.normalize());
self.fulfilled_expectations.insert(expect_id);
if let Expect(_) = diagnostic.level {
// Nothing emitted here for expected lints.
TRACK_DIAGNOSTIC(diagnostic, &mut |_| None);
Expand Down
53 changes: 49 additions & 4 deletions compiler/rustc_lint/src/expect.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,66 @@
use rustc_data_structures::fx::FxIndexMap;
use rustc_hir::{HirId, CRATE_OWNER_ID};
use rustc_middle::lint::LintExpectation;
use rustc_middle::query::Providers;
use rustc_middle::ty::TyCtxt;
use rustc_session::lint::builtin::UNFULFILLED_LINT_EXPECTATIONS;
use rustc_session::lint::LintExpectationId;
use rustc_session::lint::{Level, LintExpectationId};
use rustc_span::Symbol;

use crate::lints::{Expectation, ExpectationNote};

pub(crate) fn provide(providers: &mut Providers) {
*providers = Providers { check_expectations, ..*providers };
*providers = Providers { lint_expectations, check_expectations, ..*providers };
}

fn lint_expectations(tcx: TyCtxt<'_>, (): ()) -> Vec<(LintExpectationId, LintExpectation)> {
let krate = tcx.hir_crate_items(());

let mut expectations = Vec::new();
let mut unstable_to_stable_ids = FxIndexMap::default();

let mut record_stable = |attr_id, hir_id, attr_index| {
let expect_id = LintExpectationId::Stable { hir_id, attr_index, lint_index: None };
unstable_to_stable_ids.entry(attr_id).or_insert(expect_id);
};
let mut push_expectations = |owner| {
let lints = tcx.shallow_lint_levels_on(owner);
if lints.expectations.is_empty() {
return;
}

expectations.extend_from_slice(&lints.expectations);

let attrs = tcx.hir_attrs(owner);
for &(local_id, attrs) in attrs.map.iter() {
// Some attributes appear multiple times in HIR, to ensure they are correctly taken
// into account where they matter. This means we cannot just associate the AttrId to
// the first HirId where we see it, but need to check it actually appears in a lint
// level.
// FIXME(cjgillot): Can this cause an attribute to appear in multiple expectation ids?
Comment on lines +36 to +40
Copy link
Member

@jieyouxu jieyouxu Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: this seems a little suspicious to me, in that I would've expected AttrId <-> HirId to be a bijective map. "Some attributes appear multiple times in HIR, to ensure they are correctly taken into account where they matter" almost sounds like a bug to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This FIXME makes me sad too, and triggered the discussion here: #127884 (comment)

This map is not bijective, but it is well-defined in one direction. From a pair (HirId, attr_index), you'll get a single AttrId, but the converse is not true.

Copy link
Member

@jieyouxu jieyouxu Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification, that discussion was enlightening. I feel like this will cause a bug somewhere but I also can't immediately come up with an example for. As I can't come up with an example, I'm inclined to merge this as-is and see what cases we run into in practice through fuzzing and whatnot. And see if we can come up with a scheme that is more robust.

Copy link
Member

@jieyouxu jieyouxu Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Side remark: our lint infra and lint level computation is unfortunately incredibly convoluted)

if !lints.specs.contains_key(&local_id) {
continue;
}
for (attr_index, attr) in attrs.iter().enumerate() {
let Some(Level::Expect(_)) = Level::from_attr(attr) else { continue };
record_stable(attr.id, HirId { owner, local_id }, attr_index.try_into().unwrap());
}
}
};

push_expectations(CRATE_OWNER_ID);
for owner in krate.owners() {
push_expectations(owner);
}

tcx.dcx().update_unstable_expectation_id(unstable_to_stable_ids);
expectations
}

fn check_expectations(tcx: TyCtxt<'_>, tool_filter: Option<Symbol>) {
let lint_expectations = tcx.lint_expectations(());
let fulfilled_expectations = tcx.dcx().steal_fulfilled_expectation_ids();

tracing::debug!(?lint_expectations, ?fulfilled_expectations);

for (id, expectation) in lint_expectations {
// This check will always be true, since `lint_expectations` only
// holds stable ids
Expand Down
Loading
Loading