timotheecour / Nim

Nim is a compiled, garbage-collected systems programming language with a design that focuses on efficiency, expressiveness, and elegance (in that order of priority).
http://nim-lang.org/
Other
2 stars 0 forks source link

docgen: column reporting is wrong #658

Open timotheecour opened 3 years ago

timotheecour commented 3 years ago

/cc @a-mr

Example

see example mentioned in https://github.com/nim-lang/Nim/pull/17338#discussion_r593796121

discussion

I tried to put the line and column of Doc comment itself to n.info but immediately got a problem with nimpretty.

PNode has a info*: TLineInfo so adding fields to TLineInfo is IMO not an option as it'd increase memory use; PNode is the most common type generated by compiler; see:

adding fields conditionally (on nimdoc) would maybe be worth investigating but i have a feeling this won't solve the problem and the problem exists also for nim c

proposal

IMO the "wrong column reporting" problem can be solved as follows:

comment 2

comment 3

comment 4 (maybe also non-doc comments)



benefits:
* reduces memory consumption by compiler (40B => 32B) since `PNode` is the most frequent type
* solves column reporting
* allows faithful rendering of comments via `repr` since no information is lost
* maybe also for nimpretty (need to check)

## links
* https://github.com/nim-lang/Nim/pull/17338 discusses this issue in several places
* `commentOffsetA` (eg https://github.com/nim-lang/Nim/pull/15643) related but different issue
a-mr commented 3 years ago

I actually meant reusing existing fields line and col.

I added code around https://github.com/nim-lang/Nim/blob/4bfc5a955120b90faf07f50b54ae29f2c6e6f123/compiler/lexer.nim#L966 for both lines and columns like

      while L.buf[pos] in {CR, LF}:  # skip blank lines
+       inc tok.line
        pos = handleCRLF(L, pos)
        toStrip = 0
        while L.buf[pos] == ' ':
          inc pos
          inc toStrip
+     tok.col = toStrip

That's actually token but it gets into PNode.info by some way eventually.

That solved the problem of preserving information and I actually checked that nim doc started to work correctly (after removing the previous workaround from docgen.nim), but broke lines (not columns though in nimpretty).

The option to do it only in the nimdoc mode should solve the problem. Though I'm not sure about other problems that may arise: e.g. error reporting for nim syntax related with comments may become broken (just a guess).

a-mr commented 3 years ago

There is also another option, which is a bit ugly but seems quite safe: add convention to pass missing arguments in the string itself at its first line: number of additional lines first, comma, additional columns.

So this doc comment

##[
    comment 1
    comment 2
]##

will be turned into the string

1,4
comment 1
comment 2

After that 1 and 4 are extracted and n.line+1 and n.col+4 calculated in docgen.nim.

Of course, option like skipFirstLine should be added to rst.nim.