sequtils.toSeq produces the sequence from the iterator twice if compiles(iter.len) == true

[skilchen]

In the doc comment for sequtils.toSeq we find this example:

let
  numeric = @[1, 2, 3, 4, 5, 6, 7, 8, 9]
  odd_numbers = toSeq(filter(numeric) do (x: int) -> bool:
         if x mod 2 == 1:
           result = true)

If you run this, the filtered sequence is produced twice, as can be seen here on the nim playground with my instrumented copies of filter and toSeq. I think its the same problem that was fixed eg. in mapIt by copying the iterable to a local variable and therefore forcing the iteration to take place.

My proposed fix looks like this:

template toSeqFixed*(iter: untyped): untyped =
  when compiles(iter.len):
    iter
  else:
    var result: seq[type(iter)] = @[]
    for x in iter:
      result.add(x)
    result

but, as usual, i am not sure if this fix is correct...

[data-man]

@skilchen
It's actual for the latest Nim?
Because of this I can't repeat:

If you run this, the filtered sequence is produced twice

Or I haven't figured out what the problem is, sorry.

[skilchen]

@data-man yes it is still actual for the latest Nim.
compiles(iter.len) is only true if there are two implementations of iter: one as an iterator (here iter.len isn't compileable) and the other as procedure (here iter.len compiles). This is the case for filter but also for e.g. strutils.split.

Maybe it becomes clearer what happens, if you compile the following example and then inspect the generated C code:

import sequtils

proc test() =
  var a = "a,b,c"

  let s = toSeq(filter(a) do (x: char) -> bool: x != ',')
  echo s
  
test()

In the generated C code in nmcache you will then find an implementation of filter and test procedures both containing the same innermost loop.

Maybe a less exotic fix to this issue would be something like this:

template toSeq*(iter: untyped): untyped =
  when compiles(iter.len):
    var i = 0
    let t = iter
    var result = newSeq[type(iter)](t.len)
    for x in t:
      result[i] = x
      inc i
    result
  else:
    var result: seq[type(iter)] = @[]
    for x in iter:
      result.add(x)
    result

Important is the line let t = iter and then iterating over this t instead of iter. With this modified toSeq in the generated C code you will now see, that the inner loops in filter and test are no longer the same. test loops over the results of filter and doesn't inline the filter-iterator-loop a second time.
Unfortunately i don't master the jargon of metaprogramming, so i can not say what exactly happens in the line let t = iter, maybe something like: converting the untyped argument of the template to a concrete type...

[timotheecour]

sent out fix here: #8586