check_attr doesn't check every HIR node type that can carry attributes.

@varkor

The check_attr visitor here is missing several methods:

rust/compiler/rustc_passes/src/check_attr.rs

Lines 866 to 936 in e261649

    
           impl Visitor<'tcx> for CheckAttrVisitor<'tcx> { 
        
               type Map = Map<'tcx>; 
        
               fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { 
        
                   NestedVisitorMap::OnlyBodies(self.tcx.hir()) 
        
               } 
        
               fn visit_item(&mut self, item: &'tcx Item<'tcx>) { 
        
                   let target = Target::from_item(item); 
        
                   self.check_attributes( 
        
                       item.hir_id, 
        
                       item.attrs, 
        
                       &item.span, 
        
                       target, 
        
                       Some(ItemLike::Item(item)), 
        
                   ); 
        
                   intravisit::walk_item(self, item) 
        
               } 
        
               fn visit_trait_item(&mut self, trait_item: &'tcx TraitItem<'tcx>) { 
        
                   let target = Target::from_trait_item(trait_item); 
        
                   self.check_attributes(trait_item.hir_id, &trait_item.attrs, &trait_item.span, target, None); 
        
                   intravisit::walk_trait_item(self, trait_item) 
        
               } 
        
               fn visit_foreign_item(&mut self, f_item: &'tcx ForeignItem<'tcx>) { 
        
                   let target = Target::from_foreign_item(f_item); 
        
                   self.check_attributes( 
        
                       f_item.hir_id, 
        
                       &f_item.attrs, 
        
                       &f_item.span, 
        
                       target, 
        
                       Some(ItemLike::ForeignItem(f_item)), 
        
                   ); 
        
                   intravisit::walk_foreign_item(self, f_item) 
        
               } 
        
               fn visit_impl_item(&mut self, impl_item: &'tcx hir::ImplItem<'tcx>) { 
        
                   let target = target_from_impl_item(self.tcx, impl_item); 
        
                   self.check_attributes(impl_item.hir_id, &impl_item.attrs, &impl_item.span, target, None); 
        
                   intravisit::walk_impl_item(self, impl_item) 
        
               } 
        
               fn visit_stmt(&mut self, stmt: &'tcx hir::Stmt<'tcx>) { 
        
                   // When checking statements ignore expressions, they will be checked later. 
        
                   if let hir::StmtKind::Local(ref l) = stmt.kind { 
        
                       self.check_attributes(l.hir_id, &l.attrs, &stmt.span, Target::Statement, None); 
        
                   } 
        
                   intravisit::walk_stmt(self, stmt) 
        
               } 
        
               fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) { 
        
                   let target = match expr.kind { 
        
                       hir::ExprKind::Closure(..) => Target::Closure, 
        
                       _ => Target::Expression, 
        
                   }; 
        
                   self.check_attributes(expr.hir_id, &expr.attrs, &expr.span, target, None); 
        
                   intravisit::walk_expr(self, expr) 
        
               } 
        
               fn visit_variant( 
        
                   &mut self, 
        
                   variant: &'tcx hir::Variant<'tcx>, 
        
                   generics: &'tcx hir::Generics<'tcx>, 
        
                   item_id: HirId, 
        
               ) { 
        
                   self.check_attributes(variant.id, variant.attrs, &variant.span, Target::Variant, None); 
        
                   intravisit::walk_variant(self, variant, generics, item_id) 
        
               } 
        
           }

A quick search (for attrs:) in the HIR definition finds these additional types that can contain attributes (they're also missing from the hir::Target enum, which should probably be renamed to AttrTarget but that's besides the point):

(click to see definition code snippets)

(summary: GenericParam, MacroDef, Arm, Param, StructField)

GenericParam

rust/compiler/rustc_hir/src/hir.rs

Lines 425 to 428 in e261649

    
           pub struct GenericParam<'hir> { 
        
               pub hir_id: HirId, 
        
               pub name: ParamName, 
        
               pub attrs: &'hir [Attribute],

MacroDef (is this handled elsewhere?)

rust/compiler/rustc_hir/src/hir.rs

Lines 722 to 725 in e261649

    
           pub struct MacroDef<'hir> { 
        
               pub ident: Ident, 
        
               pub vis: Visibility<'hir>, 
        
               pub attrs: &'hir [Attribute],

Arm

rust/compiler/rustc_hir/src/hir.rs

Lines 1147 to 1151 in e261649

    
           pub struct Arm<'hir> { 
        
               #[stable_hasher(ignore)] 
        
               pub hir_id: HirId, 
        
               pub span: Span, 
        
               pub attrs: &'hir [Attribute],

Param

rust/compiler/rustc_hir/src/hir.rs

Lines 2183 to 2184 in e261649

pub struct Param<'hir> {

pub attrs: &'hir [Attribute],

StructField

rust/compiler/rustc_hir/src/hir.rs

Lines 2390 to 2397 in e261649

    
           pub struct StructField<'hir> { 
        
               pub span: Span, 
        
               #[stable_hasher(project(name))] 
        
               pub ident: Ident, 
        
               pub vis: Visibility<'hir>, 
        
               pub hir_id: HirId, 
        
               pub ty: &'hir Ty<'hir>, 
        
               pub attrs: &'hir [Attribute],

This isn't necessary a bug for some of them, as there is special validation elsewhere, e.g.:

rust/compiler/rustc_ast_passes/src/ast_validation.rs

Lines 377 to 378 in e261649

    
                                   "allow, cfg, cfg_attr, deny, \ 
        
                           forbid, and warn are the only allowed built-in attributes in function parameters",

But it's not as uniform and future-proof as it could be. It might even be worth having a contextual attribute Target, and handling all attributes on visit_attr, or a hybrid solution that ICEs when visit_attr comes across an attribute not already handled.

And I did end up finding at least one bug, this compiles without any warnings (try on playground):

fn main() {
    match 5 {
        #[inline] _ => {}
    }
}

cc @varkor @davidtwco @petrochenkov

@rustbot

@rustbot claim

Rollup merge of rust-lang#80920 - rylev:check_attr-refactor, r=davidtwco

@oli-obk

After #80641 and #80920, what's left? cc @oli-obk

Looked over those PRs, yea, this is fixed now.

Navigation Menu

Repository navigation

check_attr doesn't check every HIR node type that can carry attributes. #80048

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	impl Visitor<'tcx> for CheckAttrVisitor<'tcx> {
	type Map = Map<'tcx>;

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

	fn visit_item(&mut self, item: &'tcx Item<'tcx>) {
	let target = Target::from_item(item);
	self.check_attributes(
	item.hir_id,
	item.attrs,
	&item.span,
	target,
	Some(ItemLike::Item(item)),
	);
	intravisit::walk_item(self, item)
	}

	fn visit_trait_item(&mut self, trait_item: &'tcx TraitItem<'tcx>) {
	let target = Target::from_trait_item(trait_item);
	self.check_attributes(trait_item.hir_id, &trait_item.attrs, &trait_item.span, target, None);
	intravisit::walk_trait_item(self, trait_item)
	}

	fn visit_foreign_item(&mut self, f_item: &'tcx ForeignItem<'tcx>) {
	let target = Target::from_foreign_item(f_item);
	self.check_attributes(
	f_item.hir_id,
	&f_item.attrs,
	&f_item.span,
	target,
	Some(ItemLike::ForeignItem(f_item)),
	);
	intravisit::walk_foreign_item(self, f_item)
	}

	fn visit_impl_item(&mut self, impl_item: &'tcx hir::ImplItem<'tcx>) {
	let target = target_from_impl_item(self.tcx, impl_item);
	self.check_attributes(impl_item.hir_id, &impl_item.attrs, &impl_item.span, target, None);
	intravisit::walk_impl_item(self, impl_item)
	}

	fn visit_stmt(&mut self, stmt: &'tcx hir::Stmt<'tcx>) {
	// When checking statements ignore expressions, they will be checked later.
	if let hir::StmtKind::Local(ref l) = stmt.kind {
	self.check_attributes(l.hir_id, &l.attrs, &stmt.span, Target::Statement, None);
	}
	intravisit::walk_stmt(self, stmt)
	}

	fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) {
	let target = match expr.kind {
	hir::ExprKind::Closure(..) => Target::Closure,
	_ => Target::Expression,
	};

	self.check_attributes(expr.hir_id, &expr.attrs, &expr.span, target, None);
	intravisit::walk_expr(self, expr)
	}

	fn visit_variant(
	&mut self,
	variant: &'tcx hir::Variant<'tcx>,
	generics: &'tcx hir::Generics<'tcx>,
	item_id: HirId,
	) {
	self.check_attributes(variant.id, variant.attrs, &variant.span, Target::Variant, None);
	intravisit::walk_variant(self, variant, generics, item_id)
	}
	}

	pub struct GenericParam<'hir> {
	pub hir_id: HirId,
	pub name: ParamName,
	pub attrs: &'hir [Attribute],

	pub struct MacroDef<'hir> {
	pub ident: Ident,
	pub vis: Visibility<'hir>,
	pub attrs: &'hir [Attribute],

	pub struct Arm<'hir> {
	#[stable_hasher(ignore)]
	pub hir_id: HirId,
	pub span: Span,
	pub attrs: &'hir [Attribute],

	pub struct StructField<'hir> {
	pub span: Span,
	#[stable_hasher(project(name))]
	pub ident: Ident,
	pub vis: Visibility<'hir>,
	pub hir_id: HirId,
	pub ty: &'hir Ty<'hir>,
	pub attrs: &'hir [Attribute],

	"allow, cfg, cfg_attr, deny, \
	forbid, and warn are the only allowed built-in attributes in function parameters",

check_attr doesn't check every HIR node type that can carry attributes. #80048

Description

Activity

rylev commented on Jan 7, 2021

eddyb commented on Mar 2, 2021

oli-obk commented on Mar 2, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions