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

Symbols in when false section affect lookup #23326

Closed
arnetheduck opened this issue Feb 20, 2024 · 3 comments · Fixed by #23392
Closed

Symbols in when false section affect lookup #23326

arnetheduck opened this issue Feb 20, 2024 · 3 comments · Fixed by #23392

Comments

@arnetheduck
Copy link
Contributor

Description

A follow-up on #23091 - real code is here: arnetheduck/nim-results#35 (error happens with resultsGenericBindingWorkaround=false).

The error symbol inside when false should obviously have no effect on compiling the sample.

testit.nim:

type Result*[E] = object
  e*: E

proc error*[E](v: Result[E]): E = discard

template valueOr*[E](self: Result[E], def: untyped): int =
  when E isnot void:
    when false:
      # Comment line below to make it work
      template error(): E {.used, gensym.} = s.e
      discard
    else:
      template error(): E {.used, inject.} =
        self.e

      def
  else:
    def


block:
  let rErr = Result[string](e: "a")
  let rErrV = rErr.valueOr:
    ord(error[0])

Nim Version

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

git hash: 773c066
active boot switches: -d:release

(same error on 1.6)

Current Output

$ nim c /tmp/testit
.....................................................................
/tmp/testit.nim(23, 19) template/generic instantiation of `valueOr` from here
/tmp/testit.nim(24, 8) Error: type mismatch
Expression: ord(error)
  [1] error: proc (v: Result[system.int]): int{.noSideEffect, gcsafe.}

Expected one of (first mismatch at [position]):
[1] func ord[T: Ordinal | enum](x: T): int

Expected Output

No response

Possible Solution

No response

Additional Information

No response

@litlighilit
Copy link
Contributor

litlighilit commented Mar 6, 2024

A clearer demo:

template valueOr*(def: untyped): int =
  when false:
    # Comment line below to make it work
    template error(): int {.gensym.} = discard
    discard
  else:
    template error(): int {.inject.} = 1
  def

let rErrV = valueOr error

It seems the symbol capture doesn't cover edge cases, when it comes to gensym

@litlighilit
Copy link
Contributor

In addition,

maybe there is another factor:

template valueOr*(def: untyped): int =
  template error(): int {.gensym.} = 1
  def

let rErrV = valueOr error

gensym is not considered when lookup

@metagn
Copy link
Collaborator

metagn commented Mar 11, 2024

Culprit is maybe this?

if s.owner == c.owner and (s.kind == skParam or sfGenSym in s.flags):

The gensym template is added to scope here and the injected template declaration finds it and reuses the symbol? Presumably to allow overloading the gensym'd symbol since adding gensym again defines a different symbol, even though omitting gensym implies inject for routines. Then again why does gensym again define a different symbol when we only get to use one at a time in the template?

We could move the gensym reuse logic inside the logic for when gensym is given, if this breaks code we could allow opting out of the symbol reuse on an explicit inject.

This is all letting alone the logic of actually using error inside the template after the when block, also I'm speculating about all this from my phone so I might be wrong.

metagn added a commit to metagn/Arraymancer that referenced this issue Mar 25, 2024
For code like:

```nim
when foo():
  let data = ...
else:
  template data: untyped = ...
```

in a template, since the `let data` is automatically gensym'd, the `template data` must also be marked as `gensym`, since routines are injected by default in templates. Due to a bug in the Nim compiler (nim-lang/Nim#23326) the `template data` here was forced to being gensym due to the previous `let data` symbol. This behavior might not change, but the `template data` can be marked as gensym anyway for clarity.
Vindaar pushed a commit to mratsim/Arraymancer that referenced this issue Mar 26, 2024
For code like:

```nim
when foo():
  let data = ...
else:
  template data: untyped = ...
```

in a template, since the `let data` is automatically gensym'd, the `template data` must also be marked as `gensym`, since routines are injected by default in templates. Due to a bug in the Nim compiler (nim-lang/Nim#23326) the `template data` here was forced to being gensym due to the previous `let data` symbol. This behavior might not change, but the `template data` can be marked as gensym anyway for clarity.
@Araq Araq closed this as completed in 73b0b0d Apr 9, 2024
narimiran pushed a commit that referenced this issue May 21, 2024
…3392)

fixes #23326

In a routine declaration node in a template, if the routine is marked as
`gensym`, the compiler adds it as a new symbol to a preliminary scope of
the template. If it's not marked as gensym, then it searches the
preliminary scope of the template for the name of the routine, then when
it matches a template parameter or a gensym identifier, the compiler
replaces the name node with a symbol node of the found symbol.

This makes sense for the template parameter since it has to be replaced
later, but not really for the gensym identifier, as it doesn't allow us
to inject a routine with the same name as an identifier previously
declared as gensym (the problem in #23326 is when this is in another
`when` branch).

However this is the only channel to reuse a gensym symbol in a
declaration, so maybe removing it has side effects. For example if we
have:

```nim
proc foo(x: int) {.gensym.} = discard
proc foo(x: float) {.gensym.} = discard
```

it will not behave the same as

```nim
proc foo(x: int) {.gensym.} = discard
proc foo(x: float) = discard
```

behaved previously, which maybe allowed overloading over the gensym'd
symbols.

A note to the "undeclared identifier" error message has also been added
for a potential error code that implicitly depended on the old behavior
might give, namely ``undeclared identifier: 'abc`gensym123'``, which
happens when in a template an identifier is first declared gensym in
code that doesn't compile, then as a routine which injects by default,
then the identifier is used.

(cherry picked from commit 73b0b0d)
narimiran pushed a commit that referenced this issue May 21, 2024
…3392)

fixes #23326

In a routine declaration node in a template, if the routine is marked as
`gensym`, the compiler adds it as a new symbol to a preliminary scope of
the template. If it's not marked as gensym, then it searches the
preliminary scope of the template for the name of the routine, then when
it matches a template parameter or a gensym identifier, the compiler
replaces the name node with a symbol node of the found symbol.

This makes sense for the template parameter since it has to be replaced
later, but not really for the gensym identifier, as it doesn't allow us
to inject a routine with the same name as an identifier previously
declared as gensym (the problem in #23326 is when this is in another
`when` branch).

However this is the only channel to reuse a gensym symbol in a
declaration, so maybe removing it has side effects. For example if we
have:

```nim
proc foo(x: int) {.gensym.} = discard
proc foo(x: float) {.gensym.} = discard
```

it will not behave the same as

```nim
proc foo(x: int) {.gensym.} = discard
proc foo(x: float) = discard
```

behaved previously, which maybe allowed overloading over the gensym'd
symbols.

A note to the "undeclared identifier" error message has also been added
for a potential error code that implicitly depended on the old behavior
might give, namely ``undeclared identifier: 'abc`gensym123'``, which
happens when in a template an identifier is first declared gensym in
code that doesn't compile, then as a routine which injects by default,
then the identifier is used.

(cherry picked from commit 73b0b0d)
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