Quote do now works with result in block

[PMunch]

Previously the useful "quote do" construct had an issue were the result variable would be shadowed by the macros result variable and thus get the wrong return type. These commits fixes this by adding a hidden parameter to the intermediary template and calling the template with an added newNimNode("result") argument. This way all results are only resolved after the macro expansion and thus avoids the shadowing of the macros own result variable.

[zah]

This seems a bit hacky to me. I'm not yet sure why the problem happened in the first place, as result is not put insides quotes in the quote block. My assumption would be that somewhere else in the compiler, the result pseudo-keyword is given a special treatment that's not appropriate in this situation.

[PMunch]

Well it has to do with how the quote do magic works. What it does is something like this:

macro (someNum: int, someStr: string): typed =
  quote do:
    echo "This is a num: " & $`someNum`
    echo "This is a str: " & $`someStr`

turns into

macro (someNum: int, someStr: string): typed =
  template anonTmpl(someNum, someStr): typed =
    echo "This is a num: " & $someNum
    echo "This is a str: " & $someStr
  anonTmpl(someNum, someStr)

What this fix does is this:

macro (someNum: int, someStr: string): typed =
  quote do:
    proc newProc(): string =
      result = "This is a num: " & $`someNum` &
        " This is a str: " & $`someStr`

turns into

macro (someNum: int, someStr: string): typed =
  template anonTmpl(anonParam, someNum, someStr): typed =
    proc newProc(): string =
      anonParam = "This is a num: " & $`someNum` &
        " This is a str: " & $`someStr`
  anonTmpl(newIdentNode("result"), someNum, someStr)

This works around the template being expanded within the macro body scope and capturing the result variable by leaving the result variable out until after expansion. Note that the above is just pseudo-code that doesn't actually run, but that is more-or-less how it works

[zah]

I know how it works, because I've created the implementations of both getAst and quote. The result variable should not be captured in this situation as getAst should not subject its result to lambda lifting. This seems to be the problem here.

[PMunch]

Oh you wrote it, cool. Really useful tool! Last time I asked Araq about this he said it would be a PITA to fix properly, so at the time I devised the work-around I posted here: #7323 which I've used with great success. When I saw the above issue I decided to share my solution and figured that I could just as well incorporate it into how quote worked to avoid this issue altogether. It's not perfect, but it works, at least until someone decides to fix it properly.

[krux02]

would it be possible to explicitly capture the result variable from the macro body with backticks? I am just curious if the workaround breaks certain use cases even though I would never recommend doing so. result is an output variable and not something that should be used as an input.

[PMunch]

Yes that should still be possible with this pull request. The wild result variable is converted to an anonymous symbol, and the NimNode with result is passed in as this anonymous symbol. So any result variable in backticks should still be possible. Note that I haven't actually tried that though so there might be something that breaks it.

[kaushalmodi]

May be use doAssert here instead of echo?

[PMunch]

Yes, that would make sense. The original error would prevent this from compiling at all, which is probably why I didn't care to check the actual output of test. But it would be good to do it none the less.

[krux02]

I like it and I want it.
I think it should still be possible to explicitly capture the result variable from the macro body.
Can you add this test case please?

import macros

macro foobar(arg: untyped): untyped =
  result = arg
  result.add quote do:
    `result`

foobar:
  echo "Hallo Welt"

[PMunch]

Yes, that is already taken into consideration. I've added the test you wanted now, so you can see that it works.

[PMunch]

Since this was fairly old I rebased it against devel to bring it up to speed. Now it should be possible to merge it into 0.19.x+

[krux02]

@PMunch can you also take a look at this issue. I actually forget about it and was recently reminded that it exists. Maybe you have a similar simple solution like this one for that problem:

[PMunch]

Hmm, nothing that comes to mind immediately, but I haven't looked at the code for that so it might be just as easy to fix once you look into it.