From 4831ef51a7b46b6ba4b8e120d5ff2ba66d8f7bac Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Thu, 6 Sep 2012 19:11:22 -0400 Subject: Fix for SI-6333 - Try throws from combinators. * Added more comprehensive tests to Try. * Delineated what methods do and don't catch exceptions in docs. * Fixed combinator methods that should catch exceptions. --- src/library/scala/util/Try.scala | 46 ++++++++++++++++++++++++++++------------ test/files/run/t6333.scala | 29 +++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 13 deletions(-) create mode 100644 test/files/run/t6333.scala diff --git a/src/library/scala/util/Try.scala b/src/library/scala/util/Try.scala index f381a18b0c..39500d7aaa 100644 --- a/src/library/scala/util/Try.scala +++ b/src/library/scala/util/Try.scala @@ -52,6 +52,8 @@ import language.implicitConversions * ''Note'': only non-fatal exceptions are caught by the combinators on `Try` (see [[scala.util.control.NonFatal]]). * Serious system errors, on the other hand, will be thrown. * + * ''Note:'': all Try combinators will catch exceptions and return failure unless otherwise specified in the documentation. + * * `Try` comes to the Scala standard library after years of use as an integral part of Twitter's stack. * * @author based on Twitter's original implementation in com.twitter.util. @@ -68,12 +70,19 @@ sealed abstract class Try[+T] { def isSuccess: Boolean /** Returns the value from this `Success` or the given `default` argument if this is a `Failure`. + * + * ''Note:'': This will throw an exception if it is not a success and default throws an exception. */ - def getOrElse[U >: T](default: => U) = if (isSuccess) get else default + def getOrElse[U >: T](default: => U): U = + if (isSuccess) get else default /** Returns this `Try` if it's a `Success` or the given `default` argument if this is a `Failure`. */ - def orElse[U >: T](default: => Try[U]) = if (isSuccess) this else default + def orElse[U >: T](default: => Try[U]): Try[U] = + try if (isSuccess) this else default + catch { + case NonFatal(e) => Failure(e) + } /** Returns the value from this `Success` or throws the exception if this is a `Failure`. */ @@ -81,6 +90,8 @@ sealed abstract class Try[+T] { /** * Applies the given function `f` if this is a `Success`, otherwise returns `Unit` if this is a `Failure`. + * + * ''Note:'' If `f` throws, then this method may throw an exception. */ def foreach[U](f: T => U): Unit @@ -114,7 +125,7 @@ sealed abstract class Try[+T] { /** * Returns `None` if this is a `Failure` or a `Some` containing the value if this is a `Success`. */ - def toOption = if (isSuccess) Some(get) else None + def toOption: Option[T] = if (isSuccess) Some(get) else None /** * Transforms a nested `Try`, ie, a `Try` of type `Try[Try[T]]`, @@ -131,15 +142,21 @@ sealed abstract class Try[+T] { /** Completes this `Try` by applying the function `f` to this if this is of type `Failure`, or conversely, by applying * `s` if this is a `Success`. */ - def transform[U](s: T => Try[U], f: Throwable => Try[U]): Try[U] = this match { - case Success(v) => s(v) - case Failure(e) => f(e) - } + def transform[U](s: T => Try[U], f: Throwable => Try[U]): Try[U] = + try this match { + case Success(v) => s(v) + case Failure(e) => f(e) + } catch { + case NonFatal(e) => Failure(e) + } } object Try { - + /** Constructs a `Try` using the by-name parameter. This + * method will ensure any non-fatal exception is caught and a + * `Failure` object is returned. + */ def apply[T](r: => T): Try[T] = { try { Success(r) } catch { case NonFatal(e) => Failure(e) @@ -152,16 +169,20 @@ final case class Failure[+T](val exception: Throwable) extends Try[T] { def isFailure: Boolean = true def isSuccess: Boolean = false def recoverWith[U >: T](f: PartialFunction[Throwable, Try[U]]): Try[U] = - if (f.isDefinedAt(exception)) f(exception) else this + try { + if (f isDefinedAt exception) f(exception) else this + } catch { + case NonFatal(e) => Failure(e) + } def get: T = throw exception def flatMap[U](f: T => Try[U]): Try[U] = Failure[U](exception) def flatten[U](implicit ev: T <:< Try[U]): Try[U] = Failure[U](exception) - def foreach[U](f: T => U): Unit = {} + def foreach[U](f: T => U): Unit = () def map[U](f: T => U): Try[U] = Failure[U](exception) def filter(p: T => Boolean): Try[T] = this - def recover[U >: T](rescueException: PartialFunction[Throwable, U]): Try[U] = { + def recover[U >: T](rescueException: PartialFunction[Throwable, U]): Try[U] = try { - if (rescueException.isDefinedAt(exception)) { + if (rescueException isDefinedAt exception) { Try(rescueException(exception)) } else { this @@ -169,7 +190,6 @@ final case class Failure[+T](val exception: Throwable) extends Try[T] { } catch { case NonFatal(e) => Failure(e) } - } def failed: Try[Throwable] = Success(exception) } diff --git a/test/files/run/t6333.scala b/test/files/run/t6333.scala new file mode 100644 index 0000000000..266d95ce69 --- /dev/null +++ b/test/files/run/t6333.scala @@ -0,0 +1,29 @@ +object Test extends App { + import util.Try + + val a = "apple" + def fail: String = throw new Exception("Fail!") + def argh: Try[String] = throw new Exception("Argh!") + + // No throw tests + def tryMethods(expr: => String): Unit = { + Try(expr) orElse argh + Try(expr).transform(_ => argh, _ => argh) + Try(expr).recoverWith { case e if (a == fail) => Try(a) } + Try(expr).recoverWith { case _ => argh } + Try(expr).getOrElse(a) + // TODO - Fail getOrElse? + Try(expr) orElse argh + Try(expr) orElse Try(a) + Try(expr) map (_ => fail) + Try(expr) map (_ => a) + Try(expr) flatMap (_ => argh) + Try(expr) flatMap (_ => Try(a)) + Try(expr) filter (_ => throw new Exception("O NOES")) + Try(expr) filter (_ => true) + Try(expr) recover { case _ => fail } + Try(expr).failed + } + tryMethods(a) + tryMethods(fail) +} -- cgit v1.2.3 From ed04e04a42010eccbe57fa6a6efa7a08334ca9ed Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Thu, 6 Sep 2012 20:04:22 -0400 Subject: Minor code style alterations and performance fixes. Specifically, avoid reinstantiating an immutable object to alter the type parameter *IF* that type parameter has nothing to do with the contents of the object. --- src/library/scala/util/Try.scala | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/library/scala/util/Try.scala b/src/library/scala/util/Try.scala index 39500d7aaa..7afbfcdd66 100644 --- a/src/library/scala/util/Try.scala +++ b/src/library/scala/util/Try.scala @@ -157,11 +157,10 @@ object Try { * method will ensure any non-fatal exception is caught and a * `Failure` object is returned. */ - def apply[T](r: => T): Try[T] = { - try { Success(r) } catch { + def apply[T](r: => T): Try[T] = + try Success(r) catch { case NonFatal(e) => Failure(e) } - } } @@ -175,18 +174,16 @@ final case class Failure[+T](val exception: Throwable) extends Try[T] { case NonFatal(e) => Failure(e) } def get: T = throw exception - def flatMap[U](f: T => Try[U]): Try[U] = Failure[U](exception) - def flatten[U](implicit ev: T <:< Try[U]): Try[U] = Failure[U](exception) + def flatMap[U](f: T => Try[U]): Try[U] = this.asInstanceOf[Try[U]] + def flatten[U](implicit ev: T <:< Try[U]): Try[U] = this.asInstanceOf[Try[U]] def foreach[U](f: T => U): Unit = () - def map[U](f: T => U): Try[U] = Failure[U](exception) + def map[U](f: T => U): Try[U] = this.asInstanceOf[Try[U]] def filter(p: T => Boolean): Try[T] = this def recover[U >: T](rescueException: PartialFunction[Throwable, U]): Try[U] = try { if (rescueException isDefinedAt exception) { Try(rescueException(exception)) - } else { - this - } + } else this } catch { case NonFatal(e) => Failure(e) } @@ -197,7 +194,7 @@ final case class Failure[+T](val exception: Throwable) extends Try[T] { final case class Success[+T](value: T) extends Try[T] { def isFailure: Boolean = false def isSuccess: Boolean = true - def recoverWith[U >: T](f: PartialFunction[Throwable, Try[U]]): Try[U] = Success(value) + def recoverWith[U >: T](f: PartialFunction[Throwable, Try[U]]): Try[U] = this def get = value def flatMap[U](f: T => Try[U]): Try[U] = try f(value) -- cgit v1.2.3