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

Incorrect rewrite for defer #80

Closed
alaviss opened this issue May 2, 2021 · 3 comments · Fixed by #171
Closed

Incorrect rewrite for defer #80

alaviss opened this issue May 2, 2021 · 3 comments · Fixed by #171
Labels
bug Something isn't working

Comments

@alaviss
Copy link
Contributor

alaviss commented May 2, 2021

import cps

type
  InfiniteLoop = CatchableError
  Cont* = ref object of RootObj
    when cpsMutant:
      fn*: proc(c: var Cont) {.nimcall.}
    else:
      fn*: proc(c: Cont): Cont {.nimcall.}

var jumps: int

proc trampoline(c: Cont) =
  jumps = 0
  var c = c
  while c != nil and c.fn != nil:
    c = c.fn(c)
    inc jumps
    if jumps > 1000:
      raise newException(InfiniteLoop, $jumps & " iterations")

proc noop*(c: Cont): Cont {.cpsMagic.} = c

template check(cond: bool, msg: string) =
  if not cond:
    raise newException(Defect, "not " & astToStr(cond) & ": " & msg)

var r = 0

proc foo() {.cps: Cont.} =
  defer:
    check r == 2, "defer run before end of scope"
    inc r

  inc r
  noop()
  inc r

trampoline foo()
check r == 3, "expected 3, got: " & $r

Current output:

Error: unhandled exception: not r == 2: defer run before end of scope [Defect]
@alaviss alaviss added the bug Something isn't working label May 2, 2021
@disruptek
Copy link
Contributor

As far as where defer actually ends up, it's a bit of a head-scratcher. The best solution is probably to rewrite it in a very early pass as we do for. Did you happen to try using the same getTransformImpl hack?

@disruptek
Copy link
Contributor

This is stalled pending a compiler bug which @alaviss may decide to link here. 😘

@alaviss
Copy link
Contributor Author

alaviss commented May 23, 2021

blocked by nim-lang/Nim#18059

alaviss added a commit that referenced this issue Jun 13, 2021
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.
@alaviss alaviss linked a pull request Jun 13, 2021 that will close this issue
alaviss added a commit that 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
2 participants