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

Two findExceptionMessage methods to extract exception messages consistently #5684

Merged
merged 4 commits into from
Feb 20, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Feb 17, 2023

Pull Request Description

Creating two findExceptionMessage methods in HostEnsoUtils and in VisualizationResult. Why two? Because one of them is using org.graalvm.polyglot SDK as it runs in "normal Java" mode. The other one is using Truffle API as it is running inside of partially evaluated instrument.

There is a FindExceptionMessageTest to guarantee consistency between the two methods. It simulates some exceptions in Enso code and checks that both methods extract the same "message" from the exception. The tests verifies hosted and well as Enso exceptions - however testing other polyglot languages is only possible in other modules - as such I created PolyglotFindExceptionMessageTest - but that one doesn't have access to Truffle API - e.g. it doesn't really check the consistency - just that a reasonable message is extracted from a JavaScript exception.

Important Notes

This is not full fix of #5260 - something needs to be done on the IDE side, as the IDE seems to ignore the delivered JSON message - even if it contains properly extracted exception message.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.

try {
msg = iop.getExceptionMessage(ex);
} catch (UnsupportedMessageException ignore) {
throw CompilerDirectives.shouldNotReachHere(ignore);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we typically throw illegal state exception. is CompilerDirectives.shouldNotReachHere the new recommended way?

Copy link
Member Author

@JaroslavTulach JaroslavTulach Feb 20, 2023

Choose a reason for hiding this comment

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

I believe shouldNotReachHere has a special treatment in the compiler and leads to shorter code then regular exceptions. In case of normla exception the constructor (including fillInStackTrace call) is still executed in the partial evaluation mode - e.g. the generated code is bigger than in case of shouldNotReachHere. I performed a little experiment and tried to compile in my talk2compiler project:

    @Override
    public Object execute(VirtualFrame frame) {
      final String name = (String) frame.getArguments()[0];
      if (name.length() > 0) {
        return formatGreeting("Hello from %s!", name);
      } else {
        throw CompilerDirectives.shouldNotReachHere("The 0-th arg is wrong and leads");
        // throw new NullPointerException("The 0-th arg is wrong and leads");
      }
    }

I am attaching result of comparing NullPointerException with shouldNotReachHere handler. There is clearly a difference between NullPointerException and shouldNotReachHere:

NPEvsShouldNotReachHere

However I haven't managed to simulate the difference with IllegalStateException. Looks like there is a set of skipped exception types and those get special treatment, similar to shouldNotReachHere. The IllegalStateException has been added there two years ago - e.g. at the end we can continue to use IllegalStateException and get the same result of shouldNotReachHere.

@@ -112,7 +112,7 @@ object FrgaalJavaCompiler {
def checkTarget(x : Any) = {
val p = asPath(x)
val namesCheck = for (i <- 0 until p.getNameCount)
yield "target".equals(p.getName(i).toString())
yield "target".equals(p.getName(i).toString()) || p.getName(i).toString().endsWith("-windows") || p.getName(i).toString().endsWith("-unix")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need those now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed this to Make sure logger-service can be debugged in Enso4Igv modules. A bit unrelated, but I was trying to investigate #5683 and I cannot do that without being able to debug logger-service.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Feb 20, 2023
@mergify mergify bot merged commit 172f729 into develop Feb 20, 2023
@mergify mergify bot deleted the wip/jtulach/Exceptions_5260 branch February 20, 2023 11:27
@enso-bot
Copy link

enso-bot bot commented Feb 21, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-02-20):

Progress: - Fixed exception handling: #5684

Next Day: Working on SuggestionDB

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.

Throwing an unhandled exception in Polyglot Java results in a null in the UI and no caught error message
2 participants