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

Adding a static generic parameter to a proc via typed macro produces internal error #18254

Open
alaviss opened this issue Jun 13, 2021 · 3 comments
Labels
CPS bugs/pulls related to the cps project Generics Macros Static[T]

Comments

@alaviss
Copy link
Collaborator

alaviss commented Jun 13, 2021

Example

import macros

macro addStaticGeneric(n: typed): untyped =
  result = copyNimTree(n)
  # remove the result symbol so the compiler won't complain
  result.del 7
  
  # add [genericSym: static[int]] to the function
  let genericSym = nskGenericParam.genSym"Value"
  result[2] = nnkGenericParams.newTree:
    newIdentDefs(genericSym):
      nnkStaticTy.newTree:
        bindSym"int"
        
  # replace the function body with returning this generic parameter
  result.body = newStmtList:
    newAssignment(ident"result", genericSym)

proc foo(): int {.addStaticGeneric.} =
  discard "to be replaced"

doAssert foo[42]() == 42

Current Output

test.nim(9, 35) Error: internal error: expr(skGenericParam); unknown symbol

Expected Output

Compiles and run successfully

Additional Information

  • This is blocking CPS' finally implementation
$ nim -v
Nim Compiler Version 1.5.1 [Linux: amd64]
Compiled at 2021-06-08
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: 51ab7ccec1ffa21cef67f1a75dea889653751cfc
active boot switches: -d:release -d:nimUseLinenoise
@alaviss alaviss added Generics Static[T] CPS bugs/pulls related to the cps project labels Jun 13, 2021
alaviss added a commit to nim-works/cps that referenced this issue Jun 13, 2021
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.
alaviss added a commit to nim-works/cps 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>
@bung87
Copy link
Collaborator

bung87 commented Dec 10, 2022

it should be let genericSym = ident"Value"

@alaviss
Copy link
Collaborator Author

alaviss commented Dec 10, 2022

The main point is that you can't use genSym, which means you can't add unique symbols that are hidden to user code.

@bung87
Copy link
Collaborator

bung87 commented Dec 11, 2022

so use a random string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CPS bugs/pulls related to the cps project Generics Macros Static[T]
Projects
None yet
Development

No branches or pull requests

3 participants