Future result

[steinybot]

An implementation of hedgehog.core.PropertyTReporting#reportF to support properties of type PropertyT[F[Result]].

This requires the Monads to implement a stack safe implementation of tailRecM which I copied from cats. It is based on Stack Safety for Free.

Most of the current Monads implement this using bind which is not stack safe. I'm not too sure what to do about these.

Remaining things to do are:

• Add a Monad[Future] instance.
• Resolve stack safety issues
• Do we need to provide a default implementation of tailRecM to maintain binary compatibility? What is the current approach to changes like these?
• Consider casting the Id types
• Remove incidental whitespace changes
• Add documentation and examples

Closes #146

[charleso]

Wow, this is amazing. Thank you!

Remove incidental whitespace changes

💯

Add a Monad[Future] instance.

Did Michael implement one on his branch that you/we can use?

Do we need to provide a default implementation of tailRecM to maintain binary compatibility? What is the current approach to changes like these?

I've been very slack thus far. But it's certainly something I'd like to start taking more seriously if people are actually using the library.

As you're the biggest actual user of the library (that I know of) I'm happy to defer to you on how much you personally think it's a problem.

Consider casting the Id types

I'm not against that, although I do flinch every time I see asInstanceOf.

Resolve stack safety issues

Yeah I hadn't considered that part. Good pick up.

Just on Identity. Originally it was critical because of F[_] being used everywhere, although I won't say it was chosen with any deep knowledge in mind, just that it was the first thing I tried/used and it worked so I didn't think deeply about it until I ran in to performance issues. Since I converted to LazyList it's only used in the Tree itself. At the time I was trying to make hedgehog handle larger shrinking (in the SBT tests), but I'm afraid I can't remember if I left Identity in Tree to help significantly or not. I would be happy to remove or alter if it's not actually doing anything useful.

Dumb question, is it hard to add stack-safe instances for the current set of Monads?

Slightly related and dumb question, can tailRecM be used for traverse/sequence as well? Glancing at cats it uses Eval so I'm not sure if we would need that as well to fix the stack overflow there.

While I'm afraid, as usual, I'm not able to help on the coding side of things, but is there anything that I can do to help here otherwise?

Thanks again!!!

[steinybot]

Did Michael implement one on his branch that you/we can use?

Yep he did.

Regarding binary compatibility I don't think we should worry about it right now. There is already the comment that the API is unstable.

I hope to pickup the CI task soon so that we can make the version number explicit in the 0.x range. There are quite a few things I think we should at the very least make private.

I'm not against that, although I do flinch every time I see asInstanceOf.

Haha same here. I took another look and I don't think that the map/cast is necessary if the type is given explicitly.

Just on Identity. Originally it was critical because of F[_] being used everywhere, although I won't say it was chosen with any deep knowledge in mind, just that it was the first thing I tried/used and it worked so I didn't think deeply about it until I ran in to performance issues. Since I converted to LazyList it's only used in the Tree itself. At the time I was trying to make hedgehog handle larger shrinking (in the SBT tests), but I'm afraid I can't remember if I left Identity in Tree to help significantly or not. I would be happy to remove or alter if it's not actually doing anything useful.

I haven't taken a look too closely although when I first saw it there was already a LazyList which appeared to give the required laziness. My gut feeling is that it would be safe to remove Identity but I will have to write some tests and see what happens.

Dumb question, is it hard to add stack-safe instances for the current set of Monads?

Not dumb at all. I actually don't know. I only just came across this stack safety stuff now. I was using the naive implementation that just used bind but I noticed it wasn't stack safe in the case of Identity. I'm not entirely convinced that my current tests are correct. Cats manages to do it for all its Monads so I think it must be possible.

Slightly related and dumb question, can tailRecM be used for traverse/sequence as well? Glancing at cats it uses Eval so I'm not sure if we would need that as well to fix the stack overflow there.

They do seem similar although I can't work out how you would do it. What I came to is that A in tailRecM is thing that you are recursing over whereas in traverse it is F[A]. I don't know how to formalise that.

While I'm afraid, as usual, I'm not able to help on the coding side of things, but is there anything that I can do to help here otherwise?

Your feedback is great, I really appreciate it. Don't hesitate to tell me if you dislike my approach or if you think there is a better way of doing things.

[charleso]

My gut feeling is that it would be safe to remove Identity but I will have to write some tests and see what happens.

Yeah if you could that would be great. I have a bad feeling it made an improvement on large tests, and I'm sorry I didn't capture it. I was testing on the SBT project but you might already have something that stresses hedgehog enough to notice.

I only just came across this stack safety stuff now.

Yeah I only noticed it a few weeks ago not that I'm using cats in anger.

They do seem similar although I can't work out how you would do it. What I came to is that A in tailRecM is thing that you are recursing over whereas in traverse it is F[A]. I don't know how to formalise that.

I was thinking about it and I wonder if you could just make it work for sequence by passing on the rest of the list to process?

def sequence[F[_], A](l: List[F[A]])(implicit M: Monad[F]): F[List[A]] =
  M.tailRecM(l) {
    case Nil =>
      M.point(Right(Nil))
    case h :: t => 
       // TODO Something in here?!?
  }

Anyway, not strictly required for this PR, but it'd be great if it just fell out of using that approach.

[mpilquist]

@steinybot Can I help to get this one over the finish line?

[steinybot]

@mpilquist You are more than welcome to address any of the items in the list above. I'll pick this back up again tomorrow.

[mpilquist]

@steinybot Thanks - I sent a PR to your fork but it’s major wip and I need a break from stack safety issues. :) Feel free to take as little or as much as is helpful.

BTW, I tried removing Identity but the laziness it provides is important so I backed that out. I also tried porting to cats to see if things like Eval, map2Eval, and stack safe traverse would help but still ran in to issues with ap on Applicative[Gen]. Makes me think we need to change the formulation of Gen to include a Gosub/Bind node.

[charleso]

Thanks so much guys, I'm sorry you're having to deep-dive so much to make this change possible. I was going to suggest inlining a version Eval but sounds like it's not enough? :(

[steinybot]

I did manage to get rid of Identity and replace it with a by name parameter instead.

I tried removing the laziness from children and just having LazyList but the performance went through the floor. I think I even ended up with memory errors if I waited long enough. I never figured out why this would be the case.

It may still be the case that we need Eval in other places.

[charleso]

Yeah I found the LazyList in its current form was critical to avoiding a memory leak (when shrinking kicks in). I'm sure there's probably other ways of achieving the same thing. The only small suggestion I have is to see what the F# version does which is quite mature. As always sorry I can't actually do anything useful :(