Skip to content

Commit b5248c6

Browse files
niacdoialniacdoial
committed
lint ImproperCTypes: make checking indirection more DRY
[commit does not pass tests]
1 parent 5c7e7cc commit b5248c6

File tree

1 file changed

+88
-105
lines changed

1 file changed

+88
-105
lines changed

‎compiler/rustc_lint/src/types.rs

+88-105
Original file line numberDiff line numberDiff line change
@@ -775,6 +775,17 @@ enum TypeSizedness {
775775
UnsizedWithMetadata,
776776
}
777777

778+
/// what type indirection points to a given type
779+
#[derive(Clone, Copy)]
780+
enum IndirectionType {
781+
/// box (valid non-null pointer, owns pointee)
782+
Box,
783+
/// ref (valid non-null pointer, borrows pointee)
784+
Ref,
785+
/// raw pointer (not necessarily non-null or valid. no info on ownership)
786+
RawPtr,
787+
}
788+
778789
/// Is this type unsized because it contains (or is) a foreign type?
779790
/// (Returns Err if the type happens to be sized after all)
780791
fn get_type_sizedness<'tcx, 'a>(cx: &'a LateContext<'tcx>, ty: Ty<'tcx>) -> TypeSizedness {
@@ -1137,6 +1148,77 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
11371148
}
11381149
}
11391150

1151+
/// Checks if the given indirection (box,ref,pointer) is "ffi-safe"
1152+
fn check_indirection_for_ffi(
1153+
&self,
1154+
acc: &mut CTypesVisitorState<'tcx>,
1155+
ty: Ty<'tcx>,
1156+
inner_ty: Ty<'tcx>,
1157+
indirection_type: IndirectionType,
1158+
) -> FfiResult<'tcx> {
1159+
use FfiResult::*;
1160+
let tcx = self.cx.tcx;
1161+
match get_type_sizedness(self.cx, inner_ty) {
1162+
TypeSizedness::UnsizedWithExternType | TypeSizedness::Definite => {
1163+
// there's a nuance on what this lint should do for
1164+
// function definitions (`extern "C" fn fn_name(...) {...}`)
1165+
// versus declarations (`unsafe extern "C" {fn fn_name(...);}`).
1166+
// This is touched upon in https://github.com/rust-lang/rust/issues/66220
1167+
// and https://github.com/rust-lang/rust/pull/72700
1168+
//
1169+
// The big question is: what does "ABI safety" mean? if you have something translated to a C pointer
1170+
// (which has a stable layout) but points to FFI-unsafe type, is it safe?
1171+
// On one hand, the function's ABI will match that of a similar C-declared function API,
1172+
// on the other, dereferencing the pointer on the other side of the FFI boundary will be painful.
1173+
// In this code, the opinion on is split between function declarations and function definitions,
1174+
// with the idea that at least one side of the FFI boundary needs to treat the pointee as an opaque type.
1175+
// For declarations, we see this as unsafe, but for definitions, we see this as safe.
1176+
//
1177+
// For extern function declarations, the actual definition of the function is written somewhere else,
1178+
// meaning the declaration is free to express this opaqueness with an extern type (opaque caller-side) or a std::ffi::c_void (opaque callee-side)
1179+
// For extern function definitions, however, in the case where the type is opaque caller-side, it is not opaque callee-side,
1180+
// and having the full type information is necessary to compile the function.
1181+
if matches!(self.mode, CItemKind::Definition) {
1182+
return FfiSafe;
1183+
} else {
1184+
let inner_res = self.check_type_for_ffi(acc, inner_ty);
1185+
return match inner_res {
1186+
FfiSafe => inner_res,
1187+
_ => FfiUnsafeWrapper {
1188+
ty,
1189+
reason: fluent::lint_improper_ctypes_sized_ptr_to_unsafe_type,
1190+
wrapped: Box::new(inner_res),
1191+
help: None,
1192+
},
1193+
};
1194+
}
1195+
}
1196+
TypeSizedness::UnsizedWithMetadata => {
1197+
let help = match inner_ty.kind() {
1198+
ty::Str => Some(fluent::lint_improper_ctypes_str_help),
1199+
ty::Slice(_) => Some(fluent::lint_improper_ctypes_slice_help),
1200+
ty::Adt(def, _)
1201+
if matches!(def.adt_kind(), AdtKind::Struct | AdtKind::Union)
1202+
&& matches!(
1203+
tcx.get_diagnostic_name(def.did()),
1204+
Some(sym::cstring_type | sym::cstr_type)
1205+
)
1206+
&& !acc.base_ty.is_mutable_ptr() =>
1207+
{
1208+
Some(fluent::lint_improper_ctypes_cstr_help)
1209+
}
1210+
_ => None,
1211+
};
1212+
let reason = match indirection_type {
1213+
IndirectionType::RawPtr => fluent::lint_improper_ctypes_unsized_ptr,
1214+
IndirectionType::Ref => fluent::lint_improper_ctypes_unsized_ref,
1215+
IndirectionType::Box => fluent::lint_improper_ctypes_unsized_box,
1216+
};
1217+
FfiUnsafe { ty, reason, help }
1218+
}
1219+
}
1220+
}
1221+
11401222
/// Checks if the given type is "ffi-safe" (has a stable, well-defined
11411223
/// representation which can be exported to C code).
11421224
fn check_type_for_ffi(
@@ -1159,48 +1241,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
11591241
match *ty.kind() {
11601242
ty::Adt(def, args) => {
11611243
if let Some(inner_ty) = ty.boxed_ty() {
1162-
if let TypeSizedness::UnsizedWithExternType | TypeSizedness::Definite =
1163-
get_type_sizedness(self.cx, inner_ty)
1164-
{
1165-
// discussion on declaration vs definition:
1166-
// see the `ty::RawPtr(inner_ty, _) | ty::Ref(_, inner_ty, _)` arm
1167-
// of this `match *ty.kind()` block
1168-
if matches!(self.mode, CItemKind::Definition) {
1169-
return FfiSafe;
1170-
} else {
1171-
let inner_res = self.check_type_for_ffi(acc, inner_ty);
1172-
return match inner_res {
1173-
FfiUnsafe { .. } | FfiUnsafeWrapper { .. } => FfiUnsafeWrapper {
1174-
ty,
1175-
reason: fluent::lint_improper_ctypes_sized_ptr_to_unsafe_type,
1176-
wrapped: Box::new(inner_res),
1177-
help: None,
1178-
},
1179-
_ => inner_res,
1180-
};
1181-
}
1182-
} else {
1183-
let help = match inner_ty.kind() {
1184-
ty::Str => Some(fluent::lint_improper_ctypes_str_help),
1185-
ty::Slice(_) => Some(fluent::lint_improper_ctypes_slice_help),
1186-
ty::Adt(def, _)
1187-
if matches!(def.adt_kind(), AdtKind::Struct | AdtKind::Union)
1188-
&& matches!(
1189-
tcx.get_diagnostic_name(def.did()),
1190-
Some(sym::cstring_type | sym::cstr_type)
1191-
)
1192-
&& !acc.base_ty.is_mutable_ptr() =>
1193-
{
1194-
Some(fluent::lint_improper_ctypes_cstr_help)
1195-
}
1196-
_ => None,
1197-
};
1198-
return FfiUnsafe {
1199-
ty,
1200-
reason: fluent::lint_improper_ctypes_unsized_box,
1201-
help,
1202-
};
1203-
}
1244+
return self.check_indirection_for_ffi(acc, ty, inner_ty, IndirectionType::Box);
12041245
}
12051246
if def.is_phantom_data() {
12061247
return FfiPhantom(ty);
@@ -1363,69 +1404,11 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
13631404
FfiSafe
13641405
}
13651406

1366-
ty::RawPtr(inner_ty, _) | ty::Ref(_, inner_ty, _) => {
1367-
if let TypeSizedness::UnsizedWithExternType | TypeSizedness::Definite =
1368-
get_type_sizedness(self.cx, inner_ty)
1369-
{
1370-
// there's a nuance on what this lint should do for
1371-
// function definitions (`extern "C" fn fn_name(...) {...}`)
1372-
// versus declarations (`unsafe extern "C" {fn fn_name(...);}`).
1373-
// This is touched upon in https://github.com/rust-lang/rust/issues/66220
1374-
// and https://github.com/rust-lang/rust/pull/72700
1375-
//
1376-
// The big question is: what does "ABI safety" mean? if you have something translated to a C pointer
1377-
// (which has a stable layout) but points to FFI-unsafe type, is it safe?
1378-
// On one hand, the function's ABI will match that of a similar C-declared function API,
1379-
// on the other, dereferencing the pointer on the other side of the FFI boundary will be painful.
1380-
// In this code, the opinion on is split between function declarations and function definitions,
1381-
// with the idea that at least one side of the FFI boundary needs to treat the pointee as an opaque type.
1382-
// For declarations, we see this as unsafe, but for definitions, we see this as safe.
1383-
//
1384-
// For extern function declarations, the actual definition of the function is written somewhere else,
1385-
// meaning the declaration is free to express this opaqueness with an extern type (opaque caller-side) or a std::ffi::c_void (opaque callee-side)
1386-
// For extern function definitions, however, in the case where the type is opaque caller-side, it is not opaque callee-side,
1387-
// and having the full type information is necessary to compile the function.
1388-
if matches!(self.mode, CItemKind::Definition) {
1389-
return FfiSafe;
1390-
} else if matches!(ty.kind(), ty::RawPtr(..))
1391-
&& matches!(inner_ty.kind(), ty::Tuple(tuple) if tuple.is_empty())
1392-
{
1393-
FfiSafe
1394-
} else {
1395-
let inner_res = self.check_type_for_ffi(acc, inner_ty);
1396-
return match inner_res {
1397-
FfiSafe => inner_res,
1398-
_ => FfiUnsafeWrapper {
1399-
ty,
1400-
reason: fluent::lint_improper_ctypes_sized_ptr_to_unsafe_type,
1401-
wrapped: Box::new(inner_res),
1402-
help: None,
1403-
},
1404-
};
1405-
}
1406-
} else {
1407-
let help = match inner_ty.kind() {
1408-
ty::Str => Some(fluent::lint_improper_ctypes_str_help),
1409-
ty::Slice(_) => Some(fluent::lint_improper_ctypes_slice_help),
1410-
ty::Adt(def, _)
1411-
if matches!(def.adt_kind(), AdtKind::Struct | AdtKind::Union)
1412-
&& matches!(
1413-
tcx.get_diagnostic_name(def.did()),
1414-
Some(sym::cstring_type | sym::cstr_type)
1415-
)
1416-
&& !acc.base_ty.is_mutable_ptr() =>
1417-
{
1418-
Some(fluent::lint_improper_ctypes_cstr_help)
1419-
}
1420-
_ => None,
1421-
};
1422-
let reason = match ty.kind() {
1423-
ty::RawPtr(..) => fluent::lint_improper_ctypes_unsized_ptr,
1424-
ty::Ref(..) => fluent::lint_improper_ctypes_unsized_ref,
1425-
_ => unreachable!(),
1426-
};
1427-
FfiUnsafe { ty, reason, help }
1428-
}
1407+
ty::RawPtr(inner_ty, _) => {
1408+
return self.check_indirection_for_ffi(acc, ty, inner_ty, IndirectionType::RawPtr);
1409+
}
1410+
ty::Ref(_, inner_ty, _) => {
1411+
return self.check_indirection_for_ffi(acc, ty, inner_ty, IndirectionType::Ref);
14291412
}
14301413

14311414
ty::Array(inner_ty, _) => self.check_type_for_ffi(acc, inner_ty),

0 commit comments

Comments
 (0)