Consolidate Vector and Array methods #6218

JaroslavTulach · 2023-04-06T13:05:02Z

Pull Request Description

Fixes #5011 by making sure the same methods that are on Vector are also available on Array.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
All code follows the
Java,
All code has been tested:
- Unit tests have been written where possible.

…epresentations

radeusgd

Code and tests look good.

Unless there is a reason, I think we need to add doc comments to the methods on the Array. It's not a PRIVATE type. If it is to be private - OK, but then we should probably mark all the methods at PRIVATE. In general I think that in our libraries we should not have methods that are neither marked PRIVATE nor have at least one sentence of documentation.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ArraySlice.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java

test/Tests/src/Data/Vector_Spec.enso

test/Tests/src/Data/Array_Spec.enso

JaroslavTulach

Ready for your reviews @hubertp, @4e6, @radeusgd and @jdunkerley.

JaroslavTulach · 2023-04-11T10:20:15Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso

@@ -225,7 +225,7 @@ type Vector a

    ## Combines all the elements of the vector, by iteratively applying the
       passed function with the next element of the vector. After each step the
-       value is stored resulting in a new Vector of the same size as self.
+       value is stored resulting in a new vector of the same size as self.


Whenever there is vector in lowercase, it is replaced by array when comparing the documentation in the Array type.

JaroslavTulach · 2023-04-11T10:21:23Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso

@@ -499,12 +499,12 @@ type Vector a
    map self function =
        Vector.new self.length i-> function (self.at i)

-    ## Applies a function to each element of the vector, returning the vector
+    ## Applies a function to each element of the vector, returning the `Vector`


Sometimes we don't want to replace the Vector word in the Array documentation. I am then writing the Vector with capital first letter and quoting it.

radeusgd

Looks good.

I'd appreciate a short note on ArraySlice.createOrNull explaining what is the meaning of it returning null (we have discsused this here, but no on reading the code will know to look around this PR discussion for details, and it would make reading it much easier).

radeusgd · 2023-04-11T10:25:04Z

engine/runtime/src/test/java/org/enso/compiler/VectorArraySignatureTest.java

+          var head = arrayFns.filter((av) -> av instanceof IR$Function$Binding ab && ab.name().name().equals(b.name().name())).head();
+          var ad = documentation.get(head);
+          assertNotNull("No documentation for Array." + name, ad);
+          var exp = d.doc()
+                  .replace("]", "].to_array")
+                  .replace("a vector", "an array")
+                  .replace("vector", "array");
+          assertEquals("Bad documentation for Array." + name, exp, ad.doc());


wow 😀 neat

jdunkerley

Looks good - I do wonder if the return type of the methods should be Array instead of Vector.

JaroslavTulach · 2023-04-11T11:40:25Z

Looks good - I do wonder if the return type of the methods should be Array instead of Vector.

Usually not. Usually the return type should be Vector. Only in situations where we return self, we may return Array. In all other cases the method delegates to Vector.method and that one is going to produce a Vector.

4e6

Engine part looks good 👍

hubertp

What about Array_Proxy and ArrayOverBuffer?
Plus minor nits in type signatures.

btw I'm guessing it is intentional that Array methods often return Vector? It feels a bit weird that one operates on the former and always gets the latter.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

hubertp · 2023-04-11T13:14:19Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

+         Concatenate two single-element arrays.
+
+             [1].to_array + [2].to_array
+    + : Vector Any -> Vector Any


The type should be an Array?

The in type can be either Vector or Array. The output type is Vector.

hubertp · 2023-04-11T13:15:28Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso

+         of both elements.
+
+             [1, 2, 3].to_array.zip [4, 5, 6].to_array == [[1, 4].to_array, [2, 5].to_array, [3, 6].to_array].to_array
+    zip : Vector Any -> (Any -> Any -> Any) -> Vector Any


Suggested change

zip : Vector Any -> (Any -> Any -> Any) -> Vector Any

zip : Array Any -> (Any -> Any -> Any) -> Vector Any

Again. The proper input type is either Vector or Array.

OK, so if I was a real PITA I would say that this should be (Array Any | Vector Any) :)

Moreover that applies to the whole codebase. Everywhere Vector is used, Array can be used as well!

Rather than modifying all the type signatures, let's hope further consolidation renders these two types obsolete and allows them to be merged into one. Because Array is basically Vector in Input - e.g. Vector that can change due to changes made elsewhere and may need to be read (Inputed) again.

That sounds like a good idea for the future.

Alternatively, I'd keep the Vector/Array distinction but make the type signatures rely on some interface/typeclass - like IndexedSequence - and then both Array and Vector would 'implement' this interface/typeclass, so they could be used where it is expected.

JaroslavTulach · 2023-04-11T13:56:34Z

What about Array_Proxy and ArrayOverBuffer? Plus minor nits in type signatures.

I had to fix one test because of ArrayOverBuffer. I assume the rest works just fine. I had no issues with Array_Proxy.

btw I'm guessing it is intentional that Array methods often return Vector?
It feels a bit weird that one operates on the former and always gets the latter.

The (philosophical) difference between Array and Vector is: Array can potentially change due to external misdoings. Vector shall (almost) never change. The new Array methods delegate to Vector counterparts and those are known to create something that doesn't change - e.g. Vector. Hence the return type is (almost) always Vector and not Array.

JaroslavTulach added 2 commits April 6, 2023 15:01

Verify Array and Vector has the same methods

aca254d

Add missing Vector methods to Array

800e6a7

JaroslavTulach requested a review from hubertp April 6, 2023 13:05

JaroslavTulach requested review from jdunkerley and radeusgd as code owners April 6, 2023 13:05

JaroslavTulach self-assigned this Apr 6, 2023

JaroslavTulach added 3 commits April 6, 2023 15:12

Make sure JavaScript polyglot array can be used as Pair

e3c1a63

Expanding the test specification to be ready for alternative Vector r…

10abb57

…epresentations

Execute the Vector_Spec with an Array instead of Vector

cfba40b

JaroslavTulach requested a review from 4e6 as a code owner April 6, 2023 16:57

JaroslavTulach added 3 commits April 6, 2023 19:16

Array.sort must be equivalent to Vector.sort

62ce27d

Array.to_array is a simple identity

9f41726

Array.to_text is overriden to be consistent with Vector.to_text

793e0c2

radeusgd reviewed Apr 6, 2023

View reviewed changes

enso-bot bot mentioned this pull request Apr 6, 2023

Consolidate Vector & Array #5011

Closed

7 tasks

JaroslavTulach added 6 commits April 11, 2023 06:32

Merge branch 'develop' into wip/jtulach/Consolidate_5011

66b2b4d

More unification of createOrNull

2de4f39

Array & Vector have the same methods & behavior

db64a55

Filter out sort_builtin method

549d174

Another way to verify methods of Array and Vector are the same

e2e651a

Make sure Array methods have the same documentation as Vector ones

0d040e0

JaroslavTulach requested review from radeusgd and Akirathan April 11, 2023 10:18

JaroslavTulach commented Apr 11, 2023

View reviewed changes

radeusgd approved these changes Apr 11, 2023

View reviewed changes

radeusgd reviewed Apr 11, 2023

View reviewed changes

jdunkerley approved these changes Apr 11, 2023

View reviewed changes

4e6 approved these changes Apr 11, 2023

View reviewed changes

JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Apr 11, 2023

Merge branch 'develop' into wip/jtulach/Consolidate_5011

3594275

hubertp reviewed Apr 11, 2023

View reviewed changes

JaroslavTulach removed the CI: Ready to merge This PR is eligible for automatic merge label Apr 11, 2023

JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Apr 11, 2023

Merge branch 'develop' into wip/jtulach/Consolidate_5011

557e029

hubertp approved these changes Apr 11, 2023

View reviewed changes

mergify bot added 2 commits April 11, 2023 15:13

Merge branch 'develop' into wip/jtulach/Consolidate_5011

5fee003

Merge branch 'develop' into wip/jtulach/Consolidate_5011

38c0780

mergify bot merged commit 5b3cf6f into develop Apr 11, 2023

mergify bot deleted the wip/jtulach/Consolidate_5011 branch April 11, 2023 16:20

radeusgd mentioned this pull request Apr 14, 2023

Add split and tokenize to the Table. #6233

Merged

5 tasks

JaroslavTulach mentioned this pull request Apr 15, 2023

sort handles incomparable values #5998

Merged

5 tasks

JaroslavTulach mentioned this pull request Dec 6, 2023

Better Error Trapping in map #8307

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate Vector and Array methods #6218

Consolidate Vector and Array methods #6218

JaroslavTulach commented Apr 6, 2023 •

edited

Loading

radeusgd left a comment

JaroslavTulach left a comment

JaroslavTulach Apr 11, 2023

JaroslavTulach Apr 11, 2023

radeusgd left a comment

radeusgd Apr 11, 2023

jdunkerley left a comment

JaroslavTulach commented Apr 11, 2023

4e6 left a comment

hubertp left a comment

hubertp Apr 11, 2023

JaroslavTulach Apr 11, 2023

hubertp Apr 11, 2023

JaroslavTulach Apr 11, 2023 •

edited

Loading

hubertp Apr 11, 2023

JaroslavTulach Apr 11, 2023

radeusgd Apr 11, 2023

JaroslavTulach commented Apr 11, 2023

	zip : Vector Any -> (Any -> Any -> Any) -> Vector Any
	zip : Array Any -> (Any -> Any -> Any) -> Vector Any

Consolidate Vector and Array methods #6218

Consolidate Vector and Array methods #6218

Conversation

JaroslavTulach commented Apr 6, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

radeusgd left a comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Apr 11, 2023

4e6 left a comment

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Apr 11, 2023

JaroslavTulach commented Apr 6, 2023 •

edited

Loading

JaroslavTulach Apr 11, 2023 •

edited

Loading