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

Bug in try-except rewrite of except T as e #164

Closed
alaviss opened this issue Jun 9, 2021 · 0 comments · Fixed by #171
Closed

Bug in try-except rewrite of except T as e #164

alaviss opened this issue Jun 9, 2021 · 0 comments · Fixed by #171
Labels
bug Something isn't working

Comments

@alaviss
Copy link
Contributor

alaviss commented Jun 9, 2021

import cps

proc noop(c: Continuation): Continuation {.cpsMagic.} = c

var r = 0

proc foo() {.cps: Continuation.} =
  try:
    noop()
    inc r
    raise newException(ValueError, "something")
  except ValueError as e:
    inc r
    doAssert typeof(e) is ValueError

foo()
doAssert r == 2
/home/leorize/source/cps/test.nim(16) test
/home/leorize/source/cps/test.nim(7) foo
/home/leorize/source/cps/test.nim(14) Except
/usr/lib/nim/lib/system/assertions.nim(39) failedAssertImpl
/usr/lib/nim/lib/system/assertions.nim(29) raiseAssert
/usr/lib/nim/lib/system/fatal.nim(53) sysFatal
Error: unhandled exception: /home/leorize/source/cps/test.nim(14, 14) `typeof(e) is ValueError`  [AssertionDefect]

We are rewriting e into the continuation's exception, which is ex and it has type ref Exception...

I see two possible solutions:

  • We de-sugar except T as e into except T: let e = (ref T)(getCurrentException())
  • We rewrite e into (ref T)(ex)

The second solution come with the cost of checked conversion on every access to this local, so maybe the first solution makes more sense.

@alaviss alaviss added the bug Something isn't working label Jun 9, 2021
@alaviss alaviss linked a pull request Jun 13, 2021 that will close this issue
alaviss referenced this issue Jun 14, 2021
* cps/[transform, environment]: use one exception local per try

Nim has a "exception stack" system where inner except branches will
push a new exception to the stack then pop it after it's done.

To replicate this accurately in CPS, we use one local per cps `try`
instead of a single field like how it is done currently. This is
because we set the global exception to whatever the stored exception
was in our continuation leg, so to prevent inner cps `try`s from overriding
what is perceived to be the "current" exception of the outer branch, we
simply give them different locals.

Ideally we can tap right into Nim's exception `up` field, but it's not
exported.

* cps/transform: rewrite `n` instead of doing a roundabout

Not sure what I thought of when I wrote that code, but we can just
run filter on the outer node and don't have to think about the bugs
this `for` would cause us.

* move trampoline and bind to it in bootstrap

* tests/[preamble, ttry]: test exception properties again

* cps/rewrites: add a simplifying rewrite for except T as e

Fixes disruptek/cps#164

* cps/transform: rewrite cps try-except into one continuation

Instead of creating a continuation leg for each except branch, we
merge all except branches into one, then turn that into a continuation.

That way we only have one continuation for all handlers, allow for
a better implementation of cps exceptions in the future.

* cps/transform: try-finally transformation

Implements try-finally transformation for CPS by generating
finally as a continuation leg with a static generic for where
it will continues after.

Untested because the compiler broke.

Known issues:
- Early returns are not handled, which can be fixed by improving
  isScopeExit and making early termination an annotation.

Fixes #80.

* cps/transform: switch the implementation of try-finally to templating

Instead of waiting for nim-lang/Nim#18254
and any other templates bug to be fix. We take the initiative and
write our own continuation templater. It appears to work well enough
to use as an alternative until a better alternative become available.

I've also added an extra test that verify the exception re-raise property.

* cps/spec: remove cpsRecovery

We ended up not needing it for finally

* cps/environment: implement early returns as an annotation

We implement early returns as `cpsTerminate` then tie them up at the
end via `cpsResolver`. This way try-finally can capture early termination and
specialize to those.

Need @disruptek to review this stuff.

* tdefer: enable the test for defer across continuation

* tzevv: enable the defer test

* cps/transform: support except clause with multiple exceptions

* cps/transform: cosmetics for disruptek

Co-authored-by: Andy Davidoff <github@andy.disruptek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant