Re: master fd92023: Make checkdoc work with qualified methods

Re: master fd92023: Make checkdoc work with qualified methods

[Stefan Monnier]

>     Make checkdoc work with qualified methods
>     
>     * lisp/emacs-lisp/checkdoc.el (checkdoc--next-docstring): Handle
>     cl-defmethod in a case of its own.  Check for the presence of
>     qualifiers, and skip them accordingly until the docstring.

Any chance we could use the `doc-string-elt` property (which I just
fixed for `cl-defmethod`) for `checkdoc--next-docstring`?


        Stefan

Re: master fd92023: Make checkdoc work with qualified methods

[Mauro Aranda]

Hi Stefan,

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>     Make checkdoc work with qualified methods
>>     
>>     * lisp/emacs-lisp/checkdoc.el (checkdoc--next-docstring): Handle
>>     cl-defmethod in a case of its own.  Check for the presence of
>>     qualifiers, and skip them accordingly until the docstring.
>
> Any chance we could use the `doc-string-elt` property (which I just
> fixed for `cl-defmethod`) for `checkdoc--next-docstring`?
>
>
>         Stefan

Something along these lines?

(defun checkdoc--next-docstring ()
  "When looking at a definition with a doc string, find it.
Move to the next doc string after point, and return t.  When not
looking at a definition containing a doc string, return nil and
don't move point."
  (pcase (save-excursion (condition-case nil
                             (read (current-buffer))
                           ;; Conservatively skip syntax errors.
                           (invalid-read-syntax)
                           ;; Don't bug out if the file is empty (or a
                           ;; definition ends prematurely.
                           (end-of-file)))
    (`(,(and
         (or 'defun 'defvar 'defcustom 'defmacro 'defconst 'defsubst
             'defadvice 'cl-defun 'cl-defgeneric 'cl-defmacro 'cl-defmethod)
         def)
       ,(pred symbolp)
       ;; Require an initializer, i.e. ignore single-argument `defvar'
       ;; forms, which never have a doc string.
       ,_ . ,_)
     (down-list)
     ;; Skip over function or macro name.
     (forward-sexp 1)
     ;; And now skip until the docstring.
     (forward-sexp (1- ; We already skipped the function or macro name.
                    (pcase (function-get def 'doc-string-elt)
                      ((and (pred numberp) num) num)
                      ((and (pred functionp) fn) (funcall fn)))))
     (skip-chars-forward " \n\t")
     t)))


Note that I need to do (forward-sexp 1) so the requirements of
cl--defmethod-doc-pos are fulfilled.  It may get messy if other defining
forms declare a doc-string-elt function that assumes a different point
position.


BTW, I've noticed that I forgot to add the Bug tag to my commit, I'm
sorry.  This commit was part of Bug#46918.

Re: master fd92023: Make checkdoc work with qualified methods

[Stefan Monnier]

>     (`(,(and
>          (or 'defun 'defvar 'defcustom 'defmacro 'defconst 'defsubst
>              'defadvice 'cl-defun 'cl-defgeneric 'cl-defmacro 'cl-defmethod)
>          def)
>        ,(pred symbolp)
>        ;; Require an initializer, i.e. ignore single-argument `defvar'
>        ;; forms, which never have a doc string.
>        ,_ . ,_)

I think you can reduce that to

    (pred symbolp)

and then check that (function-get def 'doc-string-elt) is non-nil.

> Note that I need to do (forward-sexp 1) so the requirements of
> cl--defmethod-doc-pos are fulfilled.  It may get messy if other defining
> forms declare a doc-string-elt function that assumes a different point
> position.

The starting position of `doc-string-elt` is currently defined de-facto
by the code in `lisp-string-in-doc-position-p`, so
`cl--defmethod-doc-pos` had no choice in the matter ;-)

> BTW, I've noticed that I forgot to add the Bug tag to my commit, I'm
> sorry.  This commit was part of Bug#46918.

I think this deserves a harsh punishment.


        Stefan

Re: master fd92023: Make checkdoc work with qualified methods

[Mauro Aranda]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>     (`(,(and
>>          (or 'defun 'defvar 'defcustom 'defmacro 'defconst 'defsubst
>>              'defadvice 'cl-defun 'cl-defgeneric 'cl-defmacro 'cl-defmethod)
>>          def)
>>        ,(pred symbolp)
>>        ;; Require an initializer, i.e. ignore single-argument `defvar'
>>        ;; forms, which never have a doc string.
>>        ,_ . ,_)
>
> I think you can reduce that to
>
>     (pred symbolp)
>
> and then check that (function-get def 'doc-string-elt) is non-nil.

I thought about that: it would "add support" to other definitions, and I
wasn't sure if it would be desired to check the docstring for all of them.
An el-search reports the following (without counting what checkdoc
already supports):

define-abbrev-table (*)
defface
defgroup
deftheme (*)
define-ibuffer-sorter
define-ibuffer-op
define-ibuffer-filter
defimage (*)
define-skeleton
defvar-local
lambda (**)
define-widget
defmath
define-overloadable-function
define-mode-local-override
define-obsolete-function-alias
define-obsolete-variable-alias
cl-iter-defun
cl-defstruct
cl-deftype
define-derived-mode
define-minor-mode
define-globalized-minor-mode
easy-menu-define
ert-deftest
iter-defun
define-generic-mode
define-inline
define-advice
pcase-lambda (**)
pcase-defmacro
define-erc-module
define-ccl-program
defun-mh
defmacro-mh
defgroup-mh
defcustom-mh
defface-mh
ediff-defvar-local
defun-cvs-mode (*)

* the pattern would need tweaking to detect the form
** wouldn't match, because of the second (pred symbolp)

Also, for some definitions (e.g., define-erc-module), the report might
be different because the file was/wasn't loaded.

To sum it up, I feared that just checking for a non-nil doc-string-elt
could make checkdoc somewhat annoying, but I'd like to know what other
people think.

>> Note that I need to do (forward-sexp 1) so the requirements of
>> cl--defmethod-doc-pos are fulfilled.  It may get messy if other defining
>> forms declare a doc-string-elt function that assumes a different point
>> position.
>
> The starting position of `doc-string-elt` is currently defined de-facto
> by the code in `lisp-string-in-doc-position-p`, so
> `cl--defmethod-doc-pos` had no choice in the matter ;-)

I didn't know that.  No worries then.

>> BTW, I've noticed that I forgot to add the Bug tag to my commit, I'm
>> sorry.  This commit was part of Bug#46918.
>
> I think this deserves a harsh punishment.

:-(

Re: master fd92023: Make checkdoc work with qualified methods

[Stefan Monnier]

>> and then check that (function-get def 'doc-string-elt) is non-nil.
>
> I thought about that: it would "add support" to other definitions, and I
> wasn't sure if it would be desired to check the docstring for all of them.

No idea either, but it seems worth a try.


        Stefan

Re: master fd92023: Make checkdoc work with qualified methods

[Mauro Aranda]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> and then check that (function-get def 'doc-string-elt) is non-nil.
>>
>> I thought about that: it would "add support" to other definitions, and I
>> wasn't sure if it would be desired to check the docstring for all of them.
>
> No idea either, but it seems worth a try.

Very well, I've made the change to checkdoc--next-docstring to check for
a non-nil doc-string-elt property and use it.

Re: master fd92023: Make checkdoc work with qualified methods

[Stefan Monnier]

> Very well, I've made the change to checkdoc--next-docstring to check for
> a non-nil doc-string-elt property and use it.

Thanks!


        Stefan