Auto merge of #107257 - inquisitivecrystal:ffi-attr, r=davidtwco

Strengthen validation of FFI attributes

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

10 files changed, 147 insertions, 50 deletions

diff --git a/compiler/rustc_codegen_ssa/src/codegen_attrs.rs b/compiler/rustc_codegen_ssa/src/codegen_attrs.rs
index 87b819ebc98..9831e98fe37 100644
--- a/compiler/rustc_codegen_ssa/src/codegen_attrs.rs
+++ b/compiler/rustc_codegen_ssa/src/codegen_attrs.rs
@@ -85,55 +85,11 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: DefId) -> CodegenFnAttrs {
         } else if attr.has_name(sym::rustc_allocator) {
             codegen_fn_attrs.flags |= CodegenFnAttrFlags::ALLOCATOR;
         } else if attr.has_name(sym::ffi_returns_twice) {
-            if tcx.is_foreign_item(did) {
-                codegen_fn_attrs.flags |= CodegenFnAttrFlags::FFI_RETURNS_TWICE;
-            } else {
-                // `#[ffi_returns_twice]` is only allowed `extern fn`s.
-                struct_span_err!(
-                    tcx.sess,
-                    attr.span,
-                    E0724,
-                    "`#[ffi_returns_twice]` may only be used on foreign functions"
-                )
-                .emit();
-            }
+            codegen_fn_attrs.flags |= CodegenFnAttrFlags::FFI_RETURNS_TWICE;
         } else if attr.has_name(sym::ffi_pure) {
-            if tcx.is_foreign_item(did) {
-                if attrs.iter().any(|a| a.has_name(sym::ffi_const)) {
-                    // `#[ffi_const]` functions cannot be `#[ffi_pure]`
-                    struct_span_err!(
-                        tcx.sess,
-                        attr.span,
-                        E0757,
-                        "`#[ffi_const]` function cannot be `#[ffi_pure]`"
-                    )
-                    .emit();
-                } else {
-                    codegen_fn_attrs.flags |= CodegenFnAttrFlags::FFI_PURE;
-                }
-            } else {
-                // `#[ffi_pure]` is only allowed on foreign functions
-                struct_span_err!(
-                    tcx.sess,
-                    attr.span,
-                    E0755,
-                    "`#[ffi_pure]` may only be used on foreign functions"
-                )
-                .emit();
-            }
+            codegen_fn_attrs.flags |= CodegenFnAttrFlags::FFI_PURE;
         } else if attr.has_name(sym::ffi_const) {
-            if tcx.is_foreign_item(did) {
-                codegen_fn_attrs.flags |= CodegenFnAttrFlags::FFI_CONST;
-            } else {
-                // `#[ffi_const]` is only allowed on foreign functions
-                struct_span_err!(
-                    tcx.sess,
-                    attr.span,
-                    E0756,
-                    "`#[ffi_const]` may only be used on foreign functions"
-                )
-                .emit();
-            }
+            codegen_fn_attrs.flags |= CodegenFnAttrFlags::FFI_CONST;
         } else if attr.has_name(sym::rustc_nounwind) {
             codegen_fn_attrs.flags |= CodegenFnAttrFlags::NEVER_UNWIND;
         } else if attr.has_name(sym::rustc_reallocator) {
diff --git a/compiler/rustc_error_messages/locales/en-US/passes.ftl b/compiler/rustc_error_messages/locales/en-US/passes.ftl
index 6ebb188288f..0c7e02912d4 100644
--- a/compiler/rustc_error_messages/locales/en-US/passes.ftl
+++ b/compiler/rustc_error_messages/locales/en-US/passes.ftl
@@ -182,6 +182,18 @@ passes_has_incoherent_inherent_impl =
     `rustc_has_incoherent_inherent_impls` attribute should be applied to types or traits.
     .label = only adts, extern types and traits are supported
 
+passes_both_ffi_const_and_pure =
+    `#[ffi_const]` function cannot be `#[ffi_pure]`
+
+passes_ffi_pure_invalid_target =
+    `#[ffi_pure]` may only be used on foreign functions
+
+passes_ffi_const_invalid_target =
+    `#[ffi_const]` may only be used on foreign functions
+
+passes_ffi_returns_twice_invalid_target =
+    `#[ffi_returns_twice]` may only be used on foreign functions
+
 passes_must_use_async =
     `must_use` attribute on `async` functions applies to the anonymous `Future` returned by the function, not the value within
     .label = this attribute does nothing, the `Future`s returned by async functions are already `must_use`
diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs
index d67d52da497..644d701be0c 100644
--- a/compiler/rustc_passes/src/check_attr.rs
+++ b/compiler/rustc_passes/src/check_attr.rs
@@ -174,6 +174,9 @@ impl CheckAttrVisitor<'_> {
                 sym::rustc_has_incoherent_inherent_impls => {
                     self.check_has_incoherent_inherent_impls(&attr, span, target)
                 }
+                sym::ffi_pure => self.check_ffi_pure(attr.span, attrs, target),
+                sym::ffi_const => self.check_ffi_const(attr.span, target),
+                sym::ffi_returns_twice => self.check_ffi_returns_twice(attr.span, target),
                 sym::rustc_const_unstable
                 | sym::rustc_const_stable
                 | sym::unstable
@@ -1213,6 +1216,38 @@ impl CheckAttrVisitor<'_> {
         }
     }
 
+    fn check_ffi_pure(&self, attr_span: Span, attrs: &[Attribute], target: Target) -> bool {
+        if target != Target::ForeignFn {
+            self.tcx.sess.emit_err(errors::FfiPureInvalidTarget { attr_span });
+            return false;
+        }
+        if attrs.iter().any(|a| a.has_name(sym::ffi_const)) {
+            // `#[ffi_const]` functions cannot be `#[ffi_pure]`
+            self.tcx.sess.emit_err(errors::BothFfiConstAndPure { attr_span });
+            false
+        } else {
+            true
+        }
+    }
+
+    fn check_ffi_const(&self, attr_span: Span, target: Target) -> bool {
+        if target == Target::ForeignFn {
+            true
+        } else {
+            self.tcx.sess.emit_err(errors::FfiConstInvalidTarget { attr_span });
+            false
+        }
+    }
+
+    fn check_ffi_returns_twice(&self, attr_span: Span, target: Target) -> bool {
+        if target == Target::ForeignFn {
+            true
+        } else {
+            self.tcx.sess.emit_err(errors::FfiReturnsTwiceInvalidTarget { attr_span });
+            false
+        }
+    }
+
     /// Warns against some misuses of `#[must_use]`
     fn check_must_use(&self, hir_id: HirId, attr: &Attribute, target: Target) -> bool {
         if !matches!(
diff --git a/compiler/rustc_passes/src/errors.rs b/compiler/rustc_passes/src/errors.rs
index 97169a6367c..68b098e3457 100644
--- a/compiler/rustc_passes/src/errors.rs
+++ b/compiler/rustc_passes/src/errors.rs
@@ -348,6 +348,34 @@ pub struct HasIncoherentInherentImpl {
     pub span: Span,
 }
 
+#[derive(Diagnostic)]
+#[diag(passes_both_ffi_const_and_pure, code = "E0757")]
+pub struct BothFfiConstAndPure {
+    #[primary_span]
+    pub attr_span: Span,
+}
+
+#[derive(Diagnostic)]
+#[diag(passes_ffi_pure_invalid_target, code = "E0755")]
+pub struct FfiPureInvalidTarget {
+    #[primary_span]
+    pub attr_span: Span,
+}
+
+#[derive(Diagnostic)]
+#[diag(passes_ffi_const_invalid_target, code = "E0756")]
+pub struct FfiConstInvalidTarget {
+    #[primary_span]
+    pub attr_span: Span,
+}
+
+#[derive(Diagnostic)]
+#[diag(passes_ffi_returns_twice_invalid_target, code = "E0724")]
+pub struct FfiReturnsTwiceInvalidTarget {
+    #[primary_span]
+    pub attr_span: Span,
+}
+
 #[derive(LintDiagnostic)]
 #[diag(passes_must_use_async)]
 pub struct MustUseAsync {
diff --git a/tests/ui/ffi_const.rs b/tests/ui/ffi_const.rs
index 7aeb5a49a1b..aa20a4d4c65 100644
--- a/tests/ui/ffi_const.rs
+++ b/tests/ui/ffi_const.rs
@@ -3,3 +3,13 @@
 
 #[ffi_const] //~ ERROR `#[ffi_const]` may only be used on foreign functions
 pub fn foo() {}
+
+#[ffi_const] //~ ERROR `#[ffi_const]` may only be used on foreign functions
+macro_rules! bar {
+    () => ()
+}
+
+extern "C" {
+    #[ffi_const] //~ ERROR `#[ffi_const]` may only be used on foreign functions
+    static INT: i32;
+}
diff --git a/tests/ui/ffi_const.stderr b/tests/ui/ffi_const.stderr
index bc3c12eaf98..394b98f8971 100644
--- a/tests/ui/ffi_const.stderr
+++ b/tests/ui/ffi_const.stderr
@@ -4,6 +4,18 @@ error[E0756]: `#[ffi_const]` may only be used on foreign functions
 LL | #[ffi_const]
    | ^^^^^^^^^^^^
 
-error: aborting due to previous error
+error[E0756]: `#[ffi_const]` may only be used on foreign functions
+  --> $DIR/ffi_const.rs:7:1
+   |
+LL | #[ffi_const]
+   | ^^^^^^^^^^^^
+
+error[E0756]: `#[ffi_const]` may only be used on foreign functions
+  --> $DIR/ffi_const.rs:13:5
+   |
+LL |     #[ffi_const]
+   |     ^^^^^^^^^^^^
+
+error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0756`.
diff --git a/tests/ui/ffi_pure.rs b/tests/ui/ffi_pure.rs
index c37d34c8784..6d2f3a614ec 100644
--- a/tests/ui/ffi_pure.rs
+++ b/tests/ui/ffi_pure.rs
@@ -3,3 +3,13 @@
 
 #[ffi_pure] //~ ERROR `#[ffi_pure]` may only be used on foreign functions
 pub fn foo() {}
+
+#[ffi_pure] //~ ERROR `#[ffi_pure]` may only be used on foreign functions
+macro_rules! bar {
+    () => ()
+}
+
+extern "C" {
+    #[ffi_pure] //~ ERROR `#[ffi_pure]` may only be used on foreign functions
+    static INT: i32;
+}
diff --git a/tests/ui/ffi_pure.stderr b/tests/ui/ffi_pure.stderr
index bc911c85ddb..8b61a4b609f 100644
--- a/tests/ui/ffi_pure.stderr
+++ b/tests/ui/ffi_pure.stderr
@@ -4,6 +4,18 @@ error[E0755]: `#[ffi_pure]` may only be used on foreign functions
 LL | #[ffi_pure]
    | ^^^^^^^^^^^
 
-error: aborting due to previous error
+error[E0755]: `#[ffi_pure]` may only be used on foreign functions
+  --> $DIR/ffi_pure.rs:7:1
+   |
+LL | #[ffi_pure]
+   | ^^^^^^^^^^^
+
+error[E0755]: `#[ffi_pure]` may only be used on foreign functions
+  --> $DIR/ffi_pure.rs:13:5
+   |
+LL |     #[ffi_pure]
+   |     ^^^^^^^^^^^
+
+error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0755`.
diff --git a/tests/ui/ffi_returns_twice.rs b/tests/ui/ffi_returns_twice.rs
index 845e18df11b..8195d0e4863 100644
--- a/tests/ui/ffi_returns_twice.rs
+++ b/tests/ui/ffi_returns_twice.rs
@@ -3,3 +3,13 @@
 
 #[ffi_returns_twice] //~ ERROR `#[ffi_returns_twice]` may only be used on foreign functions
 pub fn foo() {}
+
+#[ffi_returns_twice] //~ ERROR `#[ffi_returns_twice]` may only be used on foreign functions
+macro_rules! bar {
+    () => ()
+}
+
+extern "C" {
+    #[ffi_returns_twice] //~ ERROR `#[ffi_returns_twice]` may only be used on foreign functions
+    static INT: i32;
+}
diff --git a/tests/ui/ffi_returns_twice.stderr b/tests/ui/ffi_returns_twice.stderr
index 2b7f5694f02..0abe7613f14 100644
--- a/tests/ui/ffi_returns_twice.stderr
+++ b/tests/ui/ffi_returns_twice.stderr
@@ -4,6 +4,18 @@ error[E0724]: `#[ffi_returns_twice]` may only be used on foreign functions
 LL | #[ffi_returns_twice]
    | ^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to previous error
+error[E0724]: `#[ffi_returns_twice]` may only be used on foreign functions
+  --> $DIR/ffi_returns_twice.rs:7:1
+   |
+LL | #[ffi_returns_twice]
+   | ^^^^^^^^^^^^^^^^^^^^
+
+error[E0724]: `#[ffi_returns_twice]` may only be used on foreign functions
+  --> $DIR/ffi_returns_twice.rs:13:5
+   |
+LL |     #[ffi_returns_twice]
+   |     ^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0724`.