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

jvm Future instances do not obey laws #589

Closed
ceedubs opened this issue Oct 27, 2015 · 9 comments
Closed

jvm Future instances do not obey laws #589

ceedubs opened this issue Oct 27, 2015 · 9 comments
Assignees
Milestone

Comments

@ceedubs
Copy link
Contributor

ceedubs commented Oct 27, 2015

Most of the type class instances in https://github.com/non/cats/blob/master/jvm/src/main/scala/cats/jvm/std/future.scala (Comonad, Eq, PartialOrder, Order) are not checked against their laws. If we were to check them against their laws they would fail, because they throw exceptions in the case of a failed future.

Furthermore, it isn't necessarily obvious how an Eq[Future[A]] instance should behave. The current one throws an exception if either Future fails. Consider the following scenario:

val fa: Future[Int] = Future.failed(new RuntimeException("foo"))

fa === fa

This last line throws an exception. One could reasonably expect it to return true instead. This does require creating an explicit Eq instance (with a supplied timeout), so it takes a little effort to get yourself into trouble. Still, I think that these instances belong in alleycats or tests but not in cats core. In addition to being unlawful, they aren't especially useful. Other than in tests, you probably always want to get back a Future[Boolean] when comparing two Future[A] values instead of blocking the thread waiting for both futures to complete.

@non
Copy link
Contributor

non commented Oct 28, 2015

Let's move all the unlawful stuff to alleycats (and also put an Eq[Future[A]] in tests).

@tpolecat
Copy link
Member

I think there's kind of a lot of subtlety involved with Future due to the way it's used in practice. But in principle I don't think any of this is a problem. Future has nothing to say about side-effects so it's really only fair to test it with referentially transparent expressions. Nontermination doesn't manifest until you force it, as with Eval.Later ... fa === fa above should not have an answer.

I think the real problem is that everyone uses Future to side-effect and nobody ever uses Await.result to get the value out, so treating it like a well-behaved data type is kind of disingenuous. I'm all in favor of banishing it to alleycats or maybe litterbox.

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 31, 2015

@non @inthenow I went to move these instances to alleycats, but it doesn't seem to have quite the same setup for JVM/JS that this project does. I'm not quite sure where this should go over there. Any pointers?

@non
Copy link
Contributor

non commented Oct 31, 2015

I think we want to mirror Cats' setup in Alleycats, but haven't had the time to do it yet.

Specifically, I imagine having alleycats.js and alleycats.jvm.

@ghost
Copy link

ghost commented Oct 31, 2015

The JS version of future started out in alleycats so no surprise to me that it's all going there,

I think we want to mirror Cats' setup in Alleycats

sbt-catalysts is ready to roll - how about we set it up in alleycats first?

@non
Copy link
Contributor

non commented Oct 31, 2015

@inthenow That's a great idea. Let's do it! 🐈 🔬

@ghost
Copy link

ghost commented Oct 31, 2015

👍 (Sunday)

@ghost ghost mentioned this issue Nov 1, 2015
@ceedubs
Copy link
Contributor Author

ceedubs commented Nov 15, 2015

@inthenow I see that non/alleycats#21 has been merged. Does that mean that alleycats is ready for this now?

@ghost
Copy link

ghost commented Nov 15, 2015

@ceedubs I think we need non/alleycats#22 in first, but otherwise yes.

ceedubs added a commit to ceedubs/cats that referenced this issue May 6, 2016
Fixes typelevel#589.

If anyone is set on using these, they can be added to alleycats.
However, I suspect that few to no people are using these. They all throw
exceptions on failed futures, which seems like it would be a
show-stopper in most codebases.
@ceedubs ceedubs self-assigned this May 6, 2016
@ceedubs ceedubs added this to the cats-1.0.0 milestone May 6, 2016
@ghost ghost mentioned this issue May 24, 2016
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

No branches or pull requests

3 participants