-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix(sem): missed discard
in try/finally
errors
#995
Conversation
## Summary A missing `discard` in a `try/finally` no longer crashes the compiler and emits an error as it should. ## Details The `discardCheck` used to ensure expressions are used or `void` did not traverse `try` appropriately. Instead of looking at the last expression in the last `except` or `try` branch, it would look at the last branch, even if that was a `finally`. This led to incomplete data being sent for error reporting, which ultimately led to an NPE. Along with the fix a regression test has been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node indexing changes in semTry
need to be reverted, otherwise the following:
try:
discard
except:
1
is no longer reported as being incorrect. This is because the type of both nkExceptBranch
and nkFinally
is always nil, meaning that the discardCheck
call will do nothing.
@zerbina I added an extra test to avoid regressing on the example provided -- nice catch. |
/merge |
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
Summary
A missing
discard
in atry/finally
no longer crashes the compilerand emits an error as it should.
Details
The
discardCheck
used to ensure expressions are used orvoid
did nottraverse
try
appropriately. Instead of looking at the last expressionin the last
except
ortry
branch, it would look at the last branch,even if that was a
finally
. This led to incomplete data being sent forerror reporting, which ultimately led to an NPE.
Along with the fix a regression test has been added.