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 respecting injections across modules #23386

Closed
arnetheduck opened this issue Mar 10, 2024 · 3 comments · Fixed by #23892
Closed

genericsOpenSym not respecting injections across modules #23386

arnetheduck opened this issue Mar 10, 2024 · 3 comments · Fixed by #23892

Comments

@arnetheduck
Copy link
Contributor

Description

Followup on #23091

This is the same test as

proc g(T: type): string =
but split up over two modules:

testit.nim:

{.experimental: "genericsOpenSym".}

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

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

testit2.nim:

import testit

type Xxx = enum
  error
  value

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

proc g(T: type): string =
  let x = f().valueOr:
    return $error

  "ok"

doAssert g(int) == "f"
Error: unhandled exception: /tmp/testit2.nim(16, 1) `g(int) == "f"`  [AssertionDefect]

The local symbol in testit2 gets precedence over the injected symbol from testit, even though the injected symbol as a more local scope than the Xxx enum.

Nim Version

Nim Compiler Version 2.1.1 [Linux: amd64]
Compiled at 2024-02-20
Copyright (c) 2006-2024 by Andreas Rumpf

git hash: 773c066

Current Output

No response

Expected Output

No response

Possible Solution

No response

Additional Information

No response

@metagn
Copy link
Collaborator

metagn commented Mar 11, 2024

The way this switch works when not enabled globally is admittedly terrible, since we want to store the information of whether a symbol should be opened so we can give a warning, caring about the switch is delayed until the instantiation context, i.e. not even in the context of the generic proc using it, but where the generic proc gets instantiated. (I'm guessing that would be the expected behavior, the switch should only matter at the generic proc declaration, when the symbols get marked as open).

I hoped that making it into its own node kind nkOpenSym as a derivative of nkSym would work so we could more cleanly store the information of what should be done but this is proving very difficult given how ingrained nkSym is for both macros and the compiler at a binary level. Probably for now we can just add another node flag and deal with it.

@arnetheduck
Copy link
Contributor Author

Right, this I guess is a bit of a broader issue in nim, that it's hard to know what pragmas apply to in general (push vs top-level vs inside-proc etc).

I guess it's not terrible though it does make it more difficult to provide a good out-of-the-box experience for a library like results that heavily benefits from this lookup order - it would be useful to be able to enable it on specific expansions that make use of it / rely on it (like the valueOr template).

#23385 is a lower-hanging fruit perhaps where I can either warn or enforce ({.error.}) it when the template gets used.

@Araq
Copy link
Member

Araq commented Mar 14, 2024

Probably for now we can just add another node flag and deal with it.

Nooooo....... :-)

@Araq Araq added the Status label Mar 14, 2024
metagn added a commit to metagn/Nim that referenced this issue May 4, 2024
Araq pushed a commit that referenced this issue Aug 12, 2024
refs #23873 (comment),
fixes #23386, fixes #23385, supersedes #23572

Turns the `nfOpenSym` node flag implemented in #23091 and extended in
#23102 and #23873, into a node kind `nkOpenSym` that forms a unary node
containing either `nkSym` or `nkOpenSymChoice`. Since this affects
macros working on generic proc AST, the node kind is now only generated
when the experimental switch `genericsOpenSym` is enabled, and a new
node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice`
when the switch is not enabled so that we can give a warning.

Now that the experimental switch has more reasonable semantics, we
define `nimHasGenericsOpenSym2`.
narimiran pushed a commit that referenced this issue Aug 14, 2024
refs #23873 (comment),
fixes #23386, fixes #23385, supersedes #23572

Turns the `nfOpenSym` node flag implemented in #23091 and extended in
containing either `nkSym` or `nkOpenSymChoice`. Since this affects
macros working on generic proc AST, the node kind is now only generated
when the experimental switch `genericsOpenSym` is enabled, and a new
node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice`
when the switch is not enabled so that we can give a warning.

Now that the experimental switch has more reasonable semantics, we
define `nimHasGenericsOpenSym2`.

(cherry picked from commit 0c890ff)
narimiran pushed a commit that referenced this issue Aug 14, 2024
refs #23873 (comment),
fixes #23386, fixes #23385, supersedes #23572

Turns the `nfOpenSym` node flag implemented in #23091 and extended in
containing either `nkSym` or `nkOpenSymChoice`. Since this affects
macros working on generic proc AST, the node kind is now only generated
when the experimental switch `genericsOpenSym` is enabled, and a new
node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice`
when the switch is not enabled so that we can give a warning.

Now that the experimental switch has more reasonable semantics, we
define `nimHasGenericsOpenSym2`.

(cherry picked from commit 0c890ff)
narimiran pushed a commit that referenced this issue Aug 14, 2024
refs #23873 (comment),
fixes #23386, fixes #23385, supersedes #23572

Turns the `nfOpenSym` node flag implemented in #23091 and extended in
containing either `nkSym` or `nkOpenSymChoice`. Since this affects
macros working on generic proc AST, the node kind is now only generated
when the experimental switch `genericsOpenSym` is enabled, and a new
node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice`
when the switch is not enabled so that we can give a warning.

Now that the experimental switch has more reasonable semantics, we
define `nimHasGenericsOpenSym2`.

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