Skip to content

Commit d4d3d7d

Browse files
davidtwcodavidtwco
committed
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 <[email protected]>
1 parent 3e7aabb commit d4d3d7d

File tree

5 files changed

+69
-51
lines changed

5 files changed

+69
-51
lines changed

‎src/librustc_lint/types.rs

+39-30
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ use rustc_attr as attr;
66
use rustc_data_structures::fx::FxHashSet;
77
use rustc_errors::Applicability;
88
use rustc_hir as hir;
9-
use rustc_hir::def_id::DefId;
109
use rustc_hir::{is_range_literal, ExprKind, Node};
1110
use rustc_index::vec::Idx;
1211
use rustc_middle::mir::interpret::{sign_extend, truncate};
@@ -511,10 +510,6 @@ enum FfiResult<'tcx> {
511510
FfiUnsafe { ty: Ty<'tcx>, reason: &'static str, help: Option<&'static str> },
512511
}
513512

514-
fn is_zst<'tcx>(tcx: TyCtxt<'tcx>, did: DefId, ty: Ty<'tcx>) -> bool {
515-
tcx.layout_of(tcx.param_env(did).and(ty)).map(|layout| layout.is_zst()).unwrap_or(false)
516-
}
517-
518513
fn ty_is_known_nonnull<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
519514
match ty.kind {
520515
ty::FnPtr(_) => true,
@@ -523,7 +518,7 @@ fn ty_is_known_nonnull<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
523518
for field in field_def.all_fields() {
524519
let field_ty =
525520
tcx.normalize_erasing_regions(ParamEnv::reveal_all(), field.ty(tcx, substs));
526-
if is_zst(tcx, field.did, field_ty) {
521+
if field_ty.is_zst(tcx, field.did) {
527522
continue;
528523
}
529524

@@ -653,32 +648,43 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
653648
};
654649
}
655650

656-
// We can't completely trust repr(C) and repr(transparent) markings;
657-
// make sure the fields are actually safe.
658-
let mut all_phantom = true;
659-
for field in &def.non_enum_variant().fields {
660-
let field_ty = cx.normalize_erasing_regions(
661-
ParamEnv::reveal_all(),
662-
field.ty(cx, substs),
663-
);
664-
// repr(transparent) types are allowed to have arbitrary ZSTs, not just
665-
// PhantomData -- skip checking all ZST fields
666-
if def.repr.transparent() && is_zst(cx, field.did, field_ty) {
667-
continue;
651+
if def.repr.transparent() {
652+
// Can assume that only one field is not a ZST, so only check
653+
// that field's type for FFI-safety.
654+
if let Some(field) =
655+
def.transparent_newtype_field(cx, self.cx.param_env)
656+
{
657+
let field_ty = cx.normalize_erasing_regions(
658+
self.cx.param_env,
659+
field.ty(cx, substs),
660+
);
661+
self.check_type_for_ffi(cache, field_ty)
662+
} else {
663+
FfiSafe
668664
}
669-
let r = self.check_type_for_ffi(cache, field_ty);
670-
match r {
671-
FfiSafe => {
672-
all_phantom = false;
673-
}
674-
FfiPhantom(..) => {}
675-
FfiUnsafe { .. } => {
676-
return r;
665+
} else {
666+
// We can't completely trust repr(C) markings; make sure the fields are
667+
// actually safe.
668+
let mut all_phantom = true;
669+
for field in &def.non_enum_variant().fields {
670+
let field_ty = cx.normalize_erasing_regions(
671+
self.cx.param_env,
672+
field.ty(cx, substs),
673+
);
674+
let r = self.check_type_for_ffi(cache, field_ty);
675+
match r {
676+
FfiSafe => {
677+
all_phantom = false;
678+
}
679+
FfiPhantom(..) => {}
680+
FfiUnsafe { .. } => {
681+
return r;
682+
}
677683
}
678684
}
679-
}
680685

681-
if all_phantom { FfiPhantom(ty) } else { FfiSafe }
686+
if all_phantom { FfiPhantom(ty) } else { FfiSafe }
687+
}
682688
}
683689
AdtKind::Union => {
684690
if !def.repr.c() && !def.repr.transparent() {
@@ -708,7 +714,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
708714
);
709715
// repr(transparent) types are allowed to have arbitrary ZSTs, not just
710716
// PhantomData -- skip checking all ZST fields.
711-
if def.repr.transparent() && is_zst(cx, field.did, field_ty) {
717+
if def.repr.transparent() && field_ty.is_zst(cx, field.did) {
712718
continue;
713719
}
714720
let r = self.check_type_for_ffi(cache, field_ty);
@@ -774,7 +780,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
774780
);
775781
// repr(transparent) types are allowed to have arbitrary ZSTs, not
776782
// just PhantomData -- skip checking all ZST fields.
777-
if def.repr.transparent() && is_zst(cx, field.did, field_ty) {
783+
if def.repr.transparent() && field_ty.is_zst(cx, field.did) {
778784
continue;
779785
}
780786
let r = self.check_type_for_ffi(cache, field_ty);
@@ -983,6 +989,9 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
983989
FfiResult::FfiPhantom(ty) => {
984990
self.emit_ffi_unsafe_type_lint(ty, sp, "composed only of `PhantomData`", None);
985991
}
992+
// If `ty` is a `repr(transparent)` newtype, and the non-zero-sized type is a generic
993+
// argument, which after substitution, is `()`, then this branch can be hit.
994+
FfiResult::FfiUnsafe { ty, .. } if is_return_type && ty.is_unit() => return,
986995
FfiResult::FfiUnsafe { ty, reason, help } => {
987996
self.emit_ffi_unsafe_type_lint(ty, sp, reason, help);
988997
}

‎src/librustc_middle/ty/mod.rs

+23
Original file line numberDiff line numberDiff line change
@@ -2390,6 +2390,29 @@ impl<'tcx> AdtDef {
23902390
pub fn sized_constraint(&self, tcx: TyCtxt<'tcx>) -> &'tcx [Ty<'tcx>] {
23912391
tcx.adt_sized_constraint(self.did).0
23922392
}
2393+
2394+
/// `repr(transparent)` structs can have a single non-ZST field, this function returns that
2395+
/// field.
2396+
pub fn transparent_newtype_field(
2397+
&self,
2398+
tcx: TyCtxt<'tcx>,
2399+
param_env: ParamEnv<'tcx>,
2400+
) -> Option<&FieldDef> {
2401+
assert!(self.is_struct() && self.repr.transparent());
2402+
2403+
for field in &self.non_enum_variant().fields {
2404+
let field_ty = tcx.normalize_erasing_regions(
2405+
param_env,
2406+
field.ty(tcx, InternalSubsts::identity_for_item(tcx, self.did)),
2407+
);
2408+
2409+
if !field_ty.is_zst(tcx, self.did) {
2410+
return Some(field);
2411+
}
2412+
}
2413+
2414+
None
2415+
}
23932416
}
23942417

23952418
impl<'tcx> FieldDef {

‎src/librustc_middle/ty/sty.rs

+5
Original file line numberDiff line numberDiff line change
@@ -2186,6 +2186,11 @@ impl<'tcx> TyS<'tcx> {
21862186
}
21872187
}
21882188
}
2189+
2190+
/// Is this a zero-sized type?
2191+
pub fn is_zst(&'tcx self, tcx: TyCtxt<'tcx>, did: DefId) -> bool {
2192+
tcx.layout_of(tcx.param_env(did).and(self)).map(|layout| layout.is_zst()).unwrap_or(false)
2193+
}
21892194
}
21902195

21912196
/// Typed constant value.

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
// check-pass
2+
13
#![deny(improper_ctypes)]
24

35
// This test checks that return types are normalized before being checked for FFI-safety, and that
@@ -10,7 +12,6 @@ extern "C" {
1012
pub fn bare() -> ();
1113
pub fn normalize() -> <() as ToOwned>::Owned;
1214
pub fn transparent() -> W<()>;
13-
//~^ ERROR uses type `W<()>`
1415
}
1516

1617
fn main() {}

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

-20
This file was deleted.

0 commit comments

Comments
 (0)