Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameter not matched in template following symchoice changes #24002

Closed
arnetheduck opened this issue Aug 22, 2024 · 13 comments · Fixed by #24007
Closed

Parameter not matched in template following symchoice changes #24002

arnetheduck opened this issue Aug 22, 2024 · 13 comments · Fixed by #24007

Comments

@arnetheduck
Copy link
Contributor

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: d6f7625
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
Copy link
Collaborator

metagn commented Aug 22, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

metagn commented Aug 22, 2024

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.

@metagn metagn changed the title Parameter not matched in template following genericsOpenSym changes Parameter not matched in template following symchoice changes Aug 22, 2024
@arnetheduck
Copy link
Contributor Author

+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
Copy link
Contributor Author

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
Copy link
Collaborator

metagn commented Aug 22, 2024

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

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
Copy link
Collaborator

metagn commented Aug 22, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

metagn commented Aug 22, 2024

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
Copy link
Member

Araq commented Aug 23, 2024

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": nim-lang/RFCs#380

@arnetheduck
Copy link
Contributor Author

The problem is usually in the opposite direction, Nim doesn't do enough of "magic": nim-lang/RFCs#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 added a commit to metagn/Nim that referenced this issue Aug 23, 2024
@metagn
Copy link
Collaborator

metagn commented Aug 23, 2024

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
Copy link
Contributor Author

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".

@Araq Araq closed this as completed in 770f8d5 Aug 28, 2024
narimiran added a commit that referenced this issue Aug 29, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
narimiran added a commit that referenced this issue Aug 29, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
narimiran added a commit that referenced this issue Aug 29, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
narimiran added a commit that referenced this issue Aug 29, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
narimiran added a commit that referenced this issue Aug 29, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
narimiran added a commit that referenced this issue Aug 29, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
narimiran pushed a commit that referenced this issue Aug 29, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
narimiran pushed a commit that referenced this issue Sep 16, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
narimiran pushed a commit that referenced this issue Sep 16, 2024
)

fixes #15314, fixes #24002

The OpenSym behavior first added to generics in #23091 now also applies
to templates, since templates can also capture symbols that are meant to
be replaced by local symbols if the context imports symbols with the
same name, as in the issue #24002. The experimental switch
`templateOpenSym` is added to enable this behavior for templates only,
and the experimental switch `openSym` is added to enable it for both
templates and generics, and the documentation now mainly mentions this
switch.

Additionally the logic for `nkOpenSymChoice` nodes that were previously
wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so
these nodes aren't wrapped in `nkOpenSym` anymore. This means
`nkOpenSym` can only have children of kind `nkSym` again, so it is more
in line with the structure of symchoice nodes. As for why they aren't
merged with `nkOpenSymChoice` nodes yet, we need some way to signal that
the node shouldn't become ambiguous if other options exist at
instantiation time, we already captured a symbol at the beginning and
another symbol can only replace it if it's closer in scope and
unambiguous.

(cherry picked from commit 770f8d5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants