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

transform static: stmt and const foo = expr into immediately invoked lambas => fixes many bugs #276

Closed
timotheecour opened this issue Oct 23, 2020 · 7 comments · Fixed by nim-lang/Nim#21351 · May be fixed by timotheecour/Nim#795
Assignees

Comments

@timotheecour
Copy link
Member

timotheecour commented Oct 23, 2020

This would solve a number of nasty scoping issues involving static and const.
the intuitiion is that block variable scoping at CT is buggy (and difficult), so we can instead reuse proc scoping via immediately invoked lambas, which offer the same semantics.
This would be done by a simple compiler transform.

example 1: would fix nim-lang/Nim#12172 (+ dups nim-lang/Nim#13795 nim-lang/Nim#14192)

this is a common bug hidden under many disguises eg see duplicates involving templates, toSeq etc

proc test =
  const TEST = block:
    let i = 0 # Error here too
    i

=> works:

proc test =
  const TEST = (proc(): auto =
    let i = 0
    i)()

example 2: would fix nim-lang/Nim#10938

static:
  for i in '1' .. '2':
    var s: set[char]
    doAssert s == {}
    incl(s, i)

=> works:

  static:
    (proc() =
      for i in '1' .. '2':
        var s: set[char]
        doAssert s == {}
        incl(s, i))()

example 3: would fix nim-lang/Nim#13918

const test = block:
  for i in 1 .. 5:
    var arr: array[3, int]
    var val: int
    echo arr, " ", val
    for j in 0 ..< len(arr):
      arr[j] = i
      val = i
  0

=> works:

const test = (proc(): auto =
  for i in 1 .. 5:
    var arr: array[3, int]
    var val: int
    echo arr, " ", val
    for j in 0 ..< len(arr):
      arr[j] = i
      val = i
  0)()

example 4: would fix nim-lang/Nim#13312

proc bar =
  for _ in 0 ..< 3:
    var s: string
    s.add("foo")
    echo s

static:
  for _ in 0 ..< 3:
    var s: string
    s.add("foo")
    echo s

  bar()

=> works:

proc bar =
  for _ in 0 ..< 3:
    var s: string
    s.add("foo")
    echo s

static: (proc() =
  for _ in 0 ..< 3:
    var s: string
    s.add("foo")
    echo s
  bar())()

EDIT: example 5: would fix nim-lang/Nim#13887

Example 1 from nim-lang/Nim#13887 adapted to use immediately invoked lambas

when true:
  static:
    (proc()=
      var x = 5
      var y = addr(x)
      y[] += 10
      doAssert x == y[])()

ditto for nim-lang/Nim#13887 (comment)

when true:
  template fun() =
    var s = @[10,11,12]
    let z = s[0].addr
    doAssert z[] == 10
    z[] = 100
    doAssert z[] == 100
    doAssert s[0] == 100
  static: (proc()=fun())()
  fun()

ditto with example 3 in that issue:

when true:
  template fun() =
    var s = @[10.1,11.2,12.3]
    echo cast[int](s[0].addr)
    let z = s[0].addr
    echo cast[int](z)
  static: (proc=fun())()

ditto with example 2 in that issue:

when true:
  template fun() =
    var s = @[10,11,12]
    echo cast[int](s[0].addr)
    let z = s[0].addr 
    echo cast[int](z)
  static: (proc=fun())()

note

I had originally proposed here: nim-lang/Nim#10938 (comment) but it was burried inside a comment, and I'm now seeing that it solves many other long standing bugs

@Araq
Copy link
Member

Araq commented Oct 24, 2020

It's good that it "would fix many bugs" but I don't see how in principle that makes the language simpler. Lambdas have a complex setup for capturing surrounding variables and that complexity is intristic whereas the bugs might just be implementation artifacts which we can fix one after another.

@timotheecour
Copy link
Member Author

It could be that capture is exactly what's needed here to avoid existing scoping bugs, and that capturing is much less buggy in the language than const/static blocks, so in the meantime we can avoid all those issues which bite us a lot in various forms. Would a tentative PR be accepted (not sure if I have time now but anyone else could too) ?

@Araq
Copy link
Member

Araq commented Oct 24, 2020

A PR would be interesting as it would tell us which different problems this solution produces. ;-)

@cooldome
Copy link
Member

cooldome commented Nov 10, 2020

I had a look. All issues above can be fixed today by removing the check in vmgen.nim

  if s.kind in {skVar, skTemp, skLet, skParam, skResult} and
      not s.isOwnedBy(c.prc.sym) and s.owner != c.module and c.mode != emRepl:
    cannotEval(c, n)

However, it would allow some invalid code to passthrough. Examples of accept invalid in this case:
tests/vmtwrongarray.nim and tests/vm/twrongwhen.nim

In my opinion the correct fix would be to reject invalid code in the sempass and remove check in vmgen.
Change sempass symbol lookup rules such that in static blocks only the compileTime variables are allowed. Basically compileTime variables will have its scopes marked as compileTime only. Runtime can lookup compileTime but not the vice versa.

This example should not pass the sempass:

proc test =
  var i = 0
  const TEST = block:
    let j = 2
    i + j # rejected in sempass not in vm codegen

@timotheecour
Copy link
Member Author

timotheecour commented Nov 13, 2020

All issues above can be fixed today by removing the check in vmgen.nim

@cooldome your suggestion would only fix example 1 above, and would not help with example 2, 3, 4, 5 (i just checked)

Whereas the immediately invoked lambda would fix all examples 1,2,3,4,5 and possibly many other issues with static/const

(note: I've just identified yet another issue that'd be fixed by this trick: nim-lang/Nim#13887)

@ringabout
Copy link
Member

see also nim-lang/Nim#21351

@metagn
Copy link
Contributor

metagn commented Sep 8, 2024

This example should not pass the sempass:

Neither should this though, the variable still needs to track where it was defined:

proc test =
  const TEST = block:
    let i = 1
    const j = i + 1 # error
    j

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment