add switch, warning, and bind support for new generic injection behavior

[metagn]

refs #23091, especially post merge comments

Unsure if experimental and bind are the perfect constructs to use but they seem to get the job done here. Symbol nodes do not get marked nfOpenSym if the bind statement is used for their symbol, and nfOpenSym nodes do not get replaced by new local symbols if the experimental switch is not enabled in the local context (meaning it also works with push experimental). However this incurs a warning as the fact that the node is marked nfOpenSym means we did not bind it, so we might want to do that or turn on the experimental switch if we didn't intend to bind it.

The experimental switch name is arbitrary and could be changed.

[arnetheduck]

bind seems befitting to the situation - at least to my limited understanding of the keyword, good idea!

the global flag solves the immediate problem but I think this is something that we need to come back to - also because it doesn't offer a way out really - ie when we want to make this feature "official", we will still need a "local" way to mark an individual symbol as ok-to-lookup-again

[metagn]

we will still need a "local" way to mark an individual symbol as ok-to-lookup-again

You mean it shouldn't be the default? It's the expected behavior in non-generic code.

I thought about mixin but that has more to do with overloads and basically never refers to new symbols inside the proc itself, and a pragma/new keyword seems super disruptive for something the language normally does by itself.

Edit: Also as said in the other comments, push experimental works inside generic instantiations, in fact the switch is only enabled in the instantiation context and not necessarily the declaration context, the docs should be clear about this.

[arnetheduck]

You mean it shouldn't be the default? It's the expected behavior in non-generic code.

If this had been the behavior from the start, it should certainly have been default. I'm not sure however how wide the impact is so we're in a bit of a bind.

Edit: Also as said in the other comments, push experimental works inside generic instantiations, in fact the switch is only enabled in the instantiation context and not necessarily the declaration context, the docs should be clear about this.

Yes - though consider the results case: I want to mark the error on the valueOr side so that users of Result are not hit by this quirk, but also so that we don't have to annotate every usage site separately.

Now as I'm writing the above, I realize that if I annotate Result, users of Result will not know that their code changed semantics. Messy.

[arnetheduck]

I'll play around with the flag and see where it leads. Much appreciated.

[metagn]

We could always do something like:

let x = f().valueOr:
  return (error {.inject.})

from a technical perspective, I just wonder which way would lead to the least surprise to the user. The case where the user needs bind (or just refer to the symbol in any other disambiguating way, as you would have to in non-generic code) should be the minority, the normal, expected case should require the least amount of flair. I understand:

let x = f().valueOr:
  {.push experimental: "genericsOpenSym".}
  return error
  {.pop.}

is not great but at least "experimental" implies that those lines are temporary.

I also just realized that any switch we add would also not work on older versions anyway, which would be a problem for most people. I don't know a workaround better than:

const error = ()
let x = f().valueOr:
  return error

Maybe the least we could do is suggest this.

[arnetheduck]

I have this beautiful rewriting hack available for old versions which I'm considering: arnetheduck/nim-results#35 - for that, it would be nice to have something like when defined(nimHasGenericsOpenSym) so I can enable it selectively

Also, I just saw {.inject.} - missed it the first time reading the PR - so the symbol replacement only happens if inject is present on the template side (ie in Result)?

[metagn]

I just saw {.inject.} - missed it the first time reading the PR

Sorry, that was a proposed syntax, I meant it would be easy to implement but obscure/ugly.

so the symbol replacement only happens if inject is present on the template side (ie in Result)?

It's whenever new symbols are introduced in local scope that were not known during the early analysis of the generic proc, which can only really be achieved by templates and macros. inject is just a word I picked in that syntax which would just mean thisCanBeReplaced, there's not much of a relation with injections otherwise.

[github-actions]

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

Hint: mm: orc; opt: speed; options: -d:release
177510 lines; 7.650s; 765.977MiB peakmem

[arnetheduck]

backportable? how far?

[metagn]

Combined with #23091 should be compatible with at least 2.0, probably 1.6 too. #23091 needed some changes to the IC code that I think goes back to 1.6.