start properly testing attributes in positions

[mejrs]

See the readme for the motivation. Just doing three attributes for now to get feedback on the overall implementation and direction.

Please let me know if there are additional positions that need testing!

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[jieyouxu]

r? jieyouxu

[ehuss]

Oh, this is great! I was just recently working on this, but happy to have someone actually make it happen.

I look forward to this being accepted. Let me know if you want any help.

I think there are a few tests that could eventually get deleted once everything gets covered. For example:

• ui/feature-gates/issue-43106-gating-of-builtin-attrs.rs -- This is a bit of a mess
• ui/issues/issue-54044.rs
• ui/issues/issue-78957.rs
• ui/lint/unused/unused_attributes-must_use.rs
• Probably others...

Some suggestions:

• Inner attributes on outline modules sometimes go through a different code path. I'm wondering if it would be helpful to include that? Unfortunately that could double the number of files.
• Some others that seem to be missing:
  • Inner attributes:
    • Trait definition
    • Const blocks (which are handled differently than regular blocks)
    • match block
    • Unstable things:
      • gen block
      • try block
  • Outer attributes:
    • Closure arguments (seems to be included in allow.rs though?)
    • Struct pattern fields
    • type: bare function parameter
    • type: bare function ...

[jieyouxu]

cc @jdonszelmann (as you may want to co-review)

[jdonszelmann]

Yes, I'd like that. might take a little because of rust week @rustbot claim

[mejrs]

I think we also ought to test combinations of attributes. Such as

#[ATTRIBUTE]
#[naked]
extern "C" fn naked(){}

Are there more like that?

Closure arguments (seems to be included in allow.rs though?)

Just added another commit with these. I originally had them..not sure where they went...

[mejrs]

Just these couple of edits have already been quite annoying and error prone. Doing this for dozens of attributes is going to be untenable in the long run.

This needs to be automatically generated, and I have some ideas about how to do that.

[jieyouxu]

Thoughts on testing attributes in positions

Preliminary: attributes

cf. the Reference on attributes.

We have several kinds of attributes in terms of name resolution behavior (roughly):

• Built-in attributes
• proc-macro attributes
• derive macro helper attributes
• tool attributes

Syntactically, attributes can also be categorized into:

• Inner attributes #![...]
  • In which a notable case is the crate-level inner attribute
• Outer attributes #[...]

Covering built-in attributes: caveats and challenges

Now, I imagine the test coverage proposed in #140948 (this PR) is primarily about built-in attributes, like #[cfg(..)] or #![allow(..)]. Built-in attributes are also quite different between each other!

One of the places where "this (built-in) attribute can be applied to this position" is checked is the CheckAttrVisitor. This has the signature

/// Checks any attribute.
fn check_attributes(
    &self,
    hir_id: HirId,
    span: Span,
    target: Target,
    item: Option<ItemLike<'_>>,
) {

where Target coarsely categorizes the "target" positions that attributes can be applied to. If we look at this, we realize several issues re. designing test coverage for builtin-attributes:

1. Certain built-in attributes can be namespaced, notably #[diagnostic::{do_not_recommend,on_unimplemented}] and possibly more.
   • This is extra complicated, because namespaced attributes might be applicable to different sets of positions, even under the same namespace.
2. Different built-in attributes can be applied to different sets of positions. However, certain built-in attributes have "refinements" to the Targets. So it's not trivial to "just" partition built-in attributes by what Targets they can be applied to.
3. Built-in attributes can also be mutually exclusive against reach other, or for specific values of each other. Exercising combinations of attributes can be... challenging.
4. Quite a few built-in attributes also have special cases where they are Technically Permitted Now in weird positions but should never have been accepted (typically causes unused_attributes lints).
5. Currently, the granularity of which attribute position validation is performed is roughly at the statement+ level. However, there does indeed exist the stmt_expr_attributes unstable feature which would permit attributes on expressions (should the attribute be appliable to said positon, ofc.).
6. Even more complicated, is for some attributes you need to account for the edition. For instance, unsafe built-in attributes like #[no_mangle].
7. Feature and stability gating.

[jdonszelmann]

though combinations of attributes are an important thing to test, it seems infeasible to do so. That starts to feel like fuzzing. I also feel like making such a file for every single attribute is a lot of work. Is there some way we could automate part of it? Now that we have a literal list of attribute parsers it might be feasible to effectively find them all and test for each of them that they're valid in all these positions automatically.

Related to that, the way parsers are designed now, when two attributes care about each other's presence, they're supposed to list all the ones they care about in their accept function. It's entirely legal for multiple parsers to care about the same attribute symbols simply to reject the combination. That'd mean we also have a reasonable heuristic for which attributes to check together. Take a look at the stability parser which accepts both stable, unstable, and rustc_stable_through_unstable_modules in one parser.

rust/compiler/rustc_attr_parsing/src/attributes/stability.rs

Lines 46 to 65 in 573a015

	(&[sym::stable], |this, cx, args| {
	reject_outside_std!(cx);
	if !this.check_duplicate(cx)
	&& let Some((feature, level)) = parse_stability(cx, args)
	{
	this.stability = Some((Stability { level, feature }, cx.attr_span));
	}
	}),
	(&[sym::unstable], |this, cx, args| {
	reject_outside_std!(cx);
	if !this.check_duplicate(cx)
	&& let Some((feature, level)) = parse_unstability(cx, args)
	{
	this.stability = Some((Stability { level, feature }, cx.attr_span));
	}
	}),
	(&[sym::rustc_allowed_through_unstable_modules], |this, cx, args| {
	reject_outside_std!(cx);
	this.allowed_through_unstable_modules = args.name_value().and_then(|i| i.value_as_str())
	}),

I'll make a draft for a #[naked] parser to show more of what I mean tomorrow because there this is quite relevant. The rough idea would be to list all the ones in the denylist in the parser, maybe with a macro, to emit a warning once naked is also found