Remove `Array.new` and `Array.copy` and move Vector functions to builtins. #7147

jdunkerley · 2023-06-28T12:33:06Z

Pull Request Description

Removed Array methods: new, copy and new_[1234].
New builtins for Vector.insert, Vector.remove and Vector.flatten.
Replaced Vector_Builder use of Array.copy to a Vector.Builder approach.

Checklist

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

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

hubertp

So the idea with Array and Vector is to make sure that they have essentially the same methods and can be used interchangeably (see also #6299). It seems like the allocate bit interfere with that.

Re builtins design - lgtm so far.

jdunkerley · 2023-06-29T16:39:37Z

So the idea with Array and Vector is to make sure that they have essentially the same methods and can be used interchangeably (see also #6299). It seems like the allocate bit interfere with that.

Ideally, I will remove allocate and copy shortly. This will mean only way to make an Array within Enso is by creating a vector and calling to_array.

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

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

engine/runtime/src/test/java/org/enso/interpreter/test/ArrayTest.java

radeusgd

Looks good, just a question.

hubertp

I'd rather we keep some of the static wrapper calls in place, if possible

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

JaroslavTulach

Overall very positive and useful change. Some comments, advises left for your consideration.

It'd be good to benchmark flatten, insert and remove and compare previous and new results.

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

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

JaroslavTulach · 2023-07-03T05:21:05Z

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

-            Array.copy vec.to_array 0 arr i vec.length
-            i + vec.length
-        Vector.from_polyglot_array arr
+    flatten self = @Builtin_Method "Vector.flatten"


I am surprised flatten isn't written in Enso and requires a builtin. Less builtins is always better...

flat x = size = x.map .length . fold 0 (+) b = Vector.new_builder size x.map arr-> arr.map e-> b.append e b.to_vector

this code seems to flatten the elements too.

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

JaroslavTulach · 2023-07-03T05:26:48Z

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso

-                  self.move_builtin destination copy_options
-                False -> self.move_builtin destination Array.empty
+                  copy_options = [StandardCopyOption.REPLACE_EXISTING]
+                  self.move_builtin destination copy_options.to_array


Nobody shall require .to_array call. The File should be fixed to work with Vector or any array-like object. The issue is already tracked as

File's input_stream type signatures are inconsistent #7133

...ain/java/org/enso/interpreter/node/expression/builtin/immutable/InsertBuiltinVectorNode.java

...src/main/java/org/enso/interpreter/node/expression/builtin/immutable/RemoveAtVectorNode.java

...time/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/CoerceArrayNode.java

engine/runtime/src/test/java/org/enso/interpreter/test/ArrayTest.java

engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/SystemProcessTest.scala

Removing `Array.empty`.

Replaces the `Array.copy` in `Vector_Builder` with a new `Builder` based approach.

Removed old test now allocate removed.

Linting type sig check fixes.

Passing `ArraySlice` would enter the fallback and crash.

Some type signature fixes.

jdunkerley force-pushed the wip/jd/s3-list-objects branch from 81f1694 to f896873 Compare June 29, 2023 12:52

hubertp reviewed Jun 29, 2023

View reviewed changes

jdunkerley force-pushed the wip/jd/s3-list-objects branch from f896873 to 7ce4ccf Compare June 30, 2023 10:20

jdunkerley changed the title ~~Wip/jd/s3 list objects~~ Remove Array.new and Array.copy and move Vector functions to builtins. Jun 30, 2023

jdunkerley added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 30, 2023

jdunkerley marked this pull request as ready for review June 30, 2023 11:01

jdunkerley requested review from radeusgd, GregoryTravis, 4e6, JaroslavTulach and Akirathan as code owners June 30, 2023 11:01

radeusgd reviewed Jun 30, 2023

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso Outdated Show resolved Hide resolved

radeusgd reviewed Jun 30, 2023

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso Outdated Show resolved Hide resolved

radeusgd reviewed Jun 30, 2023

View reviewed changes

engine/runtime/src/test/java/org/enso/interpreter/test/ArrayTest.java Outdated Show resolved Hide resolved

radeusgd approved these changes Jun 30, 2023

View reviewed changes

hubertp reviewed Jun 30, 2023

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso Outdated Show resolved Hide resolved

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso Outdated Show resolved Hide resolved

GregoryTravis approved these changes Jun 30, 2023

View reviewed changes

jdunkerley added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jun 30, 2023

JaroslavTulach approved these changes Jul 3, 2023

View reviewed changes

jdunkerley added 10 commits July 3, 2023 08:26

Removing Array.new_[1234].

b60545f

Removing `Array.empty`.

Fix doc comment.

91eda85

Remove the use to to_array.

10fd9a6

Fix failing test.

e4c134b

Restore to_array for the builtins which can support it.

a7b08d6

Fix failed test.

fcb00f2

Add remove_builtin.

c68164f

Add flatten as a builtin.

be8e931

Remove allocate and copy from builtins.

e7f4dcf

Replaces the `Array.copy` in `Vector_Builder` with a new `Builder` based approach.

Fix Array issues using Vector builtins.

258d8d3

jdunkerley and others added 12 commits July 3, 2023 08:27

More fixes.

3f0eec2

Java format.

a1adb9d

PR comments.

c10f33a

Avoid initialising Array values.

531b448

Removed old test now allocate removed.

Java format.

32c03d0

Linting type sig check fixes.

Adding Array support for insert.

310f8ca

Deal with Array-like selfs

dd6630d

Passing `ArraySlice` would enter the fallback and crash.

Adding Array support for RemoveAt.

ce11750

Adding Array support for Flatten.

71ef31d

Revert a mistake.

e40beff

Extra argument.

2cd4bf3

PR comments.

75e85c3

jdunkerley force-pushed the wip/jd/s3-list-objects branch from 7eff518 to 75e85c3 Compare July 3, 2023 07:46

jdunkerley added 2 commits July 3, 2023 08:51

Java format.

dc1800d

Fix Vector.+.

7f3c959

hubertp approved these changes Jul 3, 2023

View reviewed changes

Akirathan approved these changes Jul 3, 2023

View reviewed changes

jdunkerley added 2 commits July 3, 2023 11:28

Change self name for the static builtins.

9fefcc5

Java format.

94eb957

Some type signature fixes.

jdunkerley added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Jul 3, 2023

mergify bot merged commit 4fbe7e3 into develop Jul 3, 2023

mergify bot deleted the wip/jd/s3-list-objects branch July 3, 2023 12:41

enso-bot bot mentioned this pull request Jul 4, 2023

Recalculate only necessary nodes when a node changes #7020

Closed

Akirathan mentioned this pull request Jul 7, 2023

Enable Assertions in Enso Tests #5585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `Array.new` and `Array.copy` and move Vector functions to builtins. #7147

Remove `Array.new` and `Array.copy` and move Vector functions to builtins. #7147

jdunkerley commented Jun 28, 2023 •

edited

Loading

hubertp left a comment •

edited

Loading

jdunkerley commented Jun 29, 2023

radeusgd left a comment

hubertp left a comment

JaroslavTulach left a comment

JaroslavTulach Jul 3, 2023

JaroslavTulach Jul 3, 2023

Remove Array.new and Array.copy and move Vector functions to builtins. #7147

Remove Array.new and Array.copy and move Vector functions to builtins. #7147

Conversation

jdunkerley commented Jun 28, 2023 • edited Loading

Pull Request Description

Checklist

hubertp left a comment • edited Loading

Choose a reason for hiding this comment

jdunkerley commented Jun 29, 2023

radeusgd left a comment

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Jul 3, 2023

Choose a reason for hiding this comment

JaroslavTulach Jul 3, 2023

Choose a reason for hiding this comment

Remove `Array.new` and `Array.copy` and move Vector functions to builtins. #7147

Remove `Array.new` and `Array.copy` and move Vector functions to builtins. #7147

jdunkerley commented Jun 28, 2023 •

edited

Loading

hubertp left a comment •

edited

Loading