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

fix: var a{.foo.} = expr inside templates (refs #15920) (except when foo is overloaded) #13869

Merged
merged 10 commits into from
Aug 11, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 4, 2020

fixes partially #15920

(WAS: timotheecour#89)

template foo(lhs, typ, expr) =
  let lhs = expr
proc fun1()=
  let a {.foo.} = 1
template fun2()=
  let a {.foo.} = 1
fun1() # ok
fun2() # WAS `Error: invalid pragma: foo` here

future work

n.kind == nkConstSection and w in constPragmas:
return nil
sym = searchInScopes(c, ident)
# CHECKME: should that test also apply to `nkSym` case?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think so.

Copy link
Member Author

Choose a reason for hiding this comment

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

left for future work, I want to get this fix in first (keeping this comment as unresolved)

@Araq
Copy link
Member

Araq commented Apr 5, 2020

Don't forget the [backport:1.2] tag.

@Araq Araq closed this Apr 19, 2020
@Araq Araq reopened this Apr 19, 2020
@timotheecour timotheecour changed the title fix: var a{.foo.} = expr inside templates [WIP] fix: var a{.foo.} = expr inside templates Apr 19, 2020
@timotheecour
Copy link
Member Author

timotheecour commented Apr 19, 2020

@Araq please don't merge this yet, there's 1 more important thing missing that I fixed but I haven't pushed yet; if urgent, i'll do that tmrw
(related to https://github.com/nim-lang/Nim/pull/13869/files/522bbade6eee4f94647bce817edf99dc7d824590#diff-750ffc8fb636a0b3ddd159b478017510 + another unrelated thing)

@Araq
Copy link
Member

Araq commented Apr 19, 2020

Well it's not "urgent" but regression fixes should have some priority. :-)

@timotheecour
Copy link
Member Author

timotheecour commented Nov 13, 2020

@Araq PTAL, I've improved test coverage, and added disabled tests for cases that don't work yet (see "future work" in top post, plus tests in this PR)

Well it's not "urgent" but regression fixes should have some priority. :-)

but it's not a regression, this never worked before

@timotheecour timotheecour changed the title [WIP] fix: var a{.foo.} = expr inside templates fix: var a{.foo.} = expr inside templates (refs #15920) (except when foo is overloaded) Nov 13, 2020
@timotheecour
Copy link
Member Author

timotheecour commented Nov 14, 2020

I've reduced the single failure in important_packages (docopt) down to this:

when true:
  import macros # defines `proc genSym` symbol
  template fn() =
    var ret {.gensym.}: int
    discard ret
  fn()

=> needs fixing before this can be merged

EDIT: now fixed

@timotheecour timotheecour marked this pull request as draft November 14, 2020 00:38
@timotheecour timotheecour force-pushed the pr_fix_vardecls_pragma branch 3 times, most recently from 934850b to 757402e Compare August 2, 2021 05:22
@timotheecour timotheecour marked this pull request as ready for review August 2, 2021 23:25
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Aug 2, 2021
@timotheecour
Copy link
Member Author

@Araq PTAL, I've revisited this PR and fixed the blocker

@Araq Araq merged commit 6c1bd4b into nim-lang:devel Aug 11, 2021
var found = false
if ni.kind == nkIdent:
for a in templatePragmas:
if ni.ident == getIdent(c.c.cache, $a):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a hack IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

see test case labeled D20210801T100514

@timotheecour timotheecour deleted the pr_fix_vardecls_pragma branch August 11, 2021 10:39
@timotheecour timotheecour added TODO: followup needed remove tag once fixed or tracked elsewhere and removed Ready For Review (please take another look): ready for next review round labels Aug 11, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
…cept when `foo` is overloaded) (nim-lang#13869)

* fix: `var a{.foo.} = expr` inside templates
* add test
* improve tdecls test
* improve tests
* add failing test
* PRTEMP
* fixup
@metagn metagn removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jan 4, 2024
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.

4 participants