[parser] top-level comment attached to wrong node, makes nim doc buggy

[timotheecour]

/cc @Araq @zah

all entries below marked as BUG are bugs IMO. The compiler should fix where the comment nodes go and nim doc should be updated accordingly (instead of just working around these inconsistencies in nim doc)

top-level proc comment attached to nnkStmtList instead of nnkProcDef

proc myfun1()= ## this comment will be in nnkProcDef

proc myfun2()=
  ## BUG: this will be in nnkStmtList, should be a comment in nnkProcDef

proc myfun3()= ## this comment will be in nnkProcDef (could also be empty: ##)
  ## ok: this comment will be in nnkProcDef (continued from above comment)

  ## ok: this will be attached to nnkStmtList

Likewise with template, macro, iterator, type etc.

comment attached to nnkRecList in object definition instead of nnkObjectTy

this case is also bizarre: shouldn't the following code give compile error? instead, it attaches a comment to nnkRecList (IMO there should be no comment attached to nnkRecList)

  type Foo5* = object ##
    ## this comment will be in nnkObjectTy

    ## BUG: this comment will be in nnkRecList (and show weirdly in nim doc)

    a1: int
    a2: int ## this comment will be in nnkIdentDefs

  type Foo4* = object
    ## BUG: this comment will be in nnkRecList instead of nnkObjectTy

    a1: int ## this comment will be in nnkIdentDefs

Note: this is inconsistent with enum and named tuple, which both behave sanely:

  type
    MyEnum* = enum
      ## MyEnum comment1: this will be in nnkEnumTy
      kval1, ## comment kval1
      kval2, ## comment kval2
      kval3, # hidden comment kval3

  type MyTuple1* = tuple
    ## this comment will be in nnkTupleTy

    a1: int ## this comment will be in nnkIdentDefs

nim doc currently buggy because of comments in nnkRecList

this is how Foo5 renders in html:

Foo5 = object
  a1: int                      ## BUG: this comment will be in nnkRecList
  a2: int                      ## this comment will be in nnkIdentDefs
  
this comment will be in nnkObjectTy

as you can see the nnkRecList comment hijacks the comment for the 1st field, a1.

nim doc currently buggy because top-level comment attached to nnkStmtList in a proc

nim doc is trying to work around above mentioned parser bug by trying to attach first stmt in nnkStmtList if it's a nnkCommentStmt and if proc doesn't already have a comment; this workaround causes issues in following case, see myfun5.
There's inherent ambiguity in current approach, and my proposal would eliminate this workaround, this bug, and all ambiguity:

in fun5, a statement comment ends up in generated html as doc comment for the proc

  proc myfun4*()=
    ## ok: ends in top-level comment
    const a = 1

    ## ok: doesn't end up in top-level comment
    const a2=1

  proc myfun5*()=

    ## BUG: ends up in top-level comment despite empty newline
    const a = 1

doc generated:

proc myfun4() {...}
ok: ends in top-level comment
proc myfun5() {...}
BUG: ends up in top-level comment despite empty newline

proposal

• no comment ends up in nnkRecList
• top-level comments for object, proc etc shall be put in (respectively) nnkObjectTy, nnkProcDef etc, instead of (respectively) nnkRecList, nnkStmtList, consistent with how it's done with enum, named tuple.
• for proc, adding an empty line shall put the comment in nnkStmtList:

proc myfun2()=
  ## comment will be in nnkProcDef

  ## comment will be in nnkStmtList

• this will likely simplify nim doc and automatically fix the above mentioned nim doc bugs

[Araq]

I don't think the AST is wrong, nim doc should just be fixed.

[timotheecour]

• How can nim doc be fixed in the case I mentioned above? see paragraph mentioning "There's inherent ambiguity in current approach"
  My proposal would remove this ambiguity

• What's the use case for comments in nnkRecList as opposed to nnkObjectTy (maybe there's something I don't know regarding that point)

[Araq]

There is only nnkCommentStmt, that's part of the data model of Nim. Other comments are not. The fact that we have this n.comment field in the AST is a design mistake that must not be exposed any further.

How can nim doc be fixed in the case I mentioned above? see paragraph mentioning "There's inherent ambiguity in current approach"

Where is this ambiguity? You simply claim there is one.