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

Added freshidents to macros #16093

Closed
wants to merge 5 commits into from
Closed

Added freshidents to macros #16093

wants to merge 5 commits into from

Conversation

planetis-m
Copy link
Contributor

There are plenty re-implementations of this all around. It's good to have one in the stdlib.

@disruptek
Copy link
Contributor

Please add a test for genSym artifacts.

@cooldome
Copy link
Member

Looks good to me

## This forces the compiler to perform a new lookup pass.
case n.kind:
of nnkSym:
result = ident(n.repr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there some reasons using repr?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes, it should be $n

Copy link
Contributor Author

@planetis-m planetis-m Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As disruptek said on irc:

a symbol created with genSym needs to be rendered as ident(repr sym) in order to capture the unique name correctly.

So in the test I added ident($n) or similar will output just "tmp" and redefinition error, whereas with repr you get tmp14141 and works properly.

Copy link
Member

@cooldome cooldome Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, with repr you can also get tmp`12312 for gensym which will not compile

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, with repr you can also get tmp`12312 for gensym which will not compile

test passes so it might be fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, tmp`12312 is fine to use as an identifier once past the parser

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there's a more correct method than repr, but I am personally fine with its use here.

@Araq
Copy link
Member

Araq commented Nov 24, 2020

This is a special case of what I called toUntypedAst, we should have that instead. And also, it should be its own module and part of fusion.

@planetis-m
Copy link
Contributor Author

planetis-m commented Nov 24, 2020

Done see link. But collect needs it for a bugfix...

@Araq
Copy link
Member

Araq commented Nov 24, 2020

But collect needs it for a bugfix...

Bummer, why though? collect takes untyped arguments.

@planetis-m
Copy link
Contributor Author

A couple of points:

  1. There is a version of this in sugar called freshIdentNodes, used in a couple of places but unexported.
  2. For collect it fixes bug sugar.collect can't be used in templates #14332 see PR add collect with infered init, refs #16078 fixes #14332 #16089

@timotheecour
Copy link
Member

timotheecour commented Feb 22, 2021

If the end goal API is toUntypedAst and that is too hard to write right now (nim-lang/fusion#49 was closed), then instead of exposing freshIdentNodes in macros, how about add this to a new private std/private/macroutils, so that other modules in stdlib can at least use it in the meantime?

@stale
Copy link

stale bot commented Feb 23, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Feb 23, 2022
@stale stale bot closed this Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants