Make Array iterator classes private to hide implementation from client

text/0000-array-private-classes

@@ -0,0 +1,148 @@
+- Feature Name: collections should not expose internal classes through iterators
+- Start Date: 2022-01-08
+- RFC PR: (leave this empty)
+- Pony Issue: (leave this empty)
+
+# Summary
+
+Following information hiding design principle, the builtin classes of collection data structures must not be made visible through iterator functions like `ArrayKeys`, `ArrayValues` and `ArrayPairs`. These classes can be made private as they are only used as return types for `Array` functions `keys`, `values` and `pairs`. The return values for these functions are changed to the more general interface `Iterator`.
+
+A new interface `RewindableIterator` is defined to allow for rewindable iterators, like it is the case for `Array` `values`.
+
+This design principle is applied to the other collection classes that expose internals too like:
+
+* `List`
+* `Map`
+* persistent `Map`
+* `Vec`
+* persistent `Vec`
+* `Set`
+* `Itertools`
+
+# Motivation
+
+This change brings:
+
+- Applying the design principle of [hiding implementation details](https://en.wikipedia.org/wiki/Information_hiding) but offer a general and stable interface. Returning interfaces instead of concrete classes allows changing the implementation. Usually, one must return the most general type that fullfils the contract of the function (in the case of the functions discussed in this RFC, iteration).
+- Collections' functions `keys`, `values` and `pairs` definitions are made more general. Iterators implementation details are not public. Internal classes used by implementation like `*Keys`, `*Values` and `*Pairs` are now [opaque data types](https://en.wikipedia.org/wiki/Opaque_data_type). Generally, when using these collection classes, clients are not interested by the iterators implementation, but by the types these iterators return and that is provided by the generic parameters.
+- The generic return signature of these 3 iterating functions is simpler to understand for clients of collection classes.
+- Makes the standard library more simple by hiding 18 specialised classes from stdlib of which 3 are from `builtin`.
+- The interface `RewindableIterator` is added to create rewindable iterators (can be re-start from first value).
+
+This change remains compatible with the existing code base but for client code that is directly using the classes `*Keys`, `*Values` and `*Pairs`. A search on Github shows that the impact is very limited.
+
+# Detailed design
+
+Iterating functions in collections `keys`, `values` and `pairs` are changed to return `Iterator` and the classes that implement these iterators are made private. Here are the full implementation of these functions for the `Array` class (changes in other collection classes are identical).
+
+```pony
+  fun keys(): Iterator[USize]^ =>
+    """
+    Return an iterator over the indices in the array.
+    """
+    _ArrayKeys[A, this->Array[A]](this)
+
+  fun values(): RewindableIterator[this->A]^ =>
+    """
+    Return an iterator over the values in the array.
+    """
+    _ArrayValues[A, this->Array[A]](this)
+
+  fun pairs(): Iterator[(USize, this->A)]^ =>
+    """
+    Return an iterator over the (index, value) pairs in the array.
+    """
+    _ArrayPairs[A, this->Array[A]](this)
+```
+
+As the function `values` of class `Array` uses an iterator with a `rewind` function that is not part of the `Iterator` interface, a new interface `RewindableIterator` is added to enable creation of rewindable iterators.
+
+Note: To remain consistent with `Array` behaviour, functions `keys` and `pairs` should return a `RewindableIterator` too but we limited the API change to minimum as we did not understood why it was not already the case.
+
+```pony
+interface RewindableIterator[A] is Iterator[A]
+  """
+  A `RewindableIterator` is an iterator that can be rewinded, that is start
+  again from first item. The data structure being iterated on can't change the
+  order it return iterated items.
+  """
+  fun has_next(): Bool
+    """
+    Return `true` when function `next` can be called to get next iteration item.
+    """
+
+  fun ref next(): A ?
+    """
+    Return the next item of the iteration or an error in case there are no other
+    items. A previous call to `has_next` check if we can continue iteration.
+    """
+
+  fun ref rewind(): Iterator[A]^
+    """
+    Get a new iterator that can be used to start the iteration again from the
+    first item.
+    """
+```
+
+The code of the standard library is adapted to remove use of these now private classes, mainly in tests. Here are the files that must be changed:
+
+* `packages/builtin/array.pony` as shown above
+* `packages/itertools/iter.pony` in function `cycle`
+* `packages/collections/heap.pony` in function `values`
+* `packages/collection/builtin/_test.pony` in class `_TestArrayValuesRewind`
+* `packages/collections/list.pony`
+* `packages/collections/map.pony`
+* `packages/collections/persistent/map.pony`
+* `packages/collections/persistent/vec.pony`
+* `packages/collections/set.pony`
+* `test/libponyc/util.cc` to change the name of the class to `_ArrayValues`
+
+# How We Teach This
+
+This change keeps the code compatible in the vast majority of cases. When client classes are defining objects of these now private types, the reason is usually to get access to the function `rewind` that was not defined in `Iterator`. By adding the interface `RewindableIterator`, client code can easily be adapted, replacing `ArrayValues[A]` by `RewindableIterator[A]`.
+
+ Also, client code generally uses these functions to iterate on the returned types and does not try to access the iterator directly but is interested by the iterated items. When client code refers to the iterator type, that's generally useless and the code can be rewritten to be made shorter and more future proof.
+
+A [search on Github Pony code](https://github.com/search?q=%22ArrayValues%22+language%3APony&type=code) finds 24 files using the class `ArrayValues`, of which 6 are copies of `array.pony` file.
+
+For instance, in [xml2xpath.pony](https://github.com/redvers/pony-libxml2/blob/bbca5d98d48854bfec2c6ee110220873ecc4df34/pony-libxml2/xml2xpath.pony#L41), the code can be changed from
+
+```pony
+  fun values(): ArrayValues[Xml2node, this->Array[Xml2node]]^ ? =>
+    if (allocated) then
+      ArrayValues[Xml2node, this->Array[Xml2node]](nodearray)
+    else
+      error
+    end
+```
+
+to
+
+```pony
+  fun values(): RewindableIterator[Xml2node]^ ? =>
+    if (allocated) then
+      nodearray.values()
+    else
+      error
+    end
+```
+
+In this sample, the developer was not really concerned by the type of the iterator but that the `values` function must return an `RewindableIterator` over `Xml2node`. The new version makes the code simpler to understand.
+
+This change in `array.pony` and other collections will break such code but it can be easily adapted to use the new API. And it will make the standard library easier to learn by reducing the number of public types.
+
+# How We Test This
+
+Pony tests must continue to pass.
+
+# Drawbacks
+
+As said, some client's code must me adapted when using these classes. As these classes are just concreted implementations of `Iterator` by collection classes, their use in client code is limited and the code can very easily be changed.
+
+# Alternatives
+
+Stay as is. Continue the [discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private).
+
+# Unresolved questions
+
+None