New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

templates expand doc comments as documentation of other procedures #9432

Closed
mjoud opened this Issue Oct 18, 2018 · 12 comments

Comments

Projects
None yet
9 participants
@mjoud
Copy link

mjoud commented Oct 18, 2018

In the documentation for unittest, I noted that defaultConsoleFormatter gets a docstring from system.`>`*(x, y: untyped). This is also seen in documentation for other modules (grep "is greater" doc/html/*.html). Reproduced locally with koch docs but not with nim doc unittest.nim directly.

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Oct 18, 2018

I have seen this before, but forgot to report. I have not yet come up with a minimal recipe to reproduce that.

That system.>*(x, y: untyped) gets auto-inserted for procs missing a docstring.

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Oct 18, 2018

OK, I found a super-minimal example!

echo 'proc isValid*[T](x: T): bool = x.len > 0' > foo.nim
nim doc foo.nim

This generates:

image

It's because of x.len > 0 in that doc-string-less proc body! Huh

@Araq

@LemonBoy

This comment has been minimized.

Copy link
Collaborator

LemonBoy commented Oct 18, 2018

It's not that odd if you dig in the docgen pipeline. Let's take defaultConsoleFormatter as an example, the expanded AST is:

proc defaultConsoleFormatter(): ConsoleOutputFormatter {.raises: [],
    tags: [ReadEnvEffect].} =
  var envOutLvl = string(getEnv("NIMTEST_OUTPUT_LVL", ""))
  var colorOutput = isatty(stdout)
  if existsEnv("NIMTEST_COLOR"):
    let colorEnv = getEnv("NIMTEST_COLOR", "")
    if colorEnv == "never":
      colorOutput = false
    elif colorEnv == "always":
      colorOutput = true
  elif existsEnv("NIMTEST_NO_COLOR"):
    colorOutput = false
  var outputLevel = PRINT_ALL
  if (
    ## "is greater" operator. This is the same as ``y < x``.
    0 < len(envOutLvl)):
    for opt in countup(low(OutputLevel), high(OutputLevel), 1):
      if $opt == envOutLvl:
        outputLevel = opt
        break
  result = newConsoleOutputFormatter(outputLevel, colorOutput)

And docgen.nim's genRecComment picks the stray and only comment as docstring.

@skilchen

This comment has been minimized.

Copy link
Contributor

skilchen commented Oct 19, 2018

This oddity was introduced by:

0d68ef9f1135e934e6b7af4d69fa6e4e5f61d15b is the first bad commit
commit 0d68ef9f1135e934e6b7af4d69fa6e4e5f61d15b
Author: Andreas Rumpf <rumpf_a@web.de>
Date:   Mon Sep 3 00:40:56 2018 +0200

  runnableExample: put each example to its own file; fixes #7285

@LemonBoy

This comment has been minimized.

Copy link
Collaborator

LemonBoy commented Oct 20, 2018

There's more, here a stray paragraph is lifted under the "Module System" header

Expands operating GC stack range to theStackBottom. Does nothing if current stack bottom is already lower than theStackBottom.

@skilchen

This comment has been minimized.

Copy link
Contributor

skilchen commented Oct 20, 2018

There's more, here a stray paragraph is lifted under the "Module System" header

Yes, but this is a simple documentation error. The indentation level of the doc comment

Expands operating GC stack range to theStackBottom. Does nothing if current stack bottom is already lower than theStackBottom.

is not correct.

More documentation problems become visible if you do nim doc system.nim. Then the stray paragraph becomes:

Expands operating GC stack range to theStackBottom. Does nothing if current stack bottom is already lower than theStackBottom.Compilerprocs for strings that do not depend on the string implementation.helpers used system.nim and other modules, avoids code duplication while also minimizing symbols exposed in system.nim

The problem here is that toplevel doc-comments in included files get merged in this stray paragraph. Included modules like strmantle.nim and helpers.nim probably shouldn't have any top-level doc-comments.

The docgen oddity itself is a consequence of the fact that the shouldProcess template gives the wrong answer if used outside of closeImpl here and here.

My quick fix would be to remove these two if shouldProcess(g): checks. Without them the docgen oddity disappears. But I have really no idea, if @Araq had good reasons to insert them there.

@krux02

This comment has been minimized.

Copy link
Contributor

krux02 commented Oct 20, 2018

From the issues list, this title provides no information abut the problem. Please provide a better title.

@krux02 krux02 changed the title docgen oddity templates expand doc comments as documentation of other procedures Oct 20, 2018

@Araq Araq added the Showstopper label Oct 22, 2018

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Oct 23, 2018

see also example from #9469 : doc for toSeq shows doc of accumulateResult, see https://nim-lang.github.io/Nim/nre.html#%24%2CRegexMatch

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Oct 27, 2018

@narimiran

This comment has been minimized.

Copy link
Member

narimiran commented Oct 27, 2018

Semi-related:

I have run nim doc on my file containing several types and functions, and for one of those functions (called findNodes), which has no docstring at all, I got the following text in html:

An alias for countup(a, b, 1).

This is my function:

func findNodes*(b: Beam): seq[Point] =
  let
    n = float(b.n)
    dx = (b.p2.x - b.p1.x) / n
    dy = (b.p2.y - b.p1.y) / n
  for i in 0 .. b.n:
    result.add((b.p1.x + float(i)*dx, b.p1.y + float(i)*dy))

EDIT

I've put some docstring in that function, and that is correctly generated. But now, the function above this one got this:

Counts from ordinal value a up to b (inclusive) with the given step count. S, T may be any ordinal type, step may only be positive. Note: This fails to count to high(int) if T = int for efficiency reasons.
@Swader

This comment has been minimized.

Copy link

Swader commented Oct 27, 2018

Can confirm, encountering this.

proc update*(ctx: var Sha2Context, data: ptr byte, inlen: uint) =
  var pos = 0'u
  var length = inlen

  when ctx.bsize == 64:
    while length > 0'u:
      let offset = uint(ctx.count[0] and 0x3F)
      let size = min(64'u - offset, length)
      copyMem(addr(ctx.buffer[offset]),
              cast[pointer](cast[uint](data) + pos), size)
      pos = pos + size
      length = length - size
      ctx.count[0] += uint32(size)
      if ctx.count[0] < uint32(size):
        ctx.count[1] += 1'u32
      if (ctx.count[0] and 0x3F) == 0:
        sha256Transform(ctx.state, addr(ctx.buffer[0]))
  elif ctx.bsize == 128:
    while length > 0'u:
      let offset = uint(ctx.count[0] and 0x7F)
      let size = min(128'u - offset, length)
      copyMem(addr(ctx.buffer[offset]),
              cast[pointer](cast[uint](data) + pos), size)
      pos = pos + size
      length = length - size
      ctx.count[0] += uint64(size)
      if ctx.count[0] < uint64(size):
        ctx.count[1] += 1'u64
      if (ctx.count[0] and 0x7F) == 0:
        sha512Transform(ctx.state, addr(ctx.buffer[0]))

proc update*[T: bchar](ctx: var Sha2Context, data: openarray[T]) =
  if len(data) == 0:
    ctx.update(nil, 0)
  else:
    ctx.update(cast[ptr byte](unsafeAddr data[0]), uint(len(data)))

# ...


proc finish*(ctx: var Sha2Context): MDigest[ctx.bits] =
  discard finish(ctx, cast[ptr byte](addr result.data[0]),
                 uint(len(result.data)))

proc finish*[T: bchar](ctx: var Sha2Context, data: var openarray[T]) =
  assert(uint(len(data)) >= ctx.sizeDigest)
  discard ctx.finish(cast[ptr byte](addr data[0]), uint(len(data)))

This generates the following:

screen shot 2018-10-27 at 18 46 01

In other words, it's random which string will appear where.

@narimiran

This comment has been minimized.

Copy link
Member

narimiran commented Oct 30, 2018

The example from the official documentation:

options.isSome has following docstring: unequals operator. This is a shorthand for not (x == y).

@timotheecour timotheecour referenced this issue Oct 30, 2018

Closed

[WIP] [doc] miscellaneous issues #8477

6 of 21 tasks complete

@Araq Araq closed this in 48f73aa Oct 30, 2018

narimiran added a commit to narimiran/Nim that referenced this issue Oct 31, 2018

narimiran added a commit to narimiran/Nim that referenced this issue Nov 1, 2018

narimiran added a commit that referenced this issue Nov 1, 2018

narimiran added a commit that referenced this issue Nov 3, 2018

docgen: fixes #9432 [backport]
(cherry picked from commit 48f73aa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment