-
Notifications
You must be signed in to change notification settings - Fork 36
Inline type defining macros, avoid use of cbindgen parse.expand + nightly #610
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
base: main
Are you sure you want to change the base?
Conversation
We only used this for `rustls_result`, and in practice it only saved us having to implement `From<u32>` by hand. That's handy, but comes at the cost of needing to use the nightly toolchain w/ cbindgen so it can expand the macro before generating the `.h`. That wasn't the worst, but a nighty regression that has remained unfixed for months broke this workflow. In sum: life will be easier without the macro. RIP.
They're helpful, but require us to use cbindgen's parse.expand feature which in turn requires nightly, and is broken with nightly since ~April.
Since we have no macros anymore, it's not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
use rustls_result::*; | ||
|
||
match x { | ||
7000 => Ok, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be tempted to take a dependency instead of this, honestly. Either we could use https://docs.rs/macro_rules_attribute/latest/macro_rules_attribute/ -- keeping the macro and applying it in proc-macro style, which should work with cbindgen(?) -- or maybe https://crates.io/crates/num-derive or another option that @djc will surely know.
Another alternative is to write a test that checks x == rustls_result::from(x as u32)
for most values of x, but that's a bit annoying because the range is sparse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were using num_enum
originally, and removed it in #322
It doesn't feel to me like the pain of keeping these in-sync justifies a dependency. Is your main concern that we'll get the mapping wrong in the From
impl without noticing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could use https://docs.rs/macro_rules_attribute/latest/macro_rules_attribute/ -- keeping the macro and applying it in proc-macro style, which should work with cbindgen(?)
That approach seems a little nicer than a numeric enum dependency but I'll have to experiment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah my main concern is a maintainability one -- that it's easy to add a new enum without adding the reverse mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just write a round tripping test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just write a round tripping test?
Is there a way to write that test without creating even more overhead for adding a new enum variant? That seems like the main hangup with that idea from my perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this?
for i in 0..8000 {
assert_eq!(rustls_result::from(i) as u32, i);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ctz Would a test like ^ be sufficient from your perspective or do you think we need to switch to a proc-macro solution or a full int-enum dep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That test is tricky because this mapping is not bijective. So 6999u32 maps to rustls_result::InvalidParameter
, which maps to 7009u32. And you can't just say "ignore everything that maps to InvalidParameter
, because that is the case where an entry is missing in one table or the other.
We have a small collection of
macro_rules!
macros that define types: one helper making a u32rustls_result
enum, and a handful for defining opaque structs implementingCastable
.These are handy, but they require we use
cbindgen
'sparse.expand
functionality. This in turn requires the nightly toolchain to allowcbindgen
to expand the macro definitions before generating the.h
file. This worked OK for a while, but nightly broke the handling ofcfg
feature guards way back in April, requiring that contributors + CI both pin tonightly-2025-03-25
.Since the bug I opened upstream (rust-lang/rust#139715) hasn't received any significant attention in months let's stop using macros in this context so we can go back to
cbindgen
withoutparse.expand
and the stable toolchain. We can't stay pinned tonightly-2025-03-25
forever.