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.

Sign up for GitHub

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

Jump to bottom

MonadError[StateT[F, S, ?]] doesn't handle errors in outer effect #1640

Closed
djspiewak opened this issue Apr 28, 2017 · 8 comments
Closed

MonadError[StateT[F, S, ?]] doesn't handle errors in outer effect #1640

djspiewak opened this issue Apr 28, 2017 · 8 comments
Assignees
@ceedubs
Labels
bug
Milestone
1.0.0-MF

Comments

@djspiewak
Copy link
Member

It seems like a more correct implementation would be the following:

    def handleErrorWith[A](st: StateT[F, S, A])(f: Throwable => StateT[F, S, A]): StateT[F, S, A] = {
      val handled = F.handleErrorWith(st.runF) { t =>
        F.map(f(t).runF) { sf => s: S =>
          F.handleErrorWith(sf(s)) { t =>
            f(t).run(s)
          }
        }
      }

      StateT.applyF(handled)
    }

Specifically, if the outer F in the StateT is raiseError, it will not be handled.
@peterneyens
Copy link
Collaborator

Whoops, I think am the one to blame. Do you want to open a PR?

@djspiewak
Copy link
Member Author

It's on my stack of things. If it sits for a few days, I'll probably get to it. :-)

@djspiewak
Copy link
Member Author

As a sidebar, the Arbitrary for StateT defined here ensures that we could never possibly catch this issue in the laws. I'm relatively certain that fixing this will reveal the bug.

@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 29, 2017
@kailuowang
Copy link
Contributor

related #1616

@ceedubs
Copy link
Contributor

ceedubs commented Jun 1, 2017

I'm looking into this.

@ceedubs ceedubs self-assigned this Jun 1, 2017
@ceedubs ceedubs added the bug label Jun 1, 2017
ceedubs added a commit to ceedubs/cats that referenced this issue Jun 1, 2017
@ceedubs
Improve StateT Arbitrary instance
06011a1 
As pointed out [here](typelevel#1640 (comment))

This reveals law violations in `StateT`. It looks like `flatMap`/`ap`
consistency does not hold, as well as `MonadCombine`
right-distributivity. It's not immediately clear to me how to fix this,
so I'm opening this up in case somebody else gets to it first.
@ceedubs ceedubs mentioned this issue Jun 1, 2017
Merged
@kailuowang
Copy link
Contributor

removing from the 1.0.0-MF project since it's going to continue on #1714

@kailuowang
Copy link
Contributor

Not sure how to repeat this bug. The following test passes on both mater and 0.9.0

    val s = new StateT(None: Option[Int => Option[(Int, Int)]])
        .handleErrorWith((_: Unit) => StateT.pure(0))
    s.run(3) should === (Some((3, 0)))

@ceedubs
Copy link
Contributor

ceedubs commented Mar 24, 2018

I'm pretty sure that this is resolved, so I'm going to close this out. @djspiewak please speak up if I'm mistaken.

@ceedubs ceedubs closed this as completed Mar 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ceedubs ceedubs

Labels
bug
Projects
None yet
Milestone
1.0.0-MF
Development

No branches or pull requests
4 participants
@djspiewak @kailuowang @ceedubs @peterneyens