Skip to content

start properly testing attributes in positions #140948

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented May 12, 2025

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
Copy link
Collaborator

rustbot commented May 12, 2025

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 12, 2025
@jieyouxu
Copy link
Member

r? jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned wesleywiser May 12, 2025
@ehuss
Copy link
Contributor

ehuss commented May 12, 2025

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 jieyouxu added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label May 12, 2025
@jieyouxu
Copy link
Member

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

@jdonszelmann
Copy link
Contributor

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

@rustbot rustbot assigned jdonszelmann and unassigned jieyouxu May 12, 2025
@jieyouxu jieyouxu self-assigned this May 12, 2025
@mejrs
Copy link
Contributor Author

mejrs commented May 12, 2025

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 mejrs marked this pull request as draft May 13, 2025 09:35
@mejrs
Copy link
Contributor Author

mejrs commented May 13, 2025

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
Copy link
Member

jieyouxu commented May 15, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants