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

Symetric, transitive and reflexive equality for intersection types #11897

Merged
merged 11 commits into from
Dec 21, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Dec 17, 2024

Pull Request Description

Fixes #11845 by comparing all the types an EnsoMultiValue has been cast to.

Checklist

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

  • The documentation has been updated, read it here
  • All code follows the
    Scala,
    Java,
  • Unit tests have been written where possible.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Dec 18, 2024

There is a failure in test/Base_Tests line 324 showing that:

(x:Integer)==42 . should_be_true
(x:Fool)==42 . should_be_false
x==42 . should_be_true
42==(x.to Integer) . should_be_true

Any.to method doesn't work as expected for values with intersection types - e.g. the result is not the same as x:Integer. Made more consistent in db5353b

@JaroslavTulach JaroslavTulach changed the title EnsoMultiValue.== must be symetric, transitive and reflexive Symetric, transitive and reflexive equality for intersection types Dec 19, 2024
@JaroslavTulach JaroslavTulach added the CI: Keep up to date Automatically update this PR to the latest develop. label Dec 19, 2024
@JaroslavTulach JaroslavTulach removed the CI: Keep up to date Automatically update this PR to the latest develop. label Dec 20, 2024
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.

LGTM. Nice tests. Let's not forget to write benchmarks for intersection types before we start using them in std libs.

() -> {
var bothValue = ctx.asValue(both);
var asIntegerTo = conv.execute(0, bothValue);
var asIntegerCast = conv.execute(1, bothValue);
Copy link
Member

Choose a reason for hiding this comment

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

Seems like you forgot to test style 2 and 3, i.e.:

              var asTextTo = conv.execute(2, bothValue);
              var asTextCast = conv.execute(3, bothValue);
assertTrue(equals.execute(asTextTo, asTextCast).asBoolean());

Copy link
Member Author

@JaroslavTulach JaroslavTulach Dec 20, 2024

Choose a reason for hiding this comment

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

Good catch, not only it is not called, but it is also broken. These two new tests added in d30da41 are failing on CI

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Dec 21, 2024
@mergify mergify bot merged commit e8f781a into develop Dec 21, 2024
47 checks passed
@mergify mergify bot deleted the wip/jtulach/TransitiveReflexiveEquals11845 branch December 21, 2024 07:07
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.

EnsoMultiValue == isn't transitive
3 participants