lint/ctypes: allow () within types

Consider `()` within types to be FFI-safe, and `()` to be FFI-safe as a return type (incl. when in a transparent newtype). Signed-off-by: David Wood <[email protected]>
rust-lang · Jul 19, 2023 · 24f90fd · 24f90fd
1 parent 99b1897
commit 24f90fd
Show file tree

Hide file tree

Showing 7 changed files with 87 additions and 83 deletions.
diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs
@@ -943,30 +943,6 @@ pub(crate) fn repr_nullable_ptr<'tcx>(
 }
 
 impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
-    // Returns `true` if `ty` is a `()`, or a `repr(transparent)` type whose only non-ZST field
-    // is a generic substituted for `()` - in either case, the type is FFI-safe when used as a
-    // return type.
-    pub fn is_unit_or_equivalent(&self, ty: Ty<'tcx>) -> bool {
-        if ty.is_unit() {
-            return true;
-        }
-
-        if let ty::Adt(def, substs) = ty.kind() && def.repr().transparent() {
-            return def.variants()
-                .iter()
-                .filter_map(|variant| transparent_newtype_field(self.cx.tcx, variant))
-                .all(|field| {
-                    let field_ty = field.ty(self.cx.tcx, substs);
-                    !field_ty.has_opaque_types() && {
-                        let field_ty = self.cx.tcx.normalize_erasing_regions(self.cx.param_env, field_ty);
-                        self.is_unit_or_equivalent(field_ty)
-                    }
-                });
-        }
-
-        false
-    }
-
     /// Check if the type is array and emit an unsafe type lint.
     fn check_for_array_ty(&mut self, sp: Span, ty: Ty<'tcx>) -> bool {
         if let ty::Array(..) = ty.kind() {
@@ -1010,14 +986,19 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         use FfiResult::*;
 
         let transparent_with_all_zst_fields = if def.repr().transparent() {
-            // Transparent newtypes have at most one non-ZST field which needs to be checked..
             if let Some(field) = transparent_newtype_field(self.cx.tcx, variant) {
-                return self.check_field_type_for_ffi(cache, field, args);
-            }
+                // Transparent newtypes have at most one non-ZST field which needs to be checked..
+                match self.check_field_type_for_ffi(cache, field, args) {
+                    FfiUnsafe { ty, .. } if ty.is_unit() => (),
+                    r => return r,
+                }
 
-            // ..or have only ZST fields, which is FFI-unsafe (unless those fields are all
-            // `PhantomData`).
-            true
+                false
+            } else {
+                // ..or have only ZST fields, which is FFI-unsafe (unless those fields are all
+                // `PhantomData`).
+                true
+            }
         } else {
             false
         };
@@ -1027,6 +1008,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         for field in &variant.fields {
             all_phantom &= match self.check_field_type_for_ffi(cache, &field, args) {
                 FfiSafe => false,
+                // `()` fields are FFI-safe!
+                FfiUnsafe { ty, .. } if ty.is_unit() => false,
                 FfiPhantom(..) => true,
                 r @ FfiUnsafe { .. } => return r,
             }
@@ -1249,7 +1232,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 }
 
                 let ret_ty = sig.output();
-                if self.is_unit_or_equivalent(ret_ty) {
+                if ret_ty.is_unit() {
                     return FfiSafe;
                 }
 
@@ -1374,7 +1357,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         // Don't report FFI errors for unit return types. This check exists here, and not in
         // the caller (where it would make more sense) so that normalization has definitely
         // happened.
-        if is_return_type && self.is_unit_or_equivalent(ty) {
+        if is_return_type && ty.is_unit() {
             return;
         }
 

diff --git a/tests/ui/lint/lint-ctypes-113436-1.rs b/tests/ui/lint/lint-ctypes-113436-1.rs
@@ -0,0 +1,28 @@
+#![deny(improper_ctypes_definitions)]
+
+#[repr(C)]
+pub struct Foo {
+    a: u8,
+    b: (),
+}
+
+extern "C" fn foo(x: Foo) -> Foo {
+    todo!()
+}
+
+struct NotSafe(u32);
+
+#[repr(C)]
+pub struct Bar {
+    a: u8,
+    b: (),
+    c: NotSafe,
+}
+
+extern "C" fn bar(x: Bar) -> Bar {
+    //~^ ERROR `extern` fn uses type `NotSafe`, which is not FFI-safe
+    //~^^ ERROR `extern` fn uses type `NotSafe`, which is not FFI-safe
+    todo!()
+}
+
+fn main() {}
diff --git a/tests/ui/lint/lint-ctypes-113436-1.stderr b/tests/ui/lint/lint-ctypes-113436-1.stderr
@@ -0,0 +1,35 @@
+error: `extern` fn uses type `NotSafe`, which is not FFI-safe
+  --> $DIR/lint-ctypes-113436-1.rs:22:22
+   |
+LL | extern "C" fn bar(x: Bar) -> Bar {
+   |                      ^^^ not FFI-safe
+   |
+   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
+   = note: this struct has unspecified layout
+note: the type is defined here
+  --> $DIR/lint-ctypes-113436-1.rs:13:1
+   |
+LL | struct NotSafe(u32);
+   | ^^^^^^^^^^^^^^
+note: the lint level is defined here
+  --> $DIR/lint-ctypes-113436-1.rs:1:9
+   |
+LL | #![deny(improper_ctypes_definitions)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: `extern` fn uses type `NotSafe`, which is not FFI-safe
+  --> $DIR/lint-ctypes-113436-1.rs:22:30
+   |
+LL | extern "C" fn bar(x: Bar) -> Bar {
+   |                              ^^^ not FFI-safe
+   |
+   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
+   = note: this struct has unspecified layout
+note: the type is defined here
+  --> $DIR/lint-ctypes-113436-1.rs:13:1
+   |
+LL | struct NotSafe(u32);
+   | ^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+
diff --git a/tests/ui/lint/lint-ctypes-113436.rs b/tests/ui/lint/lint-ctypes-113436.rs
@@ -1,3 +1,4 @@
+// check-pass
 #![deny(improper_ctypes_definitions)]
 
 #[repr(C)]
@@ -7,20 +8,16 @@ pub struct Wrap<T>(T);
 pub struct TransparentWrap<T>(T);
 
 pub extern "C" fn f() -> Wrap<()> {
-    //~^ ERROR `extern` fn uses type `()`, which is not FFI-safe
     todo!()
 }
 
 const _: extern "C" fn() -> Wrap<()> = f;
-//~^ ERROR `extern` fn uses type `()`, which is not FFI-safe
 
 pub extern "C" fn ff() -> Wrap<Wrap<()>> {
-    //~^ ERROR `extern` fn uses type `()`, which is not FFI-safe
     todo!()
 }
 
 const _: extern "C" fn() -> Wrap<Wrap<()>> = ff;
-//~^ ERROR `extern` fn uses type `()`, which is not FFI-safe
 
 pub extern "C" fn g() -> TransparentWrap<()> {
     todo!()

diff --git a/tests/ui/lint/lint-ctypes-113436.stderr b/tests/ui/lint/lint-ctypes-113436.stderr
diff --git a/tests/ui/repr/repr-transparent-issue-87496.rs b/tests/ui/repr/repr-transparent-issue-87496.rs
@@ -6,7 +6,7 @@
 struct TransparentCustomZst(());
 extern "C" {
     fn good17(p: TransparentCustomZst);
-    //~^ WARNING: `extern` block uses type `()`, which is not FFI-safe
+    //~^ WARNING: `extern` block uses type `TransparentCustomZst`, which is not FFI-safe
 }
 
 fn main() {}
diff --git a/tests/ui/repr/repr-transparent-issue-87496.stderr b/tests/ui/repr/repr-transparent-issue-87496.stderr
@@ -1,11 +1,15 @@
-warning: `extern` block uses type `()`, which is not FFI-safe
+warning: `extern` block uses type `TransparentCustomZst`, which is not FFI-safe
   --> $DIR/repr-transparent-issue-87496.rs:8:18
    |
 LL |     fn good17(p: TransparentCustomZst);
    |                  ^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
-   = help: consider using a struct instead
-   = note: tuples have unspecified layout
+   = note: this struct contains only zero-sized fields
+note: the type is defined here
+  --> $DIR/repr-transparent-issue-87496.rs:6:1
+   |
+LL | struct TransparentCustomZst(());
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: `#[warn(improper_ctypes)]` on by default
 
 warning: 1 warning emitted