Ddoc does not complain when the parameters from "Params:" mismatch the actual parameters used in the function declaration.
I have a patch that issues as warning in such case. An additional case would be issuing a warning for missing parameters.
Comment #1 by bearophile_hugs — 2013-06-02T04:09:02Z
Such tests were discussed time ago, and to me they seem a good idea.
Comment #3 by bearophile_hugs — 2013-06-02T06:31:31Z
(In reply to comment #0)
> An additional case would be issuing a warning for missing parameters.
Another useful warning is when you document an argument that's not present in the argument list.
Comment #4 by lio+bugzilla — 2013-06-02T06:47:29Z
(In reply to comment #3)
> (In reply to comment #0)
>
> > An additional case would be issuing a warning for missing parameters.
>
> Another useful warning is when you document an argument that's not present in
> the argument list.
That's my first case: the parameters from "Params:" have to be present in the actual declaration. Next, I also check the parameter count.
Comment #5 by bugzilla — 2013-06-04T22:27:34Z
I have mixed feelings about this. Generally, Ddoc does not emit warnings or errors. This is deliberate, as it tries to interpret whatever random stuff the user may write as Ddoc stuff. If it isn't valid Ddoc, Ddoc shouldn't complain.
Comment #6 by lt.infiltrator — 2013-06-04T22:50:44Z
I get what you mean, Walter; but sometimes you'll make a change and forget to update the ddoc. Perhaps making it complain by default isn't the correct way, and I know you hate the idea of adding more switches, but maybe have a --check-ddoc (or I guess with a single hash for dmd) for the ability to check your ddoc and say "hey, you forgot this; this doesn't match with your code; etc."
Comment #7 by bearophile_hugs — 2013-06-05T05:04:07Z
(In reply to comment #5)
> I have mixed feelings about this. Generally, Ddoc does not emit warnings or
> errors. This is deliberate, as it tries to interpret whatever random stuff the
> user may write as Ddoc stuff. If it isn't valid Ddoc, Ddoc shouldn't complain.
Languages as Python have more flexibility thanks to their dynamic typing. D has chosen to be a (mostly) statically typed language. So let's use such static typing (and Ahead Of Time compilation) for all it's able to give.
The tests proposed here are another excellent way to improve the D documentation using the capabilities D has, just like the documentation unittests.
Having documentation not in sync with the actual code is _worse_ than not having documentation. This enhancement request offers a chance to reduce such mistakes. Documentation bugs are bugs, because they are part of the API to interface a library with the brain of the humans that use it. Introducing quick compiler-enforced tests to reduce the amount of such mistakes is a good idea.
Instead of a warning I'd like this to become an error, eventually :-) If the programmer writes no documentation string, or he/she writes only a freestyle comment, no error is generated. If he/she chooses to use the ddoc syntax to list the arguments, that later the ddoc will pick to show them in a pretty formatted way, they must be correct.
It's common for comments to go out of sync with the code. Just like "static switch" this is an automatic help to keep different parts of the code in sync.
If you don't like this proposal, then please Walter write down a list of downsides and problems that this will cause. And let's see if each one of your points will hold.
Comment #8 by lio+bugzilla — 2013-06-05T19:49:54Z
(In reply to comment #5)
> I have mixed feelings about this. Generally, Ddoc does not emit warnings or
> errors. This is deliberate, as it tries to interpret whatever random stuff the
> user may write as Ddoc stuff. If it isn't valid Ddoc, Ddoc shouldn't complain.
In fact, Ddoc already has a couple of "warnings", that's why I stuck to warnings for this issue as well. You can't expect users to use Ddoc comments and then write non-Ddoc text. If that was the case, why do we have Ddoc comments to begin with?
Comment #10 by bearophile_hugs — 2013-08-25T09:54:14Z
(In reply to comment #9)
> this patch has found numerous DDoc mistakes in phobos and druntime!
A different outcome would have shocked me :-)
Comment #11 by bearophile_hugs — 2013-08-25T10:57:20Z
(In reply to comment #9)
> FWIW, this patch has found numerous DDoc mistakes in phobos and druntime! I'd
> say that's proof enough.
Where's a compact list of all the Phobos documentation errors found by the new warnings?
Comment #12 by lio+bugzilla — 2013-08-25T18:19:43Z
(In reply to comment #11)
> (In reply to comment #9)
>
> > FWIW, this patch has found numerous DDoc mistakes in phobos and druntime! I'd
> > say that's proof enough.
>
> Where's a compact list of all the Phobos documentation errors found by the new
> warnings?
http://d.puremagic.com/issues/show_bug.cgi?id=10893
Comment #13 by lio+bugzilla — 2013-08-25T18:23:42Z
Comment #14 by lio+bugzilla — 2013-08-25T18:35:13Z
While solving the warnings in druntime, there's only one instance where I thought the current patch might need to be improved:
/**
* Resets this fiber so that it may be re-used. This routine may only be
* called for fibers that have terminated, as doing otherwise could result
* in scope-dependent functionality that is not executed. Stack-based
* classes, for example, may not be cleaned up properly if a fiber is reset
* before it has terminated.
*
* Params:
* fn = The fiber function.
* dg = The fiber function.
*
* In:
* This fiber must be in state TERM.
*/
final void reset();
/// ditto
final void reset( void function() fn );
/// ditto
final void reset( void delegate() dg );
This shows three warnings for the first reset(), no warnings for the "ditto". A quick fix would be to copy the whole ddoc for the two overloads, but perhaps there's something smarter that can be done here?
Comment #15 by bearophile_hugs — 2013-08-25T19:12:47Z
(In reply to comment #14)
> This shows three warnings for the first reset(), no warnings for the "ditto". A
> quick fix would be to copy the whole ddoc for the two overloads, but perhaps
> there's something smarter that can be done here?
Here "Params" is used in a non-standard way, it shows the merged arguments of all the overloads of the "reset" function. This merging should be accepted by the warning code only if dittos are used. So a possible solution is to create a set of all the arguments of the overloads tagged with "ditto", and then verify such set is the same as the set of arguments listed in the "Params" section.
(But usually warnings aren't 100.00% reliable. Usually there are very uncommon cases where a warning gives false positives and false negatives. This ddoc warning seems to be nearly perfect, but I don't expect it to be really perfect. I think here a error rate of 0.1% or 0.01% is acceptable. Keeping warning implementation simple is sometimes better than trying to further reduce down that tiny percentage of errors.)
Comment #16 by lio+bugzilla — 2013-08-27T04:38:47Z
(In reply to comment #15)
> (But usually warnings aren't 100.00% reliable. Usually there are very uncommon
> cases where a warning gives false positives and false negatives. This ddoc
> warning seems to be nearly perfect, but I don't expect it to be really perfect.
> I think here a error rate of 0.1% or 0.01% is acceptable. Keeping warning
> implementation simple is sometimes better than trying to further reduce down
> that tiny percentage of errors.)
I agree that this case is not a priority and the patch would be acceptable with it unsolved.
Another issue though is that I'm not checking template parameters. This is actually a bigger issue, as it happens far more often in Phobos and druntime. In fact, there are many occurences of both: many templates that do and many that don't document the template parameters.
Now I wonder whether it's a good idea to use "Params:" for both compile time and runtime parameters. In the very least the generated documentation should mention whether it's one or the other.
Comment #17 by lio+bugzilla — 2013-09-01T20:51:22Z
Pull request was updated to allow for template parameters:
* parameters in the DDoc Params section must either be function parameters of template parameters
* all function parameters must appear in the Params section, but this is not checked for template parameters.
Comment #18 by github-bugzilla — 2013-09-15T00:52:10Z