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

for comprehensions seem broken #49

Open
Profpatsch opened this issue May 31, 2016 · 6 comments
Open

for comprehensions seem broken #49

Profpatsch opened this issue May 31, 2016 · 6 comments
Labels

Comments

@Profpatsch
Copy link

Profpatsch commented May 31, 2016

I noticed this strange behaviour:

    val foo = for {
      p1 <- managed(UIPeer(ID("1")))
    } yield "foo"
    foo.either.right.get

That returns "foo", as expected.

    val foo = for {
      p1 <- managed(UIPeer(ID("1")))
      p2 <- managed(UIPeer(ID("2")))
    } yield "foo"
    foo.either.right.get
[error]  value either is not a member of resource.ManagedResource[String]
[error]     foo.either.right.get
[error]         ^

Compare:

    val foo = for {
      b1 <- Some("foo")
      b2 <- Some("baz")
    } yield "bar"
    foo.get

works just fine.

Maybe the map and/or flatMap implementation is broken (i.e. not following the monad laws)?

@jsuereth
Copy link
Owner

I finally figured this one out. IT's the difference between "ExtractableManagedResource" and a vanilla "ManagedResource". I think the library is assuming the value from teh for-expression is still "raw", i.e. you are trying to leak a resource out of the for-expression.

ManagedResource is not a Monad in the sense of "the category of all types", a.k.a. Hask. It is only a monad in a limited subset of types + functions, hence the laws only apply within this subset. However, I do think this is a bug and I know how to fix it.

@jsuereth jsuereth added the Bug label Nov 15, 2016
@jsuereth
Copy link
Owner

To be clear, this line needs to change: https://github.com/jsuereth/scala-arm/blob/master/src/main/scala/resource/ManagedResource.scala#L56 and should return "ExtractableManagedResource"

@EronWright
Copy link

@jsuereth would you consider making this change now?

@pseiferth
Copy link

@jsuereth would you consider making this change now? :)

@cchantep
Copy link
Collaborator

@jsuereth If I understand that would mean a breaking change as updating the return type of flatMap? Could only be done if bumping to a new majorversions?

@jsuereth
Copy link
Owner

Yes, this would mean bumping major version.

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

No branches or pull requests

5 participants