["BUG"] Arrow's generated Optional optics fail its own test cases

[Quantum64]

This is a follow-up to a post I made on the Arrow slack channel a few months ago.

Consider the following test case:

import arrow.core.test.generators.functionAToB
import arrow.optics.optics
import arrow.optics.test.laws.OptionalLaws
import io.kotest.core.spec.style.StringSpec
import io.kotest.property.Arb
import io.kotest.property.arbitrary.constant
import io.kotest.property.arbitrary.int

@optics
data class SampleData(
    val nullable: Int?
) {
    companion object
}

class GeneratedOptionalTest : StringSpec({
    "generated optional laws" {
        OptionalLaws.laws(
            SampleData.nullable,
            Arb.constant(SampleData(nullable = null)),
            Arb.int(),
            Arb.functionAToB(Arb.int())
        )
            .forEach { law ->
                law.test(this)
            }
    }
})

This test case fails.

java.lang.AssertionError: Property failed after 1 attempts

	Arg 0: arrow.optics.POptional$Companion$invoke$1@15f39d8d
	Arg 1: SampleData(nullable=null)
	Arg 2: 0 (shrunk from -527166501)

Repeat this test by using seed -3406815068104076785

Caused by: Found 0 but expected: null
	at arrow.optics.test.laws.OptionalLaws.setGetOption(OptionalLaws.kt:61)
	at arrow.optics.test.laws.OptionalLaws$laws$4.invokeSuspend(OptionalLaws.kt:24)
	at arrow.optics.test.laws.OptionalLaws$laws$4.invoke(OptionalLaws.kt)
	at arrow.optics.test.laws.OptionalLaws$laws$4.invoke(OptionalLaws.kt)

The issue is that the generated code for Optional optics is incorrect. When the focus of an Optional is null, the optional does not have a focus. It isn't focusing on a nullable property that happens to be null, it is focusing on nothing. Therefore, setting an Optional with a null focus should be a no-op [1].

As we can see, the generated optional optic shown below violates this specification by failing to check that a focus exists (getOrModify() is an Either.Right ) before setting the value.

inline val SampleData.Companion.nullable: arrow.optics.Optional<SampleData, Int> inline get()= arrow.optics.Optional(
  getOrModify = { sampleData: SampleData -> sampleData.`nullable`?.right() ?: sampleData.left() },
  set = { sampleData: SampleData, value: Int -> sampleData.copy(`nullable` = value) }
)

My opinion: I think that, in general, the affine traversal (or optional, as this library calls it) as an accessor for a nullable property is an unintuitive and impractical construct, and should certainly not be the default. A nullable lens, which is also generated, should be the preferred, and probably only, optic generated for a nullable property. Additionally, the use of the term optional rather than affine traversal seems to have an incorrect association with nullable lenses (see the Slack thread I linked above), so I suggest that this optic be renamed.

Of course, this issue needs to be fixed, but I suspect users will complain that generated optics for nullable properties "not working" when the focus is null, so I suggest removing optional generation first.

References
[1]: http://oleg.fi/gists/posts/2017-03-20-affine-traversal.html

[serras]

In Arrow 2.0 this problem has been solved by #2873, now you always get Lens for a field, even if it's nullable. You still can opt in the Optional behavior by composing with nullable.

As a result, this issue can be closed.