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

assigning indexed NimNode to tuple/object field copies #12457

Open
ghost opened this issue Oct 18, 2019 · 3 comments
Open

assigning indexed NimNode to tuple/object field copies #12457

ghost opened this issue Oct 18, 2019 · 3 comments
Labels

Comments

@ghost
Copy link

ghost commented Oct 18, 2019

Example

import macros

macro notBug(n: untyped): untyped =
  let node = n[0] # skip the nnkStmtList
  node[1] = newLit(2) # replace 0 with 2
  result = n

macro bug(n: untyped): untyped =
  let (node, ) = (n[0], )
  node[1] = newLit(2)
  # Another version using object:
  # type Holder = object
  #   val: NimNode
  # let node = Holder(val: n[0])
  # node.val[1] = newLit(2)
  result = n

var x = notBug: 0 * 2
var y = bug: 0 * 2
echo x
echo y

Current Output

4
0

Expected Output

4
4

Additional Information

Doesn't seem to be a regression.

@krux02 krux02 added the VM see also `const` label label Oct 18, 2019
@timotheecour timotheecour added Macros and removed VM see also `const` label labels May 21, 2020
@timotheecour
Copy link
Member

timotheecour commented May 21, 2020

attempted fix (PR staled and got closed): #13767
(note: github does not reference PR's if only specified in title; PR's because of this github bug, PR's should always reference issues in body even if also referenced in title)

issue still relevant as of 16003bf 1.3.5

@metagn
Copy link
Collaborator

metagn commented Aug 29, 2024

Works in devel and 2.0?

Araq pushed a commit that referenced this issue Aug 30, 2024
closes #1969, closes #7547, closes #7737, closes #11838, closes #12283,
closes #12714, closes #12720, closes #14053, closes #16118, closes
#19670, closes #22645

I was going to wait on these but regression tests even for recent PRs
are turning out to be important in wide reaching PRs like #24010.

The other issues with the working label felt either finnicky (#7385,
#9156, #12732, #15247), excessive to test (#12405, #12424, #17527), or I
just don't know what fixed them/what the issue was (#16128: the PR link
gives a server error by Github, #12457, #12487).
@metagn
Copy link
Collaborator

metagn commented Sep 16, 2024

Tuple version works since it's a tuple unpacking after #21563, doing let node = (n[0], )[0] doesn't work, neither does object version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants