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

Author: bors

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) {

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`

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!(

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 {

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;
+}

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`.

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;
+}

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`.

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;
+}

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`.