Open HadrienG2 opened 4 years ago
(As an aside, because Rust references are marked dereferenceable
at the LLVM level, implementing Abomonation
for them will be dangerous until #17 is resolved)
I'm a bit scrambled at the moment, but want to pop in with a quick comment.
It is generally unsafe to implement Abomonation
for any reference type, because the lifetime of the reference needs to be bound to the lifetime of the borrowed &mut [u8]
. Otherwise, you could mint a reference whose lifetime outlives that of the binary data, and badness would ensue.
Serde has the same problem, and the result seems to be to have Deserialize
contain a lifetime argument, and only implement Deserialize<'a>
for &'a T
types. The decode
method would have a lifetime in the &mut 'a self
argument to ensure that self lives at least as long.
I'm sorry for not popping in earlier with this. Very behind your rate of production.
Serde has the same problem, and the answer
I have an AbomonationRef<'a>
trait locally, though I haven't figured out who would use it yet. I'm happy to share that out if the reference stuff is a specific goal.
Yup, I could use a look at that!
(Technically, I only need &str
, but I figured out that I could just go the extra mile and try to solve the more general reference problem)
On the road atm, but will get it soon!
EDIT: Fixed after cross-checking how serde does it.
Just to see if I understand, the idea would be to...
Abomonation
into Abomonation<'bytes>
where 'bytes
will be used to represent the set of &'bytes mut [u8]
slices from which it is valid to deserialize the abomonated type.
impl Abomonation for ConcreteT
impls into impl<'bytes> Abomonation<'bytes> for ConcreteT
: if a type contains no references, it can be deserialized from any slice of bytes.impl<'target, 'bytes: 'target> Abomonation<'bytes> for &'target ConcreteT
for reference-ish types: if a type contains references, it can only be deserialized from bytes which outlive the lifetime of those references. The reason is that abomonation will actually produce &'bytes ConcreteT
, not &'target ConcreteT
, and for this substitution to be valid we need &'bytes ConcreteT: &'target ConcreteT
.Abomonation
impls. I think something like impl<'a, 'bytes: 'a, T: Abomonation<'bytes>> Abomonation<'bytes> for Generic<'a, T>
(generalized to any amount of T-s and 'a-s) should be enough, but that needs cross-checking.Abomonation<'bytes>::exhume()
to require an &'a mut [u8]
where 'a: 'bytes
. This asks the borrow checker to enforce the constraint that we set ourselves in step 1.decode()
(the compiler should ask for it anyway).As a nice side effect, this should also resolve a problem which I had when trying to express the lifetimes of my exhume_xyz
helper functions in #28.
This is basically correct yes! I appear to have deleted the file, unfortunately, but you have the spirit of it. I can reconstruct it this weekend (it was very minor, just around the &str
and &T
implementations). I'm pretty sure I had what you have above, but it might also be that you could just do
impl<'bytes> Abomonation<'bytes> for &'bytes ConcreteT
and skip the other lifetime (allow Rust to coerce the lifetime as it sees appropriate).
Serde's author seems unconvinced:
The 'de lifetime should not appear in the type to which the Deserialize impl applies.
- // Do not do this. Sooner or later you will be sad. - impl<'de> Deserialize<'de> for Q<'de> { + // Do this instead. + impl<'de: 'a, 'a> Deserialize<'de> for Q<'a> {
OTOH, since the Abomonation impl is generic, it's probably okay to kill the lifetime on Abomonation::exhume<'a>
and just use &'bytes [u8]
directly.
From the point of view of abomonation's core semantics, there is nothing wrong with providing implementations of the
Abomonation
trait for both a typeT
and a reference to it&T
. Basically, the implementation for&T
works exactly like that ofBox<T>
in abomonation's current master branch.Such implementations would be useful for high-level users of
Abomonation
, who stick with derives,encode
,decode
andmeasure
, because they would allow safely auto-derivingAbomonation
for more types. Something which, as a matter of fact, I ended up wanting for my project.However, and that's the reason why I'm opening this issue before submitting my changes as a PR, there is also a significant ergonomic cost to doing so for any low-level user of
Abomonation
who calls into the trait directly.If
Abomonation
is implemented for bothT
and&T
, then anyone who uses theAbomonation
trait directly instead of going throughencode
,decode
andmeasure
must be super-careful, because method call syntax of thex.extent()
kind becomes a deadly trap that can very easily trigger the wrongAbomonation
impl through auto-Deref
magic.Instead, one must get into the habit of only calling
Abomonation
trait method viaU::extent(&x)
syntax, or if type U is unknown go for the somewhat less safe compromise ofAbomonation::extent(&x)
.Is this a trade-off that we can tolerate for the sake of having more
Abomonation
impls?