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.

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

repr(C) enums with a variant that overflows the native enum type should warn #108069

Open
ChrisDenton opened this issue Feb 15, 2023 · 3 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-ffi Area: Foreign Function Interface (FFI) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ChrisDenton
Copy link
Contributor

Consider this:

#[repr(C)]
enum Bar {
    A = 0x1_FFFF_FFFF,
}

Assume that the targets native enum size is 4 bytes. Obviously 0x1_FFFF_FFFF will overflow. This is accepted by the compiler, without warning. Instead it will silently use a larger underlying type.

I think this should be a future incompatibility warning with an eye to making it a hard error in a future edition. Being explicit with, say, repr(u64) is better than doing so silently (especially with it being incompatible with MSVC). However, any warning would be good if others disagree.

@ChrisDenton ChrisDenton added O-windows Operating system: Windows A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 15, 2023
@workingjubilee
Copy link
Contributor

I believe the result of this declaration in C would be, as they say, "implementation-defined", so yes we should warn in this case and possibly error in the future. However we must be careful because we need to actually only get shouty if this is bigger than the C int, not the actual default #[repr(C)] enum size, since some platforms define their underlying types for enums as scaling down to 8 bits by default and thus using the minimal size, a la Rust.

@workingjubilee workingjubilee added the A-ffi Area: Foreign Function Interface (FFI) label Feb 20, 2023
@ChrisDenton
Copy link
Contributor Author

That sounds fair. It's times like these that I think the duel purpose of repr(C) as both "whatever C does on this platform" and "guaranteed layout for unsafe shenanigans" was a mistake.

Btw, I'll remove the O-windows tag because this is not really specific to any platform.

@ChrisDenton ChrisDenton removed the O-windows Operating system: Windows label Feb 21, 2023
@LunarLambda
Copy link

LunarLambda commented Dec 19, 2023

In C, prior to C23, all enumerators must fit in int, so repr(C) and overflow should warn/error (Godbolt)

In C++, the underlying type is again implementation-defined, but must not be larger than int/unsigned int unless an enumerator demands it (Godbolt). This matches what Rust seems to do currently.

C23 adapts the C++ rules. The language used there is a lot more complicated, seeming to imply that individual enumerators may have different types since they become globally scoped "integer constants" (cppreference), and rather than having an underlying type that all enumerators share, the enum as a whole has an implementation-defined "compatible" type that is capable of storing all enumerators. (unsure how to experimentally verify this) (or this could just be an incredibly roundabout way of saying it's exactly like the C++ rules)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-ffi Area: Foreign Function Interface (FFI) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Status: Rejected/Not lang
Development

No branches or pull requests

3 participants