Add indexer lens for List and Array

[laenas]

Noticed this hadn't been added after the discussion the other day - so here it is.
I've added an Array module for the same behavior - but with slightly differing semantics owing to the nature of Array.

[gusty]

Thank you very much for contributing with this.
A few notes, in addition to my comments, could you add a test case? The code you posted on slack would be just fine, or you can add it in the docs, as the docs are also kind of tests in that they should build.
We should make sure the behavior in corner cases is the same as in other common Lens ie: what happen is I set an index out of the bounds?
This implementation is a quick'n dirty one, but maybe there's room for optimization, remember these functions will be called many times.

[gusty]

BTW: Don't worry about squashing your commits, I can squash the whole stuff when merging ;)

[laenas]

I cannot begin to express my joy at hearing that 😂

[laenas]

Brain-static from the workweek!

So:
Array.setAt needed, ultimately, to get moved into Extensions - it can't be flagged private owing to the fact that it'd get called from an inline - and just inlining the logic into the lambda there was producing some lovely type resolution errors in the Tests.

List.removeAt happily handles bad indices somewhat naturally, owing to the safety in slice syntax - but I added it to both the setAt functions. To prevent weird prepending for List and exceptions for Array.

I also at least added note to them in the main docs page for Extensions.

With regards to performance and optimizations, I'm flatfooted - while rudimentary, these functions are also not doing a whole lot. I'm not sure how one would immediately optimize them. Suggestions of things to explore?

[gusty]

Array.setAt needed, ultimately, to get moved into Extensions - it can't be flagged private owing to the fact that it'd get called from an inline - and just inlining the logic into the lambda there was producing some lovely type resolution errors in the Tests.

Fine, another solution would be to include the code for them in the body of List._item making effectively private to the function.

List.removeAt happily handles bad indices somewhat naturally, owing to the safety in slice syntax - but I added it to both the setAt functions. To prevent weird prepending for List and exceptions for Array.

I also at least added note to them in the main docs page for Extensions.

Nice !

With regards to performance and optimizations, I'm flatfooted - while rudimentary, these functions are also not doing a whole lot. I'm not sure how one would immediately optimize them. Suggestions of things to explore?

I would say let's leave it like this, as I have no clue how could it be improved at the moment. Maybe that's the best we can do.

Thanks for taking care of this, we'll include it in our coming release.

It would be nice to add the example of the pages of the book in the docs, but otherwise we can do it in a separate PR.

[laenas]

May as well just do it now :)

Speaking of the setAt stuff - though - I'm happy to make that tweak, having actually sat down with sharplab after your comment and taken a look at the generated code. Some weird rust in the thinking led me to associating inline in the older, more C++-y sense of "we are going to just drop this code inline at callsites" and so I was hoping to avoid the conceptual allocations of having setAt instances at all callsites.

But that concern has abated, after the investigation and realization that inline has different semantics in F#

Yank at least Array.setAt back out of Extensions, then? Yea / Nay?

[gusty]

Yes, go ahead and inline that function or all of them.

The less functions we introduce the better, in order to keep this PR more atomic.

We can always make them public in a future PR.

EDIT: actually I'm a bit ambivalent, as now I remembered I encountered some situations where these functions had been very handy. Of course lens would have been better, but when you have to change something in a structure and you don't use lenses these functions provides a good alternative. So my 2 cents to keep them public ;)

[laenas]

EDIT: actually I'm a bit ambivalent, as now I remembered I encountered some situations where these functions had been very handy. Of course lens would have been better, but when you have to change something in a structure and you don't use lenses these functions provides a good alternative. So my 2 cents to keep them public ;)

Yeah, the List functions are things that I've seen come up a few times from folks on Slack - generally beginners conflating with ResizeArray behavior - but they are kind of nice convenience functions. I want to leave them as-is.

It's the Array one (and lens) that I would want to yank, since both Array.set (used now anyway, just without safety wrapping) and direct setting with indexing and <- (again, without the safety) both exist. Which leads me to:

I just realized this function will mutate the original Array !

It will! And I'm going to make the case that it should.

First - I will just get setAt back into being a private function. So there'd be no consistency conflicts with the rest of the Array extensions - which all take the Array.init approach. This point means we're discussing the Array._item lens in isolation.

Second - F# inherits Array semantics from the ML family: Fixed Size, Mutable Cells. So it's not alone in this choice, and folks from sibling languages and F# in general are likely to expect that 'setting' a single value at an index in an Array will mutate that Array, not replicate with a new value at the index.

That said, I see the point from the direction of pure immutability as well 😅 - and it seems like a "do one thing, and a ticket will (eventually) be filed wanting the other" situation. Since the read half of the lens is the same, having an entirely separate lens feels weird - how bad would parameterization be?

[gusty]

I think the main issue here is that if it mutates the array, the lens would also mutate the original array, which is certainly wrong.

We have to fix the function, no doubt, now the question is, do we fix it and make it private, or do we make it public, aligned with the List counter part. I am a bit inclined for the latter.

What do you think ?

[laenas]

The sticking point for me is that I'm not certain it is implicitly wrong - I suspect it has a lot to do with the individual examining the problem. For better or worse, Arrays are mutable, and the hinting from the standard library is that individual (set) or limited-range(fill) value updates are to the original Array, not a copy operation - something that tends to be reserved for full-range modifications like map or sort.
The hitch here is therefore that we split into two camps:
The folks who would 'naively' write their own Array update code with mutability - since it's natively allowed without actually needing to mutable the array; and those who are strongly and strictly immutable in semantics. Neither are objectively correct or incorrect - it's a matter of perspective.

I lean somewhat - clearly - to the mutable-cells side of things: But part of that is in keeping an eye on the imminent arrival of Block as part of FS-1094: when we're going to have that native Immutable array type that will end up making Array a choice for wanting mutable cell semantics.

To be clear: Happy to move to a new-array thing now, but I want to just acknowledge and make the best case I can for what makes sense to me - both from a "what is going to make sense to others" perspective as well as "what won't create the need for modifying this in the future" perspective 😅

[gusty]

I understand the argument about "the expectation is that an array will mutate", but if we go down that road, there's little to no sense in defining a lens over an array, since a lens is something like a pure pointer.

If you are ok with mutability, just mutate, why bother with lenses?

[laenas]

And this might be at the edge of my experience/understanding in these abstractions - so do correct me if my thinking is flawed here:
It strikes me as being a matter of consistency in composition: In the simple case I would agree, there's little sense in this, since an int array can just be modified in place. But with arrays of more complicated types, it seems to preserve consistency to handle it this way, even if we accept mutation. To use the common example - imagine we have a Library with a Book array - in order to update the book at a given index - we end up needing to special-case the lensing into Library that looks different to the lensing into other collection types (and, indeed, the inner lens to update that Book).

[gusty]

If you break lensing rules, bad things will happen, believe me :)

Regarding your example, you normally compose the individual optics, without special-casing anything.

[laenas]

Just to be clear, what I mean is this:
Imagine a hypothetical (worst-case) structure: Library with a Books, Book with a Chapters, Chapter with a Pages, Page with Contents as a string.

Let's imagine we need to update the first page of the nth book in each library.
If everything was a list, we'd have something like:
Library._books << List._item n << _Some << Book._pages << List._item n << _Some << Page._contents that we can feed into setl or view in order to get/set.

If we have an array anywhere in that chain, what's the option in the case that we want such mutable semantics? We end up needing to 'special case' how we access things - since we can't easily just create such a chain of composition any longer. Or we end up writing our mutable lens anyway.

[gusty]

If we have an array anywhere in that chain, what's the option in the case that we want such mutable semantics?

You will never want such mutable semantic in that scenario. Doing that will be the source of many headaches.
Lens have to be used over pure structures, or non-pure with the compromise they won't be mutated.

[laenas]

Got it now 😅

[wallymathieu]

👍

[wallymathieu]

Nice to see some simple tests of the code 😃

[gusty]

Thanks one more time for taking care of this.

[laenas]

Thanks for the opportunity and the discussion, I feel illuminated :)