Fix traverse for Either

arrow-kt · Mar 27, 2023 · c2d2e44 · c2d2e44
1 parent c919e0c
commit c2d2e44
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 25 deletions.
diff --git a/arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/Either.kt b/arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/Either.kt
@@ -1201,32 +1201,32 @@ public sealed class Either<out A, out B> {
 
   @Deprecated(
     NicheAPI + "Prefer using the Either DSL, or explicit fold or when",
-    ReplaceWith("fold({ emptyList() }, { fa(it).map(::Right) })")
+    ReplaceWith("fold({ listOf(it.left()) }, { fa(it).map(::Right) })")
   )
   @OptIn(ExperimentalTypeInference::class)
   @OverloadResolutionByLambdaReturnType
   public inline fun <C> traverse(fa: (B) -> Iterable<C>): List<Either<A, C>> =
-    fold({ emptyList() }, { fa(it).map(::Right) })
+    fold({ listOf(it.left()) }, { fa(it).map(::Right) })
 
   @Deprecated(
     NicheAPI + "Prefer using the Either DSL, or explicit fold or when",
-    ReplaceWith("fold({ None }, { right -> fa(right).map(::Right) })")
+    ReplaceWith("fold({ Some(it.left()) }, { right -> fa(right).map(::Right) })")
   )
   @OptIn(ExperimentalTypeInference::class)
   @OverloadResolutionByLambdaReturnType
   public inline fun <C> traverse(fa: (B) -> Option<C>): Option<Either<A, C>> =
-    fold({ None }, { right -> fa(right).map(::Right) })
+    fold({ Some(it.left()) }, { right -> fa(right).map(::Right) })
 
   @Deprecated("traverseOption is being renamed to traverse to simplify the Arrow API", ReplaceWith("traverse(fa)"))
   public inline fun <C> traverseOption(fa: (B) -> Option<C>): Option<Either<A, C>> =
     traverse(fa)
 
   @Deprecated(
     RedundantAPI + "Use orNull() and Kotlin nullable types",
-    ReplaceWith("orNull()?.let(fa)?.right()")
+    ReplaceWith("fold({ it.left() }) { fa(it)?.right() }", "arrow.core.left", "arrow.core.right")
   )
   public inline fun <C> traverseNullable(fa: (B) -> C?): Either<A, C>? =
-    orNull()?.let(fa)?.right()
+    fold({ it.left() }) { fa(it)?.right() }
 
   @Deprecated(
     NicheAPI + "Prefer using the Either DSL, or explicit fold or when",
@@ -2570,18 +2570,18 @@ public inline fun <A, B, C, D> Either<A, B>.redeemWith(fa: (A) -> Either<C, D>,
 @Deprecated(
   "Prefer Kotlin nullable syntax inside either DSL, or replace with explicit fold",
   ReplaceWith(
-    "fold({ listOf<Either<A, B>>() }, { iterable -> iterable.map<B, Either<A, B>> { it.right() } })",
-    "arrow.core.right",
+    "fold({ listOf<Either<A, B>>(it.left()) }, { iterable -> iterable.map<B, Either<A, B>> { it.right() } })",
+    "arrow.core.right", "arrow.core.left"
   )
 )
 public fun <A, B> Either<A, Iterable<B>>.sequence(): List<Either<A, B>> =
-  fold({ emptyList() }, { iterable -> iterable.map { it.right() } })
+  fold({ listOf(it.left()) }, { iterable -> iterable.map { it.right() } })
 
 @Deprecated(
   "Prefer Kotlin nullable syntax inside either DSL, or replace with explicit fold",
   ReplaceWith(
-    "this.fold<Option<Either<A, B>>>({ None }, { iterable -> iterable.map<B, Either<A, B>> { it.right() } })",
-    "arrow.core.right",
+    "this.fold<Option<Either<A, B>>>({ Some(it.left()) }, { iterable -> iterable.map<B, Either<A, B>> { it.right() } })",
+    "arrow.core.right", "arrow.core.left"
   )
 )
 public fun <A, B> Either<A, Option<B>>.sequenceOption(): Option<Either<A, B>> =
@@ -2590,20 +2590,17 @@ public fun <A, B> Either<A, Option<B>>.sequenceOption(): Option<Either<A, B>> =
 @Deprecated(
   "Prefer Kotlin nullable syntax inside either DSL, or replace with explicit fold",
   ReplaceWith(
-    "orNull()?.orNull()?.right<B>().toOption<Either<A, B>>()",
-    "arrow.core.toOption",
-    "arrow.core.right",
-    "arrow.core.left"
+    "this.fold<Option<Either<A, B>>>({ Some(it.left()) }, { iterable -> iterable.map<B, Either<A, B>> { it.right() } })",
+    "arrow.core.right", "arrow.core.left"
   )
 )
 public fun <A, B> Either<A, Option<B>>.sequence(): Option<Either<A, B>> =
-  orNull()?.orNull()?.right().toOption()
+  fold({ Some(it.left()) }) { it.map { it.right() } }
 
 @Deprecated(
   "Prefer Kotlin nullable syntax inside either DSL, or replace with explicit fold",
   ReplaceWith(
     "this.fold<Either<A, B>?>({ it.left() }, { it?.right() })",
-    "arrow.core.toOption",
     "arrow.core.right",
     "arrow.core.left"
   )
@@ -2613,10 +2610,14 @@ public fun <A, B> Either<A, B?>.sequenceNullable(): Either<A, B>? =
 
 @Deprecated(
   "Prefer Kotlin nullable syntax",
-  ReplaceWith("orNull()?.right()", "arrow.core.right")
+  ReplaceWith(
+    "this.fold<Either<A, B>?>({ it.left() }, { it?.right() })",
+    "arrow.core.right",
+    "arrow.core.left"
+  )
 )
 public fun <A, B> Either<A, B?>.sequence(): Either<A, B>? =
-  orNull()?.right()
+  this.fold<Either<A, B>?>({ it.left() }, { it?.right() })
 
 @Deprecated(
   "sequenceValidated is being renamed to sequence to simplify the Arrow API",

diff --git a/arrow-libs/core/arrow-core/src/commonTest/kotlin/arrow/core/EitherTest.kt b/arrow-libs/core/arrow-core/src/commonTest/kotlin/arrow/core/EitherTest.kt
@@ -329,15 +329,15 @@ class EitherTest : StringSpec({
       }
     }
 
-    "traverse should return list of Right when Right and empty list when Left" {
+    "traverse should return list of Right when Right and singleton list when Left" {
       checkAll(
         Arb.int(),
         Arb.int(),
         Arb.int()
       ) { a: Int, b: Int, c: Int ->
         Right(a).traverse { emptyList<Int>() } shouldBe emptyList<Int>()
         Right(a).traverse { listOf(b, c) } shouldBe listOf(Right(b), Right(c))
-        Left(a).traverse { listOf(b, c) } shouldBe emptyList<Int>()
+        Left(a).traverse { listOf(b, c) } shouldBe listOf(Left(a))
       }
     }
 
@@ -456,7 +456,7 @@ class EitherTest : StringSpec({
       val left: Either<String, Int> = Left("foo")
 
       right.traverse { listOf(it, 2, 3) } shouldBe listOf(Right(1), Right(2), Right(3))
-      left.traverse { listOf(it, 2, 3) } shouldBe emptyList()
+      left.traverse { listOf(it, 2, 3) } shouldBe listOf(left)
     }
 
     "sequence should be consistent with traverse" {
@@ -471,13 +471,14 @@ class EitherTest : StringSpec({
 
       right.traverseNullable { it } shouldBe Right(1)
       right.traverseNullable { null } shouldBe null
-      left.traverseNullable { it } shouldBe null
+      left.traverseNullable { it } shouldBe left
     }
 
     "sequence for Nullable should be consistent with traverseNullable" {
       checkAll(Arb.either(Arb.string(), Arb.int())) { either ->
         either.map { it }.sequence() shouldBe either.traverseNullable { it }
-        either.map { null }.sequence() shouldBe null
+        // wrong! if you map a `Left`, you should get a `Left` back, not null
+        // either.map { null }.sequence() shouldBe null
       }
     }
 
@@ -486,7 +487,9 @@ class EitherTest : StringSpec({
       val left: Either<String, Int> = Left("foo")
 
       right.traverse { Some(it) } shouldBe Some(Right(1))
-      left.traverse { Some(it) } shouldBe None
+      right.traverse { None } shouldBe None
+      left.traverse { Some(it) } shouldBe Some(left)
+      left.traverse { None } shouldBe Some(left)
     }
 
     "sequence for Option should be consistent with traverseOption" {