allow replacing captured syms in macro calls in generics

[metagn]

fixes #22605, separated from #22744

This marks symbol captures in macro calls in generic contexts as nfOpenSym, which means if there is a new symbol in the local instantiatied body during instantiation time, this symbol replaces the captured symbol. We have to be careful not to consider symbols outside of the instantiation body during instantiation, because this will leak symbols from the instantiation context scope rather than the original declaration scope. This is done by checking if the local context owner (maybe should be the symbol of the proc currently getting instantiated instead? not sure how to get this) is the same as or a parent owner of the owner of the replacement candidate symbol.

This solution is distinct from the symchoice mechanisms which we originally assumed had to be related, if this assumption was wrong it would explain why this solution took so long to arrive at.

[github-actions]

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 9416595

Hint: mm: orc; opt: speed; options: -d:release
177491 lines; 7.837s; 765.918MiB peakmem

[arnetheduck]

Hm, something I discussed with @Araq is that this fix changes semantics and can (and I think will) silently change runtime behavior, if I understand it right - wdyt of two prgams - one for the old behavior, one for the new and a compiler warning if neither is specified on the affected symbol?

[metagn]

It can change runtime behavior, which the test even checks for, but it would take a long time to diagnose every codebase as it's pretty niche (hopefully this also means that it's rare). We could add a --legacy switch to turn it off (--experimental could obscure it and make things worse in the long run).

[arnetheduck]

switch to turn it off

switches like this are unfortunately not helpful because one library might be ready for the new behavior and the other might not - ie they work for small toy projects but not in a world of reusable code where libraries necessarily have to be individually upgradeable or high coordination costs ensue.

a typical case where this shows up is logging, where instead of logging the error, the string "error" is logged because randomly some module had an error enum symbol imported - ie it's a runtime error, not a compile-time error, which makes it hard to detect.

but it would take a long time to diagnose

ok - that's unexpected - As in .. it seems like one could speculatively look up the symbol and if there's a match, show a warning instead of replacing?

it's pretty niche

we hit it on a monthly basis now and it's equally hard to find every time - this is a natural outcome of having more nim code around (due to the nim design with a single global symbol namespace, popular names result in conflicts, think birthday paradox).

[Araq]

It needs to be opt-in as it's a breaking change and also it always needs to warn about this case.

[metagn]

I'm out of time for now, if we need an urgent solution we can make it an experimental switch required on this line (i.e. and openSym in c.features):

Nim/compiler/semexprs.nim

Line 3070 in 8614f35

if nfOpenSym in n.flags:

which would make push experimental work locally inside of the generic proc bodies, i.e.

proc foo[T]() =
  {.push experimental: "opensym".}
  let x = bar.valueOr: error
  {.pop.}

but also might leak through generic instantiations.

[metagn]

wdyt of two prgams - one for the old behavior

We should be able to reuse bind for this, i.e. bind error, and the case where we don't bind and the experimental feature isn't turned on could create the warning.

[metagn]

Hopefully this will alleviate issues: #23102