nim-lang / RFCs

A repository for your Nim proposals.
136 stars 26 forks source link

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

Closed timotheecour closed 1 year ago

timotheecour commented 3 years ago

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 https://github.com/nim-lang/Nim/issues/12172 (+ dups https://github.com/nim-lang/Nim/issues/13795 https://github.com/nim-lang/Nim/issues/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 https://github.com/nim-lang/Nim/issues/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 https://github.com/nim-lang/Nim/issues/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 https://github.com/nim-lang/Nim/issues/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 https://github.com/nim-lang/Nim/issues/13887

Example 1 from https://github.com/nim-lang/Nim/issues/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 https://github.com/nim-lang/Nim/issues/13887#issuecomment-655829572

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: https://github.com/nim-lang/Nim/issues/10938#issuecomment-508269563 but it was burried inside a comment, and I'm now seeing that it solves many other long standing bugs

Araq commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

cooldome commented 3 years ago

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 commented 3 years ago

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: https://github.com/nim-lang/Nim/issues/13887)

ringabout commented 1 year ago

see also https://github.com/nim-lang/Nim/pull/21351