nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.64k stars 1.47k forks source link

Illegal symbol reuse in template with method call syntax #14844

Open Clyybber opened 4 years ago

Clyybber commented 4 years ago

This snippet fails:

template makeSeq: untyped =
  var i = 0
  for _ in 0..<10: # Already fails with 0..<6 which is exactly 10 / 2 + 1
    assert i in 0..<10 # This assertion fails
    inc i
  @[1] # Works with [1]

template last(s): untyped = s[s.len - 1] # Works with s[^1]

echo makeSeq().last() # This doesn't work
#echo last(makeSeq()) # This works

Current Output

test.nim(4)          test
lib/system/assertions.nim(30) failedAssertImpl
lib/system/assertions.nim(23) raiseAssert
lib/system/fatal.nim(49) sysFatal
Error: unhandled exception: test.nim(4, 12) `i`gensym4607016 in 0 ..< 10`  [AssertionDefect]
Error: execution of an external program failed: 'test '

Expected Output

1

I haven't invesitgated yet (only minimized), but the fact that it fails when the for loop range is bigger than half of the assertion range leads me to believe that the for loop is running "too far". One explanation that comes to mind is that the for loop is getting evaluated twice because of the last template, but when used with UFCS the second for loop somehow ends up using the first for loop's i, instead of its own. EDIT: Confirmed by replacing the assert with echo i.

Additional information:

$ nim -v
Nim Compiler Version #devel
https://github.com/nim-lang/Nim/commit/62394616e848e7a94bc0208d6535df6f582e74ce

This bug is the actual rootcause of #14404

Clyybber commented 4 years ago

Works in 1.0.6 (thanks @Yardanico)

ghost commented 4 years ago

git bisect gave 0e7338d65c1bd6c4e2211fa531982a4c0a0e478c as the first bad commit. EDIT: Works in the JS backend. Extended test case:

template makeSeq: untyped =
  var i = 0
  echo "starting loop"
  for _ in 0..<10: # Already fails with 0..<6 which is exactly 10 / 2 + 1
    echo "i is ", i
    assert i in 0..<10 # This assertion fails
    inc i
  @[1] # Works with [1]

template last(s): untyped = s[s.len - 1] # Works with s[^1]

echo "res - ", makeSeq().last() # This doesn't work
#echo last(makeSeq()) # This works
Clyybber commented 4 years ago

When putting the template invocation (with the method call syntax) in a proc and calling it we get C code errors, because it tries to define the same i twice.

timotheecour commented 4 years ago

this has nothing to do with for loops, title should be changed:

further reduced, and in a way that simplifies cgen'd code:

when defined case2:
  proc c_printf(frmt: cstring): cint {.importc: "printf", header: "<stdio.h>", varargs, discardable.}
  template fun: untyped =
    var i = 0.cint
    c_printf("i: %d\n", i)
    i = 12
    2
  template last(s): untyped = [s, s]
  discard fun().last() # This doesn't work
  # discard last(fun()) # This works

When putting the template invocation (with the method call syntax) in a proc and calling it we get C code errors, because it tries to define the same i twice.

indeed, producing this code:

N_LIB_PRIVATE N_NIMCALL(void, main__UbrNhOtQ6cu24BDBepHJ7w)(void) {
    tyArray__HU7qaqKu9czJLT84iCBJnsA T1_;
    int iX60gensym16202814_;
    int T2_;
    int iX60gensym16202814_;

top-level vs proc scope

another point is that the behavior depends on whether this is at top-level or not:

when defined case5:
  proc c_printf(frmt: cstring): cint {.importc: "printf", header: "<stdio.h>", varargs, discardable.}
  template fun: untyped =
    block:
      var i = 0.cint
      c_printf("i: %d\n", i)
      i = 12
      2
  template last(s): untyped = [s, s]

  discard fun().last() # bug
  when false: # this would work
    proc main=
      discard fun().last() # ok
      # discard last(fun()) # ok
    main()
Araq commented 4 years ago

Fixing this properly looks like a big refactoring which should not block 1.4.