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 1 commit
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
54 changes: 50 additions & 4 deletions compiler/rustc_lint/src/expect.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,67 @@
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, attr_id: 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
150 changes: 6 additions & 144 deletions compiler/rustc_lint/src/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use rustc_session::lint::builtin::{
use rustc_session::lint::{Level, Lint, LintExpectationId, LintId};
use rustc_session::Session;
use rustc_span::symbol::{sym, Symbol};
use rustc_span::{AttrId, Span, DUMMY_SP};
use rustc_span::{Span, DUMMY_SP};
use tracing::{debug, instrument};
use {rustc_ast as ast, rustc_hir as hir};

Expand Down Expand Up @@ -115,34 +115,6 @@ impl LintLevelSets {
}
}

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

let mut builder = LintLevelsBuilder {
sess: tcx.sess,
features: tcx.features(),
provider: QueryMapExpectationsWrapper {
tcx,
cur: hir::CRATE_HIR_ID,
specs: ShallowLintLevelMap::default(),
expectations: Vec::new(),
unstable_to_stable_ids: FxIndexMap::default(),
empty: FxIndexMap::default(),
},
lint_added_lints: false,
store,
registered_tools: tcx.registered_tools(()),
};

builder.add_command_line();
builder.add_id(hir::CRATE_HIR_ID);
tcx.hir().walk_toplevel_module(&mut builder);

tcx.dcx().update_unstable_expectation_id(builder.provider.unstable_to_stable_ids);

builder.provider.expectations
}

#[instrument(level = "trace", skip(tcx), ret)]
fn shallow_lint_levels_on(tcx: TyCtxt<'_>, owner: hir::OwnerId) -> ShallowLintLevelMap {
let store = unerased_lint_store(tcx.sess);
Expand Down Expand Up @@ -207,7 +179,7 @@ pub trait LintLevelsProvider {
fn current_specs(&self) -> &FxIndexMap<LintId, LevelAndSource>;
fn insert(&mut self, id: LintId, lvl: LevelAndSource);
fn get_lint_level(&self, lint: &'static Lint, sess: &Session) -> LevelAndSource;
fn push_expectation(&mut self, _id: LintExpectationId, _expectation: LintExpectation) {}
fn push_expectation(&mut self, id: LintExpectationId, expectation: LintExpectation);
}

impl LintLevelsProvider for TopDown {
Expand All @@ -222,6 +194,8 @@ impl LintLevelsProvider for TopDown {
fn get_lint_level(&self, lint: &'static Lint, sess: &Session) -> LevelAndSource {
self.sets.get_lint_level(lint, self.cur, Some(self.current_specs()), sess)
}

fn push_expectation(&mut self, _: LintExpectationId, _: LintExpectation) {}
}

struct LintLevelQueryMap<'tcx> {
Expand All @@ -243,46 +217,8 @@ impl LintLevelsProvider for LintLevelQueryMap<'_> {
fn get_lint_level(&self, lint: &'static Lint, _: &Session) -> LevelAndSource {
self.specs.lint_level_id_at_node(self.tcx, LintId::of(lint), self.cur)
}
}

struct QueryMapExpectationsWrapper<'tcx> {
tcx: TyCtxt<'tcx>,
/// HirId of the currently investigated element.
cur: HirId,
/// Level map for `cur`.
specs: ShallowLintLevelMap,
expectations: Vec<(LintExpectationId, LintExpectation)>,
unstable_to_stable_ids: FxIndexMap<AttrId, LintExpectationId>,
/// Empty hash map to simplify code.
empty: FxIndexMap<LintId, LevelAndSource>,
}

impl LintLevelsProvider for QueryMapExpectationsWrapper<'_> {
fn current_specs(&self) -> &FxIndexMap<LintId, LevelAndSource> {
self.specs.specs.get(&self.cur.local_id).unwrap_or(&self.empty)
}
fn insert(&mut self, id: LintId, lvl: LevelAndSource) {
self.specs.specs.get_mut_or_insert_default(self.cur.local_id).insert(id, lvl);
}
fn get_lint_level(&self, lint: &'static Lint, _: &Session) -> LevelAndSource {
// We cannot use `tcx.lint_level_at_node` because we want to know in which order the
// attributes have been inserted, in particular whether an `expect` follows a `forbid`.
self.specs.lint_level_id_at_node(self.tcx, LintId::of(lint), self.cur)
}
fn push_expectation(&mut self, id: LintExpectationId, expectation: LintExpectation) {
let LintExpectationId::Stable { attr_id: Some(attr_id), hir_id, attr_index, .. } = id
else {
bug!("unstable expectation id should already be mapped")
};

self.unstable_to_stable_ids.entry(attr_id).or_insert(LintExpectationId::Stable {
hir_id,
attr_index,
lint_index: None,
attr_id: None,
});

self.expectations.push((id.normalize(), expectation));
self.specs.expectations.push((id.normalize(), expectation))
}
}

Expand Down Expand Up @@ -367,80 +303,6 @@ impl<'tcx> Visitor<'tcx> for LintLevelsBuilder<'_, LintLevelQueryMap<'tcx>> {
}
}

impl<'tcx> LintLevelsBuilder<'_, QueryMapExpectationsWrapper<'tcx>> {
fn add_id(&mut self, hir_id: HirId) {
// Change both the `HirId` and the associated specs.
self.provider.cur = hir_id;
self.provider.specs.specs.clear();
self.add(self.provider.tcx.hir().attrs(hir_id), hir_id == hir::CRATE_HIR_ID, Some(hir_id));
}
}

impl<'tcx> Visitor<'tcx> for LintLevelsBuilder<'_, QueryMapExpectationsWrapper<'tcx>> {
type NestedFilter = nested_filter::All;

fn nested_visit_map(&mut self) -> Self::Map {
self.provider.tcx.hir()
}

fn visit_param(&mut self, param: &'tcx hir::Param<'tcx>) {
self.add_id(param.hir_id);
intravisit::walk_param(self, param);
}

fn visit_item(&mut self, it: &'tcx hir::Item<'tcx>) {
self.add_id(it.hir_id());
intravisit::walk_item(self, it);
}

fn visit_foreign_item(&mut self, it: &'tcx hir::ForeignItem<'tcx>) {
self.add_id(it.hir_id());
intravisit::walk_foreign_item(self, it);
}

fn visit_stmt(&mut self, e: &'tcx hir::Stmt<'tcx>) {
// We will call `add_id` when we walk
// the `StmtKind`. The outer statement itself doesn't
// define the lint levels.
intravisit::walk_stmt(self, e);
}

fn visit_expr(&mut self, e: &'tcx hir::Expr<'tcx>) {
self.add_id(e.hir_id);
intravisit::walk_expr(self, e);
}

fn visit_field_def(&mut self, s: &'tcx hir::FieldDef<'tcx>) {
self.add_id(s.hir_id);
intravisit::walk_field_def(self, s);
}

fn visit_variant(&mut self, v: &'tcx hir::Variant<'tcx>) {
self.add_id(v.hir_id);
intravisit::walk_variant(self, v);
}

fn visit_local(&mut self, l: &'tcx hir::LetStmt<'tcx>) {
self.add_id(l.hir_id);
intravisit::walk_local(self, l);
}

fn visit_arm(&mut self, a: &'tcx hir::Arm<'tcx>) {
self.add_id(a.hir_id);
intravisit::walk_arm(self, a);
}

fn visit_trait_item(&mut self, trait_item: &'tcx hir::TraitItem<'tcx>) {
self.add_id(trait_item.hir_id());
intravisit::walk_trait_item(self, trait_item);
}

fn visit_impl_item(&mut self, impl_item: &'tcx hir::ImplItem<'tcx>) {
self.add_id(impl_item.hir_id());
intravisit::walk_impl_item(self, impl_item);
}
}

pub struct LintLevelsBuilder<'s, P> {
sess: &'s Session,
features: &'s Features,
Expand Down Expand Up @@ -1099,7 +961,7 @@ impl<'s, P: LintLevelsProvider> LintLevelsBuilder<'s, P> {
}

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

pub(crate) fn parse_lint_and_tool_name(lint_name: &str) -> (Option<Symbol>, &str) {
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use rustc_errors::{Diag, MultiSpan};
use rustc_hir::{HirId, ItemLocalId};
use rustc_macros::HashStable;
use rustc_session::lint::builtin::{self, FORBIDDEN_LINT_GROUPS};
use rustc_session::lint::{FutureIncompatibilityReason, Level, Lint, LintId};
use rustc_session::lint::{FutureIncompatibilityReason, Level, Lint, LintExpectationId, LintId};
use rustc_session::Session;
use rustc_span::hygiene::{ExpnKind, MacroKind};
use rustc_span::{symbol, DesugaringKind, Span, Symbol, DUMMY_SP};
Expand Down Expand Up @@ -61,6 +61,7 @@ pub type LevelAndSource = (Level, LintLevelSource);
/// by the attributes for *a single HirId*.
#[derive(Default, Debug, HashStable)]
pub struct ShallowLintLevelMap {
pub expectations: Vec<(LintExpectationId, LintExpectation)>,
pub specs: SortedMap<ItemLocalId, FxIndexMap<LintId, LevelAndSource>>,
}

Expand Down
7 changes: 1 addition & 6 deletions tests/ui/error-codes/E0602.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@ warning[E0602]: unknown lint: `bogus`
= note: requested on the command line with `-D bogus`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

warning[E0602]: unknown lint: `bogus`
|
= note: requested on the command line with `-D bogus`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

warning: 4 warnings emitted
warning: 3 warnings emitted
jieyouxu marked this conversation as resolved.
Show resolved Hide resolved

For more information about this error, try `rustc --explain E0602`.
7 changes: 1 addition & 6 deletions tests/ui/lint/cli-unknown-force-warn.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@ warning[E0602]: unknown lint: `foo_qux`
= note: requested on the command line with `--force-warn foo_qux`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

warning[E0602]: unknown lint: `foo_qux`
|
= note: requested on the command line with `--force-warn foo_qux`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

warning: 4 warnings emitted
warning: 3 warnings emitted

For more information about this error, try `rustc --explain E0602`.
7 changes: 1 addition & 6 deletions tests/ui/lint/lint-removed-cmdline-deny.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,5 @@ LL | #[deny(warnings)]
| ^^^^^^^^
= note: `#[deny(unused_variables)]` implied by `#[deny(warnings)]`

error: lint `raw_pointer_derive` has been removed: using derive with raw pointers is ok
|
= note: requested on the command line with `-D raw_pointer_derive`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: aborting due to 5 previous errors
error: aborting due to 4 previous errors

7 changes: 1 addition & 6 deletions tests/ui/lint/lint-removed-cmdline.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,5 @@ LL | #[deny(warnings)]
| ^^^^^^^^
= note: `#[deny(unused_variables)]` implied by `#[deny(warnings)]`

warning: lint `raw_pointer_derive` has been removed: using derive with raw pointers is ok
|
= note: requested on the command line with `-D raw_pointer_derive`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: aborting due to 1 previous error; 4 warnings emitted
error: aborting due to 1 previous error; 3 warnings emitted

8 changes: 1 addition & 7 deletions tests/ui/lint/lint-renamed-cmdline-deny.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,5 @@ LL | #[deny(unused)]
| ^^^^^^
= note: `#[deny(unused_variables)]` implied by `#[deny(unused)]`

error: lint `bare_trait_object` has been renamed to `bare_trait_objects`
|
= help: use the new name `bare_trait_objects`
= note: requested on the command line with `-D bare_trait_object`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: aborting due to 5 previous errors
error: aborting due to 4 previous errors

8 changes: 1 addition & 7 deletions tests/ui/lint/lint-renamed-cmdline.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,5 @@ LL | #[deny(unused)]
| ^^^^^^
= note: `#[deny(unused_variables)]` implied by `#[deny(unused)]`

warning: lint `bare_trait_object` has been renamed to `bare_trait_objects`
|
= help: use the new name `bare_trait_objects`
= note: requested on the command line with `-D bare_trait_object`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: aborting due to 1 previous error; 4 warnings emitted
error: aborting due to 1 previous error; 3 warnings emitted

12 changes: 1 addition & 11 deletions tests/ui/lint/lint-unexported-no-mangle.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,5 @@ LL | pub const PUB_FOO: u64 = 1;
| |
| help: try a static value: `pub static`

warning: lint `private_no_mangle_fns` has been removed: no longer a warning, `#[no_mangle]` functions always exported
|
= note: requested on the command line with `-F private_no_mangle_fns`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

warning: lint `private_no_mangle_statics` has been removed: no longer a warning, `#[no_mangle]` statics always exported
|
= note: requested on the command line with `-F private_no_mangle_statics`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: aborting due to 2 previous errors; 8 warnings emitted
error: aborting due to 2 previous errors; 6 warnings emitted

Loading