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.

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

Adds FuncMut and PolyMut, which parallel Func and Poly. #137

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

Conversation

remexre
Copy link
Contributor

@remexre remexre commented Nov 21, 2018

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.

@ExpHP
Copy link
Collaborator

ExpHP commented Nov 21, 2018

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: #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.

@remexre
Copy link
Contributor Author

remexre commented Nov 21, 2018

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.

@Centril
Copy link
Collaborator

Centril commented Nov 22, 2018

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

@ExpHP
Copy link
Collaborator

ExpHP commented Nov 22, 2018

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
Copy link
Collaborator

ExpHP commented Nov 24, 2018

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.

Copy link
Collaborator

@ExpHP ExpHP left a comment

Choose a reason for hiding this comment

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

Looks good to me, could just use some examples/tests.

core/src/traits.rs Show resolved Hide resolved
@@ -1143,11 +1158,11 @@ where
impl<F, H, Tail, Acc> HFoldLeftable<F, Acc> for HCons<H, Tail>
where
Tail: HFoldLeftable<F, Acc>,
F: Fn(Acc, H) -> Acc,
F: FnMut(Acc, H) -> Acc,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a test for this

@@ -943,12 +958,12 @@ impl<F> HMappable<F> for HNil {

impl<F, R, H, Tail> HMappable<F> for HCons<H, Tail>
where
F: Fn(H) -> R,
F: FnMut(H) -> R,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a test for this

/// ```
/// # #[macro_use]
/// # extern crate frunk;
/// # use frunk::{FuncMut, PolyMut};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because some items are exported under multiple paths, I've been making it a point for doc examples to always show the intended paths for using stuff from frunk, so unhide this.

Likewise for the #[macro_use] extern crate

/// }
/// }
///
/// fn main() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hide the fn main() {.

There's some examples of how I would format an example like this in
https://github.com/lloydmeta/frunk/blob/master/src/validated.rs

/// fn main() {
/// let mut string = String::new();
///
/// {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really can't wait for NLL...

@lloydmeta
Copy link
Owner

lloydmeta commented Nov 25, 2018

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

Also 👍 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.

@ExpHP
Copy link
Collaborator

ExpHP commented Dec 14, 2018

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

@remexre
Copy link
Contributor Author

remexre commented Dec 14, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants