decorum icon indicating copy to clipboard operation
decorum copied to clipboard

Add `Proxy::from_ref` and `Proxy::from_mut`

Open changhe3 opened this issue 2 years ago • 2 comments

These methods allow user to cast references to f32/f64 into the appropriate Proxy type.

changhe3 avatar Sep 08 '21 07:09 changhe3

Thanks for the PR! I really like this addition. The changes are consistent with the existing code, but I think we may want to approach this a bit differently and I have a few thoughts below.

I recently started experimenting with some changes to construction and conversions of proxy types and primitives on the convert branch. I'd like to migrate away from using standard infallible traits like From and Into for conversions that panic, as this violates the documented contract for those traits. Instead, I think proxy types should be more explicit about panics at points of conversion. Similarly, I don't think AsRef and AsMut should be implemented to cast primitives to proxy types, as the panic violates the same contract of those traits (well, except for Total, where the cast is infallible).

Though a bit less convenient, I'd suggest APIs more like the following to integrate with these upcoming changes (mutable conversions not shown):

impl<T, P> Proxy<T, P>
where
    T: Float + Primitive,
    P: Constraint<T>,
{
    // Be sure to document that this is an `O(N)` operation!
    pub fn try_from_slice<'a>(slice: &'a [T]) -> Result<&'a [Self], P::Error> { ... }
}

impl<T> Total<T>
where
    T: Float + Primitive,
{
    // This is a trivial cast. `O(1)`, unlike `try_from_slice`.
    pub fn from_slice<'a>(slice: &'a [T]) -> &'a [Self] { ... }
}

impl<'a, T> From<&'a T> for &'a Total<T> {
    // This is a trivial cast.
    fn from(inner: &'a T) -> Self { ... }
}

// Use a macro to implement this for the remaining proxy and primitive types.
impl<'a> TryFrom<&'a f64> for &'a Finite<f64> {
    type Error = ConstraintViolation;
    
    fn try_from(inner: &'a f64) -> Result<Self, Self::Error> { ... }
}

This provides both fallible reference conversions for non-Total proxies and an infallible conversion for Total. We could also provide slice casts too! Unfortunately, the From and TryFrom trait implementations interact poorly, so TryFrom must be implemented for the combinations of remaining proxy types and primitive types. This can be done easily enough in this macro, which is already doing the same for by-value conversions.

Finally, I think Constraint::filter_map poses a bit of a problem here. It's designed so that constraints can do two things: report violations and map values (mapping all -0 to +0, for example). Decorum has never actually used the latter capability though and values are never mapped into something different. We should remove this first, as it conflicts with these kinds of casts. I think we can simplify Constraint like this:

pub trait Constraint<T>: Member<RealSet>
where
    T: Float + Primitive,
{
    fn check(inner: &T) -> Result<(), ConstraintViolation>;
}

I know this is a lot! Sorry for the wall of text. If you're still willing to work on this that'd be awesome! I'd slightly prefer to wait for the convert branch to land (should happen very soon) but we don't have to. Please let me know what you'd prefer to do and thanks again for opening this. I hadn't considered these casts and I think they'll be useful!

olson-sean-k avatar Sep 12 '21 05:09 olson-sean-k

I've just landed fc809c9 from the convert branch with the changes I described above. I've also landed eae7b5e, which changes Constraint::filter_map to Constraint::check. I've updated the links in my previous comment. I think we should be good to go on this plan. :tada:

olson-sean-k avatar Sep 13 '21 18:09 olson-sean-k

It's been some time, but I've landed b536468, which implements conversions from references and slices as described in my previous comment. Since that change effectively implements the features from the original PR changes, I'm going to close this. Thanks!

olson-sean-k avatar Oct 20 '22 06:10 olson-sean-k