Repository navigation
Conversation
Author
|
@emilio would it be better to split this PR into few parts? For example:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR makes zero-sized types with alignment 1 (1-ZSTs) explicitly accounted for during the generics instantiation. It fixes a number of reported issues that previously either caused a panic, or produced ill-formed bindings. In case of C++ (where generics are not monomorphized), ill-formed instantiations are reported as warnings. It also leads to the change of the mangling scheme, which might affect users relying on the old one.
Most of the files affected by the PR are the new test cases and their expectation files. Among the existing test cases, only the expectations for
opaque.rschanged, due to the new mangling.The main logic is in
monomorph.rs(the C++ check),ir/ty.rsandir/generic_path.rs(ZST propagation), andir/typedef.rs(ZST typedef instantiations). The otherir/*.rsfiles mostly just switch fromOption<Type>toResult<Type, Zst>, andmangle.rs,cdecl.rsandlibrary.rshave small related changes.tests/tests.rsadds the warning checks, which don't affect existing tests.Notable changes
Propagation of ZSTs through load and monomorphization
Currently, 1-ZSTs are dropped upon load. Because of that, it might panic (when
GenericParams::callreceives fewer arguments than expected) or incorrectly match the generic arguments and their values (when ZST is followed by other arguments with default values).To avoid this, ZSTs must be propagated all the way until the monomorphization. Thus,
GenericArgumentnow has a new variant holding a value ofZsttype. SuchZstvalue is a placeholder for any zero-sized type. All such types that are currently accounted for (i.e. dropped) are 1-aligned, and thereforeZstis an enum with a single variantZst1. Keeping this specific variant might be useful for solving issues such as #1035 in the future when ZSTs with non-1 alignment are added.Most of code in
bindgen/ir/related to loading or specialization that was returningType / Option<Type>before now returnsResult<Type, Zst>. Overall, handling of 1-ZSTs did not change: zero-sized struct fields and function arguments are dropped, and pointers to ZSTs are represented asvoid*. I have added one extra case: arrays of 1-ZSTs discovered upon specialization are also treated as 1-ZSTs; upon load, I preserve the existing behavior of returning an error immediately.Change of mangling scheme
On the bindings side, the main visible change is that 1-ZST generic arguments are represented as
1Zplaceholders. That is,Result<Foo, ()>is now mangled intoResult_Foo__1Zrather thanResult_Foo(see the changed expectations foropaque.rstest case - the only one affected). This solves problems arising from mangling into non-existent type (Option<()>intoOption) or mangling two distinct types into the same one (Result<(), u32>andResult<u32, ()>intoResult_u32).1Zis chosen so that it cannot collide with an actual Rust identifier.Detection and warning for specific ZST instantiations in C++
Generation of C bindings monomorphizes generic Rust types, emitting new data structures where ZST fields are simply dropped. With C++ however, there is no monomorphization, and generic Rust types are emitted as template types instantiated at use sites. This leads to potential problems where an instantiation with ZST creates
voidfields, arrays and function arguments.Solving it in general is out of scope of this PR. Instead, I simply check whether there are potentially problematic instantiations (which requires running
collect_monomorphs(), similar to the C case) and emit warnings for them. For that, I decided to keep track of the generic used to instantiate a given monomorph, and use it to check whether monomorphization caused any drops. This is implemented byhave_shrunkfunction that essentially followsType::specialize, i.e. detects the cases whereType::specializealtered the structure of input type; ifType::specializechanges,have_shrunkshould be adapted accordingly.This is a relatively cheap solution among the alternatives (see below).
New annotations for expected warnings
To test for the above warnings, I added two new annotations:
//! cbindgen:test-expect-no-warningsrequires that the test case produces no warningscbindgen:test-expect-warning-<lang>=<text>requires that generating bindings forlanggenerates a warning containingtextThese conditions are checked on captured
stderr. Right now these annotations are only used in the new tests.New tests
zst_valid.rs: ZST generic arguments that must not produce any warnings.zst_issues.rs: the related issue examples that produce C++ warnings.zst_generics.rs: similar tozst_valid.rs, but with examples that produce C++ warningszst_nested.rs: ZST arguments hidden in arrays and function types.zst_typedefs.rs: C++ checks of generic typedef instantiations. It also has a commented-out example that would produce non-compilable bindings (see the question about ZST typedefs below)Related issues
Closes #659, closes #99, closes #228.
S<u32, ()>GenericParams::callS<uint32_t, void>)Either<(), u8>GenericParams::callLeftbody (GCC extension)Either<void, uint8_t>)CResultTempl<(), APIError>GenericParams::callMyStruct<()>MyStructMyStructwithout template argsMyStruct<void>)Foo<()>withPhantomData<T>FooFoowithout template args"Generated, warning" means the header compiles, but the warned-about type is ill-formed in C++ once instantiated (e.g. used by value).
Questions
Should more 1-ZSTs be added?
Upstream
ty.rsonly handles specific 1-ZSTs:()(as an empty tuple),PhantomDataandPhantomPinned. I also made arrays of these to be treated as a 1-ZST (upon specialization). It seems useful to handle more types, in particular, an empty struct.Alternative implementations of C++ checks
There are other potential solutions for this (extending
Type::specializeto immediately report issues, or changing its return type to something like 'ir::Typewith ZSTs' and converting it toir::Typelater), but they seem to require many more changes.Handling of typedefs with ZST generics
Upstream doesn't account for ZST instantiations of generic typedefs specifically. Since ZSTs are dropped upon load, in C it leads to "unknown name" gcc errors or panic, and in C++ these typedefs are used without arguments which doesn't compile either. I'm not sure what is the expected solution for that in general.
In this PR, generic typedefs aliasing a ZST are not emitted at all - we display a warning instead; this is inspired by the upstream behavior mentioned below. For C++, generic typedefs instantiations are checked alongside structs etc. with
have_shrunk. For C, "unknown name" errors still appear; however, the produced C bindings are at least syntactically correct and don't cause panics. There are also options of emitting opaque structs (similar to what upstream does upon loading of non-generic ZST typedefs), or emittingtypedef void ...(but this contradicts the aforementioned upstream behavior).AI disclosure
I used Claude Code w/ Opus 5.5 to help navigating the codebase, suggesting test cases and generating small pieces of code, and also for finding the related issues. All generated code is reviewed by me. I also used AI to iterate on this PR description.