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

Regression from 1.6 to 2.0/devel with templates with gensym proc #23813

Closed
tersec opened this issue Jul 9, 2024 · 2 comments · Fixed by #23842
Closed

Regression from 1.6 to 2.0/devel with templates with gensym proc #23813

tersec opened this issue Jul 9, 2024 · 2 comments · Fixed by #23842

Comments

@tersec
Copy link
Contributor

tersec commented Jul 9, 2024

Description

template r(body: untyped) =
  proc x() {.gensym.} =
    body

template g() =
  r:
    let y = 0

  r:
    proc y() = discard
    y()

g()

Nim Version

Builds in Nim 1.6:

Nim Compiler Version 1.6.21 [Linux: amd64]
Compiled at 2024-07-09
Copyright (c) 2006-2023 by Andreas Rumpf

git hash: 786bbff1ae0c1f124fc0848948c0fd7dfd30225c
active boot switches: -d:release

Does not build in 2.0 and devel:

Nim Compiler Version 2.0.8 [Linux: amd64]
Compiled at 2024-07-08
Copyright (c) 2006-2023 by Andreas Rumpf

git hash: 5935c3bfa9fec6505394867b23510eb5cbab3dbf
active boot switches: -d:release
Nim Compiler Version 2.1.1 [Linux: amd64]
Compiled at 2024-07-09
Copyright (c) 2006-2024 by Andreas Rumpf

git hash: 732f7752a954bb33a92e839304006ba5d865400a
active boot switches: -d:release

Current Output

/tmp/m.nim(7, 9) Hint: 'y`gensym0' is declared but not used [XDeclaredButNotUsed]
/tmp/m.nim(13, 2) template/generic instantiation of `g` from here
/tmp/m.nim(11, 5) Error: undeclared identifier: 'y`gensym0'; if declared in a template, this identifier may be inconsistently marked inject or gensym
candidates (edit distance, scope distance); see '--spellSuggest': 
 (2, 2): 'x`gensym1'
 (2, 2): 'x`gensym2'

Expected Output

Builds

Possible Solution

No response

Additional Information

No response

@metagn
Copy link
Collaborator

metagn commented Jul 9, 2024

This is #23392 hence the special error message. The let y marks y as gensym but proc y injects it, previously the previous y being gensym'd also made the proc y gensym for some reason. But only the previous gensym'd y is in the template scope, injected procs don't track their identifiers as injected internally (not added to toInject) so it just uses the let y instead in y(). So a workaround is marking let y as inject or proc y as gensym (the old behavior) or making the template dirty.

I had a PR that added injected procs to toInject but it ended up not going through, I don't remember what it broke. Edit: It was #20003 but then I tried adding to toMixin instead which passed CI I believe, but I don't know if that would be too weird or breaks stuff that isn't in CI.

@metagn
Copy link
Collaborator

metagn commented Jul 15, 2024

Sorry, toMixin doesn't fix this issue since a gensym'd y still exists, so I don't think there's an easy fix. The workaround of adding {.gensym.} to the proc y mirrors exactly the previous behavior but I understand it might look ugly, maybe we can add a pragma to templates that changes the default for routine symbols to gensym and not inject since it's completely a frontend issue.

I'm going to open a PR that implements the idea mentioned here where the old behavior is kept unless the proc has an explicit inject. It might be nice to mention this in the documentation though.

metagn added a commit to metagn/Nim that referenced this issue Jul 15, 2024
Araq pushed a commit that referenced this issue Jul 16, 2024
…23842)

fixes #23813, partially reverts #23392

Before #23392, if a `gensym` symbol was defined before a proc with the
same name in a template even with an `inject` annotation, the proc would
be `gensym`. After #23392 the proc was instead changed to be `inject` as
long as no `gensym` annotation was given. Now, to keep compatibility
with the old behavior, the behavior is changed back to infer the proc as
`gensym` when no `inject` annotation is given, however an explicit
`inject` annotation will still inject the proc. This is also documented
in the manual as the old behavior was undocumented and the new behavior
is slightly different.
narimiran pushed a commit that referenced this issue Jul 16, 2024
…23842)

fixes #23813, partially reverts #23392

Before #23392, if a `gensym` symbol was defined before a proc with the
same name in a template even with an `inject` annotation, the proc would
be `gensym`. After #23392 the proc was instead changed to be `inject` as
long as no `gensym` annotation was given. Now, to keep compatibility
with the old behavior, the behavior is changed back to infer the proc as
`gensym` when no `inject` annotation is given, however an explicit
`inject` annotation will still inject the proc. This is also documented
in the manual as the old behavior was undocumented and the new behavior
is slightly different.

(cherry picked from commit cd94608)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants