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

Avoid StackOverflow when comparing unknown foreign objects #7780

Merged
merged 8 commits into from
Sep 19, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Sep 11, 2023

Pull Request Description

Closes #7677 by eliminating the stackoverflow execption. In general it seems too adventurous to walk members of random foreign objects. There can be anything including cycles. Rather than trying to be too smart in these cases, let's just rely on InteropLibrary.isIdentical message.

Important Notes

Calling sort on the numpy array no longer yields an error, but the array isn't sorted - that needs a fix on the Python side: oracle/graalpython#354 - once it is in, the elements will be treated as numbers and the sorting happens automatically (without any changes in Enso code).

Checklist

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

  • All code follows the
    Java,
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 11, 2023
@JaroslavTulach JaroslavTulach self-assigned this Sep 11, 2023
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good

test/Tests/src/Semantic/Equals_Spec.enso Outdated Show resolved Hide resolved
test/Tests/src/Semantic/Equals_Spec.enso Show resolved Hide resolved
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

I understand that a StackOverflow error when comparing numpy arrays is problematic and have to be fixed. But I am not sure I completely agree with this kind of simplification of equalsInteropObjectWithMembers. What about a compromise - keep the old version, but checking of member values would not be implemented with a child equalsNode, but rather with isIdentical interop message?

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Sep 11, 2023

What about a compromise - keep the old version, but checking of member values would not be implemented with a child equalsNode, but rather with isIdentical interop message?

What would be a point of a compromise? How could one explain that to users? Any important use-case that are covered by such a compromise? I could think of JSON, but there is a proper API for JSON in Enso and that one does == right.

@Akirathan
Copy link
Member

What about a compromise - keep the old version, but checking of member values would not be implemented with a child equalsNode, but rather with isIdentical interop message?

What would be a point of a compromise? How could one explain that to users? Any important use-case that are covered by such a compromise? I could think of JSON, but there is a proper API for JSON in Enso and that one does == right.

I guess you are right. I cannot think of any important use case for the compromise that I have suggested. Let's just keep it simple and check for isIdenticalObject as you suggest.

test/Tests/src/Semantic/Equals_Spec.enso Outdated Show resolved Hide resolved
@JaroslavTulach JaroslavTulach requested review from Akirathan and removed request for Akirathan September 19, 2023 14:57
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Sep 19, 2023
@mergify mergify bot merged commit 3b79060 into develop Sep 19, 2023
24 checks passed
@mergify mergify bot deleted the wip/jtulach/FinishSort_7677 branch September 19, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StackOverflowError in sort called on numpy array.
5 participants