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.

Sign up for GitHub

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

Jump to bottom

Vector should preserve warnings #3938

Merged
merged 9 commits into from
Dec 7, 2022
Merged

Vector should preserve warnings #3938

merged 9 commits into from
Dec 7, 2022

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Dec 1, 2022 •
edited
Loading

Pull Request Description

When Vector was created via a sequence literal, we simply dropped any associated any warnings associated with it.
Similarly doing any mapping on a Vector with warnings would create a new Vector with warnings now missing.

Rather than ducktaping every creation of Array/Vector/whatever so that they propagate warnings, decided to address the problem at the root. Array-like structures no longer remove warnings from values. Instead, whoever uses the values should ensure, if necessary, that they are gone. This was mostly problematic for polyglot calls.
Added a WarningsLibrary that exposes hasWarnings, getWarnings and removeWarnings to deal with warnings-related logic. This is primarily used in Vector which delegates all calls to the underlying storage but can be used in any collection.

Bonus points: warnings now survive multi-dimensional Vectors.

Important note

This PR addresses:

(initially the solution was very limited as it only addressed the former).

While working on polyglot calls discovered by accident that Warning.detach_selected_warnings was buggy and not tested. This is now fixed.

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 build and ./run ide watch.

@hubertp hubertp marked this pull request as ready for review December 1, 2022 14:11
@hubertp hubertp changed the title Sequence literal (Vector) should preserve warnings Vector should preserve warnings Dec 5, 2022
@hubertp hubertp force-pushed the wip/hubert/preserve-vector-warnings-182221022 branch from 4a1678b to c645b67 Compare December 5, 2022 16:00
JaroslavTulach
JaroslavTulach approved these changes Dec 6, 2022
View reviewed changes
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.

This is certainly step towards better implementation of WithWarnings. I left few questions inline - mostly related to multi-dimensional arrays.

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WarningsLibrary.java
* @return the associated warnings
*/
@GenerateLibrary.Abstract(ifExported = {"hasWarnings"})
public Warning[] getWarnings(Object receiver, Node location) throws UnsupportedMessageException {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to return any polyglot object that hasArrayElements? That might save few conversions/wrappings later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't do, it seems. Warnings need to be sorted later with per-creation timestamp

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java
Warning[] valuesWarnings = warningsLibrary.getWarnings(value, null);
allWarnings = new Warning[valuesWarnings.length + warningsArr.length];
System.arraycopy(warningsArr, 0, allWarnings, 0, warningsArr.length);
System.arraycopy(valuesWarnings, 0, allWarnings, warningsArr.length, valuesWarnings.length);
Copy link
Member

Choose a reason for hiding this comment

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

Here we are trying hard to create Warning[] and then we are going to convert it back to TruffleObject I assume.

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java
public static Array getAll(Object value, WarningsLibrary warnings) {
if (warnings.hasWarnings(value)) {
try {
return new Array((Object[]) warnings.getWarnings(value, null));
Copy link
Member

Choose a reason for hiding this comment

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

Here we take the array and wrap it again into TruffleObject. Maybe ArrayRope itself could be a TruffleObject?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My main concern is that we have a contract on returning Warnings in a specific order. I think it is a topic worth exploring but potentially in a separate PR.

engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java Show resolved Hide resolved
...e/src/main/java/org/enso/interpreter/node/expression/builtin/text/util/ExpectStringNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Array.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Array.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Array.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Array.java Outdated Show resolved Hide resolved
@hubertp
Copy link
Collaborator Author

hubertp commented Dec 6, 2022

Checked the benchmarks and they look OK

@hubertp hubertp force-pushed the wip/hubert/preserve-vector-warnings-182221022 branch from c645b67 to fed4ead Compare December 6, 2022 15:27
@hubertp
Sequence literal (Vector) should preserve warnings
9f703cd 
When Vector was created via a sequence literal, we simply dropped any
associated any warnings associated with it.
This change propagates Warnings during the creation of the Vector.
Ideally, it would be sufficient to propagate warnings from the
individual elements to the underlying storage but doesn't go well with
`Vector.fromArray`.
@hubertp
update changelog
f55f749
@hubertp
Array-like structures preserver warnings
32367eb 
Added a WarningsLibrary that exposes `hasWarnings` and `getWarnings`
messages. That way we can have a single storage that defines how to
extract warnings from an Array and the others just delegate to it.

This simplifies logic added to sequence literals to handle warnings.
@hubertp
Ensure polyglot method calls are warning-free
884fca8 
Since warnings are no longer automatically extracted from Array-like
structures, we delay the operation until an actual polyglot method call
is performed.

Discovered a bug in `Warning.detach_selected_warnings` which was missing
any usage or tests.
@hubertp
nits
8368bc5
@hubertp
Support multi-dimensional Vectors with warnings
f5ac561
@hubertp
Propagate warnings from case branches
45304b3
@hubertp
nit
74df323
@hubertp
Propagate all vector warnings when reading element
fc0b232 
Previously, accessing an element of an Array-like structure would only
return warnings of that element or of the structure itself.
Now, accessing an element also returns warnings from all its elements as
well.
@hubertp hubertp force-pushed the wip/hubert/preserve-vector-warnings-182221022 branch from 8b731f6 to fc0b232 Compare December 6, 2022 22:22
@hubertp hubertp merged commit 0855b74 into develop Dec 7, 2022
@hubertp hubertp deleted the wip/hubert/preserve-vector-warnings-182221022 branch December 7, 2022 10:10
@wdanilo wdanilo mentioned this pull request Feb 6, 2023
Open
@wdanilo wdanilo mentioned this pull request Feb 6, 2023
Closed
@Akirathan Akirathan mentioned this pull request Mar 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@jdunkerley jdunkerley jdunkerley approved these changes

@radeusgd radeusgd radeusgd approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hubertp @radeusgd @jdunkerley @JaroslavTulach