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.

Sign up for GitHub

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

Jump to bottom

options.none(T) is nnkOpenSymChoice in typed AST of proc argument #19118

Open
alaviss opened this issue Nov 10, 2021 · 2 comments · Fixed by #24184
Open

options.none(T) is nnkOpenSymChoice in typed AST of proc argument #19118

alaviss opened this issue Nov 10, 2021 · 2 comments · Fixed by #24184
Labels
CPS bugs/pulls related to the cps project Macros

Comments

@alaviss
Copy link
Collaborator

alaviss commented Nov 10, 2021

Example

import options, macros

macro typedTree(n: typed): untyped =
  result = n
  echo treeRepr n

proc foo(x = none(Natural)) {.typedTree.} = discard

Current Output

ProcDef
  Sym "foo"
  Empty
  Empty
  FormalParams
    Empty
    IdentDefs
      Sym "x"
      Empty
      Call
        OpenSymChoice 2 "none"
        Sym "Natural"
  Empty
  Empty
  DiscardStmt
    Empty

Expected Output

ProcDef
  Sym "foo"
  Empty
  Empty
  FormalParams
    Empty
    IdentDefs
      Sym "x"
      Empty
      Call
        Sym "none"
        Sym "Natural"
  Empty
  Empty
  DiscardStmt
    Empty

Additional Information

  • CPS relies on the value to have a type to infer the parameter type. However that is not the case here where somehow none was an OpenSymChoice, which should be impossible in typed AST.
$ nim -v
Nim Compiler Version 1.7.1 [Linux: amd64]
Compiled at 2021-11-08
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: 83a9c3ba31d180cd5e31026d8b7603bf7adea18c
active boot switches: -d:release -d:nimUseLinenoise
@alaviss alaviss added Macros CPS bugs/pulls related to the cps project labels Nov 10, 2021
@metagn
Copy link
Collaborator

metagn commented Sep 6, 2023

#12942

@metagn
Copy link
Collaborator

metagn commented Sep 26, 2024 •
edited
Loading

none(Natural) is typed as expected, the typed version is just not in the proc AST, it's in the AST of the param symbol, same issue as #12942. Easy fix but maybe breaks stuff when the proc is retyped?

metagn added a commit to metagn/Nim that referenced this issue Sep 26, 2024
@metagn
make default values typed in proc AST same as param sym AST
44300ee 
fixes nim-lang#12942, fixes nim-lang#19118
@metagn metagn mentioned this issue Sep 26, 2024
Merged
@Araq Araq closed this as completed in c21bf7f Sep 27, 2024
@metagn metagn mentioned this issue Sep 27, 2024
Merged
Araq pushed a commit that referenced this issue Sep 27, 2024
@metagn
Revert "make default values typed in proc AST same as param sym AST" (#…
821d080 
…24191)

Reverts #24184, reopens #12942, reopens #19118

#24184 seems to have caused a regression in
https://github.com/c-blake/thes and
https://github.com/c-blake/bu/blob/main/rp.nim#L84 reproducible with
`git clone https://github.com/c-blake/cligen; git clone
https://github.com/c-blake/thes; cd thes; nim c -p=../cligen thes`.
Changing the `const` to `let` makes it compile.

A minimization that is probably the same issue is:

```nim
const a: seq[string] = @[]

proc foo(x = a) =
  echo typeof(x)
  echo x

import macros

macro resemFoo() =
  result = getImpl(bindSym"foo")

block:
  resemFoo() # Error: cannot infer the type of parameter 'x'
```

This should be a regression test in a future reimplementation of #24184.
@narimiran narimiran reopened this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CPS bugs/pulls related to the cps project Macros
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

make default values typed in proc AST same as param sym AST metagn/Nim
3 participants
@alaviss @metagn @narimiran