Auto merge of rust-lang#113457 - davidtwco:lint-ctypes-issue-113436, r=oli-obk

lint/ctypes: fix `()` return type checks

Fixes rust-lang#113436.

`()` is normally FFI-unsafe, but is FFI-safe when used as a return type. It is also desirable that a transparent newtype for `()` is FFI-safe when used as a return type.

In order to support this, when a type was deemed FFI-unsafe, because of a `()` type, and was used in return type - then the type was considered  FFI-safe. However, this was the wrong approach - it didn't check that the `()` was part of a transparent newtype! The consequence of this is that the presence of a `()` type in a more complex return type would make it the entire type be considered safe (as long as the `()` type was the first that the lint found) - which is obviously incorrect.

Instead, this logic is removed, and after [consultation with t-lang](rust-lang#113436 (comment)), I've fixed the bugs and inconsistencies and  made `()` FFI-safe within types.

I also refactor a function, but that's not too exciting.

Author: bors

compiler/rustc_lint/messages.ftl

@@ -267,8 +267,6 @@ lint_improper_ctypes_char_help = consider using `u32` or `libc::wchar_t` instead
 lint_improper_ctypes_char_reason = the `char` type has no C equivalent
 lint_improper_ctypes_dyn = trait objects have no C equivalent
 
-lint_improper_ctypes_enum_phantomdata = this enum contains a PhantomData field
-
 lint_improper_ctypes_enum_repr_help =
     consider adding a `#[repr(C)]`, `#[repr(transparent)]`, or integer `#[repr(...)]` attribute to this enum
 

compiler/rustc_lint/src/types.rs

@@ -985,39 +985,43 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
     ) -> FfiResult<'tcx> {
         use FfiResult::*;
 
-        let transparent_safety = def.repr().transparent().then(|| {
-            // Can assume that at most one field is not a ZST, so only check
-            // that field's type for FFI-safety.
+        let transparent_with_all_zst_fields = if def.repr().transparent() {
             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,
+                }
+
+                false
             } else {
-                // All fields are ZSTs; this means that the type should behave
-                // like (), which is FFI-unsafe... except if all fields are PhantomData,
-                // which is tested for below
-                FfiUnsafe { ty, reason: fluent::lint_improper_ctypes_struct_zst, help: None }
+                // ..or have only ZST fields, which is FFI-unsafe (unless those fields are all
+                // `PhantomData`).
+                true
             }
-        });
-        // We can't completely trust repr(C) markings; make sure the fields are
-        // actually safe.
+        } else {
+            false
+        };
+
+        // We can't completely trust `repr(C)` markings, so make sure the fields are actually safe.
         let mut all_phantom = !variant.fields.is_empty();
         for field in &variant.fields {
-            match self.check_field_type_for_ffi(cache, &field, args) {
-                FfiSafe => {
-                    all_phantom = false;
-                }
-                FfiPhantom(..) if !def.repr().transparent() && def.is_enum() => {
-                    return FfiUnsafe {
-                        ty,
-                        reason: fluent::lint_improper_ctypes_enum_phantomdata,
-                        help: None,
-                    };
-                }
-                FfiPhantom(..) => {}
-                r => return transparent_safety.unwrap_or(r),
+            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,
             }
         }
 
-        if all_phantom { FfiPhantom(ty) } else { transparent_safety.unwrap_or(FfiSafe) }
+        if all_phantom {
+            FfiPhantom(ty)
+        } else if transparent_with_all_zst_fields {
+            FfiUnsafe { ty, reason: fluent::lint_improper_ctypes_struct_zst, help: None }
+        } else {
+            FfiSafe
+        }
     }
 
     /// Checks if the given type is "ffi-safe" (has a stable, well-defined
@@ -1220,25 +1224,19 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 }
 
                 let sig = tcx.erase_late_bound_regions(sig);
-                if !sig.output().is_unit() {
-                    let r = self.check_type_for_ffi(cache, sig.output());
-                    match r {
-                        FfiSafe => {}
-                        _ => {
-                            return r;
-                        }
-                    }
-                }
                 for arg in sig.inputs() {
-                    let r = self.check_type_for_ffi(cache, *arg);
-                    match r {
+                    match self.check_type_for_ffi(cache, *arg) {
                         FfiSafe => {}
-                        _ => {
-                            return r;
-                        }
+                        r => return r,
                     }
                 }
-                FfiSafe
+
+                let ret_ty = sig.output();
+                if ret_ty.is_unit() {
+                    return FfiSafe;
+                }
+
+                self.check_type_for_ffi(cache, ret_ty)
             }
 
             ty::Foreign(..) => FfiSafe,
@@ -1354,7 +1352,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         }
 
         // Don't report FFI errors for unit return types. This check exists here, and not in
-        // `check_foreign_fn` (where it would make more sense) so that normalization has definitely
+        // the caller (where it would make more sense) so that normalization has definitely
         // happened.
         if is_return_type && ty.is_unit() {
             return;
@@ -1370,9 +1368,6 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                     None,
                 );
             }
-            // If `ty` is a `repr(transparent)` newtype, and the non-zero-sized type is a generic
-            // argument, which after substitution, is `()`, then this branch can be hit.
-            FfiResult::FfiUnsafe { ty, .. } if is_return_type && ty.is_unit() => {}
             FfiResult::FfiUnsafe { ty, reason, help } => {
                 self.emit_ffi_unsafe_type_lint(ty, sp, reason, help);
             }

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

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
+

tests/ui/lint/lint-ctypes-113436.rs

@@ -0,0 +1,34 @@
+// check-pass
+#![deny(improper_ctypes_definitions)]
+
+#[repr(C)]
+pub struct Wrap<T>(T);
+
+#[repr(transparent)]
+pub struct TransparentWrap<T>(T);
+
+pub extern "C" fn f() -> Wrap<()> {
+    todo!()
+}
+
+const _: extern "C" fn() -> Wrap<()> = f;
+
+pub extern "C" fn ff() -> Wrap<Wrap<()>> {
+    todo!()
+}
+
+const _: extern "C" fn() -> Wrap<Wrap<()>> = ff;
+
+pub extern "C" fn g() -> TransparentWrap<()> {
+    todo!()
+}
+
+const _: extern "C" fn() -> TransparentWrap<()> = g;
+
+pub extern "C" fn gg() -> TransparentWrap<TransparentWrap<()>> {
+    todo!()
+}
+
+const _: extern "C" fn() -> TransparentWrap<TransparentWrap<()>> = gg;
+
+fn main() {}