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

Inconsisten Symbol Binding Rules in Templates/Generics #11184

Open
krux02 opened this issue May 6, 2019 · 3 comments · May be fixed by #23104
Open

Inconsisten Symbol Binding Rules in Templates/Generics #11184

krux02 opened this issue May 6, 2019 · 3 comments · May be fixed by #23104

Comments

@krux02
Copy link
Contributor

krux02 commented May 6, 2019

The issue that I want to talk about is the following for both templates and generic function implementations:

When a function symbol can't be resolved in the template definition, it stays as an identifier. Thi identifier will then be resolved in the context where the template is expanded. So the identifier can be resolved to a symbol from expansion context.

When a function symbol has soveral overloads, the identifier becomes an open sym choice. Open sym choice means the identifier can be resolved to a symbol from the euxpansion context.

The surprising and therefore problematic behavior happens when a symbol is resolved to a single symbol. Then the identifier becomes a node of nnkSym , that
means it cannot be resolved to a symbol from the expansion context anymore.

Example

file1.nim

# foo0 has 0 overloads

template myTemplate0() =
  foo0(bar)

# foo1 has 1 overload

proc foo1(arg: int) =
  discard

template myTemplate1() =
  foo1(bar)

# foo2 has 2 overloads

proc foo2(arg: int) =
  discard

proc foo2(arg: string) =
  discard

template myTemplate2() =
  foo2(bar)

file2.nim

import file1

type
  MyType = object

proc foo0(arg: MyType) =
  echo "foo0"

proc foo1(arg: MyType) =
  echo "foo1"

proc foo2(arg: MyType) =
  echo "foo2"

proc test() =
  var bar: MyType

  myTemplate0()
  myTemplate1() # this template is the exception that doesn't work.
  myTemplate2()

test()

Possible Solution

Everything should be an OpenSymChoice for consistent behavior. Since nim currently really gets confused with empty open sym choices I think the empty open sym choice can remain a ident node. But I like to think about an identifier like an open sym choice with no elements. This is currently the generated AST from the templates.

echo treeRepr getAst(myTemplate0())
Call
  Ident "foo0" # with 0 overloads it is an identifier
  Ident "bar"

echo treeRepr getAst(myTemplate0())
Call
  Sym "foo1" # with 1 overload it is a symbol (closed)
  Ident "bar"

echo treeRepr getAst(myTemplate0())
Call
  OpenSymChoice # with more than one overloads it is an open sym choice
    Sym "foo2"
    Sym "foo2"
  Ident "bar"

Additional Information

@krux02 krux02 changed the title inconsisten symbol binding rules in templates inconsisten symbol binding rules in templates/generics May 6, 2019
@krux02
Copy link
Contributor Author

krux02 commented May 7, 2019

comment from Araq: This should work, and private sort should not bind to anything from instantiation context.

proc privateSort(...)
proc sort[T]() = privateSort(...) # delegate
    bind privateSort <- this is not necessary

@krux02 krux02 changed the title inconsisten symbol binding rules in templates/generics Inconsisten Symbol Binding Rules in Templates/Generics May 21, 2019
@metagn
Copy link
Collaborator

metagn commented Sep 16, 2023

With the potential design described in #22605 (comment), each foo would become:

Ident "foo0"

OpenSymChoice
  Sym "foo1" {preferred}

OpenSymChoice
  Sym "foo2"
  Sym "foo2"
  # neither preferred because name is ambiguous at template declaration

Technically just OpenSymChoice(Sym "foo1") would be fine too if the necessary logic was in the compiler (it would always get overriden) but the preferred would also allow non-overloadable sym kinds to receive the same treatment.

Related/same issue #3542

@Varriount
Copy link
Contributor

Varriount commented Sep 16, 2023

I would prefer the original solution where everything is open by default. It's easier to understand, and less likely to break just because a template definition was moved around.

metagn added a commit to metagn/Nim that referenced this issue Sep 23, 2023
metagn added a commit to metagn/Nim that referenced this issue Sep 29, 2023
PMunch pushed a commit to PMunch/nimlsp that referenced this issue Sep 30, 2023
* Explicitly bind logging procs in templates

The logging templates in `logger` reference procs like `error`, `info`, `warn` etc. from the `logging` module. These are implicitly bound, as they have exactly 1 overload in the scope of the template definitions (nim-lang/Nim#11184).

If this behavior of Nim or the context in `logger` were to change, these would start preferring procs like `macros.error` instead and cause issues. Explicitly binding them keeps the current behavior consistent.

An alternative would be to directly call `logging.error join(args)` etc.

* also update frameLog
Nycto pushed a commit to Nycto/DelaunayNim that referenced this issue Sep 30, 2023
The original `->` in `edge` sorts the parameters before forming the edge, but helpers.`->` doesn't do this.

helpers.`->` also has a more precise signature (on `tuple[x, y: float]` rather than the `Point` concept), which means generic procs can prefer it if `->` is considered to be overloaded.

This has not been an issue in this package because the generic procs that use `->` only have access to 1 overload of it and so Nim considers them not overloaded. However this behavior is not reliable: nim-lang/Nim#11184, and so attempts to fix this cause issues in this library's tests.

Alternatively the tests can use another operator to form unsorted edges, or the signature of helpers.`->` can be made equally as precise as the one in `edge` (i.e. defined on `Point`).
@metagn metagn linked a pull request Dec 25, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants