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

genericsOpenSym not working when additional generally non-matching symbol with same name is present #23865

Closed
arnetheduck opened this issue Jul 20, 2024 · 2 comments · Fixed by #23873 or #23939

Comments

@arnetheduck
Copy link
Contributor

arnetheduck commented Jul 20, 2024

Description

This is the same example as in #23091, but this time there's an additional func error symbol present (as happens in nim-result) - the presence of this function causes the warning to disappear even though it should be there, and {.experimental:"genericsOpenSym".} stops working..

Removing func error from here makes the warning appear:

type Xxx = enum
  error
  value

type
  Result[T, E] = object
    when T is void:
      when E is void:
        oResultPrivate*: bool
      else:
        case oResultPrivate*: bool
        of false:
          eResultPrivate*: E
        of true:
          discard
    else:
      when E is void:
        case oResultPrivate*: bool
        of false:
          discard
        of true:
          vResultPrivate*: T
      else:
        case oResultPrivate*: bool
        of false:
          eResultPrivate*: E
        of true:
          vResultPrivate*: T

func error*[T, E](self: Result[T, E]): E =
  ## Fetch error of result if set, or raise Defect
  case self.oResultPrivate
  of true:
    when T isnot void:
      raiseResultDefect("Trying to access error when value is set", self.vResultPrivate)
    else:
      raiseResultDefect("Trying to access error when value is set")
  of false:
    when E isnot void:
      self.eResultPrivate

template valueOr[T: not void, E](self: Result[T, E], def: untyped): untyped =
  let s = (self) # TODO avoid copy
  case s.oResultPrivate
  of true:
    s.vResultPrivate
  of false:
    when E isnot void:
      template error: untyped {.used, inject.} = s.eResultPrivate
    def

proc f(): Result[int, cstring] =
  Result[int, cstring](oResultPrivate: false, eResultPrivate: "f")

proc g(T: type): string =
  let x = f().valueOr:
    return $error #[tt.Warning
            ^ a new symbol 'error' has been injected during instantiation of g, however 'error' [enumField declared in tmacroinjectedsymwarning.nim(2, 3)] captured at the proc declaration will be used instead; either enable --experimental:genericsOpenSym to use the injected symbol or `bind` this captured symbol explicitly [GenericsIgnoredInjection]]#

  "ok"


assert g(int) == "error"

Removing func error from here makes the triggering of the assert go away:

{.experimental:"genericsOpenSym".}
type Xxx = enum
  error
  value

type
  Result[T, E] = object
    when T is void:
      when E is void:
        oResultPrivate*: bool
      else:
        case oResultPrivate*: bool
        of false:
          eResultPrivate*: E
        of true:
          discard
    else:
      when E is void:
        case oResultPrivate*: bool
        of false:
          discard
        of true:
          vResultPrivate*: T
      else:
        case oResultPrivate*: bool
        of false:
          eResultPrivate*: E
        of true:
          vResultPrivate*: T

func error*[T, E](self: Result[T, E]): E =
  ## Fetch error of result if set, or raise Defect
  case self.oResultPrivate
  of true:
    when T isnot void:
      raiseResultDefect("Trying to access error when value is set", self.vResultPrivate)
    else:
      raiseResultDefect("Trying to access error when value is set")
  of false:
    when E isnot void:
      self.eResultPrivate

template valueOr[T: not void, E](self: Result[T, E], def: untyped): untyped =
  let s = (self) # TODO avoid copy
  case s.oResultPrivate
  of true:
    s.vResultPrivate
  of false:
    when E isnot void:
      template error: untyped {.used, inject.} = s.eResultPrivate
    def

proc f(): Result[int, cstring] =
  Result[int, cstring](oResultPrivate: false, eResultPrivate: "f")

proc g(T: type): string =
  let x = f().valueOr:
    return $error #[tt.Warning
            ^ a new symbol 'error' has been injected during instantiation of g, however 'error' [enumField declared in tmacroinjectedsymwarning.nim(2, 3)] captured at the proc declaration will be used instead; either enable --experimental:genericsOpenSym to use the injected symbol or `bind` this captured symbol explicitly [GenericsIgnoredInjection]]#

  "ok"


assert g(int) == "f"

Nim Version

2.0.8, devel

Current Output

No response

Expected Output

No response

Possible Solution

No response

Additional Information

No response

@metagn
Copy link
Collaborator

metagn commented Jul 21, 2024

genericsOpenSym works on nkSym but here we get nkOpenSymChoice which only looks for other overloadable symbols, we need to check if a new non-overloadable symbol was introduced (not an overloadable one, and it must be the latest introduced symbol)

Araq pushed a commit that referenced this issue Jul 25, 2024
fixes #23865

The node flag `nfOpenSym` implemented in #23091 for sym nodes is now
also implemented for open symchoices. This means the intended behavior
is still achieved when multiple overloads are in scope to be captured,
so the issue is fixed. The code for the flag is documented and moved
into a helper proc and the experimental switch is now enabled for the
compiler test suite.
@metagn metagn reopened this Aug 6, 2024
@metagn
Copy link
Collaborator

metagn commented Aug 6, 2024

Not actually fixed #23873 (comment)

The problem is that we check for non-overloadable syms to replace the open sym when the new sym is a template (that is still used in a nonoverloadable way), but working on just any sym would lose the information of the captured sym choices. Don't mean to sound like a broken record but I think we need something like #23104 for the proper logic here, or special case the nullary template symbol usage to treat it as nonoverloaded

Also due to the compilier complaining I didn't make it set the type of the symchoice to nil when marking it opensym, only sym nodes, which means the symchoice never gets sem'd, but #23892 doesn't have this problem

Unfortunately I can't look into it much right now, will look möre later

metagn added a commit to metagn/Nim that referenced this issue Aug 11, 2024
@Araq Araq closed this as completed in a64aa51 Aug 11, 2024
narimiran pushed a commit that referenced this issue Aug 14, 2024
fixes #23865

The node flag `nfOpenSym` implemented in #23091 for sym nodes is now
also implemented for open symchoices. This means the intended behavior
is still achieved when multiple overloads are in scope to be captured,
so the issue is fixed. The code for the flag is documented and moved
into a helper proc and the experimental switch is now enabled for the
compiler test suite.

(cherry picked from commit 469a604)
narimiran pushed a commit that referenced this issue Aug 14, 2024
actually fixes #23865 following up #23873

In the handling of `nkIdent` in `semExpr`, the compiler looks for the
closest symbol with the name and [checks the symbol
kind](https://github.com/nim-lang/Nim/blob/6126a0bf46f4e29a368b8baefea69a2bcae54e93/compiler/semexprs.nim#L3171)
to also consider the overloads if the symbol kind is overloadable. But
it treats the normally overloadable template/macro/module sym kinds the
same as non-overloadable symbols, just calling `semSym` on it. We need
to mirror this behavior in `semOpenSym`; we treat the captured symchoice
as a fresh identifier, so if the symbol we find is a
template/macro/module, we use that symbol immediately as opposed to
waiting for overloads.

(cherry picked from commit a64aa51)
narimiran pushed a commit that referenced this issue Aug 14, 2024
fixes #23865

The node flag `nfOpenSym` implemented in #23091 for sym nodes is now
also implemented for open symchoices. This means the intended behavior
is still achieved when multiple overloads are in scope to be captured,
so the issue is fixed. The code for the flag is documented and moved
into a helper proc and the experimental switch is now enabled for the
compiler test suite.

(cherry picked from commit 469a604)
narimiran pushed a commit that referenced this issue Aug 14, 2024
actually fixes #23865 following up #23873

In the handling of `nkIdent` in `semExpr`, the compiler looks for the
closest symbol with the name and [checks the symbol
kind](https://github.com/nim-lang/Nim/blob/6126a0bf46f4e29a368b8baefea69a2bcae54e93/compiler/semexprs.nim#L3171)
to also consider the overloads if the symbol kind is overloadable. But
it treats the normally overloadable template/macro/module sym kinds the
same as non-overloadable symbols, just calling `semSym` on it. We need
to mirror this behavior in `semOpenSym`; we treat the captured symchoice
as a fresh identifier, so if the symbol we find is a
template/macro/module, we use that symbol immediately as opposed to
waiting for overloads.

(cherry picked from commit a64aa51)
narimiran pushed a commit that referenced this issue Aug 14, 2024
fixes #23865

The node flag `nfOpenSym` implemented in #23091 for sym nodes is now
also implemented for open symchoices. This means the intended behavior
is still achieved when multiple overloads are in scope to be captured,
so the issue is fixed. The code for the flag is documented and moved
into a helper proc and the experimental switch is now enabled for the
compiler test suite.

(cherry picked from commit 469a604)
narimiran pushed a commit that referenced this issue Aug 14, 2024
actually fixes #23865 following up #23873

In the handling of `nkIdent` in `semExpr`, the compiler looks for the
closest symbol with the name and [checks the symbol
kind](https://github.com/nim-lang/Nim/blob/6126a0bf46f4e29a368b8baefea69a2bcae54e93/compiler/semexprs.nim#L3171)
to also consider the overloads if the symbol kind is overloadable. But
it treats the normally overloadable template/macro/module sym kinds the
same as non-overloadable symbols, just calling `semSym` on it. We need
to mirror this behavior in `semOpenSym`; we treat the captured symchoice
as a fresh identifier, so if the symbol we find is a
template/macro/module, we use that symbol immediately as opposed to
waiting for overloads.

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