fix #13443 Underscore cannot be defined more than once in proc

[ringabout]

fix #13443

future work

• make it work for js+vm (see disabled test)
• add test for:

template main =
  proc foo(_: int) =
    let a = _
doAssert not compiles(foo(1))

• this doesn't work inside templates:

import std/sequtils

proc main() =
  iterator fn(_, _: int, c: int): int = yield c
  doAssert toSeq(fn(1,2,3)) == @[3]
static: main() # ok
main() # ok

template main2() =
  iterator fn(_, _: int, c: int): int = yield c
main2() # bug: Error: attempt to redefine: '_`gensym3'

ditto with procs:

when true:
  template main() =
    proc fn(_, _, a: int): auto =
      a
    echo fn(1,2,3) # Error: attempt to redefine: '_`gensym0'
  main()

• this works but shouldn't:

when defined case5:
  template main() =
    proc fn(_, a: int): auto =
      echo _
      a
    echo fn(1,2)
  main()

• this is a (desirable) breaking change but should be documented in changelog:

when defined case3:
  proc fn(_: int, a: int): auto =
    echo _
    a
  echo fn(1,2)

before PR: works
after PR: Error: undeclared identifier: '_'

• see whether we should add __attribute__((unused)) in cgen for the corresponding parameters

[ringabout]

It doesn't work with templates.
Hi @timotheecour do you think I should support it? Or some suggestions regarding it?

template main() =
  var x = 0
  proc foo(_, _: int) = x += 5

  foo(1, 2)
  doAssert x == 5

[timotheecour]

do you think I should support it?

yes i think that snippet should compile. can be done in subsequent PR though

[timotheecour]

i still don't like the duplication, instead just rename this to:

proc handleUnderscore(v: PSym): bool = ...

[timotheecour]

can there be something similar for js? or what prevents this?

[ringabout]

It just didn't work for JS VM I guess a known bug

[ringabout]

tunderscore.nim(9, 27) Error: cannot evaluate at compile time: x

[timotheecour]

in this case we should leave #13443 open (with a note) until we can address this :)

[ringabout]

why, that's the bug of js vm, not related to my PR?

[timotheecour]

fine, I'll just open a new bug for js+vm then

[Clyybber]

This shouldn't be handled in the backend IMO.

[timotheecour]

maybe, what do you suggest concretely?
(see also the items i added in future work)

[metagn]

Can't we just mangle if sfGensym in s.flags?

Suggested change

	if s.name.s == "_":
	if sfGensym in s.flags:

[timotheecour]

Suggested change

	block:
	iterator fn(_, _: int, c: int): int = yield c
	doAssert toSeq(fn(1,2,3)) == @[3]

and add from std/sequtils import toSeq on top

let's think a bit more

[metagn]

This PR is needed to fix #13804 (which the checkboxes in the main post describe). There would only be a small patch in semtempl, namely putting these lines inside if param.name.s != "_":

Nim/compiler/semtempl.nim

Lines 645 to 646 in 779b1cc

	param.flags.incl sfTemplateParam
	param.flags.excl sfGenSym

If we don't want the backend mangling parameter names, we can whitelist the changes in semtypes to just templates.

As for the isDiscardUnderscore comments, we can just forward declare it in semtypes/move it there, or we can convert it to template ignoreUnderscore(v: PSym, body: untyped) which does if v.name.s == "_": v.flags.incl(sfGensym) else: body.