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

Rewrite: seperated Decode and BorrowDecode #526

Merged
merged 16 commits into from
Jun 4, 2022

Conversation

VictorKoenders
Copy link
Contributor

@VictorKoenders VictorKoenders commented Mar 15, 2022

There are a number of issues with the current design where Decode depends on BorrowDecode. The most obvious one is the implementation of Cow.

Ideally we'd want:

impl<T> BorrowDecode for Cow<T> where T: BorrowDecode {
    fn borrow_decode<D: BorrowDecoder<'de>>(decoder: &mut D) -> Result<Self, DecodeError> {
        Ok(Cow::Borrowed(T::borrow_decode(decoder)?))
    }
}

impl<T> Decode for Cow<T> where T: Decode {
    fn decode<D: Decoder<'de>>(decoder: &mut D) -> Result<Self, DecodeError> {
        Ok(Cow::Owned(T::decode(decoder)?))
    }
}

But this is currently not possible.

This issue happens with other containers types as well. One issue is Arc<str> (#527), because Arc<T> requires T: Decode.

This pull request splits the implementation of Decode and BorrowDecode to be completely separate.

Some notes:

  • I added a impl_borrow_decode macro that is publicly available, which can be used to automatically derive BorrowDecode for any type that implements Decode
    • This only works with basic types. Types with generics for example cannot use this macro
  • #[derive(Decode)] now implies #[derive(BorrowDecode)] automatically.

As far as I can tell this is only a breaking change for people who implement Decode manually. Otherwise it should hopefully be a painless change.

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #526 (f7f5700) into trunk (5b19220) will decrease coverage by 0.81%.
The diff coverage is 32.23%.

@@            Coverage Diff             @@
##            trunk     #526      +/-   ##
==========================================
- Coverage   55.33%   54.52%   -0.82%     
==========================================
  Files          48       49       +1     
  Lines        4223     4422     +199     
==========================================
+ Hits         2337     2411      +74     
- Misses       1886     2011     +125     
Impacted Files Coverage Δ
derive/src/attribute.rs 0.00% <0.00%> (ø)
derive/src/derive_enum.rs 0.00% <0.00%> (ø)
derive/src/derive_struct.rs 0.00% <0.00%> (ø)
src/atomic.rs 100.00% <ø> (+100.00%) ⬆️
src/features/impl_std.rs 65.85% <0.00%> (-9.15%) ⬇️
src/features/serde/mod.rs 47.82% <0.00%> (-4.56%) ⬇️
src/varint/decode_unsigned.rs 69.41% <ø> (ø)
tests/atomic.rs 100.00% <ø> (ø)
tests/basic_types.rs 99.33% <ø> (ø)
tests/issues/issue_431.rs 100.00% <ø> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b19220...f7f5700. Read the comment docs.

@VictorKoenders VictorKoenders marked this pull request as ready for review April 1, 2022 12:07
@VictorKoenders VictorKoenders changed the title [WIP] Rewrite: seperated Decode and BorrowDecode Rewrite: seperated Decode and BorrowDecode Apr 1, 2022
@ZoeyR
Copy link
Collaborator

ZoeyR commented Apr 4, 2022

Will this require anyone who implements Decode to also implement BorrowDecode? i.e. is it considered an error to implement just Decode?

@VictorKoenders
Copy link
Contributor Author

They should implement both, but it's not enforced in the type system. I'm not sure if we should considering it an error

@VictorKoenders
Copy link
Contributor Author

src/de/impl_tuples.rs Outdated Show resolved Hide resolved
src/features/impl_alloc.rs Outdated Show resolved Hide resolved
This was linked to issues Apr 8, 2022
fuzz/Cargo.lock Outdated Show resolved Hide resolved
src/de/impls.rs Outdated Show resolved Hide resolved
src/enc/impls.rs Show resolved Hide resolved
@VictorKoenders VictorKoenders merged commit dcda3a8 into trunk Jun 4, 2022
@VictorKoenders VictorKoenders deleted the split_decode_and_borrow_decode branch June 4, 2022 13:24
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.

Arc<str> can't be bincoded #[bincode(bound)] for generic types
2 participants