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

Add trait bounds to Default and From impls #21

Closed
wants to merge 1 commit into from
Closed

Conversation

shahn
Copy link
Contributor

@shahn shahn commented Nov 11, 2023

This should allow making the correct usage of the crate much more easily discoverable, while not adding too many bounds to unrelated functions.

Fixes #3.

This should allow making the correct usage of the crate much more
easily discoverable, while not adding too many bounds to unrelated
functions.

Fixes jonhoo#3.
@codecov-commenter
Copy link

Codecov Report

Merging #21 (b00164c) into main (5bf693f) will decrease coverage by 4.3%.
The diff coverage is 95.2%.

Files Coverage Δ
src/reader.rs 84.6% <ø> (-3.1%) ⬇️
src/tokio.rs 100.0% <100.0%> (ø)
src/writer.rs 64.0% <ø> (-16.9%) ⬇️
src/stream.rs 80.4% <93.3%> (+2.0%) ⬆️

@jonhoo
Copy link
Owner

jonhoo commented Nov 12, 2023

Following on from this comment, I don't think I want to add these. The reason why is slightly different for Default and From.

For Default, this will make it so that #[derive(Default)] will not work on types that contain a (generic) AsyncBincode* type, which is quite unfortunate. Implementors can manually implement Default with the right bounds, but forcing them to do so isn't great (and I don't think adds too much value in terms of discoverability either).

For From, this will require those building on top of async-bincode to now repeat these bounds (at least) twice, once in their constructor (which calls From) and once in their block(s) that use the other trait impls. Admittedly this is only the case for impls that are themselves generic, but I suspect that's fairly common (being generic over read/write isn't exactly niche).

To dig slightly deeper, I think the real issue at play here is that the compiler's error messages are subpar. It should be able to tell you that the reason AsyncBincodeStream doesn't implement Stream is because its generic argument doesn't meet the requisite bound (and it should tell you what those bounds are). The fact that it doesn't I think is arguably a compiler diagnostics bug (see also some of these), and I would file a bug upstream highlighting the shortcoming of the diagnostics so that they can be improved at the source!

@shahn
Copy link
Contributor Author

shahn commented Nov 12, 2023

Ok, that seems fair. I will close this then, it was a fun experiment nonetheless.

@shahn shahn closed this Nov 12, 2023
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.

Need Example
3 participants