-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[superseded] new macros.genAst
: sidesteps issues with quote do
#11722
Conversation
macros.genAst
: fixes all issues with quote do
macros.genAst
: fixes all issues with quote do
macros.genAst
: fixes all issues with quote do
macros.genAst
: fixes all issues with quote do
Looks very nice, but @krux02 needs to do the final review and he is on holidays. ;-) |
@timotheecour Can you also add some more "readme" in the macro tutorial (tut 3) to compare this to Thanks. |
yes; this can be done in subsequent PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am honestly surprised. This looks pretty cool so far. But I still have to test it on my own and see if I am break it. I will do that tomorrow, it is too late for that by now.
lib/core/macros.nim
Outdated
# `genAst` argument list. The default is unset, to avoid surprising hijacking | ||
# of local symbols by symbols in caller scope. | ||
|
||
macro genAst*(options: static set[GenAstOpt] = {}, args: varargs[untyped]): untyped = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of this options
argument at the beginning. AFAIK the default argument doesn't work here. So I always have to write the curly pair for genAst. Can't we get rid of these flags argument entirely, for example by providing two different names for genAst for the two different behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it in latest commit (doens't appear here since you closed this PR)
This looks very nice indeed. Would you please add a test for a nested application of macro createMacro(name, obj, field: untyped): untyped =
result = genAst({}, obj = newDotExpr(obj, field), lit = newLit(10)):
macro name(arg: untyped): untyped =
result = genAst({}):
echo obj
echo astToStr(field)
echo lit
echo arg
var x = @[1, 2, 3]
createMacro foo, x, len
foo 20 The expected output is:
|
@zah just added your test case, i had to make minor adjustments to make it work haven't addressed other comments yet |
I think I have to take it back. I am not impressed anymore. Your claim to fix #10326 is a lie. That issue is already fixed. I tested it. import macros
macro fails(b: static[bool]): untyped =
echo b
result = newStmtList()
macro works(b: static[int]): untyped =
echo b
result = newStmtList()
macro foo(): untyped =
var b = false
## Fails
result = genAst({},b):
fails(b)
## Works
# result = quote do:
# works(`b`)
foo() output
Your claim to fix #9745 is a lie. That issue is already fixed. I tried if #8220 works with your PR, and surprise I could get it working: import macros, strformat
macro foo(): untyped =
result = genAst({kNoExposeLocalInjects}):
let bar = "Hello, World"
echo &"Let's interpolate {bar} in the string"
foo() But still, the reasoning why this works with Your claim to fix #7589, nope. Could not get it to work. About your claim to fix #7726, I have to give you that, there is a solution for it: import macros
macro foo(): untyped =
let a = @[1, 2, 3, 4, 5]
result = genAst({}, len = a.len):
len
echo foo() # 5 But this solution is not structurally safer than the already exiting solution with quote do: macro foo(): untyped =
let a = @[1, 2, 3, 4, 5]
let len = a.len
result = quote do:
`len` Your solution still causes interal errors in the compiler if you forget to properly escape the arguments: import macros
macro foo(): untyped =
let a = @[1, 2, 3, 4, 5]
result = genAst({}):
a.len # # Error: internal error: expr: var not init a_186029
echo foo() That example also represents more closely the problem from the original bug. So sorry I can't approve your PR here as a fix. A fix would provide here a proper error message that complains about an unknown identifier Now about your own issue: #9607 import macros
proc fun1*(info:LineInfo): string = "bar"
proc fun2*(info:int): string = "bar"
macro echoL*(args: varargs[untyped]): untyped =
let info = args.lineInfoObj
let fun1 = bindSym"fun1"
let fun2 = bindSym"fun2"
result = genAst({}, fun1, info): #
fun1(info)
echo echoL()
I had enough |
@krux02 Why do you close the PR instead of asking for working on the open points? Such a review style is not making the Nim community a big favor. |
@krux02 The way you worded your remarks ("your claim is a lie", "i had enough", etc.) reeks of some sort of personal aggression you have against @timotheecour (I am saying this because this is not the first time I am seeing this from you). I have in no position to validate this PR, but looking at the amount of code and time @timotheecour could have spent on creating this, I don't think your reaction justifies this. Such angry PR closures will hurt/are hurting the Nim community. |
My initial reaction was the same as @bluenote10's and @kaushalmodi's, but after reading the PR I am more sympathetic to @krux02's frustration. From my side I have tried multiple times to understand this Even so, this could have been handled much better, this PR is dense and those checking out Nim are likely to pick out the phrases that @kaushalmodi picked out above and get a negative opinion about Nim's community. In addition, many contributors to Nim are volunteering their time and often work very hard to get something accepted into the language. Someone who is so active in Nim development (and even more importantly, somebody who's employed to work on Nim) would do well to use more positive and encouraging language. Remember that when you speak to a contributor, you are not just speaking to that contributor, you are speaking to anyone who is checking out the community. |
The common etiquette on github is to let PR author reply to review comments before closing his PR. Closing my issues and PR's immediately after leaving a review comment isn't fostering collaboration. This keeps happening, eg my latest PR #11732 also got closed and was later reopend and merged. Back to the PR: I've pushed new commits that hopefully address all comments + concerns. Unfortunately my PR was closed by @krux02 so the new commits don't appear here. With latest commit,
genAst(len=a.len): echo len
result = genAst(b): fails(b)` # works
result = quote do: fails(`b`) # fails CT error which fixes #7375
As for the last 2, #10326 and #9745, I had interpreted it in the more general context of backticks causing ambiguity, see example I had for that in test suite which failed with result = quote do: # will fail
template `a1=`(x: var Foo, val: int) =
x.a = val but indeed, you can consider that issue as fixed prior to this PR as this example is of more general scope than the issue of backticks in pragmas
I've hopefully addressed/clarified this in latest commit, eg: macro fun(a: bool, b: static bool): untyped =
let b2 = newLit b
quote do: echo (`a`, `b2`) can be replaced by: macro fun(a: bool, b: static bool): untyped =
genAst(a, b): echo (a, b) # newLit auto applied when needed |
@timotheecour what is bothering me the most are the false claims, not the technical deficiencies. Remove those false claims and apologize for bringing them up in the first place. Then we can talk about the technical deficiencies. It is the same with your other PR make sets (tables etc) 1000 times faster for pointer, int64, int etc. You just promise everything from the world. But you don't explain the technical deficiencies in the old hash and how you addressed them. Also you didn't bother to mention the shortcomings of your algorithm (string allocations for everything). If you can't prove/show/explain why your way of doing something is or should be better, then please don't claim that it is better. |
I think closing this issue was a bit immature on Krux's side, but more importantly it was also objectively irrational. Our personal feelings should not get in the way of making Nim a better language and it doesn't take much knowledge of the compiler internals to realize that @timotheecour has invented a promising new way to sidestep many of the foundational issues with the existing quasi-quoting approach. Even if the initial implementation was not perfect, the direction is worth pursuing and we should be excited by this new development. We should thank Timothee for being passionate about Nim, for spending so much time on it, and for coming up with all these new ideas. @timotheecour, you should also add a flag controlling the dirtiness of the generated template. The conclusions from previous discussions were that |
that's what the flag
that was what i tried first, but after getting more experience with it, it became clear that non-dirty was a better default to avoid surprising hijacking behavior; dirty is occasionally useful though, see unittests in tests/macros/tgenast.nim I believe I've answered that here #11722 (comment)
you've already asked me that here #11767 (comment) and I've already answered it #11767 (comment) ; happy to provide more clarifications if needed
my claim was backed by benchmarking code I've shared in the original post of that PR; anyone is welcome to verify/criticize/improve it; that benchmark showed the string alloc's cost that you mention were dwarfed by the other costs caused by collisions in the pre-existing hashing scheme. I have pending commits to fix failing tests in that PR which I have not pushed yet. |
Can you clarify this problem with some examples? |
Your false claims are still in your part of your pull request description. They need to be edited out. Then I also asked to apologise, I could not find anything like that. |
# ./mgenast.nim
proc gun(): auto = false
macro mgun*(): untyped =
genAstOpt({kDirtyTemplate}): gun()
# library writer probably intended this to call `mgenast.gun`,
# but that symbol is hijacked by the one in caller scope, eg `mylib.gun`
# ./main.nim
import./mylib # defines `proc gun(): auto = true`
# or just directly defining `gun` locally
import./mgenast
echo mgun() # true
macro bindme6UseExposeFalse*(): untyped =
## with `kDirtyTemplate`, requires passing all referenced symbols which can be tedious
genAstOpt({kDirtyTemplate}, newStringStream, writeData, readData):
var tst = "sometext"
var ss = newStringStream("anothertext")
writeData(ss, tst[0].addr, 2)
discard readData(ss, tst[0].addr, 2) |
Well, both of these are completely expected behavior. You can also use bind statements as a work-around. I think Nim will eventually have to gain support for top-level mixin/bind statements, because otherwise you cannot control the binding behavior inside type sections. They will make it slightly easier to use the dirty version of I guess we need a little bit more experience to decide what is the better default. I would advocate for a short period where the default isn't set, so we can gain more experience with the API without introducing a breaking change in the future. |
I've edited the PR description to reflect the state of this PR as of the latest commit I pushed (which doesn't appear here because this PR is closed), to the best of my understanding. If anything is still inaccurate as of latest commit, I will happily correct it.
Alternatively, I can also remove any mention of fixed issues from this PR and defer to someone else the task of figuring out which issues can be closed. I hope the new PR you've opened #11823 (on which I'm blocked from commenting) after closing my PR will materialize soon so we can finally fix |
I am reopening this PR now. But I am still not happy with it. There is a lot in your wording that bothers me a lot. The issue is still titled For the comment field issue, I am sorry. My branch is indeed on ice. My approach did not work out eventually. I will reopen your PR as it becomes relevant again. And sorry abot being blocked in my PRs. I still did not unblock your personal account. |
5dc9f40
to
c2977d9
Compare
Please add this implementation not directly to |
macros.genAst
: sidesteps issues with quote do
macros.genAst
: sidesteps issues with quote do
macros.genAst
: sidesteps issues with quote do
macros.genAst
: sidesteps issues with quote do
finally merged! (see #17426) |
provides a solution for nim-lang/RFCs#122
a new
macros.genAst
is introduced; it sidesteps virtually all issues inherent withquote do
and can be used as a replacement for itsidesteps these (see unittests tests/macros/tgenast.nim)
quote do
#7375getAstOpt({kDirtyTemplate})
see unittestsquote
)for #7889, see unittests showing this is fixed (for both
quote do
andgenAst
) by using regular call syntax instead of method call syntax; can be therefore closed as dup of #7085comparison to
quoteAst
proposalthis PR doesn't have limitations of the other
quoteAst
proposal mentioned in nim-lang/RFCs#122 (comment) : see unittesttests/macros/tgenast.nim
(macro abc(name: untyped)
test case)it doesn't require any parser change unlike what's mentioned in nim-lang/RFCs#122
it's also much simpler than #10446 (no compiler change needed, just 1 new proc in macros.nim)
so I think this PR also closes nim-lang/RFCs#122 and #10446
note
the only issue with
quote do
that is not fixed by this PR is Comments are removed in quote do expressions #10430Comments are removed in quote do expressions
, however this is in fact a pre-existing problem withgetAst
that can be easily fixed independently of this PR (possibly via an extra argument togetAst
)there are use cases for when
dirty
pragma is desirable, so there is an option to control that:getAstOpt({kDirtyTemplate}, ...)
, see unittests. However, this leads to more verbose code (each symbol used must be in capture list) and potentially surprising hijacking behavior in case some symbols are not captured (see unittests), therefore the default isnot dirty
. See nim docs for what symbols are gensym'd vs inject'd: https://nim-lang.github.io/Nim/manual.html#templates-hygiene-in-templates