Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Array.set_at #3634

Merged
merged 11 commits into from
Aug 26, 2022
Merged

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Aug 4, 2022

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/182879865

Important Notes

Note that removing set_at still does not make our arrays fully immutable - Array.copy can still be used to mutate them.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide dist and ./run ide watch.

@radeusgd radeusgd self-assigned this Aug 4, 2022
@radeusgd radeusgd marked this pull request as draft August 4, 2022 15:17
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be more consistent if append_vector_range used the same parameters as take and drop. Otherwise perfect - I am very glad Array.set is gone!

@radeusgd radeusgd force-pushed the wip/radeusgd/remove-array-set-at-182879865 branch from 0a6480c to f18628e Compare August 11, 2022 11:43
Comment on lines -132 to -145
@ExportMessage
void writeArrayElement(long index, Object value) {
items[(int) index] = value;
}

@ExportMessage
boolean isArrayElementModifiable(long index) {
return isArrayElementReadable(index);
}

@ExportMessage
boolean isArrayElementInsertable(long index) {
return false;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to remove these, since our arrays are meant to be immutable by design.

Curiously, still, Array.copy is able to overwrite contents of our arrays even if they report being 'unmodifiable'. Very weird. @JaroslavTulach do you think this may be a bug? I assume it should be somehow possible to create a completely read-only array which does not support writing (possibly can be done by wrapping it in an UnmodifiableList).
OTOH we are actually currently relying on this, as we use Array.copy for implementing many of our Vector operations.

@radeusgd
Copy link
Member Author

The removal of set_at is done and it seems to more or less work.

However many tests started failing for a separate, though related, reason. The reason is however a bug that was already there, it was just much more rare because most of our arrays used to be Enso Array type. Now, many operations like Vector.map (which relies on Vector.new) will return Vectors backed by polyglot arrays. Many of our builtins actually expect an Enso Array and if they get a polyglot array they panic reporting an unsupported type. Interestingly, the same will happen with our ArrayOverBuffer type, since it also is not a subclass of Array. This is bad and I think we need to discuss how to proceed with that. cc: @JaroslavTulach @hubertp @jdunkerley

A temporary fix would be to write a function

ensure_enso array =
    case array of
        Array -> array
        _ ->
            new_array = Array.new array.length
            Array.copy array 0 new_array 0 array.length
            new_array

which ensures that an Enso array is used, and use it everywhere we pass arrays to such builtins, as there aren't that many places that do so. But this is definitely not an ideal solution, I think we need to rework how builtins expecting arrays work so that they accept any kind of array, not just the Enso ones. Maybe we can just replace Array with Object[] or modify our builtins DSL to add such additional specializations to make it work? (cc: @hubertp / @kustosz)

@radeusgd
Copy link
Member Author

As @hubertp pointed out, the issue may be partially fixed by #3628. I think the issue may still be worth discussing - because we would still be living with 3 types of arrays which are not interchangeable (not a very polyglot state of things), but at least vectors constructed from polyglot arrays would not cause such trouble as what is causing our current test failures, so that would be a step in the right direction. Still, for example the ArrayOverBuffer is incompatible with Array and we may need to revisit this to make it work properly.

I think it will make most sense to put this PR on hold until #3628 is integrated and we can discuss the right way to proceed.

@radeusgd radeusgd force-pushed the wip/radeusgd/remove-array-set-at-182879865 branch 2 times, most recently from e579617 to 8557a74 Compare August 12, 2022 08:53
@radeusgd radeusgd force-pushed the wip/radeusgd/remove-array-set-at-182879865 branch from 8557a74 to 29c4abc Compare August 24, 2022 13:15
@radeusgd radeusgd marked this pull request as ready for review August 24, 2022 13:18
@radeusgd radeusgd requested a review from hubertp August 24, 2022 13:19
@radeusgd radeusgd force-pushed the wip/radeusgd/remove-array-set-at-182879865 branch from 29c4abc to 01011f5 Compare August 24, 2022 15:44
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Aug 25, 2022
@radeusgd radeusgd force-pushed the wip/radeusgd/remove-array-set-at-182879865 branch from e2093bf to a7c8392 Compare August 25, 2022 09:30
@mergify mergify bot merged commit fd318cf into develop Aug 26, 2022
@mergify mergify bot deleted the wip/radeusgd/remove-array-set-at-182879865 branch August 26, 2022 09:34
JaroslavTulach added a commit that referenced this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants