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

Doc Comment in Template Causes Compilation Failure #13511

Closed
krux02 opened this issue Feb 26, 2020 · 5 comments · Fixed by #16002
Closed

Doc Comment in Template Causes Compilation Failure #13511

krux02 opened this issue Feb 26, 2020 · 5 comments · Fixed by #16002

Comments

@krux02
Copy link
Contributor

krux02 commented Feb 26, 2020

A doc comment in the template foo causes a compilation failure.

Example

import macros

type
  Builder = ref object
    components: seq[Component]
  Component = object

proc add(builder: var Builder, component: Component) {.compileTime.} =
  builder.components.add(component)

macro debugAst(arg: typed): untyped =
  ## just for debugging purpose.
  echo arg.treeRepr
  return arg

static:
  var component = Component()
  var builder = Builder()

  template foo(): untyped =
    ## this doc comment causes compilation failure.
    builder

  debugAst:
    add(foo(), component)

Current Output

StmtList
  Call
    Sym "add"
    HiddenAddr
      StmtListExpr
        Empty
        Sym "builder"
    Sym "component"
stack trace: (most recent call last)

/tmp/scratch.nim(25, 9) Error: limited VM support for 'addr'

If I delete the doc comment, or convert it to a normal comment, the output is the following:

StmtList
  Call
    Sym "add"
    HiddenAddr
      Sym "builder"
    Sym "component"

The only difference here is, the StmtListExpr is missing.

Possible Solution

  • The semchecker could optimize away the StmtListExpr.
  • Patch vmgen and friends so that it works.

Additional Information

If this would be my code, I could easily work around this bug. Builder is a ref object and passed as a var parameter. This is not necessary and removing either the ref or the var would fix this specific problem as well. But that is how argparse is implemented and works. My part is the template with the doc comment. The template is in system.nim and I don't like the idea to remove the doc comment. Other people would make a PR to introduce it again and then wonder why CI fails.

$ nim -v
Nim Compiler Version 1.1.1 [Linux: amd64]
Compiled at 2020-02-26
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: e4ed19c12fcce046d990a918800d9031706e8b66
active boot switches: -d:release -d:danger
@timotheecour
Copy link
Member

timotheecour commented Feb 26, 2020

related:

in all cases, a template is involved, and the added doc comment affects semantics

proposal

  • by default, a doc comment shall disappear when the template is instantiated in some context, so that this resolves to nnkCall... instead of nnkStmtList when instantiated (a breaking change)
template myquit2*():untyped=
  ## foo
  quit(1)

nim doc shall correctly render the doc comment for such template, as currently the case

template insertSomeComment*(msg, body):untyped=
  ## this is a helper proc to create a doc comment
  ## those litteral doc comment lines won't be forwarded as per this RFC
  ## however `astDocComment(msg)` will be forwarded as a doc comment to instantiating scope
  astDocComment("current nim: " & NimVersion & " " & msg) # this creates a doc comment node for instantiation scope
  body

template bar*():untyped=
  insertSomeComment("this is bar"): echo "implementation of `bar`"

in this case nim doc will show:

  • insertSomeComment with the litteral doc comment lines
  • bar with the generated doc comment:
current nim 1.1.1 this is bar

@Araq
Copy link
Member

Araq commented Feb 27, 2020

Why not patch vmgen and friends so that it works?

@timotheecour
Copy link
Member

Why not patch vmgen and friends so that it works?

what does it refer to here? the above proposal or top code or something else?

@Araq
Copy link
Member

Araq commented Feb 27, 2020

Well it's a bug, do we know for certain it's so hard to fix that we need to change once again how the AST looks like?

@timotheecour
Copy link
Member

timotheecour added a commit to timotheecour/Nim that referenced this issue Nov 16, 2020
timotheecour added a commit to timotheecour/Nim that referenced this issue Nov 25, 2020
Araq pushed a commit that referenced this issue Nov 25, 2020
* fix #14339: fixes limited VM support for addr

* strengthen test

* reference bug #16003

* also fixes #13511

* also fixes #14420
mildred pushed a commit to mildred/Nim that referenced this issue Jan 11, 2021
…support for addr (nim-lang#16002)

* fix nim-lang#14339: fixes limited VM support for addr

* strengthen test

* reference bug nim-lang#16003

* also fixes nim-lang#13511

* also fixes nim-lang#14420
ardek66 pushed a commit to ardek66/Nim that referenced this issue Mar 26, 2021
…support for addr (nim-lang#16002)

* fix nim-lang#14339: fixes limited VM support for addr

* strengthen test

* reference bug nim-lang#16003

* also fixes nim-lang#13511

* also fixes nim-lang#14420
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants