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
better checking of tag duplicates, avoid discarding invalid variant errs #951
base: master
Are you sure you want to change the base?
Conversation
bail!( | ||
"invalid oneof variant {}::{}: oneof variants may only have a single tag", | ||
ident, | ||
variant_ident | ||
); |
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 Err
this returns is formerly always discarded via flattening the Result
. it doesn't hurt to retain the check even though i've not been able to find a way to trigger it, but if the condition does somehow occur it should actually stop the macro.
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 believe the intention here was originally to write this as .map(..).collect::<Result<Vec<_>, _>>()?;
, but the nuance was lost
2738cac
to
31aa6de
Compare
31aa6de
to
b30f84b
Compare
.sorted_unstable() | ||
.tuple_windows() | ||
.find(|(a, b)| a == b) |
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.
it would kinda be preferable to write this as .duplicates().next()
using Itertools
, but that method unfortunately requires use_std
.
Currently it seems that the "invalid oneof variant" Errs are always discarded because they are being passed directly into
flat_map
from a closure, rather than actually propagating the error.This PR also streamlines the checks for duplicated tag numbers and adds some rudimentary tests for the affected code in those proc macros.