Relax Ref#access semantics if set more than once

[armanbilge]

Following a Discord discussion with @ChristopherDavenport and @SystemFw relating to the problem implementing Ref#access for MapRef in #2464, here's a proposal to relax its semantics and thus remove the need for the AtomicBoolean in its implementation. IIUC the impracticality of the implementation outweighs its benefits, which would only apply in rare circumstances when the setter is being used non-idiomatically.

[SystemFw]

At last

[djspiewak]

I'm not totally averse to this, since I think access is kind of dumb, but it is part of the API and this is a pretty meaningful change to its semantics. For situations like MapRef, why is it not acceptable to simply delegate to a vanilla Ref inside of the access implementation to avoid the AtomicBoolean?

[armanbilge]

Does LensRef count as a MapRef-like situation? I've recently bumped my head into the Sync constraint there as well and I'd love to make it go away.

cats-effect/kernel/shared/src/main/scala/cats/effect/kernel/Ref.scala

Lines 419 to 423 in 0a6bae2

	override val access: F[(B, B => F[Boolean])] =
	F.flatMap(underlying.get) { snapshotA =>
	val snapshotB = lensGet(snapshotA)
	val setter = F.delay {
	val hasBeenCalled = new AtomicBoolean(false)

[djspiewak]

I really can't think of a situation where we can't just use a Ref inside of access rather than an AtomicBoolean. In fact, even the baseline primitive Ref could have implemented access in this fashion, it's just more performant (and equally possible) to use the AtomicBoolean.

Also fwiw, it surprises me that LensRef would need to reimplement this semantic rather than simply delegating to Ref#access.

[armanbilge]

The reason LensRef needs to re-implement is given in #827 (comment) to preserve this semantic:

writes to A that don't touch B don't break access's setter

[djspiewak]

Ah that's super annoying. Also I'm not convinced it actually works since, if A is modified, B might have changed, but we can't know without evaluating the lens. So it's probably more reliable to just invalidate the B writes unconditionally.

[armanbilge]

Hmm, actually isn't the same true of a MapRef? I haven't really thought it through but isn't one "interpretation" of a MapRef as a LensRef on a Ref[Map]?

[SystemFw]

this is a pretty meaningful change to its semantics

Is it though? My argument in the linked discussion is exactly that this AtomicBoolean doesn't really enrich the semantics, even coming up with an example is challenging, can you think of some code that would want to reuse the setter after it has returned true ? And in fact we didn't have the AtomicBoolean for the longest time, it was only introduced to match the docs

[djspiewak]

I'm cool merging this once the conflicts are resolved.

[armanbilge]

Why is it that something doesn't become officially "cool" until it conflicts 😝

[djspiewak]

Why is it that something doesn't become officially "cool" until it conflicts 😝

I like to maximize the work involved. :-P