docgen: implement doc link resolution in current module

[a-mr]

First part of nim-lang/RFCs#125, smart doc links. Only local links (inside a given current module) are supported here.

It works by adding additional anchors from docgen.nim.

An example (modified from nim-lang/RFCs#125):

type SortOrder* = enum k0, k1
proc sortOrder*() = discard
proc foo*(a: int, sortOrder: SortOrder) = discard
proc foo*[T](a: int, sortOrder: SortOrder, c: T) = discard
proc foo*() =
  ## See: SortOrder_ (will link to type)
  ## See: sortOrder_ (will link to proc)
  ## See: foo_ (will link to the whole group of procs 'foo')
  ## See: `foo(int, SortOrder, T)`_ (will link to 2nd overload)
  ## See: `foo()`_ (will link to itself)                                                                                

Implementation throws warnings in case of ambiguities. In this case one needs to specify the reference more, e.g. `iterator split`_ or `proc split`_ instead of just split_.

A syntax note:

• one-word reference can be just suffixed with _ without using enclosing backticks `
• multiple-words references should be with backticks: `...`_

TODO:

• make it more WYSIWYG: 1) references like foo_ and `proc foo`_ should display as foo proc 2) `foo(par1, par2)`_ as foo (par1, par2) proc 3) `foo(Type1, Type2)`_ as foo (Type1, Type2) proc
• when there is only 1 overload: show full name with function parameters (even if it's short-style link like foo_) UPDATE: only from tooltip on hover
• when there is more than 1 overload for short-style links: show some descriptive link like proc foo (2 overloads)
• add more tests outlined by @timotheecour
• add test with inclusion of a module
• check it in Latex backend
• check some edge cases like the order dependent example from RFC: currently not working:
  overloads with type constraints on generic parameters

cc @timotheecour

[timotheecour]

Excellent!
seems to work well and is intuitive. After playing with it a bit I found this bug (reduced from a weird behavior i was seeing after modifying compileOption links):

bug 1

when defined case1: # D20210808T110300
  proc foo*(option: string): bool =
    ## * `foo <#foo,string,string>`_ (broken link, not expected to work)

  proc foo2*(option, arg: string): bool =
    ## * foo_ (BUG: doesn't show link to `foo`)

  proc foo3*(option: string): bool =
    ## * `foo3 <#foo,string,string>`_ (BUG2: doesn't link)

  proc foo3*(option, arg: string): bool =
    ## * foo3_ (BUG3: doesn't link)

bug 2

when defined case2: # D20210808T110332
  proc compileOption5*(option: string): bool {.
    magic: "CompileOption", noSideEffect.} =
    ## See also:
    ## * `compileOption5 <#compileOption5,string,string>`_ for enum options

  proc compileOption5*(option, arg: string): bool {.
    magic: "CompileOptionArg", noSideEffect.} =
    ## See also:
    ## * compileOption5_ (BUG: should link to group)

bug 3

when defined case3: # D20210808T110352
  proc foo*(option: string): bool =
    ## See also:
    ## * `foo_ <#foo,string,string>`_  (BUG: shows as ``foo_``)

  proc foo*(option, arg: string): bool =
    ## See also:
    ## * foo_ (BUG: should link to group)

they're probably all related

[timotheecour]

links in lib/system_overview.rst (included from system.nim) work as intended, which is great; can you add a test to ensure links in includes keep working?

[a-mr]

ok, done, thank you for the idea!

[timotheecour]

can you add tests with operators? eg:

## see `[]`_
## see `[]=`_

[a-mr]

done, tests added.

[timotheecour]

can you add tests with comma separated links, eg:

## see fn1_, fn2_, `fn3`_, `fn4`_

[a-mr]

done.

[a-mr]

@timotheecour

In all those bugs you hit a (mis)feature of RST. A link like `foo <site#anchor>`_ generates a link alias foo. When you use foo_ it selects this manually set variant because it has priority. So this is as expected. In the new version a warning is emitted about this conflict with line info where all these variants were originally defined. May be I need to go even further and add to the warnings a phrase that these link aliases were implicitly generated.

Also note that handling of these links `foo <...>`_ were not changed in this PR.
Generally speaking, they are manual and not checked and when you are using them "you are on your own". After this PR and its follow-ups we are not supposed to even use these manual links for linking to symbols, though they will be still relevant for referencing external sites and whole Nim modules (though we can add syntax & automatic resolution even to the latter case at some stage too).

[a-mr]

Marked as WIP as it's a big change and there is no need to rush.

Current list of TODOs:

• when there is only 1 overload: Don't generate groups (it's better to generate groups anyway to avoid breaking external links if e.g. number of overloads goes 2 -> 1) and show full name with function parameters (even if it's short-style link like foo_)
• when there is more than 1 overload for short-style links: show some descriptive link like proc foo (2 overloads)
• add more tests outlined by @timotheecour
• check it in Latex backend
• check some edge cases like the order dependent example from RFC:
  currently not working

      ## ref fn_ (OK) and `fn(T)`_ (COLLISION) and `fn(T_2)`_ (NOT FOUND)
      proc fn*[T: SomeFloat](a: T)=discard
      proc fn*[T: SomeInteger](a: T)=discard

[timotheecour]

n all those bugs you hit a (mis)feature of RST. A link like foo <site#anchor>_ generates a link alias foo. When you use foo_ it selects this manually set variant because it has priority. So this is as expected

I see, i thought it would only specify the display text as foo, didn't realize it'd also create a link alias foo; is that something we want to preserve? but fine, if we stick to the "new form" this isn't a problem (but it is a problem if mixing new form and old form as shown in #18642 (comment)

What does it mean for existing moduels that use the existing syntax foo_ <#foo,string,string>_ ? can we upgrade them 1 by one to use instead foo_ or we also need to make sure other modules referencing it use foo_?

In the new version a warning is emitted about this conflict with line info where all these variants were originally defined.

it helps, thanks

May be I need to go even further and add to the warnings a phrase that these link aliases were implicitly generated.

maybe

[a-mr]

What does it mean for existing moduels that use the existing syntax foo_ <#foo,string,string>_ ? can we upgrade them 1 by one to use instead foo_ or we also need to make sure other modules referencing it use foo_?

Yes, we can and actually updating a module as whole eliminates a possibility of collision. Also warning will necessarily be generated so it will be impossibly to forget anything. And besides current links are almost always input as `parseCmdLine proc <#parseCmdLine,string>`_, w.r.t. to our new syntax it has a reverse of parseCmdLine and proc hence there is no collision.

And problems when referencing between modules are impossible because the format of anchors has not changed. All in all, there is almost no risk in this regard.

[Varriount]

Just want to say this is very impressive! Thanks for your work!

[timotheecour]

would this work to avoid quoting?

## See `proc \`$\`(int)`_
## See ``proc `$`(int)``_

the 2nd is more readable because it's easier to copy paste from a declaration

[a-mr]

Not now :-(

In original RST it would not even require escaping, but our implementation was tweaked in favor of Markdown-like inline markup rules, which allow to put the ending backtick anywhere and also put starting backtick anywhere. So with single-backtick syntax we are bound to escaping of backticks.

Regarding double-backticks: the syntax is sane and I like it but it's not standard and we do not support it, though it would probably be easy to implement. I think anything like this will be left for future PRs anyway to avoid conflation here.

I agree it's ugly, that's why to make it less painful I added possibility to ref. operators the short way `$`_ (instead of a long way like

`\`$\``_

)
but for any compound anchors one still has to use escaping:

`proc \`$\``_

[timotheecour]

can you add more tests especially for edge cases, eg:

## See `$`_, `[]=`_
## See `'big`_ (for user defined literals)
## See `foo_bar_` (for `foo_bar`)

[a-mr]

added. Thanks for testcases, they helped to catch some bugs.

[Varriount]

Is this still a WIP, or is it ready for a full review?

[a-mr]

Is this still a WIP, or is it ready for a full review?

WIP.

The implementation can still change significantly to add more flexibility ("WYSIWIG") and fix a bug with type constraints.
Also the current scheme of adding aliases may cause memory consumption, I need to do a simple benchmark at least.

[Varriount]

Ok, I've converted this PR to a draft - when it's ready for a full review, you can convert it back and modify the title to remove the "WIP".

[timotheecour]

maybe also mention that it works for nim files including an rst file?

[a-mr]

ok, mentioned.

[timotheecour]

maybe mention this is the preferred form because:

• otherwise ppl will get confused as to which form to use by default
• simpler = better
• more robust to API changes (eg proc => template, adding optional params etc)

(EDIT: i see you mention it later, but might as well mention it here too)

[a-mr]

i think one suggestion is enough. Besides some people may like to provide parameters to procs — I would prefer that in most cases also.

[timotheecour]

that's a bit nasty, can we avoid this?
this should work and not be ambiguous:

`func []`_ or, in detail:
`func [][T](x: openArray[T]): T`_

[a-mr]

yes, at first glance I don't see any principal obstacle. It may well be a bug in my working copy :-)

[a-mr]

it's working now.

[timotheecour]

how about adding something like this:

Furthermore, the simplest form (binarySearch_) is usually preferable because it will generate links that remain valid even if underlying API is modified (e.g. via added optional params, or changing the routine kind).

[timotheecour]

need => needs

[a-mr]

fixed.

[timotheecour]

can you add a TODO for future work somewhere? fixing this would be a nice improvement

[a-mr]

the current implementation does not use Nim compiler facilities to parse all these links, so to fix that for all possible cases would be impossible. The only solution would be to re-write this implementation which is not in my plans.

[timotheecour]

do we need both? honest question; i feel like just using the no backticks form without giving an option would be best, and simplest to use.

[a-mr]

yes, we do need both. One can want for the link to look syntactically correct (e.g. for educational reasons).

[timotheecour]

the ideal link would be a copy-paste of the declaration, possible non-ambiguous ways could be:

## see ` func `$`(a: int): string `_
## see ``func `$`(a: int): string``_
## see ```func `$`(a: int): string```_

(at expense of rst spec, which we already violate)

then we'd need only 1 way, and avoid having to give a choice with 2 sub-optimal ways

[a-mr]

I see, it's roughly the same as nim-lang/RFCs#355.

this syntax is also not perfect, it does not allow to start from a backtick. While our current syntax does:

`\`$\`(a: int)`_

(Correction: one can input

`` `$`(a: int) ``_

though, it's just that further tweaking of RST inline markup rules in favor of Markdown is needed hear.)

But I agree that such syntax would be good... It's just not among first priorities...

[a-mr]

@Araq @timotheecour @narimiran

What are you plans for review & conclusion?
Do you have any time during a month? I want to continue working on docgen but this PR kind of hinders me because quite a bit of refactoring was done here, it would be unpleasant to merge that afterwards. If you want we can have a remote session e.g. by Skype discussing this PR.

The current implementation fixes all the problems that were discovered.
It's different from the original code. Original one made a string normalization for both generated anchor and reference and did a simple hash table search. The new one parses a reference ( in dochelpers.nim) so that all parameters (including generics) can be compared with an anchor. Originally I thought about re-using parser code from the compiler but abandoned this idea because:

• only signatures need to be supported in rst.nim
• also signatures that don't abide by Nim rules are supported, e.g. one can write walkDir(d) iterator — without type and with iterator at the end. This originated from the existing practice of naming links in Nim's repo.

[Araq]

Sorry for the delay, we're reviewing it this week.

[narimiran]

It is hard to do a detailed review of a large PR, but I have tested the changes proposed in this PR by building the docs for strutils (while changing the links to the new short form) and I like the new behavior, e.g. grouping of split funcs when you link to (one of) them.

I'm merging this now; we have plenty of time until next stable version (1.8.0) to iron out any quirks/bugs that might come up which we haven't considered now.

[a-mr]

OK, thx, so I will try to convert all links e.g. in os.nim.

Also the next part of work will be cross-module link support. I hope it will be ready by 1.8.0 also.