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

This fixes #1920 but it has a backward compatibility problem. #1921

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jurgenvinju
Copy link
Member

  • stackoverflow errors were not reported properly. Instead the
    interpreter triggered many more stackoverflow errors while trying to
    report on them.
  • fixed this by capturing the deepest overflow exception and wrapping
    the current runtime stack in a cheap exception object. This object is
    then thrown an caught by the REPL loop which prints the proper
    exception stack trace.
  • We lost the ability to catch a stackOverflow() exception in Rascal
    code with this. This is problematic since there are tools that use
    that (drAmbiguity) in case of expected eternal recursions. So for now
    this is PR and I would like to hear if anybody has ideas on how to fix
    this properly without loss of this important feature.

* stackoverflow errors were not reported properly. Instead the
  interpreter triggered many more stackoverflow errors while trying to
  report on them.
* fixed this by capturing the deepest overflow exception and wrapping
  the current runtime stack in a cheap exception object. This object is
  then thrown an caught by the REPL loop which prints the proper
  exception stack trace.
* We lost the ability to catch a stackOverflow() exception in Rascal
  code with this. This is problematic since there are tools that use
  that (drAmbiguity) in case of expected eternal recursions. So for now
  this is PR and I would like to hear if anybody has ideas on how to fix
  this properly without loss of this important feature.
@jurgenvinju
Copy link
Member Author

At least this fix confirms the diagnosis that the error handler was causing new StackOverflowErrors, which explains the messages printed here #1920.

Maybe we can find a proper stack frame deeper in than the REPL is to throw the real throw.

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 5.88235% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 49%. Comparing base (f34dc15) to head (767ea0b).
Report is 18 commits behind head on main.

❗ Current head 767ea0b differs from pull request most recent head 6595ea8. Consider uploading reports for the commit 6595ea8 to get more accurate results

Files Patch % Lines
...rascalmpl/exceptions/RascalStackOverflowError.java 0% 11 Missing ⚠️
src/org/rascalmpl/repl/RascalInterpreterREPL.java 0% 3 Missing ⚠️
...rc/org/rascalmpl/semantics/dynamic/Expression.java 33% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              main   #1921   +/-   ##
=======================================
- Coverage       49%     49%   -1%     
+ Complexity    6165    6163    -2     
=======================================
  Files          661     662    +1     
  Lines        58702   58711    +9     
  Branches      8547    8548    +1     
=======================================
- Hits         28965   28961    -4     
- Misses       27545   27561   +16     
+ Partials      2192    2189    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jurgenvinju jurgenvinju force-pushed the proper-stacktrace-for-recursion-overflow branch from d79b2f4 to d089ff6 Compare February 29, 2024 15:36
@jurgenvinju
Copy link
Member Author

@PaulKlint I'm not sure if we could implement catchable stackOverlflow() exceptions anyway in the compiled context. There is no test for it, even though I've used it in the past. If we can't implement it, we might as well bite the bullet now and remove that feature from the interpreter too. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant