feature: Add a standard Result[A] type for exception-aware composition#489
Conversation
Introduce wvlet.uni.util.Result[A], a Rust-flavored single-parameter result type that wraps exceptions as values. Its combinators (map, flatMap, recover, recoverWith, ...) mirror Rx so users can switch between the two vocabularies mechanically. Rx gains a materialize method that reifies each event as a Result, plus Rx.fromResult and a Result.toRx extension for round-tripping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, materialize reused transform which converted OnError(e) into OnNext(Result.Failure(e)) but left the stream unterminated, so downstream operators waiting for completion (toSeq, lastOption) would hang on errored sources. Introduce a dedicated MaterializeOp with a runner case that emits OnCompletion after the reified error value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a standard Result[A] type to uni-core, providing a Rust-flavored container for computations that may fail with a Throwable. The implementation includes core monadic combinators, recovery methods, and extensive interop with Rx, Try, Either, and Option. Feedback focuses on aligning the implementation with the design document's package structure, optimizing the recover method by using pf.lift to avoid double evaluation, and ensuring mapError captures exceptions thrown during error transformation to maintain consistent failure propagation.
| Placed at `uni-core/src/main/scala/wvlet/uni/Result.scala` (root of | ||
| `wvlet.uni` inside `uni-core`), so it sits alongside foundational types and | ||
| is reachable from `import wvlet.uni.Result`. |
There was a problem hiding this comment.
The design document specifies that Result is placed at wvlet.uni.Result and reachable via import wvlet.uni.Result. However, the implementation in this PR places it in the wvlet.uni.util package and at the path uni-core/src/main/scala/wvlet/uni/util/Result.scala. Please update the design document to reflect the actual implementation or move the code to match the design.
| def recover[B >: A](pf: PartialFunction[Throwable, B]): Result[B] = | ||
| this match | ||
| case Failure(e) if pf.isDefinedAt(e) => | ||
| try | ||
| Result.Success(pf(e)) | ||
| catch | ||
| case NonFatal(e2) => | ||
| Result.Failure(e2) | ||
| case _ => | ||
| this |
There was a problem hiding this comment.
In foundational types like Result, it is better to avoid double evaluation of the PartialFunction (calling isDefinedAt followed by apply). Using pf.lift(e) is more idiomatic and safer, as it performs the check and execution in a single step.
def recover[B >: A](pf: PartialFunction[Throwable, B]): Result[B] =
this match
case Failure(e) =>
try
pf.lift(e) match
case Some(v) => Result.Success(v)
case None => this
catch
case NonFatal(err) =>
Result.Failure(err)
case _ =>
this| def mapError(f: Throwable => Throwable): Result[A] = | ||
| this match | ||
| case Failure(e) => | ||
| Result.Failure(f(e)) | ||
| case _ => | ||
| this |
There was a problem hiding this comment.
To maintain the "Rx-like propagation" property mentioned in the PR description (where exceptions never bypass the wrapper), mapError should also catch NonFatal exceptions thrown by the mapping function f. Currently, an exception in f would bubble up instead of being captured as a Failure.
def mapError(f: Throwable => Throwable): Result[A] =
this match
case Failure(e) =>
try
Result.Failure(f(e))
catch
case NonFatal(err) =>
Result.Failure(err)
case _ =>
thisThe try-catch NonFatal pattern was duplicated across Result's map / flatMap / filter / recover / recoverWith. Factor it into private catching / catchingResult helpers so each combinator stays a one-liner. Also drop the throwaway Try in Rx.fromResult by pattern-matching on Result directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a standard Result[A] type to uni-core, providing a functional container for values that may fail with a Throwable. The implementation includes monadic combinators, conversions to standard Scala types, and integration with the Rx library through the materialize operator and toRx extension methods. Review feedback highlights a bug in RxRunner where MaterializeOp fails to respect downstream flow control signals, suggests enhancing mapError to catch exceptions during error transformation, and notes discrepancies between the design document and the final implementation regarding package names and method locations.
| effect(OnNext(Result.Failure(e))) | ||
| effect(OnCompletion) |
There was a problem hiding this comment.
The implementation of MaterializeOp for OnError events does not respect the RxResult returned by the downstream effect. If the downstream operator (such as runOnce) returns RxResult.Stop after receiving the Result.Failure value, OnCompletion should not be emitted. Failing to check this can lead to terminal effects being executed multiple times for a single stream error.
val res = effect(OnNext(Result.Failure(e)))
if res.toContinue then effect(OnCompletion) else res| Result.Failure(f(e)) | ||
| case _ => |
There was a problem hiding this comment.
To maintain the "exception-aware composition" property where exceptions in combinators are caught and propagated as Failure, mapError should also wrap the execution of the transformation function f in a try-catch block. This ensures that if the error translation itself fails, the exception doesn't bypass the Result wrapper.
case Failure(e) =>
try
Result.Failure(f(e))
catch
case NonFatal(err) =>
Result.Failure(err)| Placed at `uni-core/src/main/scala/wvlet/uni/Result.scala` (root of | ||
| `wvlet.uni` inside `uni-core`), so it sits alongside foundational types and | ||
| is reachable from `import wvlet.uni.Result`. |
There was a problem hiding this comment.
There is a discrepancy between this design document and the actual implementation regarding the package name. The document suggests wvlet.uni, but the code uses wvlet.uni.util. Additionally, the document lists toRx as an instance method in the Core API table (line 80), while it is implemented as an extension method in the wvlet.uni.rx package. Updating this document to match the final implementation will improve long-term maintainability and clarity.
- Use PartialFunction.lift in recover / recoverWith to avoid the isDefinedAt + apply double evaluation. - Catch NonFatal exceptions thrown by mapError's translator so failures in error translation are wrapped like the rest of the combinators. - MaterializeOp now checks the downstream RxResult before emitting OnCompletion after a reified error, respecting backpressure. - Update plan document to reflect the final wvlet.uni.util.Result placement and the toRx extension surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a standard Result[A] type to uni-core, providing a container for values that may fail with a Throwable. The implementation includes the Result enum with monadic combinators, companion object factories, and interop with Rx via the materialize operator and toRx extensions. Documentation and unit tests are included. A review comment suggests that the recover method should be updated to catch exceptions thrown within its partial function to ensure consistency with the design goal of capturing all non-fatal errors within the Result wrapper.
| def recover[B >: A](pf: PartialFunction[Throwable, B]): Result[B] = | ||
| this match | ||
| case Failure(e) => | ||
| pf.lift(e) match | ||
| case Some(v) => | ||
| Result.catching(v) | ||
| case None => | ||
| this | ||
| case _: Success[?] => | ||
| this |
There was a problem hiding this comment.
The current implementation of recover does not catch exceptions thrown within the body of the partial function pf. This is inconsistent with the design goal of ensuring exceptions do not bypass the Result wrapper, and it differs from the implementation of recoverWith which correctly uses catchingResult. Wrapping the recovery logic ensures that any error during recovery is also captured as a Failure.
def recover[B >: A](pf: PartialFunction[Throwable, B]): Result[B] =
this match
case Failure(e) =>
Result.catchingResult(pf.lift(e).map(Success(_)).getOrElse(this))
case _: Success[?] =>
thisPreviously, Result.recover wrapped only the result of pf.lift(e) in catching(), but pf.lift evaluates the partial function body eagerly, so a throw inside the recovery arm would bypass the Result wrapper. Move the try/catch around the whole lift+match so recover matches recoverWith's contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a standard Result[A] type to uni-core, providing a container for values that may fail with a Throwable. The implementation includes the Result enum, companion object factories, and integration with Rx through materialize and toRx extensions. Documentation and unit tests are also provided. Feedback recommends refactoring the recover and recoverWith methods to use pattern guards instead of lift to improve performance by reducing allocations.
| def recover[B >: A](pf: PartialFunction[Throwable, B]): Result[B] = | ||
| this match | ||
| case Failure(e) => | ||
| Result.catchingResult(pf.lift(e).fold[Result[B]](this)(Result.Success(_))) | ||
| case _: Success[?] => | ||
| this |
There was a problem hiding this comment.
The implementation of recover using pf.lift(e).fold(...) is unnecessarily complex and involves extra allocations (an Option and a closure). It can be simplified using a pattern guard with isDefinedAt while still maintaining the exception-catching behavior provided by Result.catching.
def recover[B >: A](pf: PartialFunction[Throwable, B]): Result[B] =
this match
case Failure(e) if pf.isDefinedAt(e) =>
Result.catching(pf(e))
case _ =>
this| def recoverWith[B >: A](pf: PartialFunction[Throwable, Result[B]]): Result[B] = | ||
| this match | ||
| case Failure(e) => | ||
| Result.catchingResult(pf.lift(e).getOrElse(this)) | ||
| case _: Success[?] => | ||
| this |
There was a problem hiding this comment.
Similar to recover, recoverWith can be simplified to avoid the overhead of lift and getOrElse. Using isDefinedAt in a match arm is more idiomatic and efficient.
def recoverWith[B >: A](pf: PartialFunction[Throwable, Result[B]]): Result[B] =
this match
case Failure(e) if pf.isDefinedAt(e) =>
Result.catchingResult(pf(e))
case _ =>
thisGemini suggested reverting to the isDefinedAt-guarded form using Result.catching's by-name parameter. The body of pf(e) is still evaluated inside the try/catch (catching takes => A), so exceptions thrown inside the partial function arm are still captured as Failure — the dedicated test for that behavior still passes. Drops the Option allocation and fold closure the lift-based form introduced. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a standard Result[A] type to uni-core, providing a single-parameter container for values that may fail with a Throwable. The implementation includes monadic combinators such as map, flatMap, and recover that align with Rx semantics by catching non-fatal exceptions. Additionally, the PR adds Rx interop via materialize and toRx, along with comprehensive documentation and unit tests. Feedback focuses on improving exception safety in the orElse and fromOption methods by ensuring that lazy arguments are evaluated within exception-catching blocks to maintain consistency with the rest of the API.
| def orElse[B >: A](alt: => Result[B]): Result[B] = | ||
| this match | ||
| case _: Success[?] => | ||
| this | ||
| case _: Failure => | ||
| alt |
There was a problem hiding this comment.
For consistency with other combinators in this class (like map, flatMap, and recoverWith) and to align with the goal of providing an exception-aware container that mirrors Rx behavior, orElse should catch NonFatal exceptions thrown during the evaluation of the alt argument. Currently, an exception in alt would escape the Result wrapper.
def orElse[B >: A](alt: => Result[B]): Result[B] =
this match
case _: Success[?] =>
this
case _: Failure =>
Result.catchingResult(alt)| def fromOption[A](o: Option[A], ifNone: => Throwable): Result[A] = | ||
| o match | ||
| case Some(v) => | ||
| Success(v) | ||
| case None => | ||
| Failure(ifNone) |
There was a problem hiding this comment.
To maintain consistency with Result.apply, the fromOption factory should wrap the evaluation of the ifNone thunk in a catchingResult block. This ensures that if the error-producing function itself throws a NonFatal exception, it is correctly captured as a Failure rather than escaping the container.
def fromOption[A](o: Option[A], ifNone: => Throwable): Result[A] =
catchingResult {
o match
case Some(v) =>
Success(v)
case None =>
Failure(ifNone)
}Gemini noted two remaining by-name parameters that did not yet match the rest of Result's "exceptions thrown in a combinator argument become Failure" contract: orElse(alt) and fromOption(_, ifNone). Wrap both in catchingResult for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a standard Result[A] type to uni-core, providing a Rust-flavored container for values that may fail while maintaining compatibility with Scala's exception mechanism and Rx combinators. The implementation includes the Result enum, comprehensive unit tests, documentation, and a new materialize operator for Rx streams. Feedback suggests moving the materialize operator from trait Rx to trait RxOps to ensure it is available for RxOption types, aligning with the intended design.
| * terminates cleanly, so downstream operators like `toSeq` and `lastOption` work on errored | ||
| * sources. | ||
| */ | ||
| def materialize: Rx[Result[A]] = MaterializeOp(this) |
There was a problem hiding this comment.
The materialize operator is currently defined in trait Rx, which means it is not available for RxOption types. Since materialize is a general stream operator that reifies the error channel into a Result value, and the PR description mentions RxOps[A].materialize, it should be moved to trait RxOps (around line 43) to ensure consistency and broader availability across all Rx types, including RxOption.
materialize is a general stream operator for reifying events as Result values — it belongs next to recover / recoverWith on RxOps, so RxOption callers also get it. MaterializeOp already takes an RxOps input, so no runner changes are needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a standard Result[A] type to the uni-core module, providing a Rust-flavored alternative to scala.util.Try that aligns with Rx propagation semantics. The changes include the Result enum implementation, comprehensive unit tests, design documentation, and integration with Rx via the materialize operator and conversion utilities. Feedback suggests simplifying the isSuccess check using isInstanceOf and removing redundant Result. prefixes within the enum definition for better consistency.
| def isSuccess: Boolean = | ||
| this match | ||
| case _: Success[?] => | ||
| true | ||
| case _: Failure => | ||
| false |
| def mapError(f: Throwable => Throwable): Result[A] = | ||
| this match | ||
| case Failure(e) => | ||
| Result.catchingResult(Result.Failure(f(e))) |
Summary
Introduces
wvlet.uni.util.Result[A], Uni's Rust-flavored single-parameterresult type for APIs that want to represent "this may fail" as a value
without giving up Scala's exception mechanism.
Result[A](enum withSuccess/Failure(Throwable)) lives inuni-core/src/main/scala/wvlet/uni/util/Result.scala.Rx—recover,recoverWith,mapError,map/flatMapthat catchNonFataland propagate it asFailure—so switching between
Rx[A]andResult[A]is mechanical.RxOps[A].materialize: Rx[Result[A]]reifies eachevent as a value;
Rx.fromResultandResult.toRxclose the loop.Throwableerror channel — explicitly rejecting ZIO's tripleto keep the shape single-parameter like
Option[A]/Rx[A]./core/resultplus sidebar entries in both/guide/and
/blocks of the VitePress config.Design notes: plans/swirling-stargazing-crescent.md.
Test plan
./sbt scalafmtAll./sbt "uniJVM/testOnly wvlet.uni.util.ResultTest"— 12/12 pass./sbt coreJS/compile coreNative/compile— cross-platformnpm run docs:build— sidebar navigable, no dead links