Skip to content

Rewrite empty attribute lint for new attribute parser #143252

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
4 changes: 1 addition & 3 deletions compiler/rustc_attr_data_structures/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ pub enum ReprAttr {
ReprSimd,
ReprTransparent,
ReprAlign(Align),
// this one is just so we can emit a lint for it
ReprEmpty,
}
pub use ReprAttr::*;

Expand Down Expand Up @@ -291,7 +289,7 @@ pub enum AttributeKind {
PubTransparent(Span),

/// Represents [`#[repr]`](https://doc.rust-lang.org/stable/reference/type-layout.html#representations).
Repr(ThinVec<(ReprAttr, Span)>),
Repr { reprs: ThinVec<(ReprAttr, Span)>, first_span: Span },

/// Represents `#[rustc_layout_scalar_valid_range_end]`.
RustcLayoutScalarValidRangeEnd(Box<u128>, Span),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ impl AttributeKind {
Inline(..) => No,
LinkSection { .. } => No,
MacroTransparency(..) => Yes,
Repr(..) => No,
Repr { .. } => No,
Stability { .. } => Yes,
Cold(..) => No,
ConstContinue(..) => No,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_attr_data_structures/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ pub struct AttributeLint<Id> {
pub enum AttributeLintKind {
UnusedDuplicate { this: Span, other: Span, warning: bool },
IllFormedAttributeInput { suggestions: Vec<String> },
EmptyAttribute { first_span: Span },
}
4 changes: 4 additions & 0 deletions compiler/rustc_attr_parsing/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ attr_parsing_deprecated_item_suggestion =
.help = add `#![feature(deprecated_suggestion)]` to the crate root
.note = see #94785 for more details
attr_parsing_empty_attribute =
unused attribute
.suggestion = remove this attribute
attr_parsing_empty_confusables =
expected at least one confusable name
attr_parsing_expected_one_cfg_pattern =
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_attr_parsing/src/attributes/codegen_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,10 @@ impl<S: Stage> CombineAttributeParser<S> for TargetFeatureParser {
cx.expected_list(cx.attr_span);
return features;
};
if list.is_empty() {
cx.warn_empty_attribute(cx.attr_span);
return features;
}
for item in list.mixed() {
let Some(name_value) = item.meta_item() else {
cx.expected_name_value(item.span(), Some(sym::enable));
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_attr_parsing/src/attributes/repr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ pub(crate) struct ReprParser;
impl<S: Stage> CombineAttributeParser<S> for ReprParser {
type Item = (ReprAttr, Span);
const PATH: &[Symbol] = &[sym::repr];
const CONVERT: ConvertFn<Self::Item> = |items, _| AttributeKind::Repr(items);
const CONVERT: ConvertFn<Self::Item> =
|items, first_span| AttributeKind::Repr { reprs: items, first_span };
// FIXME(jdonszelmann): never used
const TEMPLATE: AttributeTemplate =
template!(List: "C | Rust | align(...) | packed(...) | <integer type> | transparent");
Expand All @@ -40,8 +41,8 @@ impl<S: Stage> CombineAttributeParser<S> for ReprParser {
};

if list.is_empty() {
// this is so validation can emit a lint
reprs.push((ReprAttr::ReprEmpty, cx.attr_span));
cx.warn_empty_attribute(cx.attr_span);
return reprs;
}

for param in list.mixed() {
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_attr_parsing/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ mod private {
#[allow(private_interfaces)]
pub trait Stage: Sized + 'static + Sealed {
type Id: Copy;
const SHOULD_EMIT_LINTS: bool;

fn parsers() -> &'static group_type!(Self);

Expand All @@ -170,6 +171,7 @@ pub trait Stage: Sized + 'static + Sealed {
#[allow(private_interfaces)]
impl Stage for Early {
type Id = NodeId;
const SHOULD_EMIT_LINTS: bool = false;

fn parsers() -> &'static group_type!(Self) {
&early::ATTRIBUTE_PARSERS
Expand All @@ -183,6 +185,7 @@ impl Stage for Early {
#[allow(private_interfaces)]
impl Stage for Late {
type Id = HirId;
const SHOULD_EMIT_LINTS: bool = true;

fn parsers() -> &'static group_type!(Self) {
&late::ATTRIBUTE_PARSERS
Expand Down Expand Up @@ -223,6 +226,9 @@ impl<'f, 'sess: 'f, S: Stage> SharedContext<'f, 'sess, S> {
/// must be delayed until after HIR is built. This method will take care of the details of
/// that.
pub(crate) fn emit_lint(&mut self, lint: AttributeLintKind, span: Span) {
if !S::SHOULD_EMIT_LINTS {
return;
}
let id = self.target_id;
(self.emit_lint)(AttributeLint { id, span, kind: lint });
}
Expand Down Expand Up @@ -404,6 +410,10 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
},
})
}

pub(crate) fn warn_empty_attribute(&mut self, span: Span) {
self.emit_lint(AttributeLintKind::EmptyAttribute { first_span: span }, span);
}
}

impl<'f, 'sess, S: Stage> Deref for AcceptContext<'f, 'sess, S> {
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_attr_parsing/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,11 @@ pub fn emit_attribute_lint<L: LintEmitter>(lint: &AttributeLint<HirId>, lint_emi
},
);
}
AttributeLintKind::EmptyAttribute { first_span } => lint_emitter.emit_node_span_lint(
rustc_session::lint::builtin::UNUSED_ATTRIBUTES,
*id,
*first_span,
session_diagnostics::EmptyAttributeList { attr_span: *first_span },
),
}
}
7 changes: 7 additions & 0 deletions compiler/rustc_attr_parsing/src/session_diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,13 @@ pub(crate) struct EmptyConfusables {
pub span: Span,
}

#[derive(LintDiagnostic)]
#[diag(attr_parsing_empty_attribute)]
pub(crate) struct EmptyAttributeList {
#[suggestion(code = "", applicability = "machine-applicable")]
pub attr_span: Span,
}

#[derive(Diagnostic)]
#[diag(attr_parsing_invalid_alignment_value, code = E0589)]
pub(crate) struct InvalidAlignmentValue {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_builtin_macros/src/deriving/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ impl<'a> TraitDef<'a> {
Annotatable::Item(item) => {
let is_packed = matches!(
AttributeParser::parse_limited(cx.sess, &item.attrs, sym::repr, item.span, item.id),
Some(Attribute::Parsed(AttributeKind::Repr(r))) if r.iter().any(|(x, _)| matches!(x, ReprPacked(..)))
Some(Attribute::Parsed(AttributeKind::Repr { reprs, .. })) if reprs.iter().any(|(x, _)| matches!(x, ReprPacked(..)))
);

let newitem = match &item.kind {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/codegen_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {

if let hir::Attribute::Parsed(p) = attr {
match p {
AttributeKind::Repr(reprs) => {
AttributeKind::Repr { reprs, first_span: _ } => {
codegen_fn_attrs.alignment = reprs
.iter()
.filter_map(
Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_hir_analysis/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1395,8 +1395,7 @@ fn check_simd(tcx: TyCtxt<'_>, sp: Span, def_id: LocalDefId) {
pub(super) fn check_packed(tcx: TyCtxt<'_>, sp: Span, def: ty::AdtDef<'_>) {
let repr = def.repr();
if repr.packed() {
if let Some(reprs) =
attrs::find_attr!(tcx.get_all_attrs(def.did()), attrs::AttributeKind::Repr(r) => r)
if let Some(reprs) = attrs::find_attr!(tcx.get_all_attrs(def.did()), attrs::AttributeKind::Repr { reprs, .. } => reprs)
{
for (r, _) in reprs {
if let ReprPacked(pack) = r
Expand Down Expand Up @@ -1619,10 +1618,10 @@ fn check_enum(tcx: TyCtxt<'_>, def_id: LocalDefId) {
if def.variants().is_empty() {
attrs::find_attr!(
tcx.get_all_attrs(def_id),
attrs::AttributeKind::Repr(rs) => {
attrs::AttributeKind::Repr { reprs, first_span } => {
struct_span_code_err!(
tcx.dcx(),
rs.first().unwrap().1,
reprs.first().map(|repr| repr.1).unwrap_or(*first_span),
E0084,
"unsupported representation for zero-variant enum"
)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/src/nonstandard_style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ impl EarlyLintPass for NonCamelCaseTypes {
fn check_item(&mut self, cx: &EarlyContext<'_>, it: &ast::Item) {
let has_repr_c = matches!(
AttributeParser::parse_limited(cx.sess(), &it.attrs, sym::repr, it.span, it.id),
Some(Attribute::Parsed(AttributeKind::Repr(r))) if r.iter().any(|(r, _)| r == &ReprAttr::ReprC)
Some(Attribute::Parsed(AttributeKind::Repr { reprs, ..})) if reprs.iter().any(|(r, _)| r == &ReprAttr::ReprC)
);

if has_repr_c {
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1521,7 +1521,8 @@ impl<'tcx> TyCtxt<'tcx> {
field_shuffle_seed ^= user_seed;
}

if let Some(reprs) = attr::find_attr!(self.get_all_attrs(did), AttributeKind::Repr(r) => r)
if let Some(reprs) =
attr::find_attr!(self.get_all_attrs(did), AttributeKind::Repr { reprs, .. } => reprs)
{
for (r, _) in reprs {
flags.insert(match *r {
Expand Down Expand Up @@ -1562,10 +1563,6 @@ impl<'tcx> TyCtxt<'tcx> {
max_align = max_align.max(Some(align));
ReprFlags::empty()
}
attr::ReprEmpty => {
/* skip these, they're just for diagnostics */
ReprFlags::empty()
}
});
}
}
Expand Down
96 changes: 32 additions & 64 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
}
Attribute::Parsed(AttributeKind::DocComment { .. }) => { /* `#[doc]` is actually a lot more than just doc comments, so is checked below*/
}
Attribute::Parsed(AttributeKind::Repr(_)) => { /* handled below this loop and elsewhere */
Attribute::Parsed(AttributeKind::Repr { .. }) => { /* handled below this loop and elsewhere */
}
Attribute::Parsed(AttributeKind::RustcObjectLifetimeDefault) => {
self.check_object_lifetime_default(hir_id);
Expand Down Expand Up @@ -1943,7 +1943,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
// #[repr(foo)]
// #[repr(bar, align(8))]
// ```
let reprs = find_attr!(attrs, AttributeKind::Repr(r) => r.as_slice()).unwrap_or(&[]);
let (reprs, first_attr_span) = find_attr!(attrs, AttributeKind::Repr { reprs, first_span } => (reprs.as_slice(), Some(*first_span))).unwrap_or((&[], None));

let mut int_reprs = 0;
let mut is_explicit_rust = false;
Expand Down Expand Up @@ -2040,31 +2040,30 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
continue;
}
}
// FIXME(jdonszelmann): move the diagnostic for unused repr attrs here, I think
// it's a better place for it.
ReprAttr::ReprEmpty => {
// catch `repr()` with no arguments, applied to an item (i.e. not `#![repr()]`)
if item.is_some() {
match target {
Target::Struct | Target::Union | Target::Enum => continue,
Target::Fn | Target::Method(_) => {
self.dcx().emit_err(errors::ReprAlignShouldBeAlign {
span: *repr_span,
item: target.name(),
});
}
_ => {
self.dcx().emit_err(errors::AttrApplication::StructEnumUnion {
hint_span: *repr_span,
span,
});
}
}
}
};
}

return;
// catch `repr()` with no arguments, applied to an item (i.e. not `#![repr()]`)
if let Some(first_attr_span) = first_attr_span
&& reprs.is_empty()
&& item.is_some()
{
match target {
Target::Struct | Target::Union | Target::Enum => {}
Target::Fn | Target::Method(_) => {
self.dcx().emit_err(errors::ReprAlignShouldBeAlign {
span: first_attr_span,
item: target.name(),
});
}
};
_ => {
self.dcx().emit_err(errors::AttrApplication::StructEnumUnion {
hint_span: first_attr_span,
span,
});
}
}
return;
}

// Just point at all repr hints if there are any incompatibilities.
Expand Down Expand Up @@ -2319,43 +2318,8 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
}

fn check_unused_attribute(&self, hir_id: HirId, attr: &Attribute, style: Option<AttrStyle>) {
// FIXME(jdonszelmann): deduplicate these checks after more attrs are parsed. This is very
// ugly now but can 100% be removed later.
if let Attribute::Parsed(p) = attr {
match p {
AttributeKind::Repr(reprs) => {
for (r, span) in reprs {
if let ReprAttr::ReprEmpty = r {
self.tcx.emit_node_span_lint(
UNUSED_ATTRIBUTES,
hir_id,
*span,
errors::Unused {
attr_span: *span,
note: errors::UnusedNote::EmptyList { name: sym::repr },
},
);
}
}
return;
}
AttributeKind::TargetFeature(features, span) if features.len() == 0 => {
self.tcx.emit_node_span_lint(
UNUSED_ATTRIBUTES,
hir_id,
*span,
errors::Unused {
attr_span: *span,
note: errors::UnusedNote::EmptyList { name: sym::target_feature },
},
);
return;
}
_ => {}
};
}

// Warn on useless empty attributes.
// FIXME(jdonszelmann): this lint should be moved to attribute parsing, see `AcceptContext::warn_empty_attribute`
let note = if attr.has_any_name(&[
sym::macro_use,
sym::allow,
Expand Down Expand Up @@ -2571,7 +2535,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
}

fn check_rustc_pub_transparent(&self, attr_span: Span, span: Span, attrs: &[Attribute]) {
if !find_attr!(attrs, AttributeKind::Repr(r) => r.iter().any(|(r, _)| r == &ReprAttr::ReprTransparent))
if !find_attr!(attrs, AttributeKind::Repr { reprs, .. } => reprs.iter().any(|(r, _)| r == &ReprAttr::ReprTransparent))
.unwrap_or(false)
{
self.dcx().emit_err(errors::RustcPubTransparent { span, attr_span });
Expand Down Expand Up @@ -2847,8 +2811,12 @@ fn check_invalid_crate_level_attr(tcx: TyCtxt<'_>, attrs: &[Attribute]) {
ATTRS_TO_CHECK.iter().find(|attr_to_check| attr.has_name(**attr_to_check))
{
(attr.span(), *a)
} else if let Attribute::Parsed(AttributeKind::Repr(r)) = attr {
(r.first().unwrap().1, sym::repr)
} else if let Attribute::Parsed(AttributeKind::Repr {
reprs: _,
first_span: first_attr_span,
}) = attr
{
(*first_attr_span, sym::repr)
} else {
continue;
};
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,7 @@ impl Item {
// don't want it it `Item::attrs`.
hir::Attribute::Parsed(AttributeKind::Deprecation { .. }) => None,
// We have separate pretty-printing logic for `#[repr(..)]` attributes.
hir::Attribute::Parsed(AttributeKind::Repr(..)) => None,
hir::Attribute::Parsed(AttributeKind::Repr { .. }) => None,
// target_feature is special-cased because cargo-semver-checks uses it
hir::Attribute::Parsed(AttributeKind::TargetFeature(features, _)) => {
let mut output = String::new();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ impl<'tcx> LateLintPass<'tcx> for ArbitrarySourceItemOrdering {
.tcx
.hir_attrs(item.hir_id())
.iter()
.any(|attr| matches!(attr, Attribute::Parsed(AttributeKind::Repr(..))))
.any(|attr| matches!(attr, Attribute::Parsed(AttributeKind::Repr{ .. })))
{
// Do not lint items with a `#[repr]` attribute as their layout may be imposed by an external API.
return;
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/attrs/repr_attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use clippy_utils::msrvs::{self, Msrv};
use super::REPR_PACKED_WITHOUT_ABI;

pub(super) fn check(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute], msrv: Msrv) {
if let Some(reprs) = find_attr!(attrs, AttributeKind::Repr(r) => r) {
if let Some(reprs) = find_attr!(attrs, AttributeKind::Repr { reprs, .. } => reprs) {
let packed_span = reprs
.iter()
.find(|(r, _)| matches!(r, ReprAttr::ReprPacked(..)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,5 +99,5 @@ fn is_zst<'tcx>(cx: &LateContext<'tcx>, field: &FieldDef, args: ty::GenericArgsR
fn has_c_repr_attr(cx: &LateContext<'_>, hir_id: HirId) -> bool {
let attrs = cx.tcx.hir_attrs(hir_id);

find_attr!(attrs, AttributeKind::Repr(r) if r.iter().any(|(x, _)| *x == ReprAttr::ReprC))
find_attr!(attrs, AttributeKind::Repr { reprs, .. } if reprs.iter().any(|(x, _)| *x == ReprAttr::ReprC))
}
Loading
Loading