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.

Sign up for GitHub

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

Jump to bottom

[Based on #26] Support reference types (and a pinch of DSTs) #28

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

[Based on #26] Support reference types (and a pinch of DSTs) #28

wants to merge 25 commits into from

Conversation

HadrienG2
Copy link

@HadrienG2 HadrienG2 commented Sep 26, 2019 •
edited
Loading

This PR implements Abomonation for most references types, and a few DSTs along the way. It is what I had in mind while opening #27 . The main advantage of implementing Abomonation for these types is that it becomes much easier to serialize types containing Rust references (which are, after all, almost like every other pointer).

This PR raises a few questions that we may want to discuss here:

  • Do we want to do this?
  • Do we also want to fully support DSTs in Abomonation?
    • Currently, the Abomonation DST impls of this PR are only here 1/as a fun experiment and 2/because they are a more convenient place for deduplicating impls than free functions)
    • If we want full DST support in Abomonation, then we'll also need to make encode/decode/measure and tests accept T: ?Sized, which from a quick attempt won't be so easy. We may also want ?Sized bounds on some generic Abomonation impls like that of Box<T>
    • This would answer Shouldn't entomb and encode take a self rather than a &self? #18 since DSTs may currently only be passed to functions by reference.
  • Can we and do we want to also implement Abomonation for dyn Trait and &[mut] dyn Trait where Trait: Abomonation?

For convenience reasons, this PR is based on #22, #24, #25 and #26. Reviewing these PRs first is recommended, but if you want to review this one in isolation, please consider doing so commit-by-commit in order to get a clean diff.

Fixes #27 .

HadrienG2 and others added 20 commits September 26, 2019 09:25
@HadrienG2
Move to a NonNull-based exhume() interface
984b0b0
@HadrienG2
Port unsafe_abomonate to NonNull-based interface
e97de1b
@HadrienG2
Clarify that the tuple_abomonate macro takes types as input
5fd4957
@HadrienG2
Port tuple_abomonate to NonNull-based interface
b182a38
@HadrienG2
Port Box<T> abomonation to NonNull-based interface
d4a78e4
@HadrienG2
Port Rust enum abomonation to NonNull-based interface
421a486
@HadrienG2
Port slice-like abomonations to NonNull-based interface (and deduplic…
5ef07a7 
…ate them)
@HadrienG2
Require Debug and use assert_eq for improved test failure messages
b23d2dc
@HadrienG2
Clarify invalid data as a source of UB
b5f7b71
@HadrienG2
Remove outdated Abomonable notion
e4a4963
@HadrienG2
Make sure that black_box does its job
b8629a3
@HadrienG2
Remove some strange whitespace
48b2b9a
@HadrienG2
Snipe some unnecessary usage of Ok(())
7388af5
@HadrienG2
Don't use manual inlining where benchmarks don't show a benefit
3f84783
@HadrienG2
Add inline(always) where it matters in benchmarks
cb8e2af
@HadrienG2
Avoid multiple codegen units in benchmarks
17a0354
@HadrienG2
Improve Abomonation documentation and make the trait unsafe
ee283b0 
By its very nature, Abomonation does very unsafe (and UB-ish) things.
But we should strive to explain these as well as we can in the current
state of the unsafe Rust formalization effort in order to reduce the
potential for known misuse.
@HadrienG2
Take writer by value
6c12b2e
@HadrienG2
Do not rely on autoderef to select the right Abomonation impl
a4377f4
@HadrienG2
Make Box<T>::entomb simpler and more consistent with others
cda9ebb
@HadrienG2 HadrienG2 changed the title [Based on #26] Support reference types [Based on #26] Support reference types (and a pinch of DSTs) Sep 26, 2019
@HadrienG2 HadrienG2 mentioned this pull request Sep 27, 2019
Open
@HadrienG2 HadrienG2 changed the title [Based on #26] Support reference types (and a pinch of DSTs) [WIP] [Based on #26] Support reference types (and a pinch of DSTs) Sep 27, 2019
@HadrienG2 HadrienG2 mentioned this pull request Sep 29, 2019
Open
@HadrienG2
Copy link
Author

HadrienG2 commented Sep 29, 2019 •
edited
Loading

I have pushed a draft of the Abomonation<'bytes> memory safety fix that we discussed in #27 , but it has a bunch of FIXMEs that I hope to improve upon before merging.

@HadrienG2
Copy link
Author

Alright, I think this one is now ready to go (though by virtue of being on the top of the PR stack, it will probably go in last).

@HadrienG2 HadrienG2 changed the title [WIP] [Based on #26] Support reference types (and a pinch of DSTs) [Based on #26] Support reference types (and a pinch of DSTs) Sep 29, 2019
HadrienG2
HadrienG2 commented Sep 29, 2019
View reviewed changes
tests/tests.rs
assert!(result == &record);
assert!(rest.len() == 0);
if bytes.pop().is_some() {
assert_eq!(unsafe { decode::<T>(&mut bytes[..]) }, None);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One-line fix for zero sized type support. Not bad!

tests/tests.rs Outdated
let mut bytes = Vec::new();
fn _test_pass<'bytes, T: Abomonation<'bytes> + Debug + Eq>(
mut bytes: &'bytes mut Vec<u8>, record: T
) {
Copy link
Author

@HadrienG2 HadrienG2 Sep 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes work around what I believe to be a limitation of Rust's current API vocabulary with respect to lifetimes.

I could not find a way to express "we want T to implement Abomonation<'bytes> for all possibly 'bytes lifetimes on this function's stack". We only have "it should work for all slices of bytes" (which excludes types containing references) and "it should work for whatever byte container was passed as a parameter to the function, which will end up borrowed forever" (what I ended up using).

Amusingly enough, this is closely related, but not quite identical, to the reason why a full port of Abomonated to the new API is not possible.

Some variant of the ugly trick that I used to make Abomonated work could probably also work here, but I don't think that this shady unsafe trick is warranted in a test harness, of all things.

If you have an idea of how to extract this dirty trick into a general-purpose utility that any client code can easily and safely use, then I may revisit this opinion.

HadrienG2
HadrienG2 commented Oct 1, 2019
View reviewed changes
src/abomonated.rs Outdated Show resolved Hide resolved
@HadrienG2 HadrienG2 changed the title [Based on #26] Support reference types (and a pinch of DSTs) [WIP] [Based on #26] Support reference types (and a pinch of DSTs) Oct 3, 2019
@HadrienG2 HadrienG2 changed the title [WIP] [Based on #26] Support reference types (and a pinch of DSTs) [Based on #26] Support reference types (and a pinch of DSTs) Oct 3, 2019
@HadrienG2 HadrienG2 changed the title [Based on #26] Support reference types (and a pinch of DSTs) [WIP] [Based on #26] Support reference types (and a pinch of DSTs) Oct 9, 2019
@HadrienG2 HadrienG2 changed the title [WIP] [Based on #26] Support reference types (and a pinch of DSTs) [Based on #26] Support reference types (and a pinch of DSTs) Oct 11, 2019
@HadrienG2 HadrienG2 force-pushed the support-more-types branch 2 times, most recently from 9bd8671 to 22756de Compare October 12, 2019 14:44
This was referenced Oct 12, 2019
Open
Open
@HadrienG2 HadrienG2 mentioned this pull request Oct 28, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankmcsherry frankmcsherry frankmcsherry left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Is it okay to implement Abomonation for both T and &T?
2 participants
@HadrienG2 @frankmcsherry