-
Notifications
You must be signed in to change notification settings - Fork 77
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
Trigger recovery when only recovery stacks are left #2057
Merged
PieterOlivier
merged 13 commits into
feat/error-recovery
from
recovery/only-recovery-stacks-left
Oct 24, 2024
Merged
Trigger recovery when only recovery stacks are left #2057
PieterOlivier
merged 13 commits into
feat/error-recovery
from
recovery/only-recovery-stacks-left
Oct 24, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Draft
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/error-recovery #2057 +/- ##
=====================================================
Coverage 49% 49%
- Complexity 6580 6593 +13
=====================================================
Files 685 685
Lines 61127 61147 +20
Branches 8841 8849 +8
=====================================================
+ Hits 30189 30215 +26
Misses 28725 28725
+ Partials 2213 2207 -6 ☔ View full report in Codecov by Sentry. |
DavyLandman
reviewed
Oct 22, 2024
PieterOlivier
force-pushed
the
feat/error-recovery
branch
from
October 22, 2024 07:55
5e06728
to
2683d8c
Compare
Experimented with max skip length
…-merge-experiment
MultiErrorBug condenses the Rascal syntax to only cover the problematic issue. The issue itself is of yet unsolved.
PieterOlivier
force-pushed
the
recovery/only-recovery-stacks-left
branch
from
October 24, 2024 05:46
89c6a5b
to
4d94b28
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, error recovery was only triggered when no more active stacks where left.
This PR improves on this by also triggering recovery when only recovery stacks are left.
This change increases recovery quality tremendously, but it also decreased performance significantly.
To combat this, filtering of "duplicate" skip nodes has been added. Only one skip node with for a specific starting position, result type, and skip length is added.
This does not impact recovery quality but it does improve performance dramatically.
The end result of these changes is improved recovery quality and improved performance.
Note that the downside is that the resulting parse forest does not include some nodes. For instance, if we have a language that allows expressions with multiple productions like
<expr> + <expr>
and<expr> - <expr>
and an error occurs before the operator, only one of these expression productions will be included as an error node in the parse tree.Note that this information can be reconstructed by combining the error nodes with the grammar so it does not really limit the future use cases.
This PR also adds support for limiting the match length. This feature has only been used in experiments but might be actively used in the future so I decided to leave it in.