From a8640cdf47b5424a1a194551f3e873e8d2726cb4 Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 1 Jun 2020 15:34:45 +0100 Subject: [PATCH 1/3] improper_ctypes: add test for #66202 This commit adds a test of the improper ctypes lint, checking that return type are normalized bethat return types are normalized before being checked for FFI-safety, and that transparent newtype wrappers are FFI-safe if the type being wrapped is FFI-safe. Signed-off-by: David Wood --- src/test/ui/lint/lint-ctypes-66202.rs | 17 +++++++++++++ src/test/ui/lint/lint-ctypes-66202.stderr | 29 +++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 src/test/ui/lint/lint-ctypes-66202.rs create mode 100644 src/test/ui/lint/lint-ctypes-66202.stderr diff --git a/src/test/ui/lint/lint-ctypes-66202.rs b/src/test/ui/lint/lint-ctypes-66202.rs new file mode 100644 index 0000000000000..df8170d8d968c --- /dev/null +++ b/src/test/ui/lint/lint-ctypes-66202.rs @@ -0,0 +1,17 @@ +#![deny(improper_ctypes)] + +// This test checks that return types are normalized before being checked for FFI-safety, and that +// transparent newtype wrappers are FFI-safe if the type being wrapped is FFI-safe. + +#[repr(transparent)] +pub struct W(T); + +extern "C" { + pub fn bare() -> (); + pub fn normalize() -> <() as ToOwned>::Owned; + //~^ ERROR uses type `()` + pub fn transparent() -> W<()>; + //~^ ERROR uses type `W<()>` +} + +fn main() {} diff --git a/src/test/ui/lint/lint-ctypes-66202.stderr b/src/test/ui/lint/lint-ctypes-66202.stderr new file mode 100644 index 0000000000000..3268d4cefbf4d --- /dev/null +++ b/src/test/ui/lint/lint-ctypes-66202.stderr @@ -0,0 +1,29 @@ +error: `extern` block uses type `()`, which is not FFI-safe + --> $DIR/lint-ctypes-66202.rs:11:27 + | +LL | pub fn normalize() -> <() as ToOwned>::Owned; + | ^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe + | +note: the lint level is defined here + --> $DIR/lint-ctypes-66202.rs:1:9 + | +LL | #![deny(improper_ctypes)] + | ^^^^^^^^^^^^^^^ + = help: consider using a struct instead + = note: tuples have unspecified layout + +error: `extern` block uses type `W<()>`, which is not FFI-safe + --> $DIR/lint-ctypes-66202.rs:13:29 + | +LL | pub fn transparent() -> W<()>; + | ^^^^^ not FFI-safe + | + = note: composed only of `PhantomData` +note: the type is defined here + --> $DIR/lint-ctypes-66202.rs:7:1 + | +LL | pub struct W(T); + | ^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors + From 3e7aabb1b3ec9ad66c7a306cd956e880a1a51483 Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 1 Jun 2020 15:41:36 +0100 Subject: [PATCH 2/3] lint: check for unit ret type after normalization This commit moves the check that skips unit return types to after where the return type has been normalized - therefore ensuring that FFI-safety lints are not emitted for types which normalize to unit. Signed-off-by: David Wood --- src/librustc_lint/types.rs | 31 +++++++++++++++-------- src/test/ui/lint/lint-ctypes-66202.rs | 1 - src/test/ui/lint/lint-ctypes-66202.stderr | 19 ++++---------- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index 703c2a7a443a9..b60867b3d2d45 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -946,7 +946,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { } } - fn check_type_for_ffi_and_report_errors(&mut self, sp: Span, ty: Ty<'tcx>, is_static: bool) { + fn check_type_for_ffi_and_report_errors( + &mut self, + sp: Span, + ty: Ty<'tcx>, + is_static: bool, + is_return_type: 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) { @@ -957,14 +963,21 @@ 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. + + // C doesn't really support passing arrays by value - the only way to pass an array by value + // is through a struct. So, 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; } + // 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 + // happened. + if is_return_type && ty.is_unit() { + return; + } + match self.check_type_for_ffi(&mut FxHashSet::default(), ty) { FfiResult::FfiSafe => {} FfiResult::FfiPhantom(ty) => { @@ -982,21 +995,19 @@ 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, false); + self.check_type_for_ffi_and_report_errors(input_hir.span, input_ty, false, false); } if let hir::FnRetTy::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, false); - } + self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty, false, true); } } 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, true); + self.check_type_for_ffi_and_report_errors(span, ty, true, false); } } diff --git a/src/test/ui/lint/lint-ctypes-66202.rs b/src/test/ui/lint/lint-ctypes-66202.rs index df8170d8d968c..3fe4560f44bcc 100644 --- a/src/test/ui/lint/lint-ctypes-66202.rs +++ b/src/test/ui/lint/lint-ctypes-66202.rs @@ -9,7 +9,6 @@ pub struct W(T); extern "C" { pub fn bare() -> (); pub fn normalize() -> <() as ToOwned>::Owned; - //~^ ERROR uses type `()` pub fn transparent() -> W<()>; //~^ ERROR uses type `W<()>` } diff --git a/src/test/ui/lint/lint-ctypes-66202.stderr b/src/test/ui/lint/lint-ctypes-66202.stderr index 3268d4cefbf4d..759c77deadc7b 100644 --- a/src/test/ui/lint/lint-ctypes-66202.stderr +++ b/src/test/ui/lint/lint-ctypes-66202.stderr @@ -1,23 +1,14 @@ -error: `extern` block uses type `()`, which is not FFI-safe - --> $DIR/lint-ctypes-66202.rs:11:27 +error: `extern` block uses type `W<()>`, which is not FFI-safe + --> $DIR/lint-ctypes-66202.rs:12:29 | -LL | pub fn normalize() -> <() as ToOwned>::Owned; - | ^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe +LL | pub fn transparent() -> W<()>; + | ^^^^^ not FFI-safe | note: the lint level is defined here --> $DIR/lint-ctypes-66202.rs:1:9 | LL | #![deny(improper_ctypes)] | ^^^^^^^^^^^^^^^ - = help: consider using a struct instead - = note: tuples have unspecified layout - -error: `extern` block uses type `W<()>`, which is not FFI-safe - --> $DIR/lint-ctypes-66202.rs:13:29 - | -LL | pub fn transparent() -> W<()>; - | ^^^^^ not FFI-safe - | = note: composed only of `PhantomData` note: the type is defined here --> $DIR/lint-ctypes-66202.rs:7:1 @@ -25,5 +16,5 @@ note: the type is defined here LL | pub struct W(T); | ^^^^^^^^^^^^^^^^^^^ -error: aborting due to 2 previous errors +error: aborting due to previous error From d4d3d7de68d331829b5dcd08be3d4aec1a7a7f2b Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 1 Jun 2020 17:00:58 +0100 Subject: [PATCH 3/3] lint: transitive FFI-safety for transparent types This commit ensures that if a `repr(transparent)` newtype's only non-zero-sized field is FFI-safe then the newtype is also FFI-safe. Previously, ZSTs were ignored for the purposes of linting FFI-safety in transparent structs - thus, only the single non-ZST would be checked for FFI-safety. However, if the non-zero-sized field is a generic parameter, and is substituted for a ZST, then the type would be considered FFI-unsafe (as when every field is thought to be zero-sized, the type is considered to be "composed only of `PhantomData`" which is FFI-unsafe). In this commit, for transparent structs, the non-zero-sized field is identified (before any substitutions are applied, necessarily) and then that field's type (now with substitutions) is checked for FFI-safety (where previously it would have been skipped for being zero-sized in this case). To handle the case where the non-zero-sized field is a generic parameter, which is substituted for `()` (a ZST), and is being used as a return type - the `FfiUnsafe` result (previously `FfiPhantom`) is caught and silenced. Signed-off-by: David Wood --- src/librustc_lint/types.rs | 69 +++++++++++++---------- src/librustc_middle/ty/mod.rs | 23 ++++++++ src/librustc_middle/ty/sty.rs | 5 ++ src/test/ui/lint/lint-ctypes-66202.rs | 3 +- src/test/ui/lint/lint-ctypes-66202.stderr | 20 ------- 5 files changed, 69 insertions(+), 51 deletions(-) delete mode 100644 src/test/ui/lint/lint-ctypes-66202.stderr diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index b60867b3d2d45..cdb0eda645a48 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -6,7 +6,6 @@ use rustc_attr as attr; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::def_id::DefId; use rustc_hir::{is_range_literal, ExprKind, Node}; use rustc_index::vec::Idx; use rustc_middle::mir::interpret::{sign_extend, truncate}; @@ -511,10 +510,6 @@ enum FfiResult<'tcx> { FfiUnsafe { ty: Ty<'tcx>, reason: &'static str, help: Option<&'static str> }, } -fn is_zst<'tcx>(tcx: TyCtxt<'tcx>, did: DefId, ty: Ty<'tcx>) -> bool { - tcx.layout_of(tcx.param_env(did).and(ty)).map(|layout| layout.is_zst()).unwrap_or(false) -} - fn ty_is_known_nonnull<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool { match ty.kind { ty::FnPtr(_) => true, @@ -523,7 +518,7 @@ fn ty_is_known_nonnull<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool { for field in field_def.all_fields() { let field_ty = tcx.normalize_erasing_regions(ParamEnv::reveal_all(), field.ty(tcx, substs)); - if is_zst(tcx, field.did, field_ty) { + if field_ty.is_zst(tcx, field.did) { continue; } @@ -653,32 +648,43 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { }; } - // We can't completely trust repr(C) and repr(transparent) markings; - // make sure the fields are actually safe. - let mut all_phantom = true; - for field in &def.non_enum_variant().fields { - let field_ty = cx.normalize_erasing_regions( - ParamEnv::reveal_all(), - field.ty(cx, substs), - ); - // repr(transparent) types are allowed to have arbitrary ZSTs, not just - // PhantomData -- skip checking all ZST fields - if def.repr.transparent() && is_zst(cx, field.did, field_ty) { - continue; + if def.repr.transparent() { + // Can assume that only one field is not a ZST, so only check + // that field's type for FFI-safety. + if let Some(field) = + def.transparent_newtype_field(cx, self.cx.param_env) + { + let field_ty = cx.normalize_erasing_regions( + self.cx.param_env, + field.ty(cx, substs), + ); + self.check_type_for_ffi(cache, field_ty) + } else { + FfiSafe } - let r = self.check_type_for_ffi(cache, field_ty); - match r { - FfiSafe => { - all_phantom = false; - } - FfiPhantom(..) => {} - FfiUnsafe { .. } => { - return r; + } else { + // We can't completely trust repr(C) markings; make sure the fields are + // actually safe. + let mut all_phantom = true; + for field in &def.non_enum_variant().fields { + let field_ty = cx.normalize_erasing_regions( + self.cx.param_env, + field.ty(cx, substs), + ); + let r = self.check_type_for_ffi(cache, field_ty); + match r { + FfiSafe => { + all_phantom = false; + } + FfiPhantom(..) => {} + FfiUnsafe { .. } => { + return r; + } } } - } - if all_phantom { FfiPhantom(ty) } else { FfiSafe } + if all_phantom { FfiPhantom(ty) } else { FfiSafe } + } } AdtKind::Union => { if !def.repr.c() && !def.repr.transparent() { @@ -708,7 +714,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { ); // repr(transparent) types are allowed to have arbitrary ZSTs, not just // PhantomData -- skip checking all ZST fields. - if def.repr.transparent() && is_zst(cx, field.did, field_ty) { + if def.repr.transparent() && field_ty.is_zst(cx, field.did) { continue; } let r = self.check_type_for_ffi(cache, field_ty); @@ -774,7 +780,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { ); // repr(transparent) types are allowed to have arbitrary ZSTs, not // just PhantomData -- skip checking all ZST fields. - if def.repr.transparent() && is_zst(cx, field.did, field_ty) { + if def.repr.transparent() && field_ty.is_zst(cx, field.did) { continue; } let r = self.check_type_for_ffi(cache, field_ty); @@ -983,6 +989,9 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { FfiResult::FfiPhantom(ty) => { self.emit_ffi_unsafe_type_lint(ty, sp, "composed only of `PhantomData`", 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() => return, FfiResult::FfiUnsafe { ty, reason, help } => { self.emit_ffi_unsafe_type_lint(ty, sp, reason, help); } diff --git a/src/librustc_middle/ty/mod.rs b/src/librustc_middle/ty/mod.rs index ffbe3a40297c1..caa1b4cb375fe 100644 --- a/src/librustc_middle/ty/mod.rs +++ b/src/librustc_middle/ty/mod.rs @@ -2390,6 +2390,29 @@ impl<'tcx> AdtDef { pub fn sized_constraint(&self, tcx: TyCtxt<'tcx>) -> &'tcx [Ty<'tcx>] { tcx.adt_sized_constraint(self.did).0 } + + /// `repr(transparent)` structs can have a single non-ZST field, this function returns that + /// field. + pub fn transparent_newtype_field( + &self, + tcx: TyCtxt<'tcx>, + param_env: ParamEnv<'tcx>, + ) -> Option<&FieldDef> { + assert!(self.is_struct() && self.repr.transparent()); + + for field in &self.non_enum_variant().fields { + let field_ty = tcx.normalize_erasing_regions( + param_env, + field.ty(tcx, InternalSubsts::identity_for_item(tcx, self.did)), + ); + + if !field_ty.is_zst(tcx, self.did) { + return Some(field); + } + } + + None + } } impl<'tcx> FieldDef { diff --git a/src/librustc_middle/ty/sty.rs b/src/librustc_middle/ty/sty.rs index 5d4c2a54267c3..7550be39d4ab0 100644 --- a/src/librustc_middle/ty/sty.rs +++ b/src/librustc_middle/ty/sty.rs @@ -2186,6 +2186,11 @@ impl<'tcx> TyS<'tcx> { } } } + + /// Is this a zero-sized type? + pub fn is_zst(&'tcx self, tcx: TyCtxt<'tcx>, did: DefId) -> bool { + tcx.layout_of(tcx.param_env(did).and(self)).map(|layout| layout.is_zst()).unwrap_or(false) + } } /// Typed constant value. diff --git a/src/test/ui/lint/lint-ctypes-66202.rs b/src/test/ui/lint/lint-ctypes-66202.rs index 3fe4560f44bcc..ebab41d143e67 100644 --- a/src/test/ui/lint/lint-ctypes-66202.rs +++ b/src/test/ui/lint/lint-ctypes-66202.rs @@ -1,3 +1,5 @@ +// check-pass + #![deny(improper_ctypes)] // This test checks that return types are normalized before being checked for FFI-safety, and that @@ -10,7 +12,6 @@ extern "C" { pub fn bare() -> (); pub fn normalize() -> <() as ToOwned>::Owned; pub fn transparent() -> W<()>; - //~^ ERROR uses type `W<()>` } fn main() {} diff --git a/src/test/ui/lint/lint-ctypes-66202.stderr b/src/test/ui/lint/lint-ctypes-66202.stderr deleted file mode 100644 index 759c77deadc7b..0000000000000 --- a/src/test/ui/lint/lint-ctypes-66202.stderr +++ /dev/null @@ -1,20 +0,0 @@ -error: `extern` block uses type `W<()>`, which is not FFI-safe - --> $DIR/lint-ctypes-66202.rs:12:29 - | -LL | pub fn transparent() -> W<()>; - | ^^^^^ not FFI-safe - | -note: the lint level is defined here - --> $DIR/lint-ctypes-66202.rs:1:9 - | -LL | #![deny(improper_ctypes)] - | ^^^^^^^^^^^^^^^ - = note: composed only of `PhantomData` -note: the type is defined here - --> $DIR/lint-ctypes-66202.rs:7:1 - | -LL | pub struct W(T); - | ^^^^^^^^^^^^^^^^^^^ - -error: aborting due to previous error -