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

better checking of tag duplicates, avoid discarding invalid variant errs #951

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mumbleskates
Copy link
Contributor

@mumbleskates mumbleskates commented Nov 25, 2023

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.

Comment on lines -421 to -425
bail!(
"invalid oneof variant {}::{}: oneof variants may only have a single tag",
ident,
variant_ident
);
Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Comment on lines +96 to +98
.sorted_unstable()
.tuple_windows()
.find(|(a, b)| a == b)
Copy link
Contributor Author

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.

@mumbleskates mumbleskates marked this pull request as ready for review November 26, 2023 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant