Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

warn less about non-exhaustive in ffi #116863

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+42 −14
Open

warn less about non-exhaustive in ffi #116863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 17 additions & 8 deletions compiler/rustc_lint/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use rustc_attr as attr;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::DiagnosticMessage;
use rustc_hir as hir;
use rustc_hir::{is_range_literal, Expr, ExprKind, Node};
use rustc_hir::{def::CtorKind, is_range_literal, Expr, ExprKind, Node};
use rustc_middle::ty::layout::{IntegerExt, LayoutOf, SizeSkeleton};
use rustc_middle::ty::GenericArgsRef;
use rustc_middle::ty::{
Expand Down Expand Up @@ -1136,16 +1136,25 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
}
}

if def.is_variant_list_non_exhaustive() && !def.did().is_local() {
return FfiUnsafe {
ty,
reason: fluent::lint_improper_ctypes_non_exhaustive,
help: None,
};
}
// non_exhaustive suggests it is possible that someone might break ABI
// see: https://github.com/rust-lang/rust/issues/44109#issuecomment-537583344
// so warn on complex enums being used outside their crate
let nonexhaustive_nonlocal_ffi =
def.is_variant_list_non_exhaustive() && !def.did().is_local();

// Check the contained variants.
for variant in def.variants() {
// but only warn about really_tagged_union reprs,
// exempt enums with unit ctors like C's (like rust-bindgen)
if nonexhaustive_nonlocal_ffi
&& !matches!(variant.ctor_kind(), Some(CtorKind::Const))
{
return FfiUnsafe {
ty,
reason: fluent::lint_improper_ctypes_non_exhaustive,
help: None,
};
};
let is_non_exhaustive = variant.is_field_list_non_exhaustive();
if is_non_exhaustive && !variant.def_id.is_local() {
return FfiUnsafe {
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/auxiliary/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,14 @@ pub enum NonExhaustiveVariants {
#[non_exhaustive] Tuple(u32),
#[non_exhaustive] Struct { field: u32 }
}

// Note the absence of repr(C): it's not necessary, and recent C code can now use repr hints too.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs don't make explicit that the ABI matches C's - https://doc.rust-lang.org/reference/type-layout.html#primitive-representation-of-field-less-enums - my sense is that's probably just an oversight?

But as-is it's not clear to me that this is accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

typedef enum Name: uint8_t {
    /* fields */
} Name;

is legal C code as-of C23 and must, by brute force of logic, match

#[repr(u8)]
enum Name {
    /* fields */
}

or fail to compile, according to my understanding. (There simply isn't any other valid representation for the two types in either language's rules, if the Rust variants are data-free: they must match.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy to revise the wording but

  • it would be inconsistent with improper_ctypes as-is to warn on u32 in FFI
  • the inadequacy of the docs is a preexisting issue it seems
  • if I had my druthers I would yeet the entire idea of linting on another crate's types in a crate that isn't the declaring crate from the lint, because it seems absurd, especially when in an extern "C" block that is only declaring a binding against C functions, not declaring a new Rust function (thus the bound function implicitly exists anyway), and this is very much the most-lamented lint in terms of false positives.

#[repr(u32)]
#[non_exhaustive]
petrochenkov marked this conversation as resolved.
Show resolved Hide resolved
pub enum NonExhaustiveCLikeEnum {
One = 1,
Two = 2,
Three = 3,
Four = 4,
Five = 5,
}
10 changes: 9 additions & 1 deletion tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ extern crate types;
// This test checks that non-exhaustive types with `#[repr(C)]` from an extern crate are considered
// improper.

use types::{NonExhaustiveEnum, NonExhaustiveVariants, NormalStruct, TupleStruct, UnitStruct};
use types::{
NonExhaustiveCLikeEnum, NonExhaustiveEnum, NonExhaustiveVariants,
NormalStruct, TupleStruct, UnitStruct,
};

extern "C" {
pub fn non_exhaustive_enum(_: NonExhaustiveEnum);
Expand All @@ -21,4 +24,9 @@ extern "C" {
//~^ ERROR `extern` block uses type `NonExhaustiveVariants`, which is not FFI-safe
}

// These should pass without remark, as they're C-compatible, despite being "non-exhaustive".
extern "C" {
pub fn non_exhaustive_c_compat_enum(_: NonExhaustiveCLikeEnum);
}

fn main() {}
10 changes: 5 additions & 5 deletions tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: `extern` block uses type `NonExhaustiveEnum`, which is not FFI-safe
--> $DIR/extern_crate_improper.rs:12:35
--> $DIR/extern_crate_improper.rs:15:35
|
LL | pub fn non_exhaustive_enum(_: NonExhaustiveEnum);
| ^^^^^^^^^^^^^^^^^ not FFI-safe
Expand All @@ -12,31 +12,31 @@ LL | #![deny(improper_ctypes)]
| ^^^^^^^^^^^^^^^

error: `extern` block uses type `NormalStruct`, which is not FFI-safe
--> $DIR/extern_crate_improper.rs:14:44
--> $DIR/extern_crate_improper.rs:17:44
|
LL | pub fn non_exhaustive_normal_struct(_: NormalStruct);
| ^^^^^^^^^^^^ not FFI-safe
|
= note: this struct is non-exhaustive

error: `extern` block uses type `UnitStruct`, which is not FFI-safe
--> $DIR/extern_crate_improper.rs:16:42
--> $DIR/extern_crate_improper.rs:19:42
|
LL | pub fn non_exhaustive_unit_struct(_: UnitStruct);
| ^^^^^^^^^^ not FFI-safe
|
= note: this struct is non-exhaustive

error: `extern` block uses type `TupleStruct`, which is not FFI-safe
--> $DIR/extern_crate_improper.rs:18:43
--> $DIR/extern_crate_improper.rs:21:43
|
LL | pub fn non_exhaustive_tuple_struct(_: TupleStruct);
| ^^^^^^^^^^^ not FFI-safe
|
= note: this struct is non-exhaustive

error: `extern` block uses type `NonExhaustiveVariants`, which is not FFI-safe
--> $DIR/extern_crate_improper.rs:20:38
--> $DIR/extern_crate_improper.rs:23:38
|
LL | pub fn non_exhaustive_variant(_: NonExhaustiveVariants);
| ^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
Expand Down