-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
fulfill #[expect] expectations from derive generated code
#158943
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
cfa8d8d
2a300cf
8786005
8f9d4e7
643122c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ use rustc_errors::{Diag, DiagCtxtHandle, Diagnostic, MultiSpan, msg}; | |
| use rustc_feature::{Features, GateIssue}; | ||
| use rustc_hir as hir; | ||
| use rustc_hir::HirId; | ||
| use rustc_hir::def::{DefKind, Res}; | ||
| use rustc_hir::intravisit::{self, Visitor}; | ||
| use rustc_index::IndexVec; | ||
| use rustc_middle::hir::nested_filter; | ||
|
|
@@ -178,6 +179,8 @@ fn shallow_lint_levels_on(tcx: TyCtxt<'_>, owner: hir::OwnerId) -> ShallowLintLe | |
| levels.add_command_line(); | ||
| } | ||
|
|
||
| levels.inherit_derive_expectations(owner); | ||
|
|
||
| match attrs.map.range(..) { | ||
| // There is only something to do if there are attributes at all. | ||
| [] => {} | ||
|
|
@@ -308,6 +311,114 @@ impl<'tcx> LintLevelsBuilder<'_, LintLevelQueryMap<'tcx>> { | |
| self.provider.cur = hir_id; | ||
| self.add(self.provider.attrs.get(hir_id.local_id), hir_id == hir::CRATE_HIR_ID); | ||
| } | ||
|
|
||
| /// An `#[expect]` attribute on an item is shared with the impls generated by `#[derive]` | ||
| /// on that item: lints triggered in the derived code are suppressed and fulfill the | ||
| /// item's expectation (see issue #150553). | ||
| /// | ||
| /// The sharing works by looking up the derived-from item here and reusing the expectation | ||
| /// ids of its `#[expect]` attributes, instead of copying the attributes to the generated | ||
| /// impl during expansion: `#[cfg]`/`#[cfg_attr]` processing re-parses the derive input | ||
| /// from tokens, which does not preserve attribute ids (see #152289 and its revert | ||
| /// #153055). One written attribute therefore corresponds to exactly one expectation, no | ||
| /// matter how many impls are derived from the item. | ||
|
Albab-Hasan marked this conversation as resolved.
Outdated
|
||
| fn inherit_derive_expectations(&mut self, owner: hir::OwnerId) { | ||
| let tcx = self.provider.tcx; | ||
| // Fast path: only trait impls can be derive-generated. | ||
| if !matches!(tcx.def_kind(owner), DefKind::Impl { of_trait: true }) { | ||
| return; | ||
| } | ||
| let hir::OwnerNode::Item(item) = tcx.hir_owner_node(owner) else { return }; | ||
| let hir::ItemKind::Impl(impl_) = &item.kind else { return }; | ||
| if !item.span.in_derive_expansion() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tested what happens with a proc-macro? Could you add a test case with @petrochenkov I know you've worked on spans/hygiene data before. Do you know if we can trust the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course not, procedural macros can assign any spans they want to the produced tokens.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The general approach from #152289 seems right to me, compared to what this PR attempts to do. Not sure why it relied on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what I also though, thanks for confirming it. @Albab-Hasan unless there is a way to not rely (at all!) on the spans (and it's hygiene data), this approach seems like a dead-end.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll look into it; see if anything can be done. if not I'll close this pr
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I proposed that to T-lang, but they explicitly wants
That's how the implementation of expectation works.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Then it does require introducing some new infrastructure to preserve "item grouping", to implement this properly. In #158980 we have a similar situation for |
||
| return; | ||
| } | ||
| // Find the item the impl was derived from through its self type. | ||
| let hir::TyKind::Path(hir::QPath::Resolved(_, path)) = impl_.self_ty.kind else { return }; | ||
| let Res::Def(DefKind::Struct | DefKind::Enum | DefKind::Union, def_id) = path.res else { | ||
| return; | ||
| }; | ||
| let Some(def_id) = def_id.as_local() else { return }; | ||
| let adt_hir_id = tcx.local_def_id_to_hir_id(def_id); | ||
|
|
||
| // The specs go on the impl's own node, so that they apply to all of the generated | ||
| // code, while the expectation ids stay keyed to the derived-from item's attributes. | ||
| // Malformed or unknown lint attributes are diagnosed when the derived-from item's | ||
| // own attributes are processed, so they are silently skipped here. Expectations are | ||
| // also registered there, so none are pushed here. | ||
| self.provider.cur = owner.into(); | ||
| for (attr_index, attr) in tcx.hir_attrs(adt_hir_id).iter().enumerate() { | ||
| if Level::from_opt_symbol(attr.name()) != Some(Level::Expect) { | ||
| continue; | ||
| } | ||
| let Some(mut metas) = attr.meta_item_list() else { continue }; | ||
|
|
||
| // Detach a `reason` (RFC 2383) at the end, like `add` does. | ||
| let mut reason = None; | ||
| if let Some(tail_li) = metas.last() | ||
| && let Some(item) = tail_li.meta_item() | ||
| && let ast::MetaItemKind::NameValue(ref name_value) = item.kind | ||
| && item.path == sym::reason | ||
| { | ||
| if let ast::LitKind::Str(rationale, _) = name_value.kind { | ||
| reason = Some(rationale); | ||
| } | ||
| metas.pop(); | ||
| } | ||
|
|
||
| for (lint_index, li) in metas.iter_mut().enumerate() { | ||
| let sp = li.span(); | ||
| let meta_item = match li { | ||
| ast::MetaItemInner::MetaItem(meta_item) if meta_item.is_word() => meta_item, | ||
| _ => continue, | ||
| }; | ||
| let tool_ident = if meta_item.path.segments.len() > 1 { | ||
| Some(meta_item.path.segments.remove(0).ident) | ||
| } else { | ||
| None | ||
| }; | ||
| let tool_name = tool_ident.map(|ident| ident.name); | ||
| let name = pprust::path_to_string(&meta_item.path); | ||
| let lint_result = | ||
| self.store.check_lint_name(&name, tool_name, self.registered_tools); | ||
| let (ids, name) = match lint_result { | ||
| CheckLintNameResult::Ok(ids) => { | ||
| let name = | ||
| meta_item.path.segments.last().expect("empty lint name").ident.name; | ||
| (ids, name) | ||
| } | ||
| CheckLintNameResult::Tool(ids, _) => { | ||
| let complete_name = format!("{}::{}", tool_ident.unwrap().name, name); | ||
| (ids, Symbol::intern(&complete_name)) | ||
| } | ||
| CheckLintNameResult::Renamed(ref replace) => { | ||
| let CheckLintNameResult::Ok(ids) = | ||
| self.store.check_lint_name(replace, None, self.registered_tools) | ||
| else { | ||
| panic!("renamed lint does not exist: {replace}"); | ||
| }; | ||
| (ids, Symbol::intern(replace)) | ||
| } | ||
| CheckLintNameResult::MissingTool | ||
| | CheckLintNameResult::NoTool | ||
| | CheckLintNameResult::Removed(_) | ||
| | CheckLintNameResult::NoLint(_) => continue, | ||
| }; | ||
|
Urgau marked this conversation as resolved.
Outdated
|
||
|
|
||
| let expect_id = StableLintExpectationId { | ||
| hir_id: adt_hir_id, | ||
| attr_index: attr_index.try_into().unwrap(), | ||
| lint_index: lint_index as u16, | ||
| }; | ||
| let src = LintLevelSource::Node { name, span: sp, reason }; | ||
| for &id in ids { | ||
| if self.check_gated_lint(id, sp, false) { | ||
| self.insert_spec(id, LevelSpec::new(Level::Expect, Some(expect_id), src)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<'tcx> Visitor<'tcx> for LintLevelsBuilder<'_, LintLevelQueryMap<'tcx>> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // An `#[expect]` on an item is shared with the impls derived from it, but not with | ||
| // hand-written impls for the same type: lints there must still fire. | ||
|
|
||
| #![deny(redundant_lifetimes)] | ||
|
|
||
| #[derive(Clone)] | ||
| #[expect(redundant_lifetimes)] | ||
| pub struct W<'a> | ||
| where | ||
| 'a: 'static, | ||
| { | ||
| pub r: &'a u8, | ||
| } | ||
|
|
||
| impl<'a> W<'a> | ||
| //~^ ERROR unnecessary lifetime parameter `'a` | ||
| where | ||
| 'a: 'static, | ||
| { | ||
| pub fn get(&self) -> &'a u8 { | ||
| self.r | ||
| } | ||
| } | ||
|
|
||
| fn main() {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // The `#[expect]` sharing with derived code must survive `#[cfg]`/`#[cfg_attr]` | ||
| // processing of the derive input: the attribute-id duplication this caused is why | ||
| // #152289 (copying the attribute to the derived impl) was reverted in #153055. | ||
| // One written attribute is one expectation, fulfilled by the item or its derived | ||
| // code, and reported exactly once when genuinely unfulfilled. | ||
|
|
||
| //@ check-pass | ||
|
|
||
| #![deny(redundant_lifetimes)] | ||
|
|
||
| use std::fmt::Debug; | ||
|
|
||
| #[derive(Debug)] | ||
| #[expect(redundant_lifetimes)] | ||
| pub struct CfgField<'a, T: Debug> | ||
| where | ||
| 'a: 'static, | ||
| { | ||
| pub t_ref: &'a T, | ||
| #[cfg(false)] | ||
| pub gone: u8, | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| #[cfg_attr(all(), expect(redundant_lifetimes))] | ||
| pub struct CfgAttrExpect<'a, T: Debug> | ||
| where | ||
| 'a: 'static, | ||
| { | ||
| pub t_ref: &'a T, | ||
| #[cfg(false)] | ||
| pub gone: u8, | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| #[expect(unexpected_cfgs)] | ||
| //~^ WARN this lint expectation is unfulfilled | ||
| pub struct Unfulfilled { | ||
| pub x: i64, | ||
| #[cfg(false)] | ||
| pub gone: u8, | ||
| } | ||
|
|
||
| fn main() {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| warning: this lint expectation is unfulfilled | ||
| --> $DIR/derive-expect-issue-150553-6.rs:36:10 | ||
| | | ||
| LL | #[expect(unexpected_cfgs)] | ||
| | ^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: `#[warn(unfulfilled_lint_expectations)]` on by default | ||
|
|
||
| warning: 1 warning emitted | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // The `#[expect]` sharing with derive-generated code also applies when the | ||
| // derive itself is expanded from a macros 2.0 macro: the derive expansion is | ||
| // the innermost one, so the expansion-kind gate recognizes the impl no matter | ||
| // what macro produced the derived item. A genuinely unfulfilled expectation | ||
| // is still reported exactly once. | ||
|
|
||
| //@ check-pass | ||
|
|
||
| #![feature(decl_macro)] | ||
| #![deny(redundant_lifetimes)] | ||
|
|
||
| use std::fmt::Debug; | ||
|
|
||
| macro fulfilled() { | ||
| #[derive(Debug)] | ||
| #[expect(redundant_lifetimes)] | ||
| pub struct RefWrapper<'a, T> | ||
| where | ||
| 'a: 'static, | ||
| T: Debug, | ||
| { | ||
| pub t_ref: &'a T, | ||
| } | ||
| } | ||
|
|
||
| fulfilled!(); | ||
|
|
||
| macro passthrough($i:item) { | ||
| $i | ||
| } | ||
|
|
||
| passthrough! { | ||
| #[derive(Debug)] | ||
| #[expect(redundant_lifetimes)] | ||
| pub struct RefWrapperPassthrough<'a, T> | ||
| where | ||
| 'a: 'static, | ||
| T: Debug, | ||
| { | ||
| pub t_ref: &'a T, | ||
| } | ||
| } | ||
|
|
||
| macro unfulfilled() { | ||
| #[derive(Debug)] | ||
| #[expect(unexpected_cfgs)] | ||
| //~^ WARN this lint expectation is unfulfilled | ||
| pub struct Unfulfilled { | ||
| pub x: i64, | ||
| #[cfg(false)] | ||
| pub gone: u8, | ||
| } | ||
| } | ||
|
|
||
| unfulfilled!(); | ||
|
|
||
| fn main() {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| warning: this lint expectation is unfulfilled | ||
| --> $DIR/derive-expect-issue-150553-7.rs:46:14 | ||
| | | ||
| LL | #[expect(unexpected_cfgs)] | ||
| | ^^^^^^^^^^^^^^^ | ||
| ... | ||
| LL | unfulfilled!(); | ||
| | -------------- in this macro invocation | ||
| | | ||
| = note: `#[warn(unfulfilled_lint_expectations)]` on by default | ||
| = note: this warning originates in the macro `unfulfilled` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| warning: 1 warning emitted | ||
|
|
Uh oh!
There was an error while loading. Please reload this page.
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.
Can you add a comment in
rust/compiler/rustc_builtin_macros/src/deriving/generic/mod.rs
Line 541 in f0b782b
#[expect]is not on that list and pointing to this code?View changes since the review