Add allowed list check on EII implementations attributes

[chenyukang]

View all comments

Fixes #158293
Fixes #159015

r? @bjorn3

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

[mejrs]

Maybe we ought to have an allowlist of attributes rather than error on these particular attributes? I'm also a bit worried about someone later down the line adding some new (possibly link related) attribute and not including it in this check.

[chenyukang]

Maybe we ought to have an allowlist of attributes rather than error on these particular attributes? I'm also a bit worried about someone later down the line adding some new (possibly link related) attribute and not including it in this check.

maybe allowlist contains lint levels, cfg, doc, inline, cold, optimize, coverage、sanitize, must_use, deprecated?

[bjorn3]

Seems like a reasonable list.

[chenyukang]

Seems like a reasonable list.

I tried to get a more overwhole status on eii, this #125418 is the best place to track.
seems the current code is different with original rfc's syntax now.
so we are moving on the implementation and then draft another rfc in future?

[bjorn3]

I don't know if the current EII syntax is expected to be stabilized eventually or if the intent is for the syntax from the RFC to be implemented eventually with #[eii] just being a temporary thing while we work on the internal implementation.

[mejrs]

For reference, attributes on eii declarations are handled via allowlist:

rust/compiler/rustc_builtin_macros/src/eii.rs

Lines 180 to 228 in da80ed0

	fn split_attrs(
	ecx: &mut ExtCtxt<'_>,
	span: Span,
	attrs: ThinVec<Attribute>,
	) -> (ThinVec<Attribute>, ThinVec<Attribute>, ThinVec<Attribute>) {
	let mut macro_attributes = ThinVec::new();
	let mut foreign_item_attributes = ThinVec::new();
	let mut default_attributes = ThinVec::new();
	for attr in attrs {
	match attr.name() {
	// Inline only matters for the default function being inlined into callsites
	Some(sym::inline) => default_attributes.push(attr),
	// If an eii is marked a lang item, that's because we want to call its declaration, so
	// mark the foreign item as the lang item
	Some(sym::lang) => foreign_item_attributes.push(attr),
	// Deprecating an eii means deprecating the macro and the foreign item
	Some(sym::deprecated) => {
	foreign_item_attributes.push(attr.clone());
	macro_attributes.push(attr);
	}
	// The stability of an EII affects the usage of the macro and calling the foreign item
	Some(sym::stable) | Some(sym::unstable) => {
	foreign_item_attributes.push(attr.clone());
	macro_attributes.push(attr);
	}
	// `#[track_caller]` goes on the foreign item only: it's the symbol callers link
	// against, so it must carry the flag for call sites to pass the caller location.
	// Implementations derive it during codegen (see `EiiImpls` in `codegen_attrs.rs`),
	// so it must not be routed onto the default impl here.
	Some(sym::track_caller) => {
	foreign_item_attributes.push(attr);
	}
	// Doc attributes should be forwarded to the macro and the foreign item, since those are
	// the two items you interact with as a user.
	// FIXME: idk yet how EIIs show up in docs, might want to customize
	_ if attr.is_doc_comment() => {
	foreign_item_attributes.push(attr.clone());
	macro_attributes.push(attr);
	}
	Some(sym::eii) => unreachable!("should already be filtered out"),
	_ => {
	ecx.dcx().emit_err(EiiAttributeNotSupported { span, attr_span: attr.span() });
	}
	}
	}
	(macro_attributes, foreign_item_attributes, default_attributes)
	}

I would expect attributes on implementations to be handled similarly.

seems the current code is different with original rfc's syntax now.
so we are moving on the implementation and then draft another rfc in future?

It's an experiment so we can diverge from the original proposal. Once we are happy with the implementation the rfc should be updated accordingly.

[chenyukang]

I moved the checking to here in ast_validation, since macro expansion finished,
and leaves the final attributes, similar with
https://github.com/chenyukang/rust/blob/30cc6b46e8bb20318b05443cb80f1e50119ba971/compiler/rustc_ast_passes/src/ast_validation.rs#L522-L532

the old place where depends on HIR, HIR converts some attributes into variants without a uniform path or span.

View changes since the review

[JonathanBrouwer]

I think the current location is the best we can currently do.
Integrating this into the attribute parsing system somehow would be ideal, but I don't see a way to do this. Doing this after attribute parsing (like in the HIR) is not desirable since the spans are gone then.

[bjorn3]

I think someone more familiar with attribute parsing/validation should review this. I don't know the advantages and disadvantages of the various locations attributes are validated.

[mejrs]

Can you update a test to include #[naked] as well?

I think someone more familiar with attribute parsing/validation should review this. I don't know the advantages and disadvantages of the various locations attributes are validated.

I think it's ~fine to do it like this. We could have the eii impl macro leave behind a trace attribute so we can still do this check post-expansion and keep it in check_attr.rs. Not sure what the best course of action is. If a trace attribute is useful elsewhere, then maybe we should do that.

r? @JonathanBrouwer

[rustbot]

JonathanBrouwer is currently at their maximum review capacity.
They may take a while to respond.

[JonathanBrouwer]

(might take me a few days to get around to it, but happy to take a look)

[JonathanBrouwer]

r=me after solving conflicts and including #[naked] as mejrs asked

View changes since this review

[mejrs]

Do these two still do anything after #159266?

View changes since the review

[chenyukang]

no, we need to remove them.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.