bug#46918: 28.0.50; checkdoc fooled by qualified method

bug#46918: 28.0.50; checkdoc fooled by qualified method

[Mauro Aranda]

Starting from emacs -Q:

1. Type the following in the *scratch* buffer
(cl-defmethod foo ((a (eql test)))
  "Return A."
  a)

(cl-defmethod foo :around ((a (eql test)))
  "Return A."
  a)

2. M-x checkdoc

3. See how checkdoc doesn't complain about the first method, but does
complain about the second method, with the following message:

Error message:
  All variables and subroutines might as well have a documentation string

which is wrong.

bug#46918: 28.0.50; checkdoc fooled by qualified method

[Mauro Aranda]

Oops, forgot to add the bug report information:

In GNU Emacs 28.0.50 (build 12, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version 1.15.10)
 of 2021-03-04 built on tbb-desktop
Repository revision: ac42f719089a880bba418c95a7a16c4d7df392dd
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Ubuntu 18.04.5 LTS

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG
LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SOUND THREADS
TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LC_MONETARY: es_AR.UTF-8
  value of $LC_NUMERIC: es_AR.UTF-8
  value of $LC_TIME: es_AR.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(checkdoc lisp-mnt thingatpt help-fns radix-tree help-mode pp shadow
sort mail-extr emacsbug message rmc puny dired dired-loaddefs rfc822 mml
mml-sec epa derived epg epg-config gnus-util rmail rmail-loaddefs
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map text-property-search time-date subr-x seq
byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs cl-lib
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core term/tty-colors frame minibuffer
cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan thai
tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian
slovak czech european ethiopic indian cyrillic chinese composite
charscript charprop case-table epa-hook jka-cmpr-hook help simple abbrev
obarray cl-preloaded nadvice button loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget hashtable-print-readable backquote threads
dbusbind inotify dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 55199 12044)
 (symbols 48 6955 4)
 (strings 32 20238 1400)
 (string-bytes 1 660821)
 (vectors 16 13795)
 (vector-slots 8 181278 17264)
 (floats 8 32 43)
 (intervals 56 381 38)
 (buffers 992 14))

bug#46918: 28.0.50; checkdoc fooled by qualified method

[Mauro Aranda]

[-- Attachment #1: Type: text/plain, Size: 182 bytes --]

tags 46918 patch
quit


I attach a patch to fix this, together with new tests.  I wanted to keep
the first case simple, so I moved the handling of cl-defmethod to a
different case.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch --]
[-- Type: text/x-patch, Size: 3973 bytes --]

From 3d6e813a42f3472c817cc512ccba60c70e04b64e Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Thu, 4 Mar 2021 08:34:58 -0300
Subject: [PATCH] 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.

* test/lisp/emacs-lisp/checkdoc-tests.el (checkdoc-cl-defmethod-qualified-ok)
(checkdoc-cl-defmethod-with-extra-qualifier-ok)
(checkdoc-cl-defmethod-with-extra-and-nil-args-ok): Add tests for the fix.
---
 lisp/emacs-lisp/checkdoc.el            | 21 +++++++++++++++++++-
 test/lisp/emacs-lisp/checkdoc-tests.el | 27 ++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 75aefdc7ba..213ab43184 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -932,7 +932,7 @@ checkdoc--next-docstring
                            ;; definition ends prematurely.
                            (end-of-file)))
     (`(,(or 'defun 'defvar 'defcustom 'defmacro 'defconst 'defsubst 'defadvice
-            'cl-defun 'cl-defgeneric 'cl-defmethod 'cl-defmacro)
+            'cl-defun 'cl-defgeneric 'cl-defmacro)
        ,(pred symbolp)
        ;; Require an initializer, i.e. ignore single-argument `defvar'
        ;; forms, which never have a doc string.
@@ -942,6 +942,25 @@ checkdoc--next-docstring
      ;; initializer or argument list.
      (forward-sexp 3)
      (skip-chars-forward " \n\t")
+     t)
+    (`(,'cl-defmethod
+        ,(pred symbolp)
+        . ,rest)
+     (down-list)
+     (forward-sexp (pcase (car rest)
+                     ;; No qualifier, so skip like we would have skipped in
+                     ;; the first clause of the outer `pcase'.
+                     ((pred listp) 3)
+                     (':extra
+                      ;; Skip the :extra qualifier together with its string too.
+                      ;; Skip any additional qualifier.
+                      (if (memq (nth 2 rest) '(:around :before :after))
+                                  6
+                                5))
+                     ;; Skip :before, :after or :around qualifier too.
+                     ((or ':around ':before ':after)
+                      4)))
+     (skip-chars-forward " \n\t")
      t)))
 
 ;;;###autoload
diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
index 93015fbb10..7a7aa9fb3c 100644
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -52,6 +52,33 @@ checkdoc-cl-defmethod-with-types-ok
     (insert "(cl-defmethod foo ((a (eql smthg)) (b list)) \"Return A+B.\")")
     (checkdoc-defun)))
 
+(ert-deftest checkdoc-cl-defmethod-qualified-ok ()
+  "Checkdoc should be happy with a `cl-defmethod' using qualifiers."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defmethod test :around ((a (eql smthg))) \"Return A.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defmethod-with-extra-qualifier-ok ()
+  "Checkdoc should be happy with a :extra qualified `cl-defmethod'."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defmethod foo :extra \"foo\" ((a (eql smthg))) \"Return A.\")")
+    (checkdoc-defun))
+
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert
+     "(cl-defmethod foo :extra \"foo\" :after ((a (eql smthg))) \"Return A.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defmethod-with-extra-qualifier-and-nil-args-ok ()
+  "Checkdoc should be happy with a 0-arity :extra qualified `cl-defmethod'."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defmethod foo :extra \"foo\" () \"Return A.\")")
+    (checkdoc-defun)))
+
 (ert-deftest checkdoc-cl-defun-with-key-ok ()
   "Checkdoc should be happy with a cl-defun using &key."
   (with-temp-buffer
-- 
2.30.0

bug#46918: 28.0.50; checkdoc fooled by qualified method

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> I attach a patch to fix this, together with new tests.  I wanted to keep
> the first case simple, so I moved the handling of cl-defmethod to a
> different case.

Looks good to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46918: 28.0.50; checkdoc fooled by qualified method

[Mauro Aranda]

close 46918 28.1
quit


Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> I attach a patch to fix this, together with new tests.  I wanted to keep
>> the first case simple, so I moved the handling of cl-defmethod to a
>> different case.
>
> Looks good to me.

Thanks, I pushed the fix to master.  It should be tweaked if the :extra
qualifier is allowed to come last (Bug#46917), but it should be easy to
do that.