Rollup merge of #66305 - elichai:2019-11-array_ffi, r=eddyb

Add by-value arrays to `improper_ctypes` lint

Hi,
C doesn't have a notion of passing arrays by value, only by reference/pointer.
Rust currently will pass it correctly by reference by it looks very misleading, and can confuse the borrow checker to think a move had occurred.

Fixes #58905 and fixes #24578.

We could also improve the borrow checker here but I think it's kinda a waste of work if we instead just tell the user it's an invalid FFI call.

(My first PR to `rustc` so if I missed some test or formatting guideline please tell me :) )

Author: tmandry

src/librustc_lint/types.rs

@@ -591,6 +591,23 @@ fn is_repr_nullable_ptr<'tcx>(
 }
 
 impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
+
+    /// 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 {
+            self.emit_ffi_unsafe_type_lint(
+                ty,
+                sp,
+                "passing raw arrays by value is not FFI-safe",
+                Some("consider passing a pointer to the array"),
+            );
+            true
+        } else {
+            false
+        }
+    }
+
+
     /// Checks if the given type is "ffi-safe" (has a stable, well-defined
     /// representation which can be exported to C code).
     fn check_type_for_ffi(&self,
@@ -825,7 +842,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             ty::RawPtr(ty::TypeAndMut { ty, .. }) |
             ty::Ref(_, ty, _) => self.check_type_for_ffi(cache, ty),
 
-            ty::Array(ty, _) => self.check_type_for_ffi(cache, ty),
+            ty::Array(inner_ty, _) => self.check_type_for_ffi(cache, inner_ty),
 
             ty::FnPtr(sig) => {
                 match sig.abi() {
@@ -937,7 +954,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         }
     }
 
-    fn check_type_for_ffi_and_report_errors(&mut self, sp: Span, ty: Ty<'tcx>) {
+    fn check_type_for_ffi_and_report_errors(&mut self, sp: Span, ty: Ty<'tcx>, is_static: bool) {
         // We have to check for opaque types before `normalize_erasing_regions`,
         // which will replace opaque types with their underlying concrete type.
         if self.check_for_opaque_ty(sp, ty) {
@@ -948,6 +965,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         // it is only OK to use this function because extern fns cannot have
         // any generic types right now:
         let ty = self.cx.tcx.normalize_erasing_regions(ParamEnv::reveal_all(), ty);
+        // C doesn't really support passing arrays by value.
+        // The only way to pass an array by value is through a struct.
+        // So we first test that the top level isn't an array,
+        // and then recursively check the types inside.
+        if !is_static && self.check_for_array_ty(sp, ty) {
+            return;
+        }
 
         match self.check_type_for_ffi(&mut FxHashSet::default(), ty) {
             FfiResult::FfiSafe => {}
@@ -966,21 +990,21 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         let sig = self.cx.tcx.erase_late_bound_regions(&sig);
 
         for (input_ty, input_hir) in sig.inputs().iter().zip(&decl.inputs) {
-            self.check_type_for_ffi_and_report_errors(input_hir.span, input_ty);
+            self.check_type_for_ffi_and_report_errors(input_hir.span, input_ty, false);
         }
 
         if let hir::Return(ref ret_hir) = decl.output {
             let ret_ty = sig.output();
             if !ret_ty.is_unit() {
-                self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty);
+                self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty, false);
             }
         }
     }
 
     fn check_foreign_static(&mut self, id: hir::HirId, span: Span) {
         let def_id = self.cx.tcx.hir().local_def_id(id);
         let ty = self.cx.tcx.type_of(def_id);
-        self.check_type_for_ffi_and_report_errors(span, ty);
+        self.check_type_for_ffi_and_report_errors(span, ty, true);
     }
 }
 

src/test/ui/lint/lint-ctypes.rs

@@ -65,6 +65,10 @@ extern {
     pub fn transparent_i128(p: TransparentI128); //~ ERROR: uses type `i128`
     pub fn transparent_str(p: TransparentStr); //~ ERROR: uses type `str`
     pub fn transparent_fn(p: TransparentBadFn); //~ ERROR: uses type `std::boxed::Box<u32>`
+    pub fn raw_array(arr: [u8; 8]); //~ ERROR: uses type `[u8; 8]`
+
+    pub static static_u128_type: u128; //~ ERROR: uses type `u128`
+    pub static static_u128_array_type: [u128; 16]; //~ ERROR: uses type `u128`
 
     pub fn good3(fptr: Option<extern fn()>);
     pub fn good4(aptr: &[u8; 4 as usize]);
@@ -83,6 +87,9 @@ extern {
     pub fn good17(p: TransparentCustomZst);
     #[allow(improper_ctypes)]
     pub fn good18(_: &String);
+    pub fn good20(arr: *const [u8; 8]);
+    pub static good21: [u8; 8];
+
 }
 
 #[allow(improper_ctypes)]

src/test/ui/lint/lint-ctypes.stderr

@@ -197,5 +197,30 @@ LL |     pub fn transparent_fn(p: TransparentBadFn);
    = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
    = note: this struct has unspecified layout
 
-error: aborting due to 20 previous errors
+error: `extern` block uses type `[u8; 8]`, which is not FFI-safe
+  --> $DIR/lint-ctypes.rs:68:27
+   |
+LL |     pub fn raw_array(arr: [u8; 8]);
+   |                           ^^^^^^^ not FFI-safe
+   |
+   = help: consider passing a pointer to the array
+   = note: passing raw arrays by value is not FFI-safe
+
+error: `extern` block uses type `u128`, which is not FFI-safe
+  --> $DIR/lint-ctypes.rs:70:34
+   |
+LL |     pub static static_u128_type: u128;
+   |                                  ^^^^ not FFI-safe
+   |
+   = note: 128-bit integers don't currently have a known stable ABI
+
+error: `extern` block uses type `u128`, which is not FFI-safe
+  --> $DIR/lint-ctypes.rs:71:40
+   |
+LL |     pub static static_u128_array_type: [u128; 16];
+   |                                        ^^^^^^^^^^ not FFI-safe
+   |
+   = note: 128-bit integers don't currently have a known stable ABI
+
+error: aborting due to 23 previous errors