Parameter not matched in template following symchoice changes

[arnetheduck]

Description

type Result[T, E] = object

func value*[T, E](self: Result[T, E]): T {.inline.} =
  discard

func value*[T: not void, E](self: var Result[T, E]): var T {.inline.} =
  discard

template unrecognizedFieldWarning =
  echo value

proc readValue*(value: var int) =
  unrecognizedFieldWarning()

Regression likely caused by #23892

Nim Version

Nim Compiler Version 2.0.9 [Linux: amd64] Compiled at 2024-08-22 Copyright (c) 2006-2023 by Andreas Rumpf

git hash: d6f7625626e4e19b9dd57147d2c8527a40fd4a03 active boot switches: -d:release

(version-2-0 branch)

Current Output

testit.nim(13, 27) Error: type mismatch
Expression: echo value
  [1] value: proc (self: Result[value.T, value.E]): T{.inline, noSideEffect.} | proc (self: var Result[value.T, value.E]): var T: not void{.inline, noSideEffect.}

Expected one of (first mismatch at [position]):
[1] proc echo(x: varargs[typed, `$`])

Expected Output

No response

Possible Solution

No response

Additional Information

No response

[metagn]

It seems like this never worked? Asking just to make sure what triggers it now

Workaround is

template unrecognizedFieldWarning =
  block:
    let value {.inject, used.} = 0
  echo value

We can easily fix it if readValue was a generic proc but it's not. To fix it in general we basically need opensym for templates, or make this workaround official into something like {.inject: value.}. Is there a chance to at least bisect which commit causes the original failure?

[arnetheduck]

It seems like this never worked? Asking just to make sure what triggers it now

hm, the plot thickens - looks like I reduced this one step too far:

import std/[typetraits]

type Result[T, E] = object

func value*[T, E](self: Result[T, E]): T {.inline.} =
  discard

func value*[T: not void, E](self: var Result[T, E]): var T {.inline.} =
  discard

template unrecognizedFieldWarning =
  echo typetraits.name(typeof value)

proc readValue*(value: var int) =
  unrecognizedFieldWarning()

this is closer to the original code and compiles

[metagn]

Bisect gives #22716

I think it always silently errored, typeof just allowed it to be ambiguous before. If you try to run the code on the versions that work typeof value echoes None.

So I would suggest using a workaround regardless or just removing the typeof value call, another option is to use a macro that turns value into an nkIdent. Even adding {.inject: value.} would need a new version, unless we backport it immediately.

Like I said to fix the general issue we have to implement opensym for templates which I think should be done by just merging nkOpenSym with nkOpenSymChoice.

[arnetheduck]

+1, just bisected the full code to the same PR (make NIM_COMMIT=8a4ee2b84f LOG_LEVEL=TRACE nimbus_beacon_node in the nimbus-eth2 clone)

and yeah, workaround works, but I can't say it makes sense at all, as a user at least ;)

[arnetheduck]

original code, for reference: https://github.com/status-im/nimbus-eth2/blob/77c36b3c59fcc02ff28d61d36897e0392c305d98/beacon_chain/spec/eth2_apis/eth2_rest_serialization.nim#L1404

the intent was to print the type of the value passed in to the serializer like so: https://github.com/status-im/nimbus-eth2/blame/77c36b3c59fcc02ff28d61d36897e0392c305d98/beacon_chain/spec/eth2_apis/eth2_rest_serialization.nim#L2285

I guess it's entirely possible that nobody ever looked at what was actually printed since it's a trace log meant to catch unanticipated changes in the json, meaning that we just assumed it would work since it compiled.

reading my reproduction, I would also broadly expect it to work because the following works:

template unrecognizedFieldWarning =
  echo value

proc readValue*(value: var int) =
  unrecognizedFieldWarning()

var x = 100
readValue(x)

importing a module doesn't feel like it should break this.

[metagn]

and yeah, workaround works, but I can't say it makes sense at all, as a user at least ;)

It is 100% a hack, a more "proper" method would be to use a macro on the template itself that replaces any sym/symchoice named value with an ident node: this works but way simpler workaround in other comment below

```nim import macros macro removeCaptures(name: static string, body) = var body = body while body.kind != nnkStmtList: body = body[^1] proc iter(name: string, n: NimNode): NimNode = result = n case n.kind of nnkSym, nnkOpenSymChoice, nnkClosedSymChoice: if $n == name: result = ident(name) else: result = n for i in 0 ..< n.len: result[i] = iter(name, result[i]) result = iter(name, body) echo result.treeRepr removeCaptures("value"): template unrecognizedFieldWarning = trace "JSON field not recognized by the current version of Nimbus. Consider upgrading", fieldName, typeName = typetraits.name(typeof value) ```

but maybe this is overkill. The best case is definitely the language providing this mechanism but like I said then there's a time delay until you can use it.

[metagn]

Never mind, there's a way simpler workaround lol

import macros

macro useIdent(name: static string): untyped =
  result = ident(name)

template unrecognizedFieldWarning =
  trace "JSON field not recognized by the current version of Nimbus. Consider upgrading",
        fieldName, typeName = typetraits.name(typeof useIdent("value"))

Sorry for spamming

[arnetheduck]

ok - that's actually .. viable to use in a crisis and good to know, as a mechanism to delay the binding - unfortunately, it doesn't quite scale - this is part of a larger story arc and something we have a lot of problems with in nim as the codebase grows, namely that a change in an unrelated module can have devastating effects on your own code - a library we're already using adds some symbol with a common name (like value in this case) and all hell breaks loose - we'd have to defensively use your hack everywhere which of course is not viable.

Thanks for looking into it.

[metagn]

Just to be clear, I wasn't suggesting it as a permanent solution, the language design definitely needs to handle this.

a change in an unrelated module can have devastating effects on your own code

I can't speak for this in every single case, for example stuff like needing to import $ for generic procs, but I can see templates being especially prone to this, with #11184 making it less obvious. For large codebases maybe we can try to come up with ways to be explicit/more restrictive about symbol captures, to sacrifice a bit of ergonomics for correctness and clarity, like maybe a way to declare here "this template uses value from the local scope of the callsite, don't capture anything for it" (bug aside), or in other cases templates that bind everything instead of opening every symbol by default. In general the language shouldn't promote open, catch-all, dynamic constructs for widespread use. Maybe I don't know what I'm talking about idk

[Araq]

Maybe I don't know what I'm talking about idk

The problem is usually in the opposite direction, Nim doesn't do enough of "magic": https://github.com/nim-lang/RFCs/issues/380

[arnetheduck]

The problem is usually in the opposite direction, Nim doesn't do enough of "magic": https://github.com/nim-lang/RFCs/issues/380

this rfc is not applicable here though - this issue is about maintaining consistent scoping rules essentially so that the scope in which the template is expanded has precedence - it's not a generic sandwich problem (there is no type to attach to)

[metagn]

I didn't think of this before but we can also make typeof accept the None type for now so the code still compiles if that'd help the situation. I was going to PR this but maybe exposing the silent failures is better?

[arnetheduck]

I was going to PR this but maybe exposing the silent failures is better?

I think it might be better to expose the problem indeed - we changed the code to be more explicit.

There will be cases however where a valid but unexpected symbol is chosen and it won't be seen until runtime - value happened to be ambiguous in this case causing an overload resolution failure but what if it had been an .. enum?

As a first step, just like with genericsOpenSym it would be useful to have a warning that this kind of non-scoped binding has happened so that we're aware - this turns it into a tractable problem at least, where we can detect it at compile time and rename or "do sometihng".