frunk icon indicating copy to clipboard operation
frunk copied to clipboard

Adds FuncMut and PolyMut, which parallel Func and Poly.

Open remexre opened this issue 5 years ago • 8 comments

This also makes HMappable<F> accept FnMut instead of just Fn; I don't believe this would be breaking, but I can split it out if it is.

remexre avatar Nov 21 '18 07:11 remexre

I definitely agree with taking the most general Fn-family bound where possible. (i.e. FnOnce on coproducts, FnMut on HLists).


For FuncMut, the picture is less clear. You can see some existing discussion here: https://github.com/lloydmeta/frunk/pull/106 . In that PR I discuss my own independent attempts at adding user-implementable function traits, and indeed I chose to add a trio of Fun/FunMut/FunOnce to parallel the stdlib. One of the big problems is that there is no ergonomical way to recover the Fun < FunMut < FunOnce subtrait tree, which only works well in Rust due to magically autogenerated impls.

The current choice of Func in frunk sidesteps the issue by not having the function even take a context. If you look closely, you'll see there is no &self argument and hence less impetus for adding a hierarchy of traits (though this no doubt comes at a great loss of power).


Now, your PR actually seems to have a neat solution to this that I'm not sure if we've tried or discussed before, which is that one can use different newtype wrappers Poly versus PolyMut to select which trait is the "primary" trait. I mentioned about a similar idea in this comment, and it seemed I wasn't too enthusiastic about it, but I can't recall why. Perhaps the terrain of design space is different, so long as we can outline precisely which use cases we are aiming to support and which ones we aren't.

ExpHP avatar Nov 21 '18 16:11 ExpHP

I'm using this so I can call a trait method on an HList of values that all belong to the same trait. The specific code is here:

https://github.com/remexre/csci5607-game/blob/75103f761dba59cbe2f67980cadf53a94df5e4ca/src/main.rs#L71-L73

https://github.com/remexre/csci5607-game/blob/75103f761dba59cbe2f67980cadf53a94df5e4ca/src/lib.rs#L53-L75

There's not really a huge reason to have PolyMut<F> where F: FuncMut over PolyMut<F> where F: FnMut, though, since I should be able to make SystemStepper into a function that returns impl 'a + FnMut instead of a struct.

remexre avatar Nov 21 '18 17:11 remexre

  • Generalizing to FnMut seems good; the order of calls don't seem confusing wrt. mutation.
  • We should add poly_fn_mut! to consistency?

Centril avatar Nov 22 '18 00:11 Centril

The question of consistency is currently a null point; Func takes no context, so poly_fn! provides no way to describe context (which is a fine limitation since it is difficult to do ergonomically).

Furthermore, @remexre's use case is a generic closure, which is even more monumentally difficult to make a comfortable interface around. It's only one step away from the most complicated example I have in my own experimental repo

unbox!{
    // Define a generic context
    pub struct Contains<'a, T>(&'a [T]) where T: 'a;

    // Make it a generic closure
    impl Fn <'b, U: 'b>(&self, u: &'b U) -> bool
    where T: PartialEq<U>,
    { self.0.iter().any(|t| t == u) }
}

(remexre's example is almost as complicated, but with a monomorphic context)


Honestly, in my own projects, I just write my own traits now for mapping over hlists in particular ways with certain types of context, since there's no generic solution I've found comfortable to use.

ExpHP avatar Nov 22 '18 01:11 ExpHP

Here are my recent thoughts:

  • If you've tried it and it works well for you, FuncMut and PolyMut are probably fine to add. My reasons for being against it are probably letting perfect be the enemy of good, and this is not normally the bar that frunk holds its features to. There are plenty of experimental features in frunk like transmogrification, which have nasty edge cases that are difficult or impossible to solve.

    If FuncMut/PolyMut isn't good enough for somebody's use case, they can create an issue and we can iterate on the design.

  • For consistency, Func should be revised to take a &self context in the next major version bump.

  • I stand firm on the position that poly_fn_mut! is unnecessary and too difficult to implement.

  • I'll go through and do a code review.

ExpHP avatar Nov 24 '18 16:11 ExpHP

I haven’t taken a thorough look (currently on vacation with spotty internet) but the proposed changes and feedback look reasonable to me :)

Also :+1: for poly_fn_mut! perhaps being too complex to implement nicely. It might be nice to have though, and I’m wondering now if we should have a frunk_extras module to hold things that are not-exactly-polished-but-still-useful like that sort of thing, along with Transmogifier, which we can iterate quickly (if needed, backward-breakingly) on as experiments.

lloydmeta avatar Nov 25 '18 03:11 lloydmeta

This has been sitting here for a while, should I just push the big green button?

ExpHP avatar Dec 14 '18 04:12 ExpHP

I need to add examples+tests still; it's finals week here so I'm gonna be a bit busy until the 19th or 20th; I should be able to do them then, though.

remexre avatar Dec 14 '18 04:12 remexre