-
Notifications
You must be signed in to change notification settings - Fork 739
Allow blocklisting anonymous enums #3067
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
05c2885
to
1d01b94
Compare
b87010a
to
3b5da54
Compare
{ | ||
debug!("Blocklisting anonymous enum."); | ||
return; | ||
} |
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.
Shouldn't this only block the enum if all variants, and not any are blocklisted? If not, can we document this behavior and why is it useful? thanks.
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.
The behavior of using any
variant matches for anonymous enums is not new, and already works when allowlisting anonymous enums, so there is a (probably small) risk that changing the way matching works could break users who are relying on the current behavior for allowlisting.
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.
But you can individually hide variants (via annotations or callbacks), right...
I'd expect allowlisting to work with any and blocklisting to work with all, if that makes sense... But let me know if you disagree, maybe my expectation is flawed somehow?
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.
No, I think that's perfectly reasonable. I changed this PR to use all
instead. This will also block all anonymous enums without variants (See my comment on test 1025 below).
e901c73
to
8bb3e15
Compare
Note: This will also block anonymous enums with no variants, but adding a typedef for such an enum should be useless anyway.
8bb3e15
to
4ea949f
Compare
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 looked into #1025 and I'm assuming that the anonymous enum with no variants was just an example and nobody actually needs a typedef for an anonymous enum with no variants (which would be blocked by this PR, unless we specifically want to preserve existing behavior, since .all()
in Rust returns true if there is nothing to iterate over).
I added some cases for the other possibilities and the typedef is still generated correctly for all the enum definitions that could be used in some way.
CC @ctaggart who reported #1025, just in case I'm missing something and they had an actual usecase for a typedef for an anonymous enum with no variants.
Currently, there is no way to blocklist anonymous enums, which is inconvenient if you want to avoid
_bindgen_tyXX
types appearing in the public API.Note: I'm not 100% sure if this is the best place to apply the blocklisting, but I couldn't find a more appropriate place.