rst: add missing line/column info for some warnings #18383

a-mr · 2021-06-28T21:29:04Z

Some rst node types are post-processed after parsing, that is in either resolveSubs or in rstgen.nim, while lines/columns are stored only in tokens during parsing stage. There was no way to print proper line/column for the corresponding warnings or errors. Now for all those 7 node kinds the line/column info is saved in a dedicated field PRstNode.loc.

Fixes bug 8 from #17340 .

Araq · 2021-06-29T09:20:51Z

lib/packages/docutils/rstast.nim

@@ -77,6 +77,11 @@ type

  PRstNode* = ref RstNode    ## an RST node
  RstNodeSeq* = seq[PRstNode]
+  PRstLocation* = ref object ## location inside a file
+    filename*: string


I would try to reuse the TLineInfo mechanism.

see also NimLineInfo refs nim-lang/RFCs#387

lib/packages/docutils/rst.nim

compiler/docgen.nim

compiler/lineinfos.nim

lib/packages/docutils/rst.nim

timotheecour · 2021-07-06T23:28:06Z

lib/packages/docutils/rst.nim

+  if fid.int < files.len:
+    result = files[fid.int]
+  else:
+    result = "input"


what is input used for? is that same as compiler/msgs.nim:177:3: commandLineDesc* = "command line" ?

just a fallback value, it should not happen. Alternatively a ValueError can be raised here.

if it shouldn't happen than maybe use doAssert? (ie indicates program logic bug, not user error)

OK, switched to doAssert.

... and doAssert caught a bug :-)

tests/stdlib/trst.nim

timotheecour · 2021-07-07T00:10:38Z

tests/stdlib/trst.nim

+          rnRef
+            rnLeaf  'brokenLink'
+      """)
+    removeFile("other.rst")


not DRY

bad style to write in the source tree and with a generic name (eg what if you ^C testament and are left with un-clean git status, or worse, it overwrites another test file)

instead use either:

import stdtest/specialpaths const tmpFile = buildDir / "D20210706T170748.rst"

or createTempFile from std/tempfiles

I copy pasted it from this very file. Let us postpone it for trst.nim refactoring PR.

lib/packages/docutils/rstast.nim

compiler/lineinfos.nim

Co-authored-by: Timothee Cour <[email protected]>

also display errors/warnings properly when footnote references are from different files

compiler/lineinfos.nim

timotheecour · 2021-07-15T20:53:58Z

tests/stdlib/trstgen.nim

-    doAssert "<h2 id=\"another2\">Another2</h2>" in output10
-    doAssert "<h3 id=\"more3\"><center>More3</center></h3>" in output10
-
+    doAssert "<h1 id=\"level1\"><center>Level1</center></h1>" in output


how about using triple quoted strings instead so that it's easier to copy paste without quoting, eg:

doAssert "<h1 id=\"level1\"><center>Level1</center></h1>" in output => doAssert """<h1 id="level1"><center>Level1</center></h1>""" in output

can be done in separate PR though

it could even be written as:

for line in """ <h1 id="level1"><center>Level1</center></h1> <h2 id="level2">Level2</h2> ... """: doAssert line in output

if you want to maintain ordering there's also testutils.greedyOrderedSubsetLines

timotheecour · 2021-07-15T21:13:40Z

tests/stdlib/trstgen.nim

+    rstGenera.renderRstToOut(rst, output)
+    doAssert rstGenera.meta[metaTitle] == "Title0"
+    doAssert rstGenera.meta[metaSubTitle] == ""
+    doAssert output ==


use multiline literal instead (can be done in separate PR)

timotheecour · 2021-07-15T21:14:49Z

tests/stdlib/trstgen.nim

@@ -628,7 +630,7 @@ let x = 1
    let p2 = """<p>Par2 <tt class="docutils literal"><span class="pre">value2</span></tt>.</p>"""
    let p3 = """<p>Par3 <tt class="docutils literal"><span class="pre">""" & id"value3" & "</span></tt>.</p>"
    let p4 = """<p>Par4 <tt class="docutils literal"><span class="pre">value4</span></tt>.</p>"""
-    let expected = p1 & p2 & "\n" & p3 & "\n" & p4 & "\n"
+    let expected = p1 & p2 & "\n" & p3 & "\n" & p4


ditto, multiline literal is much clearer than concatenating like this

timotheecour · 2021-07-15T21:23:25Z

tests/stdlib/trstgen.nim

                     "extra arguments were given to number-lines: ' let a = 1'")
    check "" == output

+  test "code-block warning":


not a blocker because the situation after this PR improves in all cases, but the line numbers and columns are off by ~1 for nested doc comments, eg:

before PR: all lines/cols are 1 (useless)
after PR: main3 is correct, main1 and main2 are off by ~1

proc main1*()= ##[ .. code:: Nim :unsupportedField: anything .. code:: unsupportedLang anything ```anotherLang someCode ``` ]## discard proc main2*()= ##[ .. code:: Nim :unsupportedField: anything .. code:: unsupportedLang anything ```anotherLang someCode ``` ]## discard proc main3*()= ## .. code:: Nim ## :unsupportedField: anything ## .. code:: unsupportedLang ## anything ## ```anotherLang ## someCode ## ``` discard

yes, line info is saved only for first # of ##, not for real comment inside.
We talked about this problem in timotheecour#658

lib/packages/docutils/rstast.nim

lib/packages/docutils/rstgen.nim

lib/packages/docutils/rst.nim

timotheecour · 2021-07-15T21:37:43Z

lib/packages/docutils/rst.nim

    footnotes: seq[FootnoteSubst] # correspondence b/w footnote label,
                                  # number, order of occurrence
    msgHandler: MsgHandler      # How to handle errors.
    findFile: FindFileHandler   # How to find files.
-    filename: string
+    filenames*: seq[string]     # map FileIndex -> file name (for storing


if it's a map, shouldn't this be a Table? this would also simplify setCurrFilename , getFilename etc

I'm fine with both ways, readability is the same.

seq[string] should be faster for typical cases when there is only 1 file

Table will be faster when there is >~ 10 files included

I don't think performance is any concern here (not a bottleneck), but it seemed to me Table would simplify code, but again, can be deferred to future work

lib/packages/docutils/rst.nim

timotheecour

LGTM, remaining comments are not critical and can be addressed in a followup PR so that this PR doesn't bitrot

Co-authored-by: Timothee Cour <[email protected]>

Error: cannot prove that it's safe to initialize 'info' with the runtime value for the discriminator 'kind'

Co-authored-by: Timothee Cour <[email protected]>

timotheecour

still LGTM (remaing comments can be addressed in followup PR)

narimiran · 2021-07-19T15:58:52Z

@a-mr I've pushed two commits, which remove imports of compiler/lineinfos (per @Araq's request). I wanted to let you know, so if you decide to push something more, to pull first.

* rst: add missing line/column info for some warnings * add workaround * use TLineInfo/FileIndex for storing file names * fix blank lines in include file (rm harmful strip) * don't use ref TLineInfo * return `hasToc` as output parameter for uniformity * Update compiler/docgen.nim Co-authored-by: Timothee Cour <[email protected]> * Update compiler/docgen.nim Co-authored-by: Timothee Cour <[email protected]> * Update lib/packages/docutils/rst.nim Co-authored-by: Timothee Cour <[email protected]> * address review - stylistic things * Update compiler/docgen.nim Co-authored-by: Timothee Cour <[email protected]> * unify RST warnings/errors names * doAssert + minor name change * fix a bug caught by doAssert * apply strbasics.strip to final HTML/Latex * rm redundant filename * fix test after rebase * delete `order` from rnFootnoteRef, also display errors/warnings properly when footnote references are from different files * Update compiler/lineinfos.nim Co-authored-by: Timothee Cour <[email protected]> * Update lib/packages/docutils/rstast.nim Co-authored-by: Timothee Cour <[email protected]> * Update lib/packages/docutils/rstast.nim Co-authored-by: Timothee Cour <[email protected]> * Update lib/packages/docutils/rstast.nim Co-authored-by: Timothee Cour <[email protected]> * revert because of error: Error: cannot prove that it's safe to initialize 'info' with the runtime value for the discriminator 'kind' * Update lib/packages/docutils/rstgen.nim Co-authored-by: Timothee Cour <[email protected]> * apply suggestion * Update lib/packages/docutils/rst.nim Co-authored-by: Timothee Cour <[email protected]> * add Table for string->file name mapping * do not import compiler/lineinfos * fix ambiguous calls Co-authored-by: Timothee Cour <[email protected]> Co-authored-by: narimiran <[email protected]>

Araq reviewed Jun 29, 2021

View reviewed changes

Araq reviewed Jul 3, 2021

View reviewed changes

lib/packages/docutils/rst.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 6, 2021

View reviewed changes

compiler/docgen.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 6, 2021

View reviewed changes

compiler/docgen.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 6, 2021

View reviewed changes

compiler/docgen.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 6, 2021

View reviewed changes

compiler/lineinfos.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 6, 2021

View reviewed changes

lib/packages/docutils/rst.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 6, 2021

View reviewed changes

lib/packages/docutils/rst.nim Show resolved Hide resolved

timotheecour reviewed Jul 6, 2021

View reviewed changes

timotheecour reviewed Jul 7, 2021

View reviewed changes

tests/stdlib/trst.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 7, 2021

View reviewed changes

lib/packages/docutils/rstast.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 7, 2021

View reviewed changes

lib/packages/docutils/rstast.nim Outdated Show resolved Hide resolved

timotheecour reviewed Jul 7, 2021

View reviewed changes

compiler/lineinfos.nim Outdated Show resolved Hide resolved

a-mr and others added 16 commits July 11, 2021 15:42

rst: add missing line/column info for some warnings

f7fa825

add workaround

e64c7fb

use TLineInfo/FileIndex for storing file names

af49d7b

fix blank lines in include file (rm harmful strip)

1b31d61

don't use ref TLineInfo

80cafde

return hasToc as output parameter for uniformity

ce5de16

Update compiler/docgen.nim

ed47dbd

Co-authored-by: Timothee Cour <[email protected]>

Update compiler/docgen.nim

cf27ed9

Co-authored-by: Timothee Cour <[email protected]>

Update lib/packages/docutils/rst.nim

19fa19e

Co-authored-by: Timothee Cour <[email protected]>

address review - stylistic things

b361cd3

Update compiler/docgen.nim

4d20a5f

Co-authored-by: Timothee Cour <[email protected]>

unify RST warnings/errors names

d13c14d

doAssert + minor name change

98b4d0d

fix a bug caught by doAssert

1e633aa

apply strbasics.strip to final HTML/Latex

f7bb08c

rm redundant filename

ae2083b

a-mr force-pushed the rst-warnings-missing-lineinfo branch from fe74530 to e8ede97 Compare July 11, 2021 12:47

delete order from rnFootnoteRef,

f784222

also display errors/warnings properly when footnote references are from different files