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

only allow re-raise in scope of except #1170

Merged

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Feb 6, 2024

Summary

Treat a re-raise (i.e., raise without operand) as a compile-time
error when used outside of the scope of an except clause -- re-
raising from within a finally clause that's within the scope of an
except clause is disallowed too.

This brings the re-raise feature into a "safe" state, where it can be
extended from again if wanted/needed.

Details

If a re-raise is allowed from anywhere, it is possible to handle an in-
flight exception from within a finally clause:

try:
  raise CatchableError.newException("")
finally:
  try:
    raise # re-raise the in-flight exception
  except:
    # handle the exception
    discard

Since the exception is handled, control-flow would have to "fall out"
of the finally clause, but this violates the expectations of control-
flow analysis in the compiler as well as those of the code generators.
There, the expectation is that if a finally clause was entered
through exceptional control-flow, raising continues when the end of
the finally is reached.

Until it's clear if, how, and under what circumstances this should
work, a finally clause handling the in-flight exception is prevented
by restricting re-raise statements to the scope of except clauses.
sempass2 implements the detection and error reporting.

The manual is updated accordingly and tests for the new behaviour are
added.

Usage

In the compiler's repository, re-raising from outside the scope of an
except clause was only done by two tests. For one of them, the re-
raise can safely be replaced with a normal raise.

The other test explicitly tested the "re-raise in finally" case. It is
removed for now.

`track` keeps track of whether directly inside a except, and only
allows a re-raise statement when this is the case.

A new report is added for the error.
The `reraiseInFinally` test case explicitly tested the now-disallowed
usage. It is removed.
The `t18203_consumed_ast_is_not_unused` test made use of the now-
disallowed `raise` usage, but it didn't rely on the functionality. The
re-raise is replaced with a normal `raise` statement.
@zerbina zerbina added compiler/sem Related to semantic-analysis system of the compiler language-design Language design syntax, semantics, types, statics and dynamics. labels Feb 6, 2024
@zerbina zerbina requested a review from saem February 6, 2024 23:31
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Minor suggestion for additional test coverage, but perhaps it's sufficiently covered in tfinally6.exceptionInFinally -- feel free to ignore it.

tests/exception/tdisallowed_reraise.nim Show resolved Hide resolved
Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
@zerbina
Copy link
Collaborator Author

zerbina commented Feb 7, 2024

/merge

Copy link

github-actions bot commented Feb 7, 2024

Merge requested by: @zerbina

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • the strategy and problem were discussed here

@chore-runner chore-runner bot added this pull request to the merge queue Feb 7, 2024
Merged via the queue into nim-works:devel with commit 6864e18 Feb 7, 2024
31 checks passed
@zerbina zerbina deleted the disallow-reraise-outside-except branch February 8, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/sem Related to semantic-analysis system of the compiler language-design Language design syntax, semantics, types, statics and dynamics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants