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

Add Bracket type class #113

Merged
merged 56 commits into from
Apr 16, 2018
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
a318a4c
Add initial bracket
Jan 10, 2018
1b8f273
Quick update
Jan 10, 2018
1f12aba
Add discipline bracket tests
Jan 12, 2018
75d8a43
Use Option for left side
Jan 12, 2018
a479184
Initial implementation WIP for EitherT and OptionT
Jan 14, 2018
8ffffcd
Move to ADT
Jan 15, 2018
e507a3a
Make Bracketresult abstract class
Jan 15, 2018
f2ee74f
Add Writer and State instances
Jan 15, 2018
3bb2106
Merge branch 'master' into add-bracket
Jan 18, 2018
43f5488
Merge branch 'master' into add-bracket
Jan 18, 2018
5e7119e
Laws
Jan 18, 2018
5fa474e
Merge branch 'add-bracket' of https://github.com/LukaJCB/cats-effect …
Jan 18, 2018
e4cc1fa
Fix 2.10 compiler error
Jan 18, 2018
4c2bea6
Remove equivalence law
Jan 21, 2018
595b1b0
Add extra law
Jan 25, 2018
cb53957
Add Test that checks if release is called no matter what use does
Jan 25, 2018
db757aa
Add Mima exceptions
Jan 25, 2018
87e2cec
Remove result type from bracketresult
Jan 31, 2018
96538fd
Revert "Remove result type from bracketresult"
Jan 31, 2018
859d50f
Merge branch 'add-bracket' of https://github.com/LukaJCB/cats-effect …
alexandru Feb 25, 2018
45edf70
Change Bracket
alexandru Feb 25, 2018
e3e730e
Merge pull request #1 from alexandru/add-bracket
Mar 1, 2018
e88bef3
Merge branch 'master' into add-bracket
Mar 1, 2018
6c41b50
Remove type lambda and add back flattap
Mar 5, 2018
0bd2149
Merge branch 'umaster' into add-bracket
Mar 9, 2018
d91060e
Raise exception when canceled during bracket
Mar 9, 2018
54e176c
Fix IO.bracket and use american spelling
Mar 16, 2018
590e6bd
Merge branch 'umaster' into add-bracket
Mar 16, 2018
7a38584
Move Bracket implementation to internals
Mar 20, 2018
1f2b8b2
Formatting
Mar 20, 2018
b994d83
Don't call fa twice in bracketReleaseLaw
Mar 20, 2018
6838b7e
Add law that describes what happens when both use and release fail
Mar 22, 2018
596c17a
Make law more generic
Mar 22, 2018
e26f667
Add first draft for Bracket-Concurrent law
Mar 22, 2018
05fd2e6
Merge branch 'master' into add-bracket
Mar 27, 2018
eadba2f
Use rightCast
Mar 27, 2018
d3b7879
Merge branch 'add-bracket' of https://github.com/LukaJCB/cats-effect …
Mar 27, 2018
a82d3fc
BracketResult final
Apr 1, 2018
c70cdec
Merge remote-tracking branch 'upstream/master' into LukaJCB-add-bracket
alexandru Apr 15, 2018
7a2b91b
Bracket changes
alexandru Apr 15, 2018
b6aebec
Fix sample to use IO instead of Task
alexandru Apr 15, 2018
e6c524c
Merge pull request #2 from alexandru/LukaJCB-add-bracket
Apr 15, 2018
00d6f5b
Fix MiMa errors
alexandru Apr 15, 2018
09f83c5
Add mima exception
Apr 15, 2018
646ec4c
Merge branch 'add-bracket' of https://github.com/LukaJCB/cats-effect …
Apr 15, 2018
05032dd
Merge branch 'add-bracket' of https://github.com/LukaJCB/cats-effect …
alexandru Apr 15, 2018
cd9c318
Rename bracketE -> bracketCase; add microsite documentation
alexandru Apr 15, 2018
796c519
Rename laws
alexandru Apr 15, 2018
aab8173
Merge pull request #3 from alexandru/LukaJCB-add-bracket2
Apr 15, 2018
8a4b74f
Merge branch 'master' into add-bracket
Apr 15, 2018
f536d79
Add mima exception for missing eitherT eval
Apr 15, 2018
84edccd
Merge branch 'add-bracket' of https://github.com/LukaJCB/cats-effect …
Apr 15, 2018
28a05b4
Typo
Apr 15, 2018
008f29f
More Mima exceptions
Apr 15, 2018
596f0cd
Make ExitCase serializable
Apr 15, 2018
5bbbea7
Add product
Apr 16, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,14 @@ val mimaSettings = Seq(
// - catsStateTLiftIO
// - catsWriterTLiftIO
exclude[MissingTypesProblem]("cats.effect.LiftIO$"),
exclude[MissingTypesProblem]("cats.effect.Effect$"),
exclude[IncompatibleTemplateDefProblem]("cats.effect.AsyncInstances"),
exclude[IncompatibleTemplateDefProblem]("cats.effect.IOInstances"),
exclude[ReversedMissingMethodProblem]("cats.effect.SyncInstances#WriterTSync.bracket"),
exclude[InheritedNewAbstractMethodProblem]("cats.effect.Bracket.bracket"),
exclude[ReversedMissingMethodProblem]("cats.effect.SyncInstances#StateTSync.bracket"),
exclude[ReversedMissingMethodProblem]("cats.effect.SyncInstances#OptionTSync.bracket"),
exclude[ReversedMissingMethodProblem]("cats.effect.SyncInstances#EitherTSync.bracket"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances$OptionTLiftIO"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances$KleisliLiftIO"),
Expand Down
45 changes: 45 additions & 0 deletions core/shared/src/main/scala/cats/effect/Bracket.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright (c) 2017-2018 The Typelevel Cats-effect Project Developers
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package cats
package effect

trait Bracket[F[_], E] extends MonadError[F, E] {
def bracket[A, B](acquire: F[A])(use: A => F[B])
(release: (A, BracketResult[E]) => F[Unit]): F[B]
}

sealed abstract class BracketResult[+E]

object BracketResult {
case object Completed extends BracketResult[Nothing]
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be final.

case class Error[+E](e: E) extends BracketResult[E]
case class Canceled[+E](e: Option[E]) extends BracketResult[E]
Copy link

Choose a reason for hiding this comment

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

I have asked this before, but I don’t see where we need to pass the E here. I don’t see us using this feature in this code. Has this changed at all with the introduction of Concurrent?

If we do need the Option E can we add a comment as to why here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for responding so late @johnynek, the Option[E] here, is so that we can support other cancelation schemes such as async exceptions where cancelation always requires an E. I'll add a comment 👍

Copy link
Member

Choose a reason for hiding this comment

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

N.B. our Concurrent type class does not admit signaling an error on cancellation like that, but I think it's a good idea to allow at least bracket to have it, because then we can admit implementations that need it, like the upcoming IO in Scalaz 8.

The cool thing about bracket is that it can work as a higher level Concurrent.cancelable builder. And it's not a big price to pay IMO.

Choose a reason for hiding this comment

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

I can see y’all really want this, but I would make the call of either always requiring it or never.

For instance can the cancelation type be a type member of MonadBracket? In a usual case it may be E but for some instances it can be E.

My concern is that it should not be dynamic, but statically known based on the Bracket. It feels like making this look like it depends on the data is the wrong way to go.

I think we could do this if we move this whole type inside trait MonadBracket.

Choose a reason for hiding this comment

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

Side note: if you don’t support cancelation you can make the Canceled wrap Nothing, which is proof it never happens.


def complete[E]: BracketResult[E] = Completed
def error[E](e: E): BracketResult[E] = Error[E](e)
def cancelled[E](e: Option[E] = None): BracketResult[E] = Canceled(e)
Copy link

Choose a reason for hiding this comment

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

Cats usually doesn’t use default parameters due to how to interacts with converting to lambdas or something I thought.

Should we have two methods, cancelled and maybe cancelledWith?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do!

Choose a reason for hiding this comment

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

Are there plans to pick a consistent spelling for canceled?

Copy link
Member

Choose a reason for hiding this comment

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

English hasn't picked a consistent spelling, but we should. Good catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is my mistake, thanks!


def attempt[E, A](value: Either[E, A]): BracketResult[E] =
value match {
case Left(e) => BracketResult.error(e)
case Right(_) => BracketResult.complete
}
}

object Bracket {
def apply[F[_], E](implicit ev: Bracket[F, E]): Bracket[F, E] = ev

Choose a reason for hiding this comment

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

want to implement MonadError for BracketResult? Maybe Foldable?

}
27 changes: 25 additions & 2 deletions core/shared/src/main/scala/cats/effect/IO.scala
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ import cats.effect.internals._
import cats.effect.internals.Callback.Extensions
import cats.effect.internals.TrampolineEC.immediate
import cats.effect.internals.IOPlatform.fusionMaxStackDepth

import scala.annotation.unchecked.uncheckedVariance
import scala.concurrent.{ExecutionContext, Future, Promise}
import scala.concurrent.{CancellationException, ExecutionContext, Future, Promise}
import scala.concurrent.duration._
import scala.util.{Failure, Left, Right, Success}

Expand Down Expand Up @@ -383,6 +384,19 @@ sealed abstract class IO[+A] extends internals.IOBinaryCompat[A] {
final def to[F[_]](implicit F: LiftIO[F]): F[A @uncheckedVariance] =
F.liftIO(this)

private final val cancelException = new CancellationException("cancel in bracket")

def bracket[B](use: A => IO[B])(release: (A, BracketResult[Throwable]) => IO[Unit]): IO[B] =
Copy link
Member

Choose a reason for hiding this comment

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

Note due to the cancellable IO changes, this needs a different implementation.

Fortunately I've already thought about it, by introducing onCancelRaiseError 😉

See implementation: https://github.com/monix/monix/blob/master/monix-eval/shared/src/main/scala/monix/eval/internal/TaskBracket.scala#L35

Copy link
Member Author

Choose a reason for hiding this comment

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

You're the best!

for {
a <- this
etb <- use(a).onCancelRaiseError(cancelException).attempt
Copy link
Member

Choose a reason for hiding this comment

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

io.attempt.flatMap is slow because it implies an extra bind, along with extra boxing and we can optimize this.

For IO specifically we have an optimization that isn't exposed via IOFrame. So an IOFrame can supply the A => IO[B] function that is needed by flatMap, BUT it can also supply an error handler.

See for example handleErrorWith that is effectively a flatMap on an IOFrame instance. Usage of an IOFrame is basically attempt.flatMap in one, without the extra bind and the extra boxing in Either.

What this means is that you can do the same thing that I did in Monix's TaskBracket.

Also I would move this whole implementation into an cats.effect.internal.IOBracket.

I love transparent, elegant FP implementations, but it's far better to kick some ass in terms of performance 😀

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely right 😄

_ <- release(a, etb match {
Copy link

Choose a reason for hiding this comment

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

Can we not use attempt on etb?

Copy link

Choose a reason for hiding this comment

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

BracketResult.attempt I mean.

Copy link

Choose a reason for hiding this comment

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

But why do we unconditionally send error on an exception rather than sending cancelled(Some(cancelException))

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, will need to fix.

case Left(e) => BracketResult.error[Throwable](e)
case Right(_) => BracketResult.complete
})
b <- IO.fromEither(etb)
} yield b

Choose a reason for hiding this comment

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

in library code, I prefer to write things the most efficient way, Wonder about all the use of fold and allocation of closures here. What about:

for {
  a <- this
  etb <- use(a).attempt
  _ <- release(a, etb)
  b <- IO.fromEither(etb)
} yield b

Also note, IO.fromEither would probably be better as:

def fromEither[A](e: Either[Throwable, A]): IO[A] =
  e match {
    case Right(a) => pure(a)
    case Left(err) => raiseError(err)
  }

and avoid the allocations of the closures.


override def toString = this match {
case Pure(a) => s"IO($a)"
case RaiseError(e) => s"IO(throw $e)"
Expand Down Expand Up @@ -486,6 +500,11 @@ private[effect] abstract class IOInstances extends IOLowPriorityInstances {
case Left(a) => tailRecM(a)(f)
case Right(b) => pure(b)
}

override def bracket[A, B](acquire: IO[A])
(use: A => IO[B])
(release: (A, BracketResult[Throwable]) => IO[Unit]): IO[B] =
acquire.bracket(use)(release)
}

implicit val ioParallel: Parallel[IO, IO.Par] =
Expand Down Expand Up @@ -776,7 +795,11 @@ object IO extends IOInstances {
* Lifts an Either[Throwable, A] into the IO[A] context raising the throwable
* if it exists.
*/
def fromEither[A](e: Either[Throwable, A]): IO[A] = e.fold(IO.raiseError, IO.pure)
def fromEither[A](e: Either[Throwable, A]): IO[A] =
Copy link
Member

@gvolpe gvolpe Mar 16, 2018

Choose a reason for hiding this comment

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

Might be nice to use fold here? Seems more concise and idiomatic.

def fromEither[A](e: Either[Throwable, A]): IO[A] = e.fold(raiseError, pure)

Copy link
Member Author

Choose a reason for hiding this comment

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

We recently got rid of all internal uses of fold since it incurs a moderate performance penalty :)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about it after I commented this XD Are there any benchmarks to back this decision? I'm interested in seeing how big (percentage-wise) is the performance cost since at work we are also using folds everywhere instead of pattern matching.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is an issue that includes some benchmarks: typelevel/cats#1951

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot!

e match {
case Right(a) => pure(a)
case Left(err) => raiseError(err)
}

/**
* Asynchronous boundary described as an effectful `IO`, managed
Expand Down
75 changes: 74 additions & 1 deletion core/shared/src/main/scala/cats/effect/Sync.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package cats
package effect

import simulacrum._
import cats.syntax.all._
import cats.data.{EitherT, IndexedStateT, OptionT, StateT, WriterT}
import cats.effect.internals.{AndThen, NonFatal}

Expand All @@ -26,7 +27,7 @@ import cats.effect.internals.{AndThen, NonFatal}
* in the `F[_]` context.
*/
@typeclass
trait Sync[F[_]] extends MonadError[F, Throwable] {
trait Sync[F[_]] extends Bracket[F, Throwable] {
/**
* Suspends the evaluation of an `F` reference.
*
Expand Down Expand Up @@ -69,6 +70,20 @@ object Sync {
def raiseError[A](e: Throwable) =
EitherT.left(Eval.now(e))

def bracket[A, B](acquire: EitherT[Eval, Throwable, A])
(use: A => EitherT[Eval, Throwable, B])
(release: (A, BracketResult[Throwable]) => EitherT[Eval, Throwable, Unit]): EitherT[Eval, Throwable, B] = {

acquire.flatMap { a =>
EitherT(FlatMap[Eval].flatTap(use(a).value){ etb =>
release(a, etb match {
case Left(e) => BracketResult.Error(e)
case Right(_) => BracketResult.Completed
}).value
})
}
}

def flatMap[A, B](fa: EitherT[Eval, Throwable, A])(f: A => EitherT[Eval, Throwable, B]): EitherT[Eval, Throwable, B] =
fa.flatMap(f)

Expand Down Expand Up @@ -126,6 +141,23 @@ object Sync {
def raiseError[A](e: Throwable): EitherT[F, L, A] =
EitherT.liftF(F.raiseError(e))

def bracket[A, B](acquire: EitherT[F, L, A])
(use: A => EitherT[F, L, B])
(release: (A, BracketResult[Throwable]) => EitherT[F, L, Unit]): EitherT[F, L, B] = {

EitherT(F.bracket(acquire.value) {
case Right(a) => use(a).value
case e @ Left(_) => F.pure(e.asInstanceOf[Either[L, B]])

Choose a reason for hiding this comment

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

Can we use rightCast here (a cats syntactic enrichment) which is safe. This may be safe, but each reader has to make sure you didn’t get the L type wrong.

} { (ea, br) =>
ea match {
case Right(a) =>
release(a, br).value.map(_ => ())
case Left(_) =>
F.unit // nothing to release
}
})
}

def flatMap[A, B](fa: EitherT[F, L, A])(f: A => EitherT[F, L, B]): EitherT[F, L, B] =
fa.flatMap(f)

Expand All @@ -147,6 +179,21 @@ object Sync {
def raiseError[A](e: Throwable): OptionT[F, A] =
OptionT.catsDataMonadErrorForOptionT[F, Throwable].raiseError(e)

def bracket[A, B](acquire: OptionT[F, A])
(use: A => OptionT[F, B])
(release: (A, BracketResult[Throwable]) => OptionT[F, Unit]): OptionT[F, B] = {

OptionT(F.bracket(acquire.value) {
case Some(a) => use(a).value
case None => F.pure[Option[B]](None)
} {
case (Some(a), br) =>
release(a, br).value.map(_ => ())
case _ =>
F.unit
})
}

def flatMap[A, B](fa: OptionT[F, A])(f: A => OptionT[F, B]): OptionT[F, B] =
fa.flatMap(f)

Expand All @@ -168,6 +215,19 @@ object Sync {
def raiseError[A](e: Throwable): StateT[F, S, A] =
StateT.liftF(F.raiseError(e))

def bracket[A, B](acquire: StateT[F, S, A])
(use: A => StateT[F, S, B])
(release: (A, BracketResult[Throwable]) => StateT[F, S, Unit]): StateT[F, S, B] = {

StateT { startS =>
F.bracket(acquire.run(startS)) { case (s, a) =>
use(a).run(s)
} { case ((s, a), br) =>
release(a, br).run(s).map(_ => ())
}
}
}

override def map[A, B](fa: StateT[F, S, A])(f: A => B): StateT[F, S, B] =
IndexedStateT.applyF[F, S, S, B](F.map(fa.runF) { safsba =>
AndThen(safsba).andThen(fa => F.map(fa) { case (s, a) => (s, f(a)) })
Expand Down Expand Up @@ -202,6 +262,19 @@ object Sync {
def raiseError[A](e: Throwable): WriterT[F, L, A] =
WriterT.catsDataMonadErrorForWriterT[F, L, Throwable].raiseError(e)

def bracket[A, B](acquire: WriterT[F, L, A])
(use: A => WriterT[F, L, B])
(release: (A, BracketResult[Throwable]) => WriterT[F, L, Unit]): WriterT[F, L, B] = {

acquire.flatMap { a =>
WriterT(
F.bracket(F.pure(a))(use.andThen(_.run)){ (a, res) =>
release(a, res).value
}
)
}
}

def flatMap[A, B](fa: WriterT[F, L, A])(f: A => WriterT[F, L, B]): WriterT[F, L, B] =
fa.flatMap(f)

Expand Down
41 changes: 41 additions & 0 deletions laws/shared/src/main/scala/cats/effect/laws/BracketLaws.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright (c) 2017-2018 The Typelevel Cats-effect Project Developers
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package cats
package effect
package laws

import cats.implicits._
import cats.laws._

trait BracketLaws[F[_], E] extends MonadErrorLaws[F, E] {

Choose a reason for hiding this comment

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

can you add a comment giving an example of why all MonadError does not satify these laws.

implicit def F: Bracket[F, E]

def bracketWithPureUnitIsEqvMap[A, B](fa: F[A], f: A => B) =
F.bracket(fa)(a => f(a).pure[F])((_, _) => F.unit) <-> F.map(fa)(f)

def bracketWithPureUnitIsEqvFlatMap[A, B](fa: F[A], f: A => F[B]) =
F.bracket(fa)(f)((_, _) => F.unit) <-> F.flatMap(fa)(f)

Choose a reason for hiding this comment

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

can we simplify the laws a bit and have the following and maybe a similar one for cancelation:

  def bracketWithFailureIsEqvToRunningRecover[A, B](a: F[A], err: E, recover: F[Unit]) =
     F.bracket(a)(_ => F.raiseError[B](err))((_, _) => recover) <-> (a *> F.raiseError[B](err)).handleErrorWith { e => recover *> F.raiseError[B](err) }

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @johnynek, I've been trying to build up the two laws you're talking about in the SyncLaws, since that's where we can much easier observe effects.
However, testing is currently still difficult and I'm pretty time constrained.

def bracketFailureInAcquisitionRemainsFailure[A, B](e: E, f: A => F[B], release: F[Unit]) =
F.bracket(F.raiseError[A](e))(f)((_, _) => release) <-> F.raiseError(e)
Copy link
Member

Choose a reason for hiding this comment

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

I think we may need a law like this:

def someLawName[A, B](acquire: F[A], use: A => F[B], release: F[Unit]) = {
  val expected = acquire.flatMap(a => use.attempt.flatMap {
    case Left(e) => release.flatMap(_ => F.raiseError(e))
    case Right(a) => release.map(_ => a)
  })

  F.bracket(acquire)(use)(release) <-> expected
} 

I think it's interesting here what happens if both use and release fail. Haskell for example emits the error of use, not of release. Should we require that too?

Also we can't observe side effects in release, not without dealing with Sync. So maybe we can add extra laws in Sync. But we need to ensure that we can cover everything we can in Bracket.

Copy link
Member

Choose a reason for hiding this comment

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

Btw, I see there are now laws in Sync too, so ignore that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexandru I thought the consensus was that bracket cannot be implemented as a combinator like in your proposed law (discussion here: typelevel/cats#1662)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yea, you might be right.

}

object BracketLaws {
def apply[F[_], E](implicit F0: Bracket[F, E]): BracketLaws[F, E] = new BracketLaws[F, E] {
val F = F0
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import scala.Predef.{identity => id}
trait ConcurrentLaws[F[_]] extends AsyncLaws[F] {
implicit def F: Concurrent[F]

//TODO Add law that ensures Bracket works with cancelation

def asyncCancelableCoherence[A](r: Either[Throwable, A]) = {
F.async[A](cb => cb(r)) <-> F.cancelable[A] { cb => cb(r); IO.unit }
}
Expand Down
12 changes: 11 additions & 1 deletion laws/shared/src/main/scala/cats/effect/laws/SyncLaws.scala
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,19 @@ package laws
import cats.implicits._
import cats.laws._

trait SyncLaws[F[_]] extends MonadErrorLaws[F, Throwable] {
trait SyncLaws[F[_]] extends BracketLaws[F, Throwable] {
implicit def F: Sync[F]

def bracketReleaseIsAlwaysCalled[A, B](fa: F[A], fb: F[B], g: A => A, a1: A) = {

var input = a1
val update = F.delay { input = g(input) }
val read = F.delay(input)

F.bracket(fa)(_ => fb)((a, _) => update) *> read <-> fa *> fb *> F.pure(g(a1))
Copy link
Member Author

Choose a reason for hiding this comment

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

I missed the fact here that if fb fails then read won't get called and neither will F.pure(g(a1)), so this isn't complete.

}


def delayConstantIsPure[A](a: A) =
F.delay(a) <-> F.pure(a)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright (c) 2017-2018 The Typelevel Cats-effect Project Developers
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package cats
package effect
package laws
package discipline

import cats.data._
import cats.laws.discipline._
import cats.laws.discipline.SemigroupalTests.Isomorphisms
import org.scalacheck._
import Prop.forAll

trait BracketTests[F[_], E] extends MonadErrorTests[F, E] {
def laws: BracketLaws[F, E]

def bracket[A: Arbitrary: Eq, B: Arbitrary: Eq, C: Arbitrary: Eq](
implicit
ArbFA: Arbitrary[F[A]],
ArbFB: Arbitrary[F[B]],
ArbFC: Arbitrary[F[C]],
ArbFU: Arbitrary[F[Unit]],
ArbFAtoB: Arbitrary[F[A => B]],
ArbFBtoC: Arbitrary[F[B => C]],
ArbE: Arbitrary[E],
CogenA: Cogen[A],
CogenB: Cogen[B],
CogenC: Cogen[C],
CogenE: Cogen[E],
EqFA: Eq[F[A]],
EqFB: Eq[F[B]],
EqFC: Eq[F[C]],
EqE: Eq[E],
EqFEitherEU: Eq[F[Either[E, Unit]]],
EqFEitherEA: Eq[F[Either[E, A]]],
EqEitherTFEA: Eq[EitherT[F, E, A]],
EqFABC: Eq[F[(A, B, C)]],
EqFInt: Eq[F[Int]],
iso: Isomorphisms[F]): RuleSet = {
new RuleSet {
val name = "bracket"
val bases = Nil
val parents = Seq(monadError[A, B, C])

val props = Seq(
"bracket with pure unit on release is eqv to map" -> forAll(laws.bracketWithPureUnitIsEqvMap[A, B] _),
"bracket with failure in acquisition remains failure" -> forAll(laws.bracketFailureInAcquisitionRemainsFailure[A, B] _),
"bracket with pure unit on release is eqv to flatMap" -> forAll(laws.bracketWithPureUnitIsEqvFlatMap[A, B] _)
)

Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: get rid of extra newlines.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

}
}
}

object BracketTests {
def apply[F[_], E](implicit ev: Bracket[F, E]): BracketTests[F, E] = new BracketTests[F, E] {
def laws = BracketLaws[F, E]
}
}
Loading