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

Switch to Truffle PE mode before calling into Enso #5783

Merged
merged 10 commits into from
Mar 3, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Mar 1, 2023

Pull Request Description

Enter partial evaluation mode via CallTarget.call before invoking InteropLibrary. Fixes #5782.

Checklist

Please include the following checklist in your PR:

  • All code conforms to the
    Java,
    style guides.
  • All code has been tested:
    • Existing Unit tests must continue to pass

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 1, 2023
@JaroslavTulach JaroslavTulach added this to the Design Partners milestone Mar 1, 2023
@JaroslavTulach JaroslavTulach self-assigned this Mar 1, 2023
@@ -449,4 +454,53 @@ public String getExceptionMessage(PanicException panic) {
context.getThreadManager().leave(p);
}
}

@SuppressWarnings("unchecked")
private static <E extends Exception> E raise(Class<E> type, Exception ex) throws E {
Copy link
Collaborator

Choose a reason for hiding this comment

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

gotta love Java :)

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this method instead of putting throw ex directly at callsite?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I see that for example ArityException or UnsupportedTypeException extend InteropException which apparently does not seem to inherit RuntimeException.

So I guess this is the reason for this magic?

But that seems totally unsafe - since we do an unchecked cast to RuntimeException of something that we know that it doesn't extend RuntimeException.

If the cast is unchecked, I guess it will go through, but it will violate the typesystem rules. That sounds very worrying at first sight.

Can we get a comment (in the code, as part of this function's doc comment) explaining why we are doing it and why is it safe in this context? (If it is safe...)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a standard way to propagate checked exception thru Java language. Java language has (too) strict rules about checked exceptions, but there is nothing like that in the JVM (as demonstrated by Scala ignoring checked exceptions altogether). Sometimes it is useful to propagate checked exception even the Java method signatures are against it. This is the case. We have to tunnel the exception thru CallTarget interface now. Rather than wrapping the exception into runtime and unpacking it, I am trying to throw it directly. Btw. this is not the first place where this coding technique is used. There are other raise methods around the codebase already.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Mar 2, 2023
@mergify mergify bot merged commit f64edd8 into develop Mar 3, 2023
@mergify mergify bot deleted the wip/jtulach/SwitchToPe_5782 branch March 3, 2023 14: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.

ExecutionService.execute is running in uncached (e.g. slow) mode
4 participants