Skip to content

Commit

Permalink
lint: unify enum variant, union and struct logic
Browse files Browse the repository at this point in the history 
This commit applies the changes introduced in rust-lang#72890 to both enum
variants and unions - where the logic prior to rust-lang#72890 was duplicated.

Signed-off-by: David Wood <david@davidtw.co>
  • Loading branch information
@davidtwco
davidtwco committed Jun 19, 2020
1 parent 76ad38d commit 0cccaa0
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 152 deletions.
207 changes: 82 additions & 125 deletions src/librustc_lint/types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ struct ImproperCTypesVisitor<'a, 'tcx> {
enum FfiResult<'tcx> {
FfiSafe,
FfiPhantom(Ty<'tcx>),
FfiUnsafe { ty: Ty<'tcx>, reason: &'static str, help: Option<&'static str> },
FfiUnsafe { ty: Ty<'tcx>, reason: String, help: Option<String> },
}

fn ty_is_known_nonnull<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
Expand Down Expand Up @@ -613,6 +613,50 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
}
}

/// Checks if the given `VariantDef`'s field types are "ffi-safe".
fn check_variant_for_ffi(
&self,
cache: &mut FxHashSet<Ty<'tcx>>,
ty: Ty<'tcx>,
def: &ty::AdtDef,
variant: &ty::VariantDef,
substs: SubstsRef<'tcx>,
) -> FfiResult<'tcx> {
use FfiResult::*;

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) = variant.transparent_newtype_field(self.cx.tcx, self.cx.param_env) {
self.check_field_type_for_ffi(cache, field, substs)
} else {
bug!("malformed transparent type");
}
} else {
// We can't completely trust repr(C) markings; 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, substs) {
FfiSafe => {
all_phantom = false;
}
FfiPhantom(..) if def.is_enum() => {
return FfiUnsafe {
ty,
reason: "this enum contains a PhantomData field".into(),
help: None,
};
}
FfiPhantom(..) => {}
r => return r,
}
}

if all_phantom { FfiPhantom(ty) } else { FfiSafe }
}
}

/// 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, cache: &mut FxHashSet<Ty<'tcx>>, ty: Ty<'tcx>) -> FfiResult<'tcx> {
Expand All @@ -634,15 +678,18 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
return FfiPhantom(ty);
}
match def.adt_kind() {
AdtKind::Struct => {
AdtKind::Struct | AdtKind::Union => {
let kind = if def.is_struct() { "struct" } else { "union" };

if !def.repr.c() && !def.repr.transparent() {
return FfiUnsafe {
ty,
reason: "this struct has unspecified layout",
help: Some(
reason: format!("this {} has unspecified layout", kind),
help: Some(format!(
"consider adding a `#[repr(C)]` or \
`#[repr(transparent)]` attribute to this struct",
),
`#[repr(transparent)]` attribute to this {}",
kind
)),
};
}

Expand All @@ -651,93 +698,20 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
if is_non_exhaustive && !def.did.is_local() {
return FfiUnsafe {
ty,
reason: "this struct is non-exhaustive",
reason: format!("this {} is non-exhaustive", kind),
help: None,
};
}

if def.non_enum_variant().fields.is_empty() {
return FfiUnsafe {
ty,
reason: "this struct has no fields",
help: Some("consider adding a member to this struct"),
reason: format!("this {} has no fields", kind),
help: Some(format!("consider adding a member to this {}", kind)),
};
}

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)
{
self.check_field_type_for_ffi(cache, field, substs)
} else {
FfiSafe
}
} 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 r = self.check_field_type_for_ffi(cache, field, substs);
match r {
FfiSafe => {
all_phantom = false;
}
FfiPhantom(..) => {}
FfiUnsafe { .. } => {
return r;
}
}
}

if all_phantom { FfiPhantom(ty) } else { FfiSafe }
}
}
AdtKind::Union => {
if !def.repr.c() && !def.repr.transparent() {
return FfiUnsafe {
ty,
reason: "this union has unspecified layout",
help: Some(
"consider adding a `#[repr(C)]` or \
`#[repr(transparent)]` attribute to this union",
),
};
}

if def.non_enum_variant().fields.is_empty() {
return FfiUnsafe {
ty,
reason: "this union has no fields",
help: Some("consider adding a field to this union"),
};
}

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() && field_ty.is_zst(cx, field.did) {
continue;
}
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 }
self.check_variant_for_ffi(cache, ty, def, def.non_enum_variant(), substs)
}
AdtKind::Enum => {
if def.variants.is_empty() {
Expand All @@ -752,11 +726,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
if !is_repr_nullable_ptr(cx, ty, def, substs) {
return FfiUnsafe {
ty,
reason: "enum has no representation hint",
reason: "enum has no representation hint".into(),
help: Some(
"consider adding a `#[repr(C)]`, \
`#[repr(transparent)]`, or integer `#[repr(...)]` \
attribute to this enum",
attribute to this enum"
.into(),
),
};
}
Expand All @@ -765,7 +740,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
if def.is_variant_list_non_exhaustive() && !def.did.is_local() {
return FfiUnsafe {
ty,
reason: "this enum is non-exhaustive",
reason: "this enum is non-exhaustive".into(),
help: None,
};
}
Expand All @@ -776,51 +751,31 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
if is_non_exhaustive && !variant.def_id.is_local() {
return FfiUnsafe {
ty,
reason: "this enum has non-exhaustive variants",
reason: "this enum has non-exhaustive variants".into(),
help: None,
};
}

for field in &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() && field_ty.is_zst(cx, field.did) {
continue;
}
let r = self.check_type_for_ffi(cache, field_ty);
match r {
FfiSafe => {}
FfiUnsafe { .. } => {
return r;
}
FfiPhantom(..) => {
return FfiUnsafe {
ty,
reason: "this enum contains a PhantomData field",
help: None,
};
}
}
match self.check_variant_for_ffi(cache, ty, def, variant, substs) {
FfiSafe => (),
r => return r,
}
}

FfiSafe
}
}
}

ty::Char => FfiUnsafe {
ty,
reason: "the `char` type has no C equivalent",
help: Some("consider using `u32` or `libc::wchar_t` instead"),
reason: "the `char` type has no C equivalent".into(),
help: Some("consider using `u32` or `libc::wchar_t` instead".into()),
},

ty::Int(ast::IntTy::I128) | ty::Uint(ast::UintTy::U128) => FfiUnsafe {
ty,
reason: "128-bit integers don't currently have a known stable ABI",
reason: "128-bit integers don't currently have a known stable ABI".into(),
help: None,
},

Expand All @@ -829,24 +784,24 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {

ty::Slice(_) => FfiUnsafe {
ty,
reason: "slices have no C equivalent",
help: Some("consider using a raw pointer instead"),
reason: "slices have no C equivalent".into(),
help: Some("consider using a raw pointer instead".into()),
},

ty::Dynamic(..) => {
FfiUnsafe { ty, reason: "trait objects have no C equivalent", help: None }
FfiUnsafe { ty, reason: "trait objects have no C equivalent".into(), help: None }
}

ty::Str => FfiUnsafe {
ty,
reason: "string slices have no C equivalent",
help: Some("consider using `*const u8` and a length instead"),
reason: "string slices have no C equivalent".into(),
help: Some("consider using `*const u8` and a length instead".into()),
},

ty::Tuple(..) => FfiUnsafe {
ty,
reason: "tuples have unspecified layout",
help: Some("consider using a struct instead"),
reason: "tuples have unspecified layout".into(),
help: Some("consider using a struct instead".into()),
},

ty::RawPtr(ty::TypeAndMut { ty, .. }) | ty::Ref(_, ty, _) => {
Expand All @@ -860,10 +815,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
Abi::Rust | Abi::RustIntrinsic | Abi::PlatformIntrinsic | Abi::RustCall => {
return FfiUnsafe {
ty,
reason: "this function pointer has Rust-specific calling convention",
reason: "this function pointer has Rust-specific calling convention"
.into(),
help: Some(
"consider using an `extern fn(...) -> ...` \
function pointer instead",
function pointer instead"
.into(),
),
};
}
Expand Down Expand Up @@ -897,7 +854,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
// While opaque types are checked for earlier, if a projection in a struct field
// normalizes to an opaque type, then it will reach this branch.
ty::Opaque(..) => {
FfiUnsafe { ty, reason: "opaque types have no C equivalent", help: None }
FfiUnsafe { ty, reason: "opaque types have no C equivalent".into(), help: None }
}

ty::Param(..)
Expand Down Expand Up @@ -1004,7 +961,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
// 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);
self.emit_ffi_unsafe_type_lint(ty, sp, &reason, help.as_deref());
}
}
}
Expand Down
52 changes: 25 additions & 27 deletions src/librustc_middle/ty/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1807,6 +1807,31 @@ impl<'tcx> VariantDef {
pub fn is_field_list_non_exhaustive(&self) -> bool {
self.flags.intersects(VariantFlags::IS_FIELD_LIST_NON_EXHAUSTIVE)
}

/// `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> {
for field in &self.fields {
let field_ty = field.ty(tcx, InternalSubsts::identity_for_item(tcx, self.def_id));

// `normalize_erasing_regions` will fail for projections that contain generic
// parameters, so check these before normalizing.
if field_ty.has_projections() && field_ty.needs_subst() {
return Some(field);
}

let field_ty = tcx.normalize_erasing_regions(param_env, field_ty);
if !field_ty.is_zst(tcx, self.def_id) {
return Some(field);
}
}

None
}
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, RustcEncodable, RustcDecodable, HashStable)]
Expand Down Expand Up @@ -2376,33 +2401,6 @@ 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 = field.ty(tcx, InternalSubsts::identity_for_item(tcx, self.did));

// `normalize_erasing_regions` will fail for projections that contain generic
// parameters, so check these before normalizing.
if field_ty.has_projections() && field_ty.needs_subst() {
return Some(field);
}

let field_ty = tcx.normalize_erasing_regions(param_env, field_ty);
if !field_ty.is_zst(tcx, self.did) {
return Some(field);
}
}

None
}
}

impl<'tcx> FieldDef {
Expand Down

0 comments on commit 0cccaa0

Please sign in to comment.