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

try fix nested dirty template #23890

Draft
wants to merge 4 commits into
base: devel
Choose a base branch
from
Draft

Conversation

Graveflo
Copy link
Contributor

@Graveflo Graveflo commented Jul 25, 2024

@Graveflo
Copy link
Contributor Author

Passed CI, but of course, this solution reeks of invalid code acceptance. Seeing as how its much more annoying to make failure test cases and that failures should not be expected in the nimble packages the question is: what did I break? I dunno.

If / when this is fixed fixed, judging from the effects of this change, these additional items could be closed: #21376 #5975 #21249 #21376

@Graveflo
Copy link
Contributor Author

Graveflo commented Jul 25, 2024

well this compiles now:

proc p[T](x: T) =
  echo something

var something = 5

p(5)

clearly this change defers bindings for too long. I think it might get weird if the inner template is evaluated earlier and same if the proc's syms are bound before processing the outer template's call site. Maybe the flag in lookup is still the way to go, but only under certain conditions?

maybe that condition is just a check to see if the processing call stack is inside a template or macro, regardless of how nested it is, because in reference to above, this does make sense:

template t() =
  proc p[T](x: T) =
    echo something

var something = 5
t()
p(5)

also, no redefinition error in cases like this:

template inner(i: int) {.dirty.} =
  let thing = 1

template outer() =
  proc p[T](x: T) =
    inner(5)
    var thing = 5
    echo thing

outer()
p(0)

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 this pull request may close these issues.

1 participant