Strengthen validation of FFI attributes

[inquisitivecrystal]

Previously, codegen_attrs validated the attributes #[ffi_pure], #[ffi_const], and #[ffi_returns_twice] to make sure that they were only used on foreign functions. However, this validation was insufficient in two ways:

1. codegen_attrs only sees items for which code must be generated, so it was unable to raise errors when the attribute was incorrectly applied to macros and the like.
2. the validation code only checked that the item with the attr was foreign, but not that it was a foreign function, allowing these attributes to be applied to foreign statics as well.

This PR moves the validation to check_attr, which sees all items. It additionally changes the validation to ensure that the attribute's target is Target::ForeignFunction, only allowing the attributes on foreign functions and not foreign statics. Because these attributes are unstable, there is no risk for backwards compatibility. The changes also ending up making the code much easier to read.

This PR is best reviewed commit by commit. Additionally, I was considering moving the tests to the attribute subdirectory, to get them out of the general UI directory. I could do that as part of this PR or a follow-up, as the reviewer prefers.

CC: #58328, #58329

[rustbot]

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

[bryangarza]

Not an official reviewer, but I looked through the commits and it looks good to me.

Additionally, I was considering moving the tests to the attribute subdirectory, to get them out of the general UI directory. I could do that as part of this PR or a follow-up, as the reviewer prefers.

I think it'd be better if you move them into the attribute dir as part of this PR so if someone is looking at the commit for the tests, they can git diff that commit hash and get the rest of the code changes too

[davidtwco]

LGTM, but this is technically a breaking change, so I think we'll want to do a crater run to see how safe this is to land.

[davidtwco]

r? @davidtwco

[davidtwco]

@bors try

[bors]

⌛ Trying commit 0983ac8b4283fec01e83fe3d41cf3add0597691a with merge cc7a22a8fa04e430abcb30a0b627b66011dd0ec3...

[inquisitivecrystal]

LGTM, but this is technically a breaking change, so I think we'll want to do a crater run to see how safe this is to land.

All these attributes are still unstable, so it shouldn't be a problem? Unless we're worried about breaking nightly code.

[davidtwco]

LGTM, but this is technically a breaking change, so I think we'll want to do a crater run to see how safe this is to land.

All these attributes are still unstable, so it shouldn't be a problem? Unless we're worried about breaking nightly code.

Oh, I should have checked that, great - that's much simpler.

[davidtwco]

@bors r+

[bors]

📌 Commit 0983ac8b4283fec01e83fe3d41cf3add0597691a has been approved by davidtwco

It is now in the queue for this repository.

[inquisitivecrystal]

Oh, I should have checked that, great - that's much simpler.

I did mention it in the OP, but I don't blame you for missing it - I'm not the most concise. Thanks for the review! ❤️

[bors]

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing cc7a22a8fa04e430abcb30a0b627b66011dd0ec3 to master...

[bors]

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

[compiler-errors]

For future reference, do not approve a PR when a try-build is running, or else it will try to be merged into master 😬

[davidtwco]

For future reference, do not approve a PR when a try-build is running, or else it will try to be merged into master 😬

Oh, oops, hadn't done that before. My bad.

[inquisitivecrystal]

I'm going to force push and then close and reopen to reset things (at least, those are the steps I've seen before; not sure which of the two is required, but doing both can't hurt).

[inquisitivecrystal]

I think if someone approves again we should be set? The change to the last commit only affected the commit message.

[rust-timer]

Finished benchmarking commit (cc7a22a8fa04e430abcb30a0b627b66011dd0ec3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.3%, 3.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

[inquisitivecrystal]

@davidtwco: could you r+ this again?

[davidtwco]

@bors r+

[bors]

📌 Commit bc23e9a has been approved by davidtwco

It is now in the queue for this repository.

[bors]

⌛ Testing commit bc23e9a with merge 11d96b5...

[bors]

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 11d96b5 to master...

[rust-timer]

Finished benchmarking commit (11d96b5): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.1%, -3.0%]	2
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Cycles

This benchmark run did not return any relevant results for this metric.