From e6691a1fac8b3061c88d6db629d1421e7398ab63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Sat, 8 Jan 2022 23:00:26 -0500 Subject: [PATCH 01/24] Initial creation --- text/0071-array-private-classes | 105 ++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 text/0071-array-private-classes diff --git a/text/0071-array-private-classes b/text/0071-array-private-classes new file mode 100644 index 00000000..3c576317 --- /dev/null +++ b/text/0071-array-private-classes @@ -0,0 +1,105 @@ +- Feature Name: array_private_classes +- Start Date: 2022-01-08 +- RFC PR: (leave this empty) +- Pony Issue: (leave this empty) + +# Summary + +Using information hiding design principle, the builtin classes `ArrayKeys`, `ArrayValues` and `ArrayPairs` 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 is be changed to general `Iterator`. A new interface `Rewindable` is defined to allow for rewindable iterators. + +# 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. +- `Array` functions `keys`, `values` and `pairs` definitions are made more general. `Array` iterators implementation details are not public. `_ArrayKeys`, `_ArrayValues` and `_ArrayPairs` are now [opaque data types](https://en.wikipedia.org/wiki/Opaque_data_type). +- Makes the standard library more simple by removing 3 specialised classes. +- The interface `Rewindable` can be combined to create rewindable iterators or other types that can be rewinded. + +This change is compatible with the existing code base but for client code that is directly using the classes `ArrayKeys`, `ArrayValues` and `ArrayPairs`. A search on Github shows that the impact is very limited. + +# Detailed design + +`Array` functions `keys`, `values` and `pairs` are changed to return `Iterator` and the classes that implement these iterators are make private. Here are the full implementation of these functions. + +```pony + fun keys(): Iterator[USize]^ => + """ + Return an iterator over the indices in the array. + """ + _ArrayKeys[A, this->Array[A]](this) + + fun values(): (Iterator[this->A] & Rewindable[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` uses an iterator with a `rewind` function that is not part of the `Iterator` interface, a new interface `Rewindable` is added to enbale creation of rewindable iterators. + +```pony +interface Rewindable[A] + """ + An `Iterator` is rewindable when it can be rewinded, that is start again from + first value. + """ + fun ref rewind(): Iterator[A]^ +``` + +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: + +* `iter.pony` in function `cycle` +* `heap.pony` in function `values` +* `_test.pony` in class `_TestArrayValuesRewind` +* `util.cc` to change the name of the class to `_ArrayValues` + +# How We Teach This + +This change keeps the code compatible in most cases. When client classes are defining objects of these types, the reason is usually to get access to the function `rewind` that was not defined in `Iterator`. By adding the interface `Rewindable`, client code can easily be adapted, replacing `ArrayValues[A]` by `(Iterator[A] & Rewindable[A])`. 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(): (Iterator[Xml2node] & Rewindable[Xml2node]) ? => + if (allocated) then + nodearray.values() + else + error + end +``` + +This change in `array.pony` will break such code but it can be easily adapted to use the new API. And it will make the standard library easier when reducing the number of builin types exposed. + +# 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` for use by `Array`, their use in client code is limited and the code can very easily be changed. + +# Alternatives + +Stay as is with these 3 builtin classes. Continue the [discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). + +# Unresolved questions + +None From 84535ad476fd7ab7d12c42a8266e76735abba14e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Sat, 8 Jan 2022 23:06:35 -0500 Subject: [PATCH 02/24] Remove RFC number --- text/{0071-array-private-classes => 0000-array-private-classes} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename text/{0071-array-private-classes => 0000-array-private-classes} (100%) diff --git a/text/0071-array-private-classes b/text/0000-array-private-classes similarity index 100% rename from text/0071-array-private-classes rename to text/0000-array-private-classes From 42b09a7dba61d056fb116e6d1d5275fc96ef799f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Tue, 18 Jan 2022 20:42:06 -0500 Subject: [PATCH 03/24] Update RFC to apply API change to all collection classes --- text/0000-array-private-classes | 87 ++++++++++++++++++++++++--------- 1 file changed, 65 insertions(+), 22 deletions(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index 3c576317..d5a5b369 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -1,26 +1,39 @@ -- Feature Name: array_private_classes +- 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 -Using information hiding design principle, the builtin classes `ArrayKeys`, `ArrayValues` and `ArrayPairs` 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 is be changed to general `Iterator`. A new interface `Rewindable` is defined to allow for rewindable iterators. +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. -- `Array` functions `keys`, `values` and `pairs` definitions are made more general. `Array` iterators implementation details are not public. `_ArrayKeys`, `_ArrayValues` and `_ArrayPairs` are now [opaque data types](https://en.wikipedia.org/wiki/Opaque_data_type). -- Makes the standard library more simple by removing 3 specialised classes. -- The interface `Rewindable` can be combined to create rewindable iterators or other types that can be rewinded. +- 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 is compatible with the existing code base but for client code that is directly using the classes `ArrayKeys`, `ArrayValues` and `ArrayPairs`. A search on Github shows that the impact is very limited. +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 -`Array` functions `keys`, `values` and `pairs` are changed to return `Iterator` and the classes that implement these iterators are make private. Here are the full implementation of these functions. +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]^ => @@ -29,7 +42,7 @@ This change is compatible with the existing code base but for client code that i """ _ArrayKeys[A, this->Array[A]](this) - fun values(): (Iterator[this->A] & Rewindable[this->A]) => + fun values(): RewindableIterator[this->A]^ => """ Return an iterator over the values in the array. """ @@ -42,27 +55,55 @@ This change is compatible with the existing code base but for client code that i _ArrayPairs[A, this->Array[A]](this) ``` -As the function `values` uses an iterator with a `rewind` function that is not part of the `Iterator` interface, a new interface `Rewindable` is added to enbale creation of rewindable iterators. +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 Rewindable[A] +interface RewindableIterator[A] is Iterator[A] """ - An `Iterator` is rewindable when it can be rewinded, that is start again from - first value. + 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: -* `iter.pony` in function `cycle` -* `heap.pony` in function `values` -* `_test.pony` in class `_TestArrayValuesRewind` -* `util.cc` to change the name of the class to `_ArrayValues` +* `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 most cases. When client classes are defining objects of these types, the reason is usually to get access to the function `rewind` that was not defined in `Iterator`. By adding the interface `Rewindable`, client code can easily be adapted, replacing `ArrayValues[A]` by `(Iterator[A] & Rewindable[A])`. 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. +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 @@ -78,7 +119,7 @@ For instance, in [xml2xpath.pony](https://github.com/redvers/pony-libxml2/blob/b to ```pony - fun values(): (Iterator[Xml2node] & Rewindable[Xml2node]) ? => + fun values(): RewindableIterator[Xml2node]^ ? => if (allocated) then nodearray.values() else @@ -86,7 +127,9 @@ to end ``` -This change in `array.pony` will break such code but it can be easily adapted to use the new API. And it will make the standard library easier when reducing the number of builin types exposed. +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 @@ -94,11 +137,11 @@ 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` for use by `Array`, their use in client code is limited and the code can very easily be changed. +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 with these 3 builtin classes. Continue the [discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). +Stay as is. Continue the [discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). # Unresolved questions From 5f5d79f3617350c7d906e7946c643abcf565b749 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Tue, 18 Jan 2022 21:03:09 -0500 Subject: [PATCH 04/24] Line break on column 80 --- text/0000-array-private-classes | 96 +++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 23 deletions(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index d5a5b369..0266cbf5 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -1,15 +1,24 @@ -- Feature Name: collections should not expose internal classes through iterators +- Feature Name: array_private_classes - 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`. +Collection classes should not expose internal classes through iterators functions. -A new interface `RewindableIterator` is defined to allow for rewindable iterators, like it is the case for `Array` `values`. +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`. -This design principle is applied to the other collection classes that expose internals too like: +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` @@ -23,17 +32,36 @@ This design principle is applied to the other collection classes that expose int 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. +- 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). +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]^ => @@ -55,9 +83,13 @@ Iterating functions in collections `keys`, `values` and `pairs` are changed to r _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. +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. +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] @@ -84,7 +116,8 @@ interface RewindableIterator[A] is Iterator[A] """ ``` -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: +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` @@ -99,13 +132,23 @@ The code of the standard library is adapted to remove use of these now private c # 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]`. +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. +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. +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 +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]]^ ? => @@ -127,9 +170,13 @@ to 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. +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. +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 @@ -137,11 +184,14 @@ 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. +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). +Stay as is. Continue the +[discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). # Unresolved questions From a52eb5f31079102a2df1aa1cee96e3c140c66e11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 19 Jan 2022 17:32:28 -0500 Subject: [PATCH 05/24] List signature changes in collections --- text/0000-array-private-classes | 58 +++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index 0266cbf5..1a9ad390 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -130,6 +130,64 @@ classes, mainly in tests. Here are the files that must be changed: * `packages/collections/set.pony` * `test/libponyc/util.cc` to change the name of the class to `_ArrayValues` +## Detailed changes + +In order to judge how the API becomes simpler to use understand clients of the +collections classes, here are the changes in the functions' signatures. The `-` +line shows the old signature while the `+` one is the new: + +```pony +// Array +- fun keys(): ArrayKeys[A, this->Array[A]]^ => ++ fun keys(): Iterator[USize]^ => +- fun values(): ArrayValues[A, this->Array[A]]^ => ++ fun values(): RewindableIterator[this->A]^ => +- fun pairs(): ArrayPairs[A, this->Array[A]]^ => ++ fun pairs(): Iterator[(USize, this->A)]^ => + +// Heap +- fun values(): ArrayValues[A, this->Array[A]]^ => ++ fun values(): Iterator[this->A]^ => + +// List +- fun nodes(): ListNodes[A, this->ListNode[A]]^ => ++ fun nodes(): Iterator[this->ListNode[A]]^ => +- fun rnodes(): ListNodes[A, this->ListNode[A]]^ => ++ fun rnodes(): Iterator[this->ListNode[A]]^ => +- fun values(): ListValues[A, this->ListNode[A]]^ => ++ fun values(): Iterator[this->A]^ => +- fun rvalues(): ListValues[A, this->ListNode[A]]^ => ++ fun rvalues(): Iterator[this->A]^ => + +// Map +- fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ => ++ fun keys(): Iterator[this->K]^ => +- fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ => ++ fun values(): Iterator[this->V]^ => +- fun pairs(): MapPairs[K, V, H, this->HashMap[K, V, H]]^ => ++ fun pairs(): Iterator[(this->K, this->V)]^ => + +// Persistent Map +- fun val keys(): MapKeys[K, V, H] => ++ fun val keys(): Iterator[K] => +- fun val values(): MapValues[K, V, H] => ++ fun val values(): Iterator[V] => +- fun val pairs(): MapPairs[K, V, H] => ++ fun val pairs(): Iterator[(K, V)] => + +// Persistent Vec +- fun val keys(): VecKeys[A]^ => ++ fun val keys(): Iterator[USize]^ => +- fun val values(): VecValues[A]^ => ++ fun val values(): Iterator[A]^ => +- fun val pairs(): VecPairs[A]^ => ++ fun val pairs(): Iterator[(USize, A)]^ => + +// Set +- fun values(): SetValues[A, H, this->HashSet[A, H]]^ => ++ fun values(): Iterator[this->A]^ => +``` + # How We Teach This This change keeps the code compatible in the vast majority of cases. When client From 73099eed0aca732f75a94af4ce8163ede6e376fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 19 Jan 2022 17:39:15 -0500 Subject: [PATCH 06/24] https://github.com/ponylang/rfcs/pull/193#discussion_r787306884 --- text/0000-array-private-classes | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index 1a9ad390..70b633d4 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -28,6 +28,10 @@ internals too like: * `Set` * `Itertools` +This is a breaking change for collections' client code that use now internal +classes but a search on Github repositories shows that the impact should be +limited. + # Motivation This change brings: From 873c378d2bb5adb51cc3a24ad7a453032cca6899 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 19 Jan 2022 17:43:18 -0500 Subject: [PATCH 07/24] https://github.com/ponylang/rfcs/pull/193#discussion_r787309484 --- text/0000-array-private-classes | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index 70b633d4..1d839f68 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -51,8 +51,9 @@ 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`. +- Reduces the number of public classes in the standard library by hiding 18 +specialised classes (iterators implementations) of which 3 are from the +`builtin` module. - The interface `RewindableIterator` is added to create rewindable iterators (can be re-start from first value). @@ -136,7 +137,7 @@ classes, mainly in tests. Here are the files that must be changed: ## Detailed changes -In order to judge how the API becomes simpler to use understand clients of the +In order to judge how the API becomes simpler to understand for clients of the collections classes, here are the changes in the functions' signatures. The `-` line shows the old signature while the `+` one is the new: From b0fd35461030c5f51dd0fce593a8c6f96f06ddc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 19 Jan 2022 17:45:47 -0500 Subject: [PATCH 08/24] https://github.com/ponylang/rfcs/pull/193#discussion_r787298485 --- text/0000-array-private-classes | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index 1d839f68..5edf3903 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -68,6 +68,14 @@ return `Iterator` and the classes that implement these iterators are made privat Here are the full implementation of these functions for the `Array` class (changes in other collection classes are identical). +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 fun keys(): Iterator[USize]^ => """ @@ -88,14 +96,6 @@ in other collection classes are identical). _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] """ From f492370db33c287b4e2c9500a886ac6a167dce69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 19 Jan 2022 17:47:35 -0500 Subject: [PATCH 09/24] https://github.com/ponylang/rfcs/pull/193#discussion_r787310073 --- text/0000-array-private-classes | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index 5edf3903..aad93f60 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -116,8 +116,7 @@ interface RewindableIterator[A] is Iterator[A] fun ref rewind(): Iterator[A]^ """ - Get a new iterator that can be used to start the iteration again from the - first item. + Start the iterator over again from the beginning. """ ``` From 62a7b030c3f093a0e11206d977b2963fd477aa26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 19 Jan 2022 17:49:58 -0500 Subject: [PATCH 10/24] https://github.com/ponylang/rfcs/pull/193#discussion_r787311214 --- text/0000-array-private-classes | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index aad93f60..6bd56d53 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -242,7 +242,8 @@ easier to learn by reducing the number of public types. # How We Test This -Pony tests must continue to pass. +Pony tests must continue to pass. No additional tests are need as after review +the existing coverage in Pony standard library tests is sufficient # Drawbacks From 48fc8bd0f2600122d592010468a75621f18c38cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 19 Jan 2022 17:51:10 -0500 Subject: [PATCH 11/24] https://github.com/ponylang/rfcs/pull/193#discussion_r787302750 --- text/0000-array-private-classes | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index 6bd56d53..d79739eb 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -247,9 +247,8 @@ the existing coverage in Pony standard library tests is sufficient # 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. +Will break any existing code that uses any of the classes that are currently +public and will be made private by this RFC. # Alternatives From 255c487fb14782121d1b05df53fd04de7f1677a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 19 Jan 2022 18:22:17 -0500 Subject: [PATCH 12/24] https://github.com/ponylang/rfcs/pull/193#discussion_r787303413 --- text/0000-array-private-classes | 35 ++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index d79739eb..c47e078f 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -72,10 +72,6 @@ As the function `values` of class `Array` uses an iterator with a `rewind` funct 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 fun keys(): Iterator[USize]^ => """ @@ -96,6 +92,10 @@ as we did not understood why it was not already the case. _ArrayPairs[A, this->Array[A]](this) ``` +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] """ @@ -255,6 +255,31 @@ public and will be made private by this RFC. Stay as is. Continue the [discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). +Instead of defining a new type of iterator with the `RewindableIterator` interface, +we can consider only the rewindable part of it in an `Rewindable` interface: + +```pony +interface Rewindable[A] + fun ref rewind(): Iterator[A]^ + """ + Start the iterator over again from the beginning. + """ +``` + +Then one can create a rewindable iterator by composing these two interfaces: + +```pony +type RewindableIterator[A] is (Iterator[A] & Rewindable[A]) +``` + +The rewindable type can be used with other types than `Iterator`, like a data +structure that would implement a rewindable property. This alternative was +[put aside](https://github.com/ponylang/rfcs/pull/193#discussion_r780793165) to +prevent name colisions in `builtin` with user-named types. + # Unresolved questions -None +Pony language definition is not precise about +[type variance](https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)) +and its impact in collections classes, particularly for covariant return types +of functions (in this RFC, `keys`, `values` and `pairs`). From 869eb82c96e3902cc1282eb03b40d267213bfb54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 9 Feb 2022 22:00:28 -0500 Subject: [PATCH 13/24] Include changes from Github comments --- text/0000-array-private-classes | 75 +++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes index c47e078f..c1358ec2 100644 --- a/text/0000-array-private-classes +++ b/text/0000-array-private-classes @@ -14,7 +14,7 @@ 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, +A new interface `CollectionIterator` 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 @@ -54,7 +54,7 @@ understand for clients of collection classes. - Reduces the number of public classes in the standard library by hiding 18 specialised classes (iterators implementations) of which 3 are from the `builtin` module. -- The interface `RewindableIterator` is added to create rewindable iterators +- The interface `CollectionIterator` 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 @@ -69,23 +69,23 @@ Here are the full implementation of these functions for the `Array` class (chang in other collection classes are identical). 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` +that is not part of the `Iterator` interface, a new interface `CollectionIterator` is added to enable creation of rewindable iterators. ```pony - fun keys(): Iterator[USize]^ => + fun keys(): CollectionIterator[USize]^ => """ Return an iterator over the indices in the array. """ _ArrayKeys[A, this->Array[A]](this) - fun values(): RewindableIterator[this->A]^ => + fun values(): CollectionIterator[this->A]^ => """ Return an iterator over the values in the array. """ _ArrayValues[A, this->Array[A]](this) - fun pairs(): Iterator[(USize, this->A)]^ => + fun pairs(): CollectionIterator[(USize, this->A)]^ => """ Return an iterator over the (index, value) pairs in the array. """ @@ -93,13 +93,12 @@ 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. +will return a `CollectionIterator`. ```pony -interface RewindableIterator[A] is Iterator[A] +interface CollectionIterator[A] is Iterator[A] """ - A `RewindableIterator` is an iterator that can be rewinded, that is start + A `CollectionIterator` 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. """ @@ -145,7 +144,7 @@ line shows the old signature while the `+` one is the new: - fun keys(): ArrayKeys[A, this->Array[A]]^ => + fun keys(): Iterator[USize]^ => - fun values(): ArrayValues[A, this->Array[A]]^ => -+ fun values(): RewindableIterator[this->A]^ => ++ fun values(): CollectionIterator[this->A]^ => - fun pairs(): ArrayPairs[A, this->Array[A]]^ => + fun pairs(): Iterator[(USize, this->A)]^ => @@ -192,13 +191,36 @@ line shows the old signature while the `+` one is the new: + fun values(): Iterator[this->A]^ => ``` +For instance, in `Array` + +```pony +- fun keys(): ArrayKeys[A, this->Array[A]]^ => ++ fun keys(): Iterator[USize]^ => +``` + +now the client user knows now that she gets an iterator over `USize`. + +Another more complex example with `Map`, + +```pony +- fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ => ++ fun keys(): Iterator[this->K]^ => +- fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ => ++ fun values(): Iterator[this->V]^ => +``` + +we see that we iterate over keys (`K` type) or values (`V`). The initial signature +gave complex generic classes that the user has to find in the documentation to +understand that they are iterators, and then dive deeper to understand on what +they iterate. + # 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]`. +adding the interface `CollectionIterator`, client code can easily be adapted, +replacing `ArrayValues[A]` by `CollectionIterator[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 @@ -224,7 +246,7 @@ the code can be changed from to ```pony - fun values(): RewindableIterator[Xml2node]^ ? => + fun values(): CollectionIterator[Xml2node]^ ? => if (allocated) then nodearray.values() else @@ -233,7 +255,7 @@ to ``` 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`. +but that the `values` function must return an `CollectionIterator` 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 @@ -252,34 +274,35 @@ public and will be made private by this RFC. # Alternatives -Stay as is. Continue the +1. Stay as is. Continue the [discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). -Instead of defining a new type of iterator with the `RewindableIterator` interface, +2. Update the existing concrete classes to include `rewind` where needed. + +3. Instead of defining a new type of iterator with the `CollectionIterator` interface, we can consider only the rewindable part of it in an `Rewindable` interface: ```pony interface Rewindable[A] - fun ref rewind(): Iterator[A]^ + fun ref rewind(): A^ """ - Start the iterator over again from the beginning. + Rewind the type `A`. """ ``` -Then one can create a rewindable iterator by composing these two interfaces: +Then one can create a rewindable iterator by combining these two interfaces: ```pony -type RewindableIterator[A] is (Iterator[A] & Rewindable[A]) +type CollectionIterator[A] is (Iterator[A] & Rewindable[Iterator[A]]) ``` +Perhaps, a more general interface name instead if `Rewindable` would be `Resetable` +to define the traits of a type that can be reset to its initial state, as in the +case of iterators `reset` == `rewind`. + The rewindable type can be used with other types than `Iterator`, like a data structure that would implement a rewindable property. This alternative was [put aside](https://github.com/ponylang/rfcs/pull/193#discussion_r780793165) to prevent name colisions in `builtin` with user-named types. # Unresolved questions - -Pony language definition is not precise about -[type variance](https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)) -and its impact in collections classes, particularly for covariant return types -of functions (in this RFC, `keys`, `values` and `pairs`). From 7bf7641d0fc8c33334b4edc67d30894f148a9a26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 18:15:09 -0500 Subject: [PATCH 14/24] Generate HTML --- public/array-private-classes.html | 293 ++++++++++++++++++++++++++++++ 1 file changed, 293 insertions(+) create mode 100644 public/array-private-classes.html diff --git a/public/array-private-classes.html b/public/array-private-classes.html new file mode 100644 index 00000000..3e22a034 --- /dev/null +++ b/public/array-private-classes.html @@ -0,0 +1,293 @@ +
    +
  • Feature Name: array_private_classes
    • +
  • Start Date: 2022-01-08
    • +
  • RFC PR: (leave this empty)
    • +
  • Pony Issue: (leave this empty)
    • +
+

Summary

+

Collection classes should not expose internal classes through iterators functions.

+

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 CollectionIterator 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
    • +
+

This is a breaking change for collections' client code that use now internal classes but a search on Github repositories shows that the impact should be limited.

+

Motivation

+

This change brings:

+
    +
  • Applying the design principle of hiding implementation details 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. 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.
    • +
  • Reduces the number of public classes in the standard library by hiding 18 specialised classes (iterators implementations) of which 3 are from the builtin module.
    • +
  • The interface CollectionIterator 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.

+

To quote Antoine de Saint-Exupéry:

+
+

Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.

+
+

Currently the API makes a guarantee that the return value of each of these functions is a specific concrete class with a specific public name. That particular guarantee does not actually serve any practical needs of users (except, arguably, the virtual calls caveat that was mentioned on Github discussion). If it is not helpful to users in practice, it can be removed. And if it can be removed, then it should be removed, in the name of taking another step toward stabilizing the Pony standard library's API.

+

It's worth mentioning that if this change was being debated in reverse (the current standard library had minimal interfaces for the return values, and somebody proposed to make them public concrete classes instead, because they found some tangible benefit in doing so), then it would not be a breaking change at all, because the new proposed type would be a subtype of the existing return types.

+

It'll always be easier to get more specific with these return types than more general. So we should default to offering the most general return type that is still useful to users, and ratchet down to more specific subtypes as needed (as tangible use cases for more specific return types are discovered). Ratcheting down will always be possible without a breaking change, but ratcheting up will be a breaking change each time. It's best to make such "ratchet up" breaking changes like this RFC before we reach Pony 1.0.0 as part of a general effort to make Pony and its standard library stable.

+

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).

+

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 CollectionIterator is added to enable creation of rewindable iterators.

+
  fun keys(): CollectionIterator[USize]^ =>
+    """
+    Return an iterator over the indices in the array.
+    """
+    _ArrayKeys[A, this->Array[A]](this)
+
+  fun values(): CollectionIterator[this->A]^ =>
+    """
+    Return an iterator over the values in the array.
+    """
+    _ArrayValues[A, this->Array[A]](this)
+
+  fun pairs(): CollectionIterator[(USize, this->A)]^ =>
+    """
+    Return an iterator over the (index, value) pairs in the array.
+    """
+    _ArrayPairs[A, this->Array[A]](this)
+
+

Note: To remain consistent with Array behaviour, functions keys and pairs will return a CollectionIterator.

+
interface CollectionIterator[A] is Iterator[A]
+  """
+  A `CollectionIterator` 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]^
+    """
+    Start the iterator over again from the beginning.
+    """
+
+

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
    • +
+

Detailed changes

+

In order to judge how the API becomes simpler to understand for clients of the collections classes, here are the changes in the functions' signatures. The - line shows the old signature while the + one is the new:

+
// Array
+-  fun keys(): ArrayKeys[A, this->Array[A]]^ =>
++  fun keys(): Iterator[USize]^ =>
+-  fun values(): ArrayValues[A, this->Array[A]]^ =>
++  fun values(): CollectionIterator[this->A]^ =>
+-  fun pairs(): ArrayPairs[A, this->Array[A]]^ =>
++  fun pairs(): Iterator[(USize, this->A)]^ =>
+
+// Heap
+-  fun values(): ArrayValues[A, this->Array[A]]^ =>
++  fun values(): Iterator[this->A]^ =>
+
+// List
+-  fun nodes(): ListNodes[A, this->ListNode[A]]^ =>
++  fun nodes(): Iterator[this->ListNode[A]]^ =>
+-  fun rnodes(): ListNodes[A, this->ListNode[A]]^ =>
++  fun rnodes(): Iterator[this->ListNode[A]]^ =>
+-  fun values(): ListValues[A, this->ListNode[A]]^ =>
++  fun values(): Iterator[this->A]^ =>
+-  fun rvalues(): ListValues[A, this->ListNode[A]]^ =>
++  fun rvalues(): Iterator[this->A]^ =>
+
+// Map
+-  fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun keys(): Iterator[this->K]^ =>
+-  fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun values(): Iterator[this->V]^ =>
+-  fun pairs(): MapPairs[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun pairs(): Iterator[(this->K, this->V)]^ =>
+
+// Persistent Map
+-  fun val keys(): MapKeys[K, V, H] =>
++  fun val keys(): Iterator[K] =>
+-  fun val values(): MapValues[K, V, H] =>
++  fun val values(): Iterator[V] =>
+-  fun val pairs(): MapPairs[K, V, H] =>
++  fun val pairs(): Iterator[(K, V)] =>
+
+// Persistent Vec
+-  fun val keys(): VecKeys[A]^ =>
++  fun val keys(): Iterator[USize]^ =>
+-  fun val values(): VecValues[A]^ =>
++  fun val values(): Iterator[A]^ =>
+-  fun val pairs(): VecPairs[A]^ =>
++  fun val pairs(): Iterator[(USize, A)]^ =>
+
+// Set
+-  fun values(): SetValues[A, H, this->HashSet[A, H]]^ =>
++  fun values(): Iterator[this->A]^ =>
+
+

For instance, in Array

+
-  fun keys(): ArrayKeys[A, this->Array[A]]^ =>
++  fun keys(): Iterator[USize]^ =>
+
+

now the client user knows now that she gets an iterator over USize.

+

Another more complex example with Map,

+
-  fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun keys(): Iterator[this->K]^ =>
+-  fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun values(): Iterator[this->V]^ =>
+
+

we see that we iterate over keys (K type) or values (V). The initial signature gave complex generic classes that the user has to find in the documentation to understand that they are iterators, and then dive deeper to understand on what they iterate.

+

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 CollectionIterator, client code can easily be adapted, replacing ArrayValues[A] by CollectionIterator[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 finds 24 files using the class ArrayValues, of which 6 are copies of array.pony file.

+

For instance, in xml2xpath.pony, the code can be changed from

+
  fun values(): ArrayValues[Xml2node, this->Array[Xml2node]]^ ? =>
+    if (allocated) then
+      ArrayValues[Xml2node, this->Array[Xml2node]](nodearray)
+    else
+      error
+    end
+
+

to

+
  fun values(): CollectionIterator[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 CollectionIterator 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. No additional tests are need as after review the existing coverage in Pony standard library tests is sufficient

+

Drawbacks

+

Will break any existing code that uses any of the classes that are currently public and will be made private by this RFC.

+

Alternatives

+
    +

  1. Stay as is. Continue the discussion on Zulip.

    2. +

  2. Update the existing concrete classes to include rewind where needed.

    3. +

  3. Instead of defining a new type of iterator with the CollectionIterator interface, we can consider only the rewindable part of it in an Rewindable interface:

    4. +
+
interface Rewindable[A]
+  fun ref rewind(): A^
+    """
+    Rewind the type `A`.
+    """
+
+

Then one can create a rewindable iterator by combining these two interfaces:

+
type CollectionIterator[A] is (Iterator[A] & Rewindable[Iterator[A]])
+
+

Perhaps, a more general interface name instead of Rewindable would be Resetable to define the traits of a type that can be reset to its initial state, as in the case of iterators reset == rewind.

+

The rewindable type can be used with other types than Iterator, like a data structure that would implement a rewindable property. This alternative was put aside to prevent name colisions in builtin with user-named types.

+

Comparison of alternatives

+
    +
  1. Do nothing: return public concrete classes.
    2. +
  2. Update the existing return classes and add rewind.
    3. +
  3. Add a CollectionIterator interface for collections.
    4. +
  4. Create union type by defining a Rewindable interface and combine with Iterator.
    5. +
  5. Change Iterator to add rewind.
    6. +
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
Justification / Alternative #12345
a. Simpler stdlib API------+++++++
b. Simpler function signatures------+++++++
c. Stay compatible with existing stdlib+++++++++++
d. Evolutive---++++++++
e. No impact on compiler++++++-+++-
f. No impact on performance+++++++++++++++
g. Limit stdlib pollution with interfaces+++++++++++
g. Limit stdlib pollution with classes------++++++++
+

Unresolved questions

+
    +
  • Must analyze how Range and Reverse would be impacted if defined as CollectionIterator. Particularly, impact on existing client code.
    • +
  • Possible candidates to be analyzed: StringBytes, StringRunes, ByteSeqIter and probably others will be found in code.
    • +
From e60bb5a055a9dcb432a19499621fdf16764090e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 18:24:28 -0500 Subject: [PATCH 15/24] Move to docs --- {public => docs}/array-private-classes.html | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename {public => docs}/array-private-classes.html (100%) diff --git a/public/array-private-classes.html b/docs/array-private-classes.html similarity index 100% rename from public/array-private-classes.html rename to docs/array-private-classes.html From 16ec9ef3af6bb83cd5774e1a4778bc7bba040304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 18:25:31 -0500 Subject: [PATCH 16/24] Set theme jekyll-theme-slate --- docs/_config.yml | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/_config.yml diff --git a/docs/_config.yml b/docs/_config.yml new file mode 100644 index 00000000..c7418817 --- /dev/null +++ b/docs/_config.yml @@ -0,0 +1 @@ +theme: jekyll-theme-slate \ No newline at end of file From 7fda29d98e05c9c66e26fdca7f2e5e946b573574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 19:42:13 -0500 Subject: [PATCH 17/24] Rename to index.html --- docs/index.html | 293 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 293 insertions(+) create mode 100644 docs/index.html diff --git a/docs/index.html b/docs/index.html new file mode 100644 index 00000000..3e22a034 --- /dev/null +++ b/docs/index.html @@ -0,0 +1,293 @@ +
    +
  • Feature Name: array_private_classes
    • +
  • Start Date: 2022-01-08
    • +
  • RFC PR: (leave this empty)
    • +
  • Pony Issue: (leave this empty)
    • +
+

Summary

+

Collection classes should not expose internal classes through iterators functions.

+

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 CollectionIterator 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
    • +
+

This is a breaking change for collections' client code that use now internal classes but a search on Github repositories shows that the impact should be limited.

+

Motivation

+

This change brings:

+
    +
  • Applying the design principle of hiding implementation details 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. 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.
    • +
  • Reduces the number of public classes in the standard library by hiding 18 specialised classes (iterators implementations) of which 3 are from the builtin module.
    • +
  • The interface CollectionIterator 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.

+

To quote Antoine de Saint-Exupéry:

+
+

Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.

+
+

Currently the API makes a guarantee that the return value of each of these functions is a specific concrete class with a specific public name. That particular guarantee does not actually serve any practical needs of users (except, arguably, the virtual calls caveat that was mentioned on Github discussion). If it is not helpful to users in practice, it can be removed. And if it can be removed, then it should be removed, in the name of taking another step toward stabilizing the Pony standard library's API.

+

It's worth mentioning that if this change was being debated in reverse (the current standard library had minimal interfaces for the return values, and somebody proposed to make them public concrete classes instead, because they found some tangible benefit in doing so), then it would not be a breaking change at all, because the new proposed type would be a subtype of the existing return types.

+

It'll always be easier to get more specific with these return types than more general. So we should default to offering the most general return type that is still useful to users, and ratchet down to more specific subtypes as needed (as tangible use cases for more specific return types are discovered). Ratcheting down will always be possible without a breaking change, but ratcheting up will be a breaking change each time. It's best to make such "ratchet up" breaking changes like this RFC before we reach Pony 1.0.0 as part of a general effort to make Pony and its standard library stable.

+

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).

+

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 CollectionIterator is added to enable creation of rewindable iterators.

+
  fun keys(): CollectionIterator[USize]^ =>
+    """
+    Return an iterator over the indices in the array.
+    """
+    _ArrayKeys[A, this->Array[A]](this)
+
+  fun values(): CollectionIterator[this->A]^ =>
+    """
+    Return an iterator over the values in the array.
+    """
+    _ArrayValues[A, this->Array[A]](this)
+
+  fun pairs(): CollectionIterator[(USize, this->A)]^ =>
+    """
+    Return an iterator over the (index, value) pairs in the array.
+    """
+    _ArrayPairs[A, this->Array[A]](this)
+
+

Note: To remain consistent with Array behaviour, functions keys and pairs will return a CollectionIterator.

+
interface CollectionIterator[A] is Iterator[A]
+  """
+  A `CollectionIterator` 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]^
+    """
+    Start the iterator over again from the beginning.
+    """
+
+

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
    • +
+

Detailed changes

+

In order to judge how the API becomes simpler to understand for clients of the collections classes, here are the changes in the functions' signatures. The - line shows the old signature while the + one is the new:

+
// Array
+-  fun keys(): ArrayKeys[A, this->Array[A]]^ =>
++  fun keys(): Iterator[USize]^ =>
+-  fun values(): ArrayValues[A, this->Array[A]]^ =>
++  fun values(): CollectionIterator[this->A]^ =>
+-  fun pairs(): ArrayPairs[A, this->Array[A]]^ =>
++  fun pairs(): Iterator[(USize, this->A)]^ =>
+
+// Heap
+-  fun values(): ArrayValues[A, this->Array[A]]^ =>
++  fun values(): Iterator[this->A]^ =>
+
+// List
+-  fun nodes(): ListNodes[A, this->ListNode[A]]^ =>
++  fun nodes(): Iterator[this->ListNode[A]]^ =>
+-  fun rnodes(): ListNodes[A, this->ListNode[A]]^ =>
++  fun rnodes(): Iterator[this->ListNode[A]]^ =>
+-  fun values(): ListValues[A, this->ListNode[A]]^ =>
++  fun values(): Iterator[this->A]^ =>
+-  fun rvalues(): ListValues[A, this->ListNode[A]]^ =>
++  fun rvalues(): Iterator[this->A]^ =>
+
+// Map
+-  fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun keys(): Iterator[this->K]^ =>
+-  fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun values(): Iterator[this->V]^ =>
+-  fun pairs(): MapPairs[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun pairs(): Iterator[(this->K, this->V)]^ =>
+
+// Persistent Map
+-  fun val keys(): MapKeys[K, V, H] =>
++  fun val keys(): Iterator[K] =>
+-  fun val values(): MapValues[K, V, H] =>
++  fun val values(): Iterator[V] =>
+-  fun val pairs(): MapPairs[K, V, H] =>
++  fun val pairs(): Iterator[(K, V)] =>
+
+// Persistent Vec
+-  fun val keys(): VecKeys[A]^ =>
++  fun val keys(): Iterator[USize]^ =>
+-  fun val values(): VecValues[A]^ =>
++  fun val values(): Iterator[A]^ =>
+-  fun val pairs(): VecPairs[A]^ =>
++  fun val pairs(): Iterator[(USize, A)]^ =>
+
+// Set
+-  fun values(): SetValues[A, H, this->HashSet[A, H]]^ =>
++  fun values(): Iterator[this->A]^ =>
+
+

For instance, in Array

+
-  fun keys(): ArrayKeys[A, this->Array[A]]^ =>
++  fun keys(): Iterator[USize]^ =>
+
+

now the client user knows now that she gets an iterator over USize.

+

Another more complex example with Map,

+
-  fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun keys(): Iterator[this->K]^ =>
+-  fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ =>
++  fun values(): Iterator[this->V]^ =>
+
+

we see that we iterate over keys (K type) or values (V). The initial signature gave complex generic classes that the user has to find in the documentation to understand that they are iterators, and then dive deeper to understand on what they iterate.

+

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 CollectionIterator, client code can easily be adapted, replacing ArrayValues[A] by CollectionIterator[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 finds 24 files using the class ArrayValues, of which 6 are copies of array.pony file.

+

For instance, in xml2xpath.pony, the code can be changed from

+
  fun values(): ArrayValues[Xml2node, this->Array[Xml2node]]^ ? =>
+    if (allocated) then
+      ArrayValues[Xml2node, this->Array[Xml2node]](nodearray)
+    else
+      error
+    end
+
+

to

+
  fun values(): CollectionIterator[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 CollectionIterator 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. No additional tests are need as after review the existing coverage in Pony standard library tests is sufficient

+

Drawbacks

+

Will break any existing code that uses any of the classes that are currently public and will be made private by this RFC.

+

Alternatives

+
    +

  1. Stay as is. Continue the discussion on Zulip.

    2. +

  2. Update the existing concrete classes to include rewind where needed.

    3. +

  3. Instead of defining a new type of iterator with the CollectionIterator interface, we can consider only the rewindable part of it in an Rewindable interface:

    4. +
+
interface Rewindable[A]
+  fun ref rewind(): A^
+    """
+    Rewind the type `A`.
+    """
+
+

Then one can create a rewindable iterator by combining these two interfaces:

+
type CollectionIterator[A] is (Iterator[A] & Rewindable[Iterator[A]])
+
+

Perhaps, a more general interface name instead of Rewindable would be Resetable to define the traits of a type that can be reset to its initial state, as in the case of iterators reset == rewind.

+

The rewindable type can be used with other types than Iterator, like a data structure that would implement a rewindable property. This alternative was put aside to prevent name colisions in builtin with user-named types.

+

Comparison of alternatives

+
    +
  1. Do nothing: return public concrete classes.
    2. +
  2. Update the existing return classes and add rewind.
    3. +
  3. Add a CollectionIterator interface for collections.
    4. +
  4. Create union type by defining a Rewindable interface and combine with Iterator.
    5. +
  5. Change Iterator to add rewind.
    6. +
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
Justification / Alternative #12345
a. Simpler stdlib API------+++++++
b. Simpler function signatures------+++++++
c. Stay compatible with existing stdlib+++++++++++
d. Evolutive---++++++++
e. No impact on compiler++++++-+++-
f. No impact on performance+++++++++++++++
g. Limit stdlib pollution with interfaces+++++++++++
g. Limit stdlib pollution with classes------++++++++
+

Unresolved questions

+
    +
  • Must analyze how Range and Reverse would be impacted if defined as CollectionIterator. Particularly, impact on existing client code.
    • +
  • Possible candidates to be analyzed: StringBytes, StringRunes, ByteSeqIter and probably others will be found in code.
    • +
From 5dbe1ca72102e38ba39acf57a4e6d748590105ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 19:42:54 -0500 Subject: [PATCH 18/24] Rename to index.html --- docs/array-private-classes.html | 293 ------------------------------ text/0000-array-private-classes | 308 -------------------------------- 2 files changed, 601 deletions(-) delete mode 100644 docs/array-private-classes.html delete mode 100644 text/0000-array-private-classes diff --git a/docs/array-private-classes.html b/docs/array-private-classes.html deleted file mode 100644 index 3e22a034..00000000 --- a/docs/array-private-classes.html +++ /dev/null @@ -1,293 +0,0 @@ -
    -
  • Feature Name: array_private_classes
    • -
  • Start Date: 2022-01-08
    • -
  • RFC PR: (leave this empty)
    • -
  • Pony Issue: (leave this empty)
    • -
-

Summary

-

Collection classes should not expose internal classes through iterators functions.

-

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 CollectionIterator 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
    • -
-

This is a breaking change for collections' client code that use now internal classes but a search on Github repositories shows that the impact should be limited.

-

Motivation

-

This change brings:

-
    -
  • Applying the design principle of hiding implementation details 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. 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.
    • -
  • Reduces the number of public classes in the standard library by hiding 18 specialised classes (iterators implementations) of which 3 are from the builtin module.
    • -
  • The interface CollectionIterator 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.

-

To quote Antoine de Saint-Exupéry:

-
-

Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.

-
-

Currently the API makes a guarantee that the return value of each of these functions is a specific concrete class with a specific public name. That particular guarantee does not actually serve any practical needs of users (except, arguably, the virtual calls caveat that was mentioned on Github discussion). If it is not helpful to users in practice, it can be removed. And if it can be removed, then it should be removed, in the name of taking another step toward stabilizing the Pony standard library's API.

-

It's worth mentioning that if this change was being debated in reverse (the current standard library had minimal interfaces for the return values, and somebody proposed to make them public concrete classes instead, because they found some tangible benefit in doing so), then it would not be a breaking change at all, because the new proposed type would be a subtype of the existing return types.

-

It'll always be easier to get more specific with these return types than more general. So we should default to offering the most general return type that is still useful to users, and ratchet down to more specific subtypes as needed (as tangible use cases for more specific return types are discovered). Ratcheting down will always be possible without a breaking change, but ratcheting up will be a breaking change each time. It's best to make such "ratchet up" breaking changes like this RFC before we reach Pony 1.0.0 as part of a general effort to make Pony and its standard library stable.

-

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).

-

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 CollectionIterator is added to enable creation of rewindable iterators.

-
  fun keys(): CollectionIterator[USize]^ =>
-    """
-    Return an iterator over the indices in the array.
-    """
-    _ArrayKeys[A, this->Array[A]](this)
-
-  fun values(): CollectionIterator[this->A]^ =>
-    """
-    Return an iterator over the values in the array.
-    """
-    _ArrayValues[A, this->Array[A]](this)
-
-  fun pairs(): CollectionIterator[(USize, this->A)]^ =>
-    """
-    Return an iterator over the (index, value) pairs in the array.
-    """
-    _ArrayPairs[A, this->Array[A]](this)
-
-

Note: To remain consistent with Array behaviour, functions keys and pairs will return a CollectionIterator.

-
interface CollectionIterator[A] is Iterator[A]
-  """
-  A `CollectionIterator` 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]^
-    """
-    Start the iterator over again from the beginning.
-    """
-
-

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
    • -
-

Detailed changes

-

In order to judge how the API becomes simpler to understand for clients of the collections classes, here are the changes in the functions' signatures. The - line shows the old signature while the + one is the new:

-
// Array
--  fun keys(): ArrayKeys[A, this->Array[A]]^ =>
-+  fun keys(): Iterator[USize]^ =>
--  fun values(): ArrayValues[A, this->Array[A]]^ =>
-+  fun values(): CollectionIterator[this->A]^ =>
--  fun pairs(): ArrayPairs[A, this->Array[A]]^ =>
-+  fun pairs(): Iterator[(USize, this->A)]^ =>
-
-// Heap
--  fun values(): ArrayValues[A, this->Array[A]]^ =>
-+  fun values(): Iterator[this->A]^ =>
-
-// List
--  fun nodes(): ListNodes[A, this->ListNode[A]]^ =>
-+  fun nodes(): Iterator[this->ListNode[A]]^ =>
--  fun rnodes(): ListNodes[A, this->ListNode[A]]^ =>
-+  fun rnodes(): Iterator[this->ListNode[A]]^ =>
--  fun values(): ListValues[A, this->ListNode[A]]^ =>
-+  fun values(): Iterator[this->A]^ =>
--  fun rvalues(): ListValues[A, this->ListNode[A]]^ =>
-+  fun rvalues(): Iterator[this->A]^ =>
-
-// Map
--  fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun keys(): Iterator[this->K]^ =>
--  fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun values(): Iterator[this->V]^ =>
--  fun pairs(): MapPairs[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun pairs(): Iterator[(this->K, this->V)]^ =>
-
-// Persistent Map
--  fun val keys(): MapKeys[K, V, H] =>
-+  fun val keys(): Iterator[K] =>
--  fun val values(): MapValues[K, V, H] =>
-+  fun val values(): Iterator[V] =>
--  fun val pairs(): MapPairs[K, V, H] =>
-+  fun val pairs(): Iterator[(K, V)] =>
-
-// Persistent Vec
--  fun val keys(): VecKeys[A]^ =>
-+  fun val keys(): Iterator[USize]^ =>
--  fun val values(): VecValues[A]^ =>
-+  fun val values(): Iterator[A]^ =>
--  fun val pairs(): VecPairs[A]^ =>
-+  fun val pairs(): Iterator[(USize, A)]^ =>
-
-// Set
--  fun values(): SetValues[A, H, this->HashSet[A, H]]^ =>
-+  fun values(): Iterator[this->A]^ =>
-
-

For instance, in Array

-
-  fun keys(): ArrayKeys[A, this->Array[A]]^ =>
-+  fun keys(): Iterator[USize]^ =>
-
-

now the client user knows now that she gets an iterator over USize.

-

Another more complex example with Map,

-
-  fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun keys(): Iterator[this->K]^ =>
--  fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun values(): Iterator[this->V]^ =>
-
-

we see that we iterate over keys (K type) or values (V). The initial signature gave complex generic classes that the user has to find in the documentation to understand that they are iterators, and then dive deeper to understand on what they iterate.

-

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 CollectionIterator, client code can easily be adapted, replacing ArrayValues[A] by CollectionIterator[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 finds 24 files using the class ArrayValues, of which 6 are copies of array.pony file.

-

For instance, in xml2xpath.pony, the code can be changed from

-
  fun values(): ArrayValues[Xml2node, this->Array[Xml2node]]^ ? =>
-    if (allocated) then
-      ArrayValues[Xml2node, this->Array[Xml2node]](nodearray)
-    else
-      error
-    end
-
-

to

-
  fun values(): CollectionIterator[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 CollectionIterator 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. No additional tests are need as after review the existing coverage in Pony standard library tests is sufficient

-

Drawbacks

-

Will break any existing code that uses any of the classes that are currently public and will be made private by this RFC.

-

Alternatives

-
    -

  1. Stay as is. Continue the discussion on Zulip.

    2. -

  2. Update the existing concrete classes to include rewind where needed.

    3. -

  3. Instead of defining a new type of iterator with the CollectionIterator interface, we can consider only the rewindable part of it in an Rewindable interface:

    4. -
-
interface Rewindable[A]
-  fun ref rewind(): A^
-    """
-    Rewind the type `A`.
-    """
-
-

Then one can create a rewindable iterator by combining these two interfaces:

-
type CollectionIterator[A] is (Iterator[A] & Rewindable[Iterator[A]])
-
-

Perhaps, a more general interface name instead of Rewindable would be Resetable to define the traits of a type that can be reset to its initial state, as in the case of iterators reset == rewind.

-

The rewindable type can be used with other types than Iterator, like a data structure that would implement a rewindable property. This alternative was put aside to prevent name colisions in builtin with user-named types.

-

Comparison of alternatives

-
    -
  1. Do nothing: return public concrete classes.
    2. -
  2. Update the existing return classes and add rewind.
    3. -
  3. Add a CollectionIterator interface for collections.
    4. -
  4. Create union type by defining a Rewindable interface and combine with Iterator.
    5. -
  5. Change Iterator to add rewind.
    6. -
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Justification / Alternative #12345
a. Simpler stdlib API------+++++++
b. Simpler function signatures------+++++++
c. Stay compatible with existing stdlib+++++++++++
d. Evolutive---++++++++
e. No impact on compiler++++++-+++-
f. No impact on performance+++++++++++++++
g. Limit stdlib pollution with interfaces+++++++++++
g. Limit stdlib pollution with classes------++++++++
-

Unresolved questions

-
    -
  • Must analyze how Range and Reverse would be impacted if defined as CollectionIterator. Particularly, impact on existing client code.
    • -
  • Possible candidates to be analyzed: StringBytes, StringRunes, ByteSeqIter and probably others will be found in code.
    • -
diff --git a/text/0000-array-private-classes b/text/0000-array-private-classes deleted file mode 100644 index c1358ec2..00000000 --- a/text/0000-array-private-classes +++ /dev/null @@ -1,308 +0,0 @@ -- Feature Name: array_private_classes -- Start Date: 2022-01-08 -- RFC PR: (leave this empty) -- Pony Issue: (leave this empty) - -# Summary - -Collection classes should not expose internal classes through iterators functions. - -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 `CollectionIterator` 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` - -This is a breaking change for collections' client code that use now internal -classes but a search on Github repositories shows that the impact should be -limited. - -# 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. -- Reduces the number of public classes in the standard library by hiding 18 -specialised classes (iterators implementations) of which 3 are from the -`builtin` module. -- The interface `CollectionIterator` 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). - -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 `CollectionIterator` -is added to enable creation of rewindable iterators. - -```pony - fun keys(): CollectionIterator[USize]^ => - """ - Return an iterator over the indices in the array. - """ - _ArrayKeys[A, this->Array[A]](this) - - fun values(): CollectionIterator[this->A]^ => - """ - Return an iterator over the values in the array. - """ - _ArrayValues[A, this->Array[A]](this) - - fun pairs(): CollectionIterator[(USize, this->A)]^ => - """ - Return an iterator over the (index, value) pairs in the array. - """ - _ArrayPairs[A, this->Array[A]](this) -``` - -Note: To remain consistent with `Array` behaviour, functions `keys` and `pairs` -will return a `CollectionIterator`. - -```pony -interface CollectionIterator[A] is Iterator[A] - """ - A `CollectionIterator` 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]^ - """ - Start the iterator over again from the beginning. - """ -``` - -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` - -## Detailed changes - -In order to judge how the API becomes simpler to understand for clients of the -collections classes, here are the changes in the functions' signatures. The `-` -line shows the old signature while the `+` one is the new: - -```pony -// Array -- fun keys(): ArrayKeys[A, this->Array[A]]^ => -+ fun keys(): Iterator[USize]^ => -- fun values(): ArrayValues[A, this->Array[A]]^ => -+ fun values(): CollectionIterator[this->A]^ => -- fun pairs(): ArrayPairs[A, this->Array[A]]^ => -+ fun pairs(): Iterator[(USize, this->A)]^ => - -// Heap -- fun values(): ArrayValues[A, this->Array[A]]^ => -+ fun values(): Iterator[this->A]^ => - -// List -- fun nodes(): ListNodes[A, this->ListNode[A]]^ => -+ fun nodes(): Iterator[this->ListNode[A]]^ => -- fun rnodes(): ListNodes[A, this->ListNode[A]]^ => -+ fun rnodes(): Iterator[this->ListNode[A]]^ => -- fun values(): ListValues[A, this->ListNode[A]]^ => -+ fun values(): Iterator[this->A]^ => -- fun rvalues(): ListValues[A, this->ListNode[A]]^ => -+ fun rvalues(): Iterator[this->A]^ => - -// Map -- fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ => -+ fun keys(): Iterator[this->K]^ => -- fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ => -+ fun values(): Iterator[this->V]^ => -- fun pairs(): MapPairs[K, V, H, this->HashMap[K, V, H]]^ => -+ fun pairs(): Iterator[(this->K, this->V)]^ => - -// Persistent Map -- fun val keys(): MapKeys[K, V, H] => -+ fun val keys(): Iterator[K] => -- fun val values(): MapValues[K, V, H] => -+ fun val values(): Iterator[V] => -- fun val pairs(): MapPairs[K, V, H] => -+ fun val pairs(): Iterator[(K, V)] => - -// Persistent Vec -- fun val keys(): VecKeys[A]^ => -+ fun val keys(): Iterator[USize]^ => -- fun val values(): VecValues[A]^ => -+ fun val values(): Iterator[A]^ => -- fun val pairs(): VecPairs[A]^ => -+ fun val pairs(): Iterator[(USize, A)]^ => - -// Set -- fun values(): SetValues[A, H, this->HashSet[A, H]]^ => -+ fun values(): Iterator[this->A]^ => -``` - -For instance, in `Array` - -```pony -- fun keys(): ArrayKeys[A, this->Array[A]]^ => -+ fun keys(): Iterator[USize]^ => -``` - -now the client user knows now that she gets an iterator over `USize`. - -Another more complex example with `Map`, - -```pony -- fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ => -+ fun keys(): Iterator[this->K]^ => -- fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ => -+ fun values(): Iterator[this->V]^ => -``` - -we see that we iterate over keys (`K` type) or values (`V`). The initial signature -gave complex generic classes that the user has to find in the documentation to -understand that they are iterators, and then dive deeper to understand on what -they iterate. - -# 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 `CollectionIterator`, client code can easily be adapted, -replacing `ArrayValues[A]` by `CollectionIterator[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(): CollectionIterator[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 `CollectionIterator` 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. No additional tests are need as after review -the existing coverage in Pony standard library tests is sufficient - -# Drawbacks - -Will break any existing code that uses any of the classes that are currently -public and will be made private by this RFC. - -# Alternatives - -1. Stay as is. Continue the -[discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). - -2. Update the existing concrete classes to include `rewind` where needed. - -3. Instead of defining a new type of iterator with the `CollectionIterator` interface, -we can consider only the rewindable part of it in an `Rewindable` interface: - -```pony -interface Rewindable[A] - fun ref rewind(): A^ - """ - Rewind the type `A`. - """ -``` - -Then one can create a rewindable iterator by combining these two interfaces: - -```pony -type CollectionIterator[A] is (Iterator[A] & Rewindable[Iterator[A]]) -``` - -Perhaps, a more general interface name instead if `Rewindable` would be `Resetable` -to define the traits of a type that can be reset to its initial state, as in the -case of iterators `reset` == `rewind`. - -The rewindable type can be used with other types than `Iterator`, like a data -structure that would implement a rewindable property. This alternative was -[put aside](https://github.com/ponylang/rfcs/pull/193#discussion_r780793165) to -prevent name colisions in `builtin` with user-named types. - -# Unresolved questions From 68b76953a8dfd16efa894a0de89f705ae4de2b81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 19:53:25 -0500 Subject: [PATCH 19/24] With Front Matter --- docs/index.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/index.html b/docs/index.html index 3e22a034..f8d1d9c6 100644 --- a/docs/index.html +++ b/docs/index.html @@ -1,3 +1,5 @@ +
+

layout: post title: array private classes

  • Feature Name: array_private_classes
  • Start Date: 2022-01-08
    • From c0da56dd30f1bdab6760c25eaf5f612387eafd8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 20:25:45 -0500 Subject: [PATCH 20/24] Added Front Matter --- docs/index.html | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/docs/index.html b/docs/index.html index f8d1d9c6..c2396c3a 100644 --- a/docs/index.html +++ b/docs/index.html @@ -1,11 +1,15 @@ -
    -

    layout: post title: array private classes

    +--- +layout: post +title: array private classes +--- +
    • Feature Name: array_private_classes
    • Start Date: 2022-01-08
    • RFC PR: (leave this empty)
    • Pony Issue: (leave this empty)
    +

    Rendered RFC: https://pmetras.github.io/rfcs/

    Summary

    Collection classes should not expose internal classes through iterators functions.

    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.

    @@ -197,7 +201,7 @@

    Alternatives

    Rewind the type `A`. """ -

    Then one can create a rewindable iterator by combining these two interfaces:

    +

    Then one can create a rewindable iterator by combining these two interfaces into a intersection type:

    type CollectionIterator[A] is (Iterator[A] & Rewindable[Iterator[A]])

    Perhaps, a more general interface name instead of Rewindable would be Resetable to define the traits of a type that can be reset to its initial state, as in the case of iterators reset == rewind.

    @@ -207,7 +211,7 @@

    Comparison of alternatives

  • Do nothing: return public concrete classes.
  • Update the existing return classes and add rewind.
  • Add a CollectionIterator interface for collections.
    • -
  • Create union type by defining a Rewindable interface and combine with Iterator.
    • +
  • Create intersection type by defining a Rewindable interface and combine with Iterator.
  • Change Iterator to add rewind.
    • @@ -293,3 +297,8 @@

    Unresolved questions

  • Must analyze how Range and Reverse would be impacted if defined as CollectionIterator. Particularly, impact on existing client code.
  • Possible candidates to be analyzed: StringBytes, StringRunes, ByteSeqIter and probably others will be found in code.
    • + + From b53901818ab2abeb5beb9fd975cdef49d16a536d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 20:36:04 -0500 Subject: [PATCH 21/24] Change layout to default --- docs/index.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/index.html b/docs/index.html index c2396c3a..cd1a3718 100644 --- a/docs/index.html +++ b/docs/index.html @@ -1,5 +1,5 @@ --- -layout: post +layout: default title: array private classes --- From 44103dd7d6863b58f0de50ca3753046d84f56df9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 20:41:45 -0500 Subject: [PATCH 22/24] Improve Front Matter --- docs/index.html | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/index.html b/docs/index.html index cd1a3718..0b340d30 100644 --- a/docs/index.html +++ b/docs/index.html @@ -1,6 +1,8 @@ --- layout: default -title: array private classes +title: Array Private Classes +description: Simpler collection iterators +show_downloads: false ---
      From d61d65650ee74da1c38fe310799762fce58c2b91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 20:45:54 -0500 Subject: [PATCH 23/24] Add rendered page and updates --- docs/index.html | 5 +- text/0000-array-private-classes.md | 371 +++++++++++++++++++++++++++++ 2 files changed, 374 insertions(+), 2 deletions(-) create mode 100644 text/0000-array-private-classes.md diff --git a/docs/index.html b/docs/index.html index 0b340d30..54e6ead6 100644 --- a/docs/index.html +++ b/docs/index.html @@ -297,10 +297,11 @@

      Comparison of alternatives

      Unresolved questions

      • Must analyze how Range and Reverse would be impacted if defined as CollectionIterator. Particularly, impact on existing client code.
        • -
      • Possible candidates to be analyzed: StringBytes, StringRunes, ByteSeqIter and probably others will be found in code.
        • +
      • Possible candidates to be analyzed: StringBytes, StringRunes, ByteSeqIter and probably others will be found in stdlib code.
        • +
      • Understand why the intersection type proposal is not considered as it seems to provide a better flexibility and is easier to understand.
      diff --git a/text/0000-array-private-classes.md b/text/0000-array-private-classes.md new file mode 100644 index 00000000..0872c72b --- /dev/null +++ b/text/0000-array-private-classes.md @@ -0,0 +1,371 @@ +- Feature Name: array_private_classes +- Start Date: 2022-01-08 +- RFC PR: (leave this empty) +- Pony Issue: (leave this empty) + +Rendered RFC: https://pmetras.github.io/rfcs/ + +# Summary + +Collection classes should not expose internal classes through iterators functions. + +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 `CollectionIterator` 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` + +This is a breaking change for collections' client code that use now internal +classes but a search on Github repositories shows that the impact should be +limited. + +# 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. +- Reduces the number of public classes in the standard library by hiding 18 +specialised classes (iterators implementations) of which 3 are from the +`builtin` module. +- The interface `CollectionIterator` 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. + +To quote Antoine de Saint-Exupéry: + +> Perfection is achieved, not when there is nothing more to add, but when there +> is nothing left to take away. + +Currently the API makes a guarantee that the return value of each of these functions +is a specific concrete class with a specific public name. That particular guarantee +does not actually serve any practical needs of users (except, arguably, the virtual +calls caveat that was mentioned on [Github discussion](https://github.com/ponylang/rfcs/pull/193#issuecomment-1009733064)). If it is not helpful to users in practice, +it can be removed. And if it can be removed, then it should be removed, in the +name of taking another step toward stabilizing the Pony standard library's API. + +It's worth mentioning that if this change was being debated in reverse (the +current standard library had minimal interfaces for the return values, and +somebody proposed to make them public concrete classes instead, because they +found some tangible benefit in doing so), then it would not be a breaking change +at all, because the new proposed type would be a subtype of the existing return +types. + +It'll always be easier to get more specific with these return types than more +general. So we should default to offering the most general return type that is +still useful to users, and ratchet down to more specific subtypes as needed (as +tangible use cases for more specific return types are discovered). Ratcheting down +will always be possible without a breaking change, but ratcheting up will be a +breaking change each time. It's best to make such "ratchet up" breaking changes +like this RFC before we reach Pony 1.0.0 as part of a general effort to make Pony +and its standard library stable. + +# 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). + +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 `CollectionIterator` +is added to enable creation of rewindable iterators. + +```pony + fun keys(): CollectionIterator[USize]^ => + """ + Return an iterator over the indices in the array. + """ + _ArrayKeys[A, this->Array[A]](this) + + fun values(): CollectionIterator[this->A]^ => + """ + Return an iterator over the values in the array. + """ + _ArrayValues[A, this->Array[A]](this) + + fun pairs(): CollectionIterator[(USize, this->A)]^ => + """ + Return an iterator over the (index, value) pairs in the array. + """ + _ArrayPairs[A, this->Array[A]](this) +``` + +Note: To remain consistent with `Array` behaviour, functions `keys` and `pairs` +will return a `CollectionIterator`. + +```pony +interface CollectionIterator[A] is Iterator[A] + """ + A `CollectionIterator` 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]^ + """ + Start the iterator over again from the beginning. + """ +``` + +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` + +## Detailed changes + +In order to judge how the API becomes simpler to understand for clients of the +collections classes, here are the changes in the functions' signatures. The `-` +line shows the old signature while the `+` one is the new: + +```pony +// Array +- fun keys(): ArrayKeys[A, this->Array[A]]^ => ++ fun keys(): Iterator[USize]^ => +- fun values(): ArrayValues[A, this->Array[A]]^ => ++ fun values(): CollectionIterator[this->A]^ => +- fun pairs(): ArrayPairs[A, this->Array[A]]^ => ++ fun pairs(): Iterator[(USize, this->A)]^ => + +// Heap +- fun values(): ArrayValues[A, this->Array[A]]^ => ++ fun values(): Iterator[this->A]^ => + +// List +- fun nodes(): ListNodes[A, this->ListNode[A]]^ => ++ fun nodes(): Iterator[this->ListNode[A]]^ => +- fun rnodes(): ListNodes[A, this->ListNode[A]]^ => ++ fun rnodes(): Iterator[this->ListNode[A]]^ => +- fun values(): ListValues[A, this->ListNode[A]]^ => ++ fun values(): Iterator[this->A]^ => +- fun rvalues(): ListValues[A, this->ListNode[A]]^ => ++ fun rvalues(): Iterator[this->A]^ => + +// Map +- fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ => ++ fun keys(): Iterator[this->K]^ => +- fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ => ++ fun values(): Iterator[this->V]^ => +- fun pairs(): MapPairs[K, V, H, this->HashMap[K, V, H]]^ => ++ fun pairs(): Iterator[(this->K, this->V)]^ => + +// Persistent Map +- fun val keys(): MapKeys[K, V, H] => ++ fun val keys(): Iterator[K] => +- fun val values(): MapValues[K, V, H] => ++ fun val values(): Iterator[V] => +- fun val pairs(): MapPairs[K, V, H] => ++ fun val pairs(): Iterator[(K, V)] => + +// Persistent Vec +- fun val keys(): VecKeys[A]^ => ++ fun val keys(): Iterator[USize]^ => +- fun val values(): VecValues[A]^ => ++ fun val values(): Iterator[A]^ => +- fun val pairs(): VecPairs[A]^ => ++ fun val pairs(): Iterator[(USize, A)]^ => + +// Set +- fun values(): SetValues[A, H, this->HashSet[A, H]]^ => ++ fun values(): Iterator[this->A]^ => +``` + +For instance, in `Array` + +```pony +- fun keys(): ArrayKeys[A, this->Array[A]]^ => ++ fun keys(): Iterator[USize]^ => +``` + +now the client user knows now that she gets an iterator over `USize`. + +Another more complex example with `Map`, + +```pony +- fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ => ++ fun keys(): Iterator[this->K]^ => +- fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ => ++ fun values(): Iterator[this->V]^ => +``` + +we see that we iterate over keys (`K` type) or values (`V`). The initial signature +gave complex generic classes that the user has to find in the documentation to +understand that they are iterators, and then dive deeper to understand on what +they iterate. + +# 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 `CollectionIterator`, client code can easily be adapted, +replacing `ArrayValues[A]` by `CollectionIterator[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(): CollectionIterator[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 `CollectionIterator` 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. No additional tests are need as after review +the existing coverage in Pony standard library tests is sufficient + +# Drawbacks + +Will break any existing code that uses any of the classes that are currently +public and will be made private by this RFC. + +# Alternatives + +1. Stay as is. Continue the +[discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). + +2. Update the existing concrete classes to include `rewind` where needed. + +3. Instead of defining a new type of iterator with the `CollectionIterator` interface, +we can consider only the rewindable part of it in an `Rewindable` interface: + +```pony +interface Rewindable[A] + fun ref rewind(): A^ + """ + Rewind the type `A`. + """ +``` + +Then one can create a rewindable iterator by combining these two interfaces into +a intersection type: + +```pony +type CollectionIterator[A] is (Iterator[A] & Rewindable[Iterator[A]]) +``` + +Perhaps, a more general interface name instead of `Rewindable` would be `Resetable` +to define the traits of a type that can be reset to its initial state, as in the +case of iterators `reset` == `rewind`. + +The rewindable type can be used with other types than `Iterator`, like a data +structure that would implement a rewindable property. This alternative was +[put aside](https://github.com/ponylang/rfcs/pull/193#discussion_r780793165) to +prevent name colisions in `builtin` with user-named types. + +## Comparison of alternatives + +1. Do nothing: return public concrete classes. +2. Update the existing return classes and add `rewind`. +3. Add a `CollectionIterator` interface for collections. +4. Create intersection type by defining a `Rewindable` interface and combine +with `Iterator`. +5. Change `Iterator` to add `rewind`. + +| Justification / Alternative # | 1 | 2 | 3 | 4 | 5 | +|:------------------------------------------|:---:|:---:|:---:|:---:|:---:| +| a. Simpler stdlib API | --- | --- | + | +++ | +++ | +| b. Simpler function signatures | --- | --- | + | +++ | +++ | +| c. Stay compatible with existing stdlib | +++ | +++ | + | +++ | + | +| d. Evolutive | --- | + | +++ | +++ | + | +| e. No impact on compiler | +++ | +++ | - | +++ | - | +| f. No impact on performance | +++ | +++ | +++ | +++ | +++ | +| g. Limit stdlib pollution with interfaces | +++ | +++ | + | + | +++ | +| g. Limit stdlib pollution with classes | --- | --- | +++ | ++ | +++ | + +# Unresolved questions + +- Must analyze how `Range` and `Reverse` would be impacted if defined as +`CollectionIterator`. Particularly, impact on existing client code. +- Possible candidates to be analyzed: `StringBytes`, `StringRunes`, `ByteSeqIter` +and probably others will be found in stdlib code. +- Understand why the intersection type proposal is not considered as it seems to +provide a better flexibility and is easier to understand. + + From 18f8d49c99c89e74e720d40ae662bce7dfb6268f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20M=C3=A9tras?= Date: Wed, 23 Feb 2022 21:08:50 -0500 Subject: [PATCH 24/24] Use Github rendering instead of pandoc --- docs/_config.yml | 1 - docs/index.html | 307 ----------------------------- text/0000-array-private-classes.md | 7 +- 3 files changed, 1 insertion(+), 314 deletions(-) delete mode 100644 docs/_config.yml delete mode 100644 docs/index.html diff --git a/docs/_config.yml b/docs/_config.yml deleted file mode 100644 index c7418817..00000000 --- a/docs/_config.yml +++ /dev/null @@ -1 +0,0 @@ -theme: jekyll-theme-slate \ No newline at end of file diff --git a/docs/index.html b/docs/index.html deleted file mode 100644 index 54e6ead6..00000000 --- a/docs/index.html +++ /dev/null @@ -1,307 +0,0 @@ ---- -layout: default -title: Array Private Classes -description: Simpler collection iterators -show_downloads: false ---- - -
        -
      • Feature Name: array_private_classes
        • -
      • Start Date: 2022-01-08
        • -
      • RFC PR: (leave this empty)
        • -
      • Pony Issue: (leave this empty)
        • -
      -

      Rendered RFC: https://pmetras.github.io/rfcs/

      -

      Summary

      -

      Collection classes should not expose internal classes through iterators functions.

      -

      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 CollectionIterator 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
        • -
      -

      This is a breaking change for collections' client code that use now internal classes but a search on Github repositories shows that the impact should be limited.

      -

      Motivation

      -

      This change brings:

      -
        -
      • Applying the design principle of hiding implementation details 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. 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.
        • -
      • Reduces the number of public classes in the standard library by hiding 18 specialised classes (iterators implementations) of which 3 are from the builtin module.
        • -
      • The interface CollectionIterator 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.

      -

      To quote Antoine de Saint-Exupéry:

      -
      -

      Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.

      -
      -

      Currently the API makes a guarantee that the return value of each of these functions is a specific concrete class with a specific public name. That particular guarantee does not actually serve any practical needs of users (except, arguably, the virtual calls caveat that was mentioned on Github discussion). If it is not helpful to users in practice, it can be removed. And if it can be removed, then it should be removed, in the name of taking another step toward stabilizing the Pony standard library's API.

      -

      It's worth mentioning that if this change was being debated in reverse (the current standard library had minimal interfaces for the return values, and somebody proposed to make them public concrete classes instead, because they found some tangible benefit in doing so), then it would not be a breaking change at all, because the new proposed type would be a subtype of the existing return types.

      -

      It'll always be easier to get more specific with these return types than more general. So we should default to offering the most general return type that is still useful to users, and ratchet down to more specific subtypes as needed (as tangible use cases for more specific return types are discovered). Ratcheting down will always be possible without a breaking change, but ratcheting up will be a breaking change each time. It's best to make such "ratchet up" breaking changes like this RFC before we reach Pony 1.0.0 as part of a general effort to make Pony and its standard library stable.

      -

      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).

      -

      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 CollectionIterator is added to enable creation of rewindable iterators.

      -
        fun keys(): CollectionIterator[USize]^ =>
-    """
-    Return an iterator over the indices in the array.
-    """
-    _ArrayKeys[A, this->Array[A]](this)
-
-  fun values(): CollectionIterator[this->A]^ =>
-    """
-    Return an iterator over the values in the array.
-    """
-    _ArrayValues[A, this->Array[A]](this)
-
-  fun pairs(): CollectionIterator[(USize, this->A)]^ =>
-    """
-    Return an iterator over the (index, value) pairs in the array.
-    """
-    _ArrayPairs[A, this->Array[A]](this)
-
      -

      Note: To remain consistent with Array behaviour, functions keys and pairs will return a CollectionIterator.

      -
      interface CollectionIterator[A] is Iterator[A]
-  """
-  A `CollectionIterator` 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]^
-    """
-    Start the iterator over again from the beginning.
-    """
-
      -

      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
        • -
      -

      Detailed changes

      -

      In order to judge how the API becomes simpler to understand for clients of the collections classes, here are the changes in the functions' signatures. The - line shows the old signature while the + one is the new:

      -
      // Array
--  fun keys(): ArrayKeys[A, this->Array[A]]^ =>
-+  fun keys(): Iterator[USize]^ =>
--  fun values(): ArrayValues[A, this->Array[A]]^ =>
-+  fun values(): CollectionIterator[this->A]^ =>
--  fun pairs(): ArrayPairs[A, this->Array[A]]^ =>
-+  fun pairs(): Iterator[(USize, this->A)]^ =>
-
-// Heap
--  fun values(): ArrayValues[A, this->Array[A]]^ =>
-+  fun values(): Iterator[this->A]^ =>
-
-// List
--  fun nodes(): ListNodes[A, this->ListNode[A]]^ =>
-+  fun nodes(): Iterator[this->ListNode[A]]^ =>
--  fun rnodes(): ListNodes[A, this->ListNode[A]]^ =>
-+  fun rnodes(): Iterator[this->ListNode[A]]^ =>
--  fun values(): ListValues[A, this->ListNode[A]]^ =>
-+  fun values(): Iterator[this->A]^ =>
--  fun rvalues(): ListValues[A, this->ListNode[A]]^ =>
-+  fun rvalues(): Iterator[this->A]^ =>
-
-// Map
--  fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun keys(): Iterator[this->K]^ =>
--  fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun values(): Iterator[this->V]^ =>
--  fun pairs(): MapPairs[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun pairs(): Iterator[(this->K, this->V)]^ =>
-
-// Persistent Map
--  fun val keys(): MapKeys[K, V, H] =>
-+  fun val keys(): Iterator[K] =>
--  fun val values(): MapValues[K, V, H] =>
-+  fun val values(): Iterator[V] =>
--  fun val pairs(): MapPairs[K, V, H] =>
-+  fun val pairs(): Iterator[(K, V)] =>
-
-// Persistent Vec
--  fun val keys(): VecKeys[A]^ =>
-+  fun val keys(): Iterator[USize]^ =>
--  fun val values(): VecValues[A]^ =>
-+  fun val values(): Iterator[A]^ =>
--  fun val pairs(): VecPairs[A]^ =>
-+  fun val pairs(): Iterator[(USize, A)]^ =>
-
-// Set
--  fun values(): SetValues[A, H, this->HashSet[A, H]]^ =>
-+  fun values(): Iterator[this->A]^ =>
-
      -

      For instance, in Array

      -
      -  fun keys(): ArrayKeys[A, this->Array[A]]^ =>
-+  fun keys(): Iterator[USize]^ =>
-
      -

      now the client user knows now that she gets an iterator over USize.

      -

      Another more complex example with Map,

      -
      -  fun keys(): MapKeys[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun keys(): Iterator[this->K]^ =>
--  fun values(): MapValues[K, V, H, this->HashMap[K, V, H]]^ =>
-+  fun values(): Iterator[this->V]^ =>
-
      -

      we see that we iterate over keys (K type) or values (V). The initial signature gave complex generic classes that the user has to find in the documentation to understand that they are iterators, and then dive deeper to understand on what they iterate.

      -

      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 CollectionIterator, client code can easily be adapted, replacing ArrayValues[A] by CollectionIterator[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 finds 24 files using the class ArrayValues, of which 6 are copies of array.pony file.

      -

      For instance, in xml2xpath.pony, the code can be changed from

      -
        fun values(): ArrayValues[Xml2node, this->Array[Xml2node]]^ ? =>
-    if (allocated) then
-      ArrayValues[Xml2node, this->Array[Xml2node]](nodearray)
-    else
-      error
-    end
-
      -

      to

      -
        fun values(): CollectionIterator[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 CollectionIterator 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. No additional tests are need as after review the existing coverage in Pony standard library tests is sufficient

      -

      Drawbacks

      -

      Will break any existing code that uses any of the classes that are currently public and will be made private by this RFC.

      -

      Alternatives

      -
        -

      1. Stay as is. Continue the discussion on Zulip.

        2. -

      2. Update the existing concrete classes to include rewind where needed.

        3. -

      3. Instead of defining a new type of iterator with the CollectionIterator interface, we can consider only the rewindable part of it in an Rewindable interface:

        4. -
      -
      interface Rewindable[A]
-  fun ref rewind(): A^
-    """
-    Rewind the type `A`.
-    """
-
      -

      Then one can create a rewindable iterator by combining these two interfaces into a intersection type:

      -
      type CollectionIterator[A] is (Iterator[A] & Rewindable[Iterator[A]])
-
      -

      Perhaps, a more general interface name instead of Rewindable would be Resetable to define the traits of a type that can be reset to its initial state, as in the case of iterators reset == rewind.

      -

      The rewindable type can be used with other types than Iterator, like a data structure that would implement a rewindable property. This alternative was put aside to prevent name colisions in builtin with user-named types.

      -

      Comparison of alternatives

      -
        -
      1. Do nothing: return public concrete classes.
        2. -
      2. Update the existing return classes and add rewind.
        3. -
      3. Add a CollectionIterator interface for collections.
        4. -
      4. Create intersection type by defining a Rewindable interface and combine with Iterator.
        5. -
      5. Change Iterator to add rewind.
        6. -
      -
    - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
    Justification / Alternative #12345
    a. Simpler stdlib API------+++++++
    b. Simpler function signatures------+++++++
    c. Stay compatible with existing stdlib+++++++++++
    d. Evolutive---++++++++
    e. No impact on compiler++++++-+++-
    f. No impact on performance+++++++++++++++
    g. Limit stdlib pollution with interfaces+++++++++++
    g. Limit stdlib pollution with classes------++++++++
    -

    Unresolved questions

    -
      -
    • Must analyze how Range and Reverse would be impacted if defined as CollectionIterator. Particularly, impact on existing client code.
      • -
    • Possible candidates to be analyzed: StringBytes, StringRunes, ByteSeqIter and probably others will be found in stdlib code.
      • -
    • Understand why the intersection type proposal is not considered as it seems to provide a better flexibility and is easier to understand.
      • -
    - - diff --git a/text/0000-array-private-classes.md b/text/0000-array-private-classes.md index 0872c72b..88b48f43 100644 --- a/text/0000-array-private-classes.md +++ b/text/0000-array-private-classes.md @@ -3,7 +3,7 @@ - RFC PR: (leave this empty) - Pony Issue: (leave this empty) -Rendered RFC: https://pmetras.github.io/rfcs/ +Rendered RFC: https://github.com/pmetras/rfcs/blob/rfc-array/text/0000-array-private-classes.md # Summary @@ -364,8 +364,3 @@ with `Iterator`. and probably others will be found in stdlib code. - Understand why the intersection type proposal is not considered as it seems to provide a better flexibility and is easier to understand. - -