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

Always try to resolve conversion for Any type #6184

Merged
merged 4 commits into from
Apr 4, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Apr 3, 2023

Pull Request Description

Fixes #5898 by removing Catch.panic and speeding the sieve.enso benchmark from 1058 ms to 514 ms. Should there be no dedicated conversion, let's use one defined on Any type - e.g. defining a conversion from(Any) makes such a conversion is always available.

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 Apr 3, 2023
@JaroslavTulach JaroslavTulach self-assigned this Apr 3, 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.

Maybe worth adding a few test cases for it?

I'd imagine at least 3:

  1. Have a conversion both on type Foo and Any: verify that for an instance of type Foo the more specific one (Foo, not Any) is selected.
  2. Have a conversion on Any but not Foo - check that Any is selected - essentially what this PR does and is implicitly tested by the == not breaking, but I think it's good to have an explicit test for it.
  3. Have no conversion defined on neither Foo nor Any - to ensure that the conversion resolution error is still correctly reported.

@JaroslavTulach
Copy link
Member Author

Maybe worth adding a few test cases for it?

There were fifteen failing tests in test/Tests project when I changed the implementation of Any.== - I had to fix them all. E.g. while no test was added, the code is certainly tested.

@radeusgd
Copy link
Member

radeusgd commented Apr 3, 2023

Maybe worth adding a few test cases for it?

There were fifteen failing tests in test/Tests project when I changed the implementation of Any.== - I had to fix them all. E.g. while no test was added, the code is certainly tested.

Sure, but these explicit tests would also serve as documentation on what is the behaviour we currently expect.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 4, 2023

Maybe worth adding a few test cases for it?

There is a lot of tests in Conversion_Spec.enso including those that you propose:

Have a conversion both on type Foo and Any: verify that for an instance of type Foo the more specific one (Foo, not Any) is selected.

Not_Foo type & its tests verify this.

Have a conversion on Any but not Foo - check that Any is selected - essentially what this PR does and is implicitly tested by the == not breaking, but I think it's good to have an explicit test for it.

That's "should be able to convert Any" test, I'd say.

Have no conversion defined on neither Foo nor Any - to ensure that the conversion resolution error is still correctly reported.

There are two "should fail graciously..." tests that seem to cover this behavior.

At the end I am just adding two tests for foreign Java object and a type: 40cc46b

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Apr 4, 2023
@JaroslavTulach JaroslavTulach removed the CI: Ready to merge This PR is eligible for automatic merge label Apr 4, 2023
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 4, 2023

Running benchmarks for this PR. Manual comparison with develop branch shows no slowdowns, and indicates few speedups:

obrazek

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Apr 4, 2023
@JaroslavTulach JaroslavTulach requested a review from radeusgd April 4, 2023 09:18
@mergify mergify bot merged commit 519df66 into develop Apr 4, 2023
@mergify mergify bot deleted the wip/jtulach/ConversionFromAnyFallback_5898 branch April 4, 2023 09:24
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.

Any.== slowed down by Panic.catch 2x times
4 participants