allow runnableExamples in forward procs

timotheecour commented 3 years ago

This issue came up here: https://github.com/nim-lang/Nim/pull/16484/files#r550295803, and prevents adding runnableExamples in procs like cmpMem which are forward declared without going through complex contorsions.

Example 1

proc main*() =
  ## comment
  runnableExamples: discard
proc main() = discard # Error: redefinition of 'main'; previous declaration here:

Example 2

proc main*()
  ## comment
  runnableExamples: discard # Error: invalid indentation

Example 3

defined(nimdoc) has other issues, eg:

when defined(nimdoc):
  proc main*(): int =
    ## comment
    runnableExamples: discard
when not defined(nimdoc):
  proc main*(): int = result = 2
static: doAssert main() == 2 # would fail

Expected Output

There should be a simple way to add runnableExamples in a forward declared proc.

If we make Example 1 work, there's an inherent ambiguity, because a proc with no body (and an =) is valid and returns default value.

If we make Example 2 work, it'd require a parser change (which would need to be backported otherwise stdlib can't use this feature), and this may have other complications.

Example 3 creates more complications and is more complex to use.

Proposed solution

Introduce a pragma {.forward.} which indicates a proc is forward declared, and has 0 ambiguity.

proc main*() {.forward.} =
  ## comment
  runnableExamples: discard
proc main() = discard # works

Additional Information

https://github.com/nim-lang/RFCs/issues/163 is related

EDIT: removed when true: to simplify discussion

disruptek commented 3 years ago

Can you explain why the runnable example needs to appear on the predeclaration?

I'm struggling to understand the purpose of the contortions in the first place... 🤔

timotheecour commented 3 years ago

Can you explain why the runnable example needs to appear on the predeclaration?

because putting it in the definition would be a noop, even with --docInternal

when true:
  proc main*()
    ## comment
  proc main() =
    runnableExamples: echo 12 # not run, not shown in docs
    discard

juancarlospaco commented 3 years ago

Use a RST .. code-block:: block with the :test: thingy ?.

timotheecour commented 3 years ago

Use a RST .. code-block:: block with the :test: thingy ?.

code-block is a relic from the past and needs to disappear (EDIT: at least from nim files):

it's too easy to misuse (i can never remember the exact spacing to use) and doesn't give any warning/error when you misuse it (instead it silently won't run the test)

it has the wrong default, which is doing no compiling/running by default, and is often wrong, eg nim doc -r lib/std/compilesettings.nim doesn't complain even though compileSettingSeq is a typo:

proc querySettingSeq*(setting: MultipleValueSetting): seq[string] {.
compileTime, noSideEffect.} = discard
## Can be used to get a multi-string compile-time option. Example:
##
## .. code-block:: Nim
##   const nimblePaths = compileSettingSeq(MultipleValueSetting.nimblePaths)

it doesn't syntax highlight the code block
it's slow (eg nim rst2html doc/manual.rst takes 18 seconds) because, unlike runnableExamples, there is no test joining

it has many edge cases, eg if you don't set nimcache it fails with a cryptic:

nim doc /pathto/t11617.nim
Error: unhandled exception: Failed to create '/Users/timothee/git_clone/nim/timn/build/nimcache/t11617' [IOError]

it doesn't have many of the features of runnableExamples (eg --doccmd)

Furthermore, there's exactly 0 examples in stdlib using .. code-block:: with :test: at proc scope (again, because it's hard to remember how to use it)

That said, this does work:

when true: # works
  proc main*()
    ## comment 1
    ##
    ## .. code-block:: Nim
    ##    :test: "nim r $1"
    ##   doAssert 1+1 == 2, "zook1"
    ## comment 2
  proc main() = discard

but if you forget a newline before code-block:: Nim or if you use:

    ## .. code-block:: Nim
    ##   :test: "nim r $1"

it'll silently not run the test.

With this RFC you can instead write:

when true:
  proc main*() {.forward.} =
    ## comment 1
    runnableExamples:
      doAssert 1+1 == 2, "zook1"
    ## comment 2
  proc main() = discard

which is objectively better, easier to read/write/edit, allows one-liners (runnableExamples: doAssert 1+1 == 2) etc

disruptek commented 3 years ago

I didn't find your explanation very helpful, so I decided to modify your example until it matched my expectation of what a rational, thinking programmer might actually write:

proc main*()
proc main*() =
  ## comment
  runnableExamples:
    echo 12 # not run, not shown in docs
  echo 14

Note that there are a few differences from your example:

the procedures share the same signature
the procedures are not inside a when statement
the comment appears inside the definition, as does the example
the runnable example is generated by nim doc
both forms of comment are successfully documented

Is there a reason we cannot merely fix the documentation generator or fix the documentation? I guess that's really the question I was trying to ask in the first place, and I apologize if this was confusing.

juancarlospaco commented 3 years ago

~- Should :test: thingy be deprecated then?, looks very broken-ish by the description, -~ ~- I can not even find any use of it on the Nim repo, nor nimble, nor anywhere. -~

ringabout commented 3 years ago

Should :test: thingy be deprecated then?, looks very broken-ish by the description, I can not even find any use of it on the Nim repo, nor nimble, nor anywhere. 🤔

see it in manual.rst and .rst. It is suitable for `.rst` for the time being.

timotheecour commented 3 years ago

decided to modify your example until it matched my expectation of what a rational, thinking programmer might actually write:

A rational, thinking programmer would want to stick to one definition rule to avoid duplication of doc comment and runnableExamples, which is what you'd end up with with your proposal if there are multiple implementations, eg:

proc main*()
when defined(js):
  proc main*() =
    ## comment
    runnableExamples: discard
    impl()
else:
  proc main*() =
    ## comment
    runnableExamples: discard
    impl()

Furthermore, your suggestion doesn't work with memCmp, which was one of the motivations for this RFC; here's a reduced example (the difference with your example being then when block): nim doc -r main => won't run example, won't show doc comments

when not defined(js):
  proc main*()
  proc main*() =
    ## comment
    runnableExamples:
      echo 12 # not run, not shown in docs
    echo 14

whether or not this is a bug and can be fixed, the 1st problem is the bigger one.

disruptek commented 3 years ago

If you don't want to duplicate the comment and the example, wouldn't it be wiser to perform alternative implementations in a template? Then you can be assured that you have a single comment and a single runnable example that works regardless of when gymnastics and correctly follows the main exported API symbol.

timotheecour commented 3 years ago

wouldn't it be wiser to perform alternative implementations in a template?

I don't understand what you're suggesting, please write a working example of how this would look like.

disruptek commented 3 years ago

when true:
  template mainImpl() = echo 3
else:
  template mainImpl() = echo 4
proc main*()
proc main*() =
  ## comment
  runnableExamples:
    main()
  mainImpl()

alaviss commented 3 years ago

@disruptek Your example doesn't scale for bigger procs, esp when generics come into play.

This feature is desirable for people developing cross-platform APIs, where implementations are spread out between multiple files.

disruptek commented 3 years ago

Am I the only one required to defend assertions?

alaviss commented 3 years ago

It's very simple, really.

when true:
  template mainImpl() = echo 3
else:
  template mainImpl() = echo 4
proc main*() =
  ## comment
  runnableExamples:
    main()
  mainImpl()

Declaring a prototype that refers to an implementation. It's almost as if that is the entire point of forward declaration.

And what happen when you start doing overloads?

type SQL = distinct string

when true:
  template fooImpl(a: string) = echo a

  template fooImpl(sql: SQL) =
     doSqlThings(sql)
else:
  template fooImpl(a: string) = echo repr a
  template fooImpl(sql: SQL) = echo string sql

proc foo*(a: string) =
  ## comment
  runnableExamples:
    foo("string")
  fooImpl(a)

proc foo*(sql: SQL) =
  ## comment
  runnableExamples:
    foo(SQL "drop table users;")
  fooImpl(sql)

Yea, it's basically forward declaration with extra steps. Why would anyone ever want to do that?

disruptek commented 3 years ago

Because it's not only what you want, it's also correct by construction. There's no way to overlook an implementation, omit documentation, or have the runnable example (which is a test) not work. Regardless of how many implementations you have, you will only ever have one test/doc per exported signature.

If you use your alternative, you'll find that it's at least the same amount of work but offers none of these guarantees of correctness.

alaviss commented 3 years ago

Isn't this RFC about fixing forward declaration so that you can put docs there and solve the point you're raising?

disruptek commented 3 years ago

No, my point is that the language doesn't need this extension. I'm :-1: on changing the language with yet another form of syntax that is not required.

Araq commented 3 years ago

Not yet another pragma... We should get rid of forward declarations altogether, once we have IC we'll make that happen.

nim-lang / RFCs