Conversation
kyay10
changed the title
|
Note to self: There seems to be a strange compilation mishap with |
serras
left a comment
serras
left a comment
There was a problem hiding this comment.
I'm not a fan of any of these changes. In particular, I don't think "monotone list builder" is a useful concept here, or even a expected one. Unless we can ensure that at least one element is present in a list, the usual:
buidList {
}.toNonEmptyListOrThrow()seems to be good enough for most purposes.
| map: (A, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10) -> R, | ||
| ): NonEmptyList<R> { | ||
| contract { callsInPlace(map, InvocationKind.AT_LEAST_ONCE) } | ||
| return all.zip(t1.all, t2.all, t3.all, t4.all, t5.all, t6.all, t7.all, t8.all, t9.all, t10.all, map).wrapAsNonEmptyListOrThrow() |
There was a problem hiding this comment.
Please try not to change inline code as much as possible. One should be especially careful with such a big piece of code, for two reasons:
- It makes more difficult to us to change anything in the API. With the current approach we could improve
zipandwrapAsNonEmptyListOrThrow, and everybody would benefit; whereas this is a very low-level implementation. - The bytecode size of the user of the library is very inflated, and this is a concern in Android environments.
There was a problem hiding this comment.
In this particular case, zip is already inline, so the bytecode size concern is out the window.
I don't quite see how wrapAsNonEmptyListOrThrow can ever be "improved", but regardless, the final code doesn't do anything remotely like it, since it instead statically guarantees that the list is non-empty.
Is your concern one of code duplication then? As in, that it's a maintenance burden for Arrow to have 2 copies of zip, one for Iterable, and another for Nel?
If so, I can extract a general "Iterator zip" algorithm out of this that assumes the iterators are non-empty. Then, the Nel implementation trivially then just summons iterator()s, and the Iterable implementation summons iterator()s, and early-returns with an empty list if any iterator happens to be empty (i.e. hasNext() returns false immediately).
I didn't do this immediately because it'd end up being @PublishedApi. If duplication is a concern, though, then it's reasonable to expose it as such.
It's also completely reasonable if you'd instead prefer the original implementation. I simply wanted to reduce wrapAsNonEmptyListOrThrow calls, as well as any WRONG_INVOCATION_KIND warnings.
| /** | ||
| * A builder for collections that can only grow by adding elements. | ||
| * Removing elements is not supported to preserve monotonicity. | ||
| */ |
There was a problem hiding this comment.
I don't think this is required at all. A buildNonEmptyList function should have just a single contract: by the end of the block you should have at least one element (regardless of using add, addAll, remove, or whatever you want). Here you're introducing a completely different notion.
There was a problem hiding this comment.
Practically, I don't see how it can be done any other way while offering a static guarantee that the list is non-empty. Sure, you can throw if the resulting list is non-empty, but that's far from a static guarantee. One can always create intermediate Lists and remove and add to them, as long as they always have a call that guarantees the non-emptiness of the builder.
I think it's interesting to see that the Arrow functions affected got by just fine with add and addAll, never needing remove.
Just to make sure I'm fully clear, this API guarantees that your list is non-empty. The compiler will complain if you don't have an add or addAll(NonEmptyCollection), or your own custom method that has the apt contract (where, of course, you'd be expected to ultimately call add or addAll). The only way to get around this guarantee is through a bad user-defined contract (to which there are compiler checkers coming eventually), or a cast to MonotoneCollectionBuilder.NonEmpty. This is similar to how you can already break NonEmptyList if you cast its all to MutableList, at least on the JVM.
If it helps, MonotoneCollectionBuilder is a suspend-less SequenceScope/FlowCollector. add/addAll correspond to yield/yieldAll and emit/emitAll.
MonotoneCollectionBuilder is designed as an effect. It's basically a write-only accumulator (maybe we should name it as such). It doesn't know, or care, how the data is stored, just that it's written somewhere. In fact, it's exactly the effect version of the Writer monad. This is incredibly useful because it doesn't force the implementation to use a List, any monoid will do. That's why the Set implementation is so easy, since that's another monoid.
What the buildNonEmptyX functions do is that they let you use any semigroup because they guarantee at least one add or non-empty addAll call. NonEmptyList and NonEmptySet are semigroups.
One can even imagine extending MonotoneCollectionBuilder (which ought to be called Writer or Accumulator) further by adding effect combinators like what we have for Raise. A mapWriter is easily implementable, and serves as a more efficient way to map a List.
Some caffienated handwaving about connections to monoids
There's a deeper connection here in the form of `MonotoneCollectionBuilder.() -> Unit` actually being kind of a free monoid over `T`, and hence isomorphic to `List`. (Technically, it's actually isomorphic to `() -> List` because of exceptions and non-termination and such, and you can add `suspend` modifiers to both, too, with the isomorphism staying). Handwaving a bit, sequencing of `MonotoneCollectionBuilder.() -> Unit` functions corresponds exactly to the monoid's `combine`, with `val id: MonotoneCollectionBuilder.() -> Unit = { }` serving as the identity for the monoid. If you've ever seen how a monoid is modelled in CT, it's through a bunch of morphisms. In some way, `MonotoneCollectionBuilder` lets you map any `T` to a morphism, with the morphism composition being the sequencing. `MonotoneCollectionBuilder.() -> Unit` is the "mother of all monoids", while `List` is the free monoid, analogously to (multi-shot) `Continuation` being the "mother of all monads", while `Free` is the free monad. There's something about "initial" vs "final" encoding here that I don't know enough to comment about. What I do konw is that `MonotoneCollectionBuilder.() -> Unit` is more performant than a `List` because it doesn't involve intermediate lists, in a similar way to how continuations are more performant than `Free` because it doesn't involve intermediate values.There was a problem hiding this comment.
If it helps, here's a very simple version of the API to play around with (playground). Let me know if you find any way to break it!
There was a problem hiding this comment.
The question here for me is whether providing this API is useful enough to include it in arrow-core, given that at this point we have no way to enforce that at least one call to add or addAll is done. We really made a big effort to reduce Arrow's API to its minimum, and this PR goes into the opposite direction.
There was a problem hiding this comment.
given that at this point we have no way to enforce that at least one call to add or addAll is done
Do you mean that buildNonEmptyList doesn't enforce that? Because it does! See the playground link above. It's all compile-time checked, relying on contracts and a lambda with an intersection return type.
I can have a go at reducing the API surface here if you'd like. I was aiming to have an API similar to MutableList/Set (without the remove methods), but I think only a fraction of the API is really necessary, and we can always extend it later
|
FWIW, I can move the builders to a new module, and add an |
2 participants
The builders work by giving the caller a
MonotoneMutableList/Setand forcing the caller to return aNonEmpty & List/Set. Operations onMonotoneMutableList/Setthat are sure to add items, likeaddandaddAll(NonEmptyCollection), have a contract that gives their receiver the apt intersection type.This PR also removes most usages of
NonEmptyList/Set(), as in the unsafe constructors, and most usages ofPotentiallyUnsafeNonEmptyOperation. This also meant that a lot of warning suppressions forWRONG_INVOCATION_KINDandLEAKED_IN_PLACE_LAMBDAwere finally removed!The PR doesn't compromise on inline bytecode size, either. In particular, it would be easy to call inline lambdas multiple times to get
AT_LEAST_ONCEto comply. Instead, I've extensively useddo-while, which resulted in cleaner code, anyway.