docgen: move to shared RST state (fix #16990) #18256

a-mr · 2021-06-13T17:03:44Z

Previously each doc comment inside a .nim file was processed as a separate document.

This PR:

A. makes doc comments related through a common (per-file) RST shared state. (Strictly speaking this would be enough for fixing #16990)
B. splits RST processing into 2 phases:

pass 1 where string doc comment are converted to RST AST PRstNode
pass 2 (finishGenerateDoc), which traverses RST AST and performs final resolution (headings + links + footnotes + RST substitutions) converting PRstNode to HTML or Latex string

B. is done by saving PRstNode of any doc comment to a data structure after pass 1.

So point B. makes .nim doc comments feel like a single document. This style becomes possible:

## Ref lnk_.
##
## See lnk_ and [1]_ for |nimlang|.

proc f*() =
  ## See again lnk_ and [1]_ for |nimlang|.
  discard

## .. _lnk: http://example.org/
## .. [1] Footnote 1.
## .. |nimlang| replace:: Nim programming language

This PR aids in implementing smart links nim-lang/RFCs#125, because previously doc comments were converted immediately, that is before the whole .nim AST tree were traversed and hence the list of all local symbols (procs etc) were not yet known. Now finishGenerateDoc will be the right place to resolve all the links.

cc @timotheecour

timotheecour · 2021-06-13T17:27:13Z

just a side-note, @a-mr, please write fix #16990 in the PR body (in title is useful but not enough, and github doesn't recognize fixing #16990) so that github cross-references it properly (eg so that it'll show "may be fixed by ...` in that issue); as noted in contributing.html

compiler/main.nim

timotheecour · 2021-06-13T17:38:40Z

lib/packages/docutils/rst.nim


-  PSharedState = ref SharedState
+  RstSharedState* = ref SharedState


don't violate the convention type PFoo = ref Foo though, if you do that, then also rename SharedState to RstSharedState, ditto with newSharedState => newRstSharedState (not newSharedStateRst)

(if you want, you can make a separate PR with just those renames maybe that can be reviewed fast; looks like those symbols are all private anyways)

ok, ok, i don't argue. Done.

don't violate the convention type PFoo = ref Foo though

That's an old and outdated convention.

timotheecour · 2021-06-13T17:41:46Z

lib/packages/docutils/rst.nim

+proc newSharedStateRst*(options: RstParseOptions,
+                        filename: string,
+                        findFile: FindFileHandler,
+                        msgHandler: MsgHandler): RstSharedState =
  new(result)


in this or (preferably i guess) separate cleanup PR:
result = RstSharedState(a1: a1, a2: a2, ...)

and these are not needed anymore (relic from the past):
result.subs = @[]

timotheecour · 2021-06-13T17:45:35Z

lib/packages/docutils/rst.nim

                             p.col + currentTok(p).col, msgKind, arg)

+proc rstMessage(s: RstSharedState, msgKind: MsgKind, arg: string) =
+  ## Handle message `arg`. Its position can not be localized at the moment.


doc comment unclear, can you try to explain can not be localized better in the doc comment?

rephrased it.

timotheecour · 2021-06-13T17:47:25Z

lib/packages/docutils/rst.nim

  var key = addNodes(n)
  # the spec says: if no exact match, try one without case distinction:
-  for i in countup(0, high(p.s.subs)):
-    if key == p.s.subs[i].key:
+  for i in countup(0, high(s.subs)):


it's ok to cleanup lines that you are editing; 0..high(s.subs) is shorter and better

agree but let us to do it in follow-up

lib/packages/docutils/rst.nim

timotheecour · 2021-06-13T18:00:11Z

tests/stdlib/trstgen.nim

@@ -1034,7 +1034,8 @@ Test1
      """
    var warnings8 = new seq[string]
    let output8 = input8.toHtml(warnings=warnings8)
-    check(warnings8[] == @["input(4, 1) Warning: unknown substitution " &
+    # TODO: the line 1 is arbitrary because reference lines are not preserved


is that a regression or was 4 meaningless/wrong? (i know that line numbers were often wrong in nim rst2html)

not a regression: it's wrong in both cases. Before it was always last line of text, after it's always first line. The problem is that line/column info has never been preserved in any references :-(. We need to add it to (P)RstNode, a good task for one of future PRs.

timotheecour · 2021-06-13T18:06:10Z

compiler/docgen.nim

+    case isRst: bool
+    of true:
+      rst: PRstNode
+    of false:


add a comment indicating what it corresponds to if isRst is false

compiler/docgen.nim

timotheecour · 2021-06-13T18:07:52Z

compiler/docgen.nim

+  Item = object        ## Any item in documentation, e.g. type or
+                       ## proc entry. Configuration variable ``doc.item``
+                       ## is used for its HTML rendering.
+    descRst: ItemPre     ## Description of the item (may contain runnable


call a cat a cat (runnableExamples), more searchable

compiler/docgen.nim

timotheecour · 2021-06-13T18:20:05Z

compiler/docgen.nim

  if not isVisible(d, nameNode): return
  var
    name = getName(d, nameNode)
    comm = genRecComment(d, n)
    r: TSrcGen
  initTokRender(r, n, {renderNoBody, renderNoComments, renderDocComments})
-  result = %{ "name": %name, "type": %($k), "line": %n.info.line.int,
-                 "col": %n.info.col}
+  result.json = %{ "name": %name, "type": %($k), "line": %n.info.line.int,


in future work, IMO we should use jsonutils here, doing a similar simplification as was done in #18100, which allows more type-safety (via a type that represents the spec of what's serialized, making deserialization + serialization easier)

compiler/docgen.nim

timotheecour · 2021-06-13T18:24:34Z

compiler/docgen.nim

@@ -1357,6 +1434,7 @@ proc commandRst2TeX*(cache: IdentCache, conf: ConfigRef) =
  commandRstAux(cache, conf, conf.projectFull, TexExt)

 proc commandJson*(cache: IdentCache, conf: ConfigRef) =
+  ## implementation of a deprecated jsondoc0 command


then should we rename commandJson as commandJson0 ? (can be done in future PR)

I thought about deleting this jsondoc0 altogether. It always crashes for me, including stable 1.4 version. Does it work for you?

crashes with options.nim(637, 3) not conf.outFile.isEmpty [AssertionDefect] but that doesn't mean it'd be hard to fix; if this were to be removed, it should be a separate PR though (IIRC the use case is for include files where semcheck don't work; just like nim doc0; it's hacky but ./koch docs relies on at least nim doc0)

may be but i'm not planning on doing any fixes, because i don't understand the use cases of jsondoc/jsondoc0 commands in the 1st place :-/

nim jsondoc use case is for tooling, so that a program can use the json output generated by nim for further processing (eg to support queries, such as find all procs matching a given signature) without having to link against nim or use compiler API, refs https://github.com/nim-lang/Nim/issues?q=is%3Aissue+is%3Aopen+jsondoc.

But yes, defering to future work is good

timotheecour · 2021-06-13T18:38:09Z

compiler/docgen.nim

+      of true:
+        var resolved = resolveSubs(d.sharedState, f.rst)
+        var str: string
+        renderRstToOut(d[], resolved, str)


can we make this work? (ie, make sure that renderRstToOut doesn't clear its 3rd arg but insteads just appeands, as is good practice)
renderRstToOut(d[], resolved, result)

yes, well spotted, thanks.

timotheecour · 2021-06-13T18:51:19Z

@a-mr I'm inclined to accept it as-is, I tested it locally and went through everything and the comments I have can be addressed in a followup cleanup PR (to avoid making this PR bitrot for too long)
rst: runnableExamples affects rst underline headings #16990 now does work but a test case can be added in followup PR
it wasn't mentioned in PR description, but IIUC this also helps with nim jsondoc ? is that being tested?

timotheecour

LGTM, remaining points can be addressed in followup PR

a-mr · 2021-06-13T19:53:47Z

this also helps with nim jsondoc?

I tested jsondoc manually, yes, it works as expected.

As i wrote above jsondoc0 always crashes for me (incl. 1.4).

timotheecour · 2021-06-13T21:16:53Z

does it also fix any of these ?
https://github.com/nim-lang/Nim/issues?q=is%3Aissue+is%3Aopen+jsondoc

lib/packages/docutils/rst.nim

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

* docgen: move to shared RST state (fix nim-lang#16990) * Update lib/packages/docutils/rst.nim Co-authored-by: Andreas Rumpf <rumpf_a@web.de> * Update lib/packages/docutils/rst.nim Co-authored-by: Andreas Rumpf <rumpf_a@web.de> * Update lib/packages/docutils/rst.nim Co-authored-by: Andreas Rumpf <rumpf_a@web.de> * Update compiler/docgen.nim Co-authored-by: Timothee Cour <timothee.cour2@gmail.com> * Update compiler/docgen.nim Co-authored-by: Timothee Cour <timothee.cour2@gmail.com> * Update compiler/docgen.nim Co-authored-by: Timothee Cour <timothee.cour2@gmail.com> * Update lib/packages/docutils/rst.nim Co-authored-by: Timothee Cour <timothee.cour2@gmail.com> * rename `cmdDoc2` to `cmdDoc` * fix (P)RstSharedState convention * new style of initialization * misc suggestions * 1 more rename * fix a regression Co-authored-by: Andreas Rumpf <rumpf_a@web.de> Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

docgen: move to shared RST state (fix nim-lang#16990)

9408713