[Codegen] function call is not emitted

[yglukhov]

Description

# Dummy symbols. Comment any of foo to make the sample work
proc foo1() = discard
proc foo2() = discard
proc foo3() = discard
proc foo4() = discard
proc foo5() = discard
proc foo6() = discard
proc foo7() = discard
proc foo8() = discard
proc foo9() = discard
proc foo10() = discard
proc foo11() = discard
proc foo12() = discard
proc foo13() = discard
proc foo14() = discard
proc foo15() = discard
proc foo16() = discard
proc foo17() = discard
proc foo18() = discard
proc foo19() = discard
proc foo20() = discard
proc foo22() = discard
proc foo23() = discard
proc foo24() = discard
proc foo25() = discard
proc foo26() = discard
proc foo27() = discard
proc foo28() = discard
proc foo29() = discard
proc foo30() = discard
proc foo31() = discard
proc foo32() = discard
proc foo33() = discard
proc foo34() = discard
proc foo35() = discard
proc foo36() = discard
proc foo37() = discard
proc foo38() = discard
proc foo39() = discard
proc foo40() = discard
proc foo41() = discard

var registerCalled = false

proc registerCmd(handler: int) =
  registerCalled = true

proc registerCmd(handler: proc()) =
  registerCalled = true

registerCmd() do():
  discard

doAssert(registerCalled)

Nim Version

Nim Compiler Version 1.9.1 [Linux: amd64] Compiled at 2023-01-17 Copyright (c) 2006-2023 by Andreas Rumpf

git hash: 30da566d9dc81feab9ba3015d293d93b5c6785af active boot switches: -d:release

Current Output

.../test.nim(54) test
.../lib/std/assertions.nim(46) failedAssertImpl
.../lib/std/assertions.nim(36) raiseAssert
.../lib/system/fatal.nim(51) sysFatal
Error: unhandled exception: .../test.nim(54, 9) `registerCalled`  [AssertionDefect]

Expected Output

None

Possible Solution

No response

Additional Information

The generated C sources do not mention registerCmd at all, but they should.

[ringabout]

I haven't found some useful clues, but there are some observations:

• the code works in 1.4.8, 1.2.18
• to reproduce the issue, the number of functions starting with "foo" should be exactly 41
• changing the order of registerCmd declaration makes it work
• the scope contains two registerCmd, but nextIdentIter cannot find the other overload

I suspect there is something wrong with hash, but I had no luck to find the root cause.

[deech]

The fact that the foo* symbols are proc's doesn't seem to make a difference, I can reproduce the issue using any combination of let and var as well:

var p1 = 0
let p2 = 0
let p3 = 0
let p4 = 0
let p5 = 0
let p6 = 0
let p7 = 0
let p8 = 0
let p9 = 0
let p10 = 0
let p11 = 0
let p12 = 0
let p13 = 0
let p14 = 0
let p15 = 0
let p16 = 0
let p17 = 0
let p18 = 0
let p19 = 0
let p20 = 0
let p22 = 0
let p23 = 0
let p24 = 0
let p25 = 0
let p26 = 0
let p27 = 0
let p28 = 0
let p29 = 0
let p30 = 0
let p31 = 0
let p32 = 0
let p33 = 0
let p34 = 0
let p35 = 0
let p36 = 0
let p37 = 0
let p38 = 0
let p39 = 0
let p40 = 0
let p41 = 0
let p42 = 0

proc call(i: int) =
  discard

proc call(p: proc()) =
  p1 = 1

call(proc () = discard)

echo p1

doAssert p1 == 1

[deech]

This also only seems to trigger when passing a proc as an argument, if the second proc call ... is changed to:

proc call (s: string) = 
  p1 = 1

and the invocation is changed to call("hello world") it works.

Wrapping the call(proc () = discard) in an anonymous proc also seems to work: (proc () = call(proc () = discard))() but that probably doesn't mean anything since an anonymous proc introduces a top level symbol.

I also tried unsuccessfully to force call into scope by adding a mixin call before the invocation.

[deech]

Putting the invocation in a block also seems to work:

block:
   call(proc() = discard)

[Araq]

The AST for call(proc () = discard) never reaches the code generator. The bug must be earlier in the compilation pipeline.

[SirOlaf]

Already wrong during sem checking, semExpr spits out nkEmpty after semDirectOp

Funnily enough it does work if you put r = resolveOverloads(c, n, nOrig, filter, flags, errors, efExplain in flags) after https://github.com/nim-lang/Nim/blob/devel/compiler/semcall.nim#L606 (so running it twice)

Going deeper than this, matches in pickBestCandidate finds new symbols and moving it above the counter check also makes it work because then recalc triggers for it

[deech]

A related issue is that nextOverloadIter does not find the proc call(p: proc()) overload in the cache. There maybe some issue with the way identifier chains are getting built up in the TStrTable symbol table.

[SirOlaf]

Took another peek. Issue seems to be caused by mergeShadowScope in some way as it works with closeShadowScope in noMatch, matchesAux

[SirOlaf]

mustRehash makes it work with * 2 for both length and counter, but not knowledgeable enough on this to know if that would cause other issues/move this issue to another magic number.

Logic error on my side, would make it always use second case because of assert, but interesting that that does make it work (edit from future: magic numbers from below apply, just other numbers)

Ok yeah this is interesting, quick python script tells us the other magic numbers where it breaks, make variables up until b-2 (so for b=22 you put p1-p20) and the lambda you pass will break it because regrow is triggered

a = 1
b = 0

for _ in range(2000):
    if (a * 2 < b * 3) or (a - b < 4):
        print(f"Grow at {a=} {b=}")
        a *= 2

    b += 1

[Araq]

Why does it rehash to begin with? Because of mergeShallowScope?

[SirOlaf]

Yeah merging it makes it put the lambda :anonymous into parent scope so it grows and reaches the rehash threshold (magic numbers), seemingly messing with overload resolution, that's why it worked with closeShadowScope instead of mergeShadowScope in match. Also explains why overload order mattered, tries to match and then merges when noMatch is hit, so if it matches on first attempt it would work

Not sure how to fix this, so best I can do is give you what I figured out, but point of all this is that rehash breaks the iterator.

Idea of moving the call to match up by one line might work though, because then recalc triggers like expected instead of sym being nil prematurely (or just don't merge scopes)

[Araq]

Not sure how to fix this, so best I can do is give you what I figured out, but point of all this is that rehash breaks the iterator.

Iterate and put it into a seq in a first step so that we know it's not mutated behind our backs.