scroll icon indicating copy to clipboard operation
scroll copied to clipboard

[Question] Is it intentional that you can impl Try{From,Into}Ctx multiple times for a type using different ctx types?

Open kitlith opened this issue 7 years ago • 3 comments

impl<'a> TryFromCtx<'a, ()> for SomeType { /* does one thing */ }

impl<'a> TryFromCtx<'a, scroll::Endian> for SomeType { /* does something completely different */ }

is a situation that is totally possible to come up, and I bet that in this sort of case the methods pread and gread would not function as it wouldn't be able to figure out which trait implementation to use.

pread_with and gread_with should still function because you can disambiguate the context by providing an argument.

However, really, this begs the question: did you intend for this to be possible in the first place? Do you still want it to be possible?

kitlith avatar Sep 10 '18 23:09 kitlith

Yes it's possible, and will have the ambiguous behavior in the case of pread as you describe. I haven't thought much about whether I want it to be possible or not; I guess not?

If you have an idea how to eliminate ambiguity that is backwards compatible, I'd be interested, but I'm not sure whether it is worth a large time investment, since I don't think this issue comes up for most people (why do they want two different contexts, and if so, they should just use the _with methods to disambiguate it)

m4b avatar Sep 11 '18 03:09 m4b

I'm not sure if there's a way to do it with backwards compatibility, but I think you'd accomplish only being able to implement once by having the context type be an associated type instead of a generic parameter. Whether you can create a backwards-compatible wrapper or not is another question.

Maybe put it on a "if I'm already going to make a breaking change, think about making a breaking change here too" list?

kitlith avatar Sep 18 '18 21:09 kitlith

I think it makes sense for it to be possible. It's a nice elegant way to describe how to read the same type in different ways:

let str1 = buf.gread_with::<&str>(offset, LengthPrefixed);
let str2 = buf.gread_with::<&str>(offset, FixedLength(123));

// LengthPrefixed and FixedLength definitions left as an exercise for the reader

whatisaphone avatar Sep 23 '19 21:09 whatisaphone