lint/ctypes: stricter () return type checks

`()` 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 an 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 a unit return type or a transparent
wrapper around a unit is checked for directly for functions and fn-ptrs.

Signed-off-by: David Wood <david@davidtw.co>

Author: davidtwco

compiler/rustc_lint/src/types.rs

@@ -943,6 +943,30 @@ 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() {
@@ -1220,25 +1244,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 self.is_unit_or_equivalent(ret_ty) {
+                    return FfiSafe;
+                }
+
+                self.check_type_for_ffi(cache, ret_ty)
             }
 
             ty::Foreign(..) => FfiSafe,
@@ -1357,9 +1375,9 @@ 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() {
+        if is_return_type && self.is_unit_or_equivalent(ty) {
             return;
         }
 
@@ -1373,9 +1391,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.rs

@@ -0,0 +1,37 @@
+#![deny(improper_ctypes_definitions)]
+
+#[repr(C)]
+pub struct Wrap<T>(T);
+
+#[repr(transparent)]
+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!()
+}
+
+const _: extern "C" fn() -> TransparentWrap<()> = g;
+
+pub extern "C" fn gg() -> TransparentWrap<TransparentWrap<()>> {
+    todo!()
+}
+
+const _: extern "C" fn() -> TransparentWrap<TransparentWrap<()>> = gg;
+
+fn main() {}

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

@@ -0,0 +1,43 @@
+error: `extern` fn uses type `()`, which is not FFI-safe
+  --> $DIR/lint-ctypes-113436.rs:9:26
+   |
+LL | pub extern "C" fn f() -> Wrap<()> {
+   |                          ^^^^^^^^ not FFI-safe
+   |
+   = help: consider using a struct instead
+   = note: tuples have unspecified layout
+note: the lint level is defined here
+  --> $DIR/lint-ctypes-113436.rs:1:9
+   |
+LL | #![deny(improper_ctypes_definitions)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: `extern` fn uses type `()`, which is not FFI-safe
+  --> $DIR/lint-ctypes-113436.rs:14:10
+   |
+LL | const _: extern "C" fn() -> Wrap<()> = f;
+   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
+   |
+   = help: consider using a struct instead
+   = note: tuples have unspecified layout
+
+error: `extern` fn uses type `()`, which is not FFI-safe
+  --> $DIR/lint-ctypes-113436.rs:17:27
+   |
+LL | pub extern "C" fn ff() -> Wrap<Wrap<()>> {
+   |                           ^^^^^^^^^^^^^^ not FFI-safe
+   |
+   = help: consider using a struct instead
+   = note: tuples have unspecified layout
+
+error: `extern` fn uses type `()`, which is not FFI-safe
+  --> $DIR/lint-ctypes-113436.rs:22:10
+   |
+LL | const _: extern "C" fn() -> Wrap<Wrap<()>> = ff;
+   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
+   |
+   = help: consider using a struct instead
+   = note: tuples have unspecified layout
+
+error: aborting due to 4 previous errors
+