Skip to content

Commit 2f931ef

Browse files
niacdoialniacdoial
committed
lint: rework some ImproperCTypes messages (especially around indirections to !Sized)
1 parent e6b1f2d commit 2f931ef

14 files changed

+195
-67
lines changed

‎compiler/rustc_lint/messages.ftl

+11-2
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,6 @@ lint_improper_ctypes_128bit = 128-bit integers don't currently have a known stab
360360
lint_improper_ctypes_array_help = consider passing a pointer to the array
361361
362362
lint_improper_ctypes_array_reason = passing raw arrays by value is not FFI-safe
363-
lint_improper_ctypes_box = box cannot be represented as a single pointer
364363
365364
lint_improper_ctypes_char_help = consider using `u32` or `libc::wchar_t` instead
366365
@@ -378,7 +377,9 @@ lint_improper_ctypes_enum_repr_help =
378377
lint_improper_ctypes_enum_repr_reason = enum has no representation hint
379378
lint_improper_ctypes_fnptr_help = consider using an `extern fn(...) -> ...` function pointer instead
380379
380+
lint_improper_ctypes_fnptr_indirect_reason = the function pointer to `{$ty}` is FFI-unsafe due to `{$inner_ty}`
381381
lint_improper_ctypes_fnptr_reason = this function pointer has Rust-specific calling convention
382+
382383
lint_improper_ctypes_non_exhaustive = this enum is non-exhaustive
383384
lint_improper_ctypes_non_exhaustive_variant = this enum has non-exhaustive variants
384385
@@ -389,7 +390,11 @@ lint_improper_ctypes_opaque = opaque types have no C equivalent
389390
lint_improper_ctypes_pat_help = consider using the base type instead
390391
391392
lint_improper_ctypes_pat_reason = pattern types have no C equivalent
392-
lint_improper_ctypes_slice_help = consider using a raw pointer instead
393+
394+
lint_improper_ctypes_sized_ptr_to_unsafe_type =
395+
this reference (`{$ty}`) is can safely be translated into a C pointer, but `{$inner_ty}` itself is not FFI-safe
396+
397+
lint_improper_ctypes_slice_help = consider using a raw pointer to the slice's first element (and a length) instead
393398
394399
lint_improper_ctypes_slice_reason = slices have no C equivalent
395400
lint_improper_ctypes_str_help = consider using `*const u8` and a length instead
@@ -415,6 +420,10 @@ lint_improper_ctypes_union_layout_help = consider adding a `#[repr(C)]` or `#[re
415420
lint_improper_ctypes_union_layout_reason = this union has unspecified layout
416421
lint_improper_ctypes_union_non_exhaustive = this union is non-exhaustive
417422
423+
lint_improper_ctypes_unsized_box = this box for an unsized rust type contains metadata, which makes it incompatible with a C pointer
424+
lint_improper_ctypes_unsized_ptr = this pointer to an unsized rust type contains metadata, which makes it incompatible with a C pointer
425+
lint_improper_ctypes_unsized_ref = this reference to an unsized rust type contains metadata, which makes it incompatible with a C pointer
426+
418427
lint_incomplete_include =
419428
include macro expected single expression in source
420429

‎compiler/rustc_lint/src/types.rs

+115-21
Original file line numberDiff line numberDiff line change
@@ -599,8 +599,6 @@ enum FfiResult<'tcx> {
599599
reason: DiagMessage,
600600
help: Option<DiagMessage>,
601601
},
602-
// NOTE: this `allow` is only here for one retroactively-added commit
603-
#[allow(dead_code)]
604602
FfiUnsafeWrapper {
605603
ty: Ty<'tcx>,
606604
reason: DiagMessage,
@@ -915,16 +913,47 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
915913

916914
match *ty.kind() {
917915
ty::Adt(def, args) => {
918-
if let Some(boxed) = ty.boxed_ty()
919-
&& matches!(self.mode, CItemKind::Definition)
920-
{
921-
if boxed.is_sized(tcx, self.cx.param_env) {
922-
return FfiSafe;
916+
if let Some(inner_ty) = ty.boxed_ty() {
917+
if inner_ty.is_sized(tcx, self.cx.param_env)
918+
|| matches!(inner_ty.kind(), ty::Foreign(..))
919+
{
920+
// discussion on declaration vs definition:
921+
// see the `ty::RawPtr(inner_ty, _) | ty::Ref(_, inner_ty, _)` arm
922+
// of this `match *ty.kind()` block
923+
if matches!(self.mode, CItemKind::Definition) {
924+
return FfiSafe;
925+
} else {
926+
let inner_res = self.check_type_for_ffi(acc, inner_ty);
927+
return match inner_res {
928+
FfiUnsafe { .. } | FfiUnsafeWrapper { .. } => FfiUnsafeWrapper {
929+
ty,
930+
reason: fluent::lint_improper_ctypes_sized_ptr_to_unsafe_type,
931+
wrapped: Box::new(inner_res),
932+
help: None,
933+
},
934+
_ => inner_res,
935+
};
936+
}
923937
} else {
938+
let help = match inner_ty.kind() {
939+
ty::Str => Some(fluent::lint_improper_ctypes_str_help),
940+
ty::Slice(_) => Some(fluent::lint_improper_ctypes_slice_help),
941+
ty::Adt(def, _)
942+
if matches!(def.adt_kind(), AdtKind::Struct | AdtKind::Union)
943+
&& matches!(
944+
tcx.get_diagnostic_name(def.did()),
945+
Some(sym::cstring_type | sym::cstr_type)
946+
)
947+
&& !acc.base_ty.is_mutable_ptr() =>
948+
{
949+
Some(fluent::lint_improper_ctypes_cstr_help)
950+
}
951+
_ => None,
952+
};
924953
return FfiUnsafe {
925954
ty,
926-
reason: fluent::lint_improper_ctypes_box,
927-
help: None,
955+
reason: fluent::lint_improper_ctypes_unsized_box,
956+
help,
928957
};
929958
}
930959
}
@@ -1087,15 +1116,6 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
10871116
help: Some(fluent::lint_improper_ctypes_tuple_help),
10881117
},
10891118

1090-
ty::RawPtr(ty, _) | ty::Ref(_, ty, _)
1091-
if {
1092-
matches!(self.mode, CItemKind::Definition)
1093-
&& ty.is_sized(self.cx.tcx, self.cx.param_env)
1094-
} =>
1095-
{
1096-
FfiSafe
1097-
}
1098-
10991119
ty::RawPtr(ty, _)
11001120
if match ty.kind() {
11011121
ty::Tuple(tuple) => tuple.is_empty(),
@@ -1105,7 +1125,66 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
11051125
FfiSafe
11061126
}
11071127

1108-
ty::RawPtr(ty, _) | ty::Ref(_, ty, _) => self.check_type_for_ffi(acc, ty),
1128+
ty::RawPtr(inner_ty, _) | ty::Ref(_, inner_ty, _) => {
1129+
if inner_ty.is_sized(tcx, self.cx.param_env)
1130+
|| matches!(inner_ty.kind(), ty::Foreign(..))
1131+
{
1132+
// there's a nuance on what this lint should do for function definitions
1133+
// (touched upon in https://github.com/rust-lang/rust/issues/66220 and https://github.com/rust-lang/rust/pull/72700)
1134+
//
1135+
// (`extern "C" fn fn_name(...) {...}`) versus declarations (`extern "C" {fn fn_name(...);}`).
1136+
// The big question is: what does "ABI safety" mean? if you have something translated to a C pointer
1137+
// (which has a stable layout) but points to FFI-unsafe type, is it safe?
1138+
// on one hand, the function's ABI will match that of a similar C-declared function API,
1139+
// on the other, dereferencing the pointer in not-rust will be painful.
1140+
// In this code, the opinion is split between function declarations and function definitions.
1141+
// For declarations, we see this as unsafe, but for definitions, we see this as safe.
1142+
// This is mostly because, for extern function declarations, the actual definition of the function is written somewhere else,
1143+
// so the fact that a pointer's pointee should be treated as opaque to one side or the other can be explicitely written out.
1144+
// For extern function definitions, however, both callee and some callers can be written in rust,
1145+
// so developers need to keep as much typing information as possible.
1146+
if matches!(self.mode, CItemKind::Definition) {
1147+
return FfiSafe;
1148+
} else if matches!(ty.kind(), ty::RawPtr(..))
1149+
&& matches!(inner_ty.kind(), ty::Tuple(tuple) if tuple.is_empty())
1150+
{
1151+
FfiSafe
1152+
} else {
1153+
let inner_res = self.check_type_for_ffi(acc, inner_ty);
1154+
return match inner_res {
1155+
FfiSafe => inner_res,
1156+
_ => FfiUnsafeWrapper {
1157+
ty,
1158+
reason: fluent::lint_improper_ctypes_sized_ptr_to_unsafe_type,
1159+
wrapped: Box::new(inner_res),
1160+
help: None,
1161+
},
1162+
};
1163+
}
1164+
} else {
1165+
let help = match inner_ty.kind() {
1166+
ty::Str => Some(fluent::lint_improper_ctypes_str_help),
1167+
ty::Slice(_) => Some(fluent::lint_improper_ctypes_slice_help),
1168+
ty::Adt(def, _)
1169+
if matches!(def.adt_kind(), AdtKind::Struct | AdtKind::Union)
1170+
&& matches!(
1171+
tcx.get_diagnostic_name(def.did()),
1172+
Some(sym::cstring_type | sym::cstr_type)
1173+
)
1174+
&& !acc.base_ty.is_mutable_ptr() =>
1175+
{
1176+
Some(fluent::lint_improper_ctypes_cstr_help)
1177+
}
1178+
_ => None,
1179+
};
1180+
let reason = match ty.kind() {
1181+
ty::RawPtr(..) => fluent::lint_improper_ctypes_unsized_ptr,
1182+
ty::Ref(..) => fluent::lint_improper_ctypes_unsized_ref,
1183+
_ => unreachable!(),
1184+
};
1185+
FfiUnsafe { ty, reason, help }
1186+
}
1187+
}
11091188

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

@@ -1123,7 +1202,14 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
11231202
for arg in sig.inputs() {
11241203
match self.check_type_for_ffi(acc, *arg) {
11251204
FfiSafe => {}
1126-
r => return r,
1205+
r => {
1206+
return FfiUnsafeWrapper {
1207+
ty,
1208+
reason: fluent::lint_improper_ctypes_fnptr_indirect_reason,
1209+
help: None,
1210+
wrapped: Box::new(r),
1211+
};
1212+
}
11271213
}
11281214
}
11291215

@@ -1132,7 +1218,15 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
11321218
return FfiSafe;
11331219
}
11341220

1135-
self.check_type_for_ffi(acc, ret_ty)
1221+
match self.check_type_for_ffi(acc, ret_ty) {
1222+
r @ (FfiSafe | FfiPhantom(_)) => r,
1223+
r => FfiUnsafeWrapper {
1224+
ty: ty.clone(),
1225+
reason: fluent::lint_improper_ctypes_fnptr_indirect_reason,
1226+
help: None,
1227+
wrapped: Box::new(r),
1228+
},
1229+
}
11361230
}
11371231

11381232
ty::Foreign(..) => FfiSafe,

‎tests/ui/extern/extern-C-non-FFI-safe-arg-ice-52334.stderr

+2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ warning: `extern` fn uses type `CStr`, which is not FFI-safe
44
LL | type Foo = extern "C" fn(::std::ffi::CStr);
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
66
|
7+
= note: the function pointer to `extern "C" fn(CStr)` is FFI-unsafe due to `CStr`
78
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
89
= note: `CStr`/`CString` do not have a guaranteed layout
910
= note: `#[warn(improper_ctypes_definitions)]` on by default
@@ -14,6 +15,7 @@ warning: `extern` block uses type `CStr`, which is not FFI-safe
1415
LL | fn meh(blah: Foo);
1516
| ^^^ not FFI-safe
1617
|
18+
= note: the function pointer to `extern "C" fn(CStr)` is FFI-unsafe due to `CStr`
1719
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
1820
= note: `CStr`/`CString` do not have a guaranteed layout
1921
= note: `#[warn(improper_ctypes)]` on by default

‎tests/ui/extern/extern-C-str-arg-ice-80125.stderr

+2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ warning: `extern` fn uses type `str`, which is not FFI-safe
44
LL | type ExternCallback = extern "C" fn(*const u8, u32, str);
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
66
|
7+
= note: the function pointer to `extern "C" fn(*const u8, u32, str)` is FFI-unsafe due to `str`
78
= help: consider using `*const u8` and a length instead
89
= note: string slices have no C equivalent
910
= note: `#[warn(improper_ctypes_definitions)]` on by default
@@ -14,6 +15,7 @@ warning: `extern` fn uses type `str`, which is not FFI-safe
1415
LL | pub extern "C" fn register_something(bind: ExternCallback) -> Struct {
1516
| ^^^^^^^^^^^^^^ not FFI-safe
1617
|
18+
= note: the function pointer to `extern "C" fn(*const u8, u32, str)` is FFI-unsafe due to `str`
1719
= help: consider using `*const u8` and a length instead
1820
= note: string slices have no C equivalent
1921

‎tests/ui/lint/extern-C-fnptr-lints-slices.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
// It's an improper ctype (a slice) arg in an extern "C" fnptr.
44

55
pub type F = extern "C" fn(&[u8]);
6-
//~^ ERROR: `extern` fn uses type `[u8]`, which is not FFI-safe
6+
//~^ ERROR: `extern` fn uses type `&[u8]`, which is not FFI-safe
77

88

99
fn main() {}

‎tests/ui/lint/extern-C-fnptr-lints-slices.stderr

+4-3
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
1-
error: `extern` fn uses type `[u8]`, which is not FFI-safe
1+
error: `extern` fn uses type `&[u8]`, which is not FFI-safe
22
--> $DIR/extern-C-fnptr-lints-slices.rs:5:14
33
|
44
LL | pub type F = extern "C" fn(&[u8]);
55
| ^^^^^^^^^^^^^^^^^^^^ not FFI-safe
66
|
7-
= help: consider using a raw pointer instead
8-
= note: slices have no C equivalent
7+
= note: the function pointer to `for<'a> extern "C" fn(&'a [u8])` is FFI-unsafe due to `&[u8]`
8+
= help: consider using a raw pointer to the slice's first element (and a length) instead
9+
= note: this reference to an unsized rust type contains metadata, which makes it incompatible with a C pointer
910
note: the lint level is defined here
1011
--> $DIR/extern-C-fnptr-lints-slices.rs:1:8
1112
|

‎tests/ui/lint/lint-ctypes-73249-2.stderr

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ error: `extern` block uses type `Qux`, which is not FFI-safe
44
LL | fn lint_me() -> A<()>;
55
| ^^^^^ not FFI-safe
66
|
7+
= note: this reference (`&Qux`) is can safely be translated into a C pointer, but `Qux` itself is not FFI-safe
78
= note: opaque types have no C equivalent
89
note: the lint level is defined here
910
--> $DIR/lint-ctypes-73249-2.rs:2:9

0 commit comments

Comments
 (0)