bug#37063: 26.2.90; Problems with recent CL support in checkdoc

bug#37063: 26.2.90; Problems with recent CL support in checkdoc

[Damien Cassou]

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

In the context of bug#37034, some initial support for CL functions and
methods was added to checkdoc.  On my code, the changes make checkdoc
trigger warnings on perfectly valid code IMO. For example,

  (cl-defmethod foo ((a list)) "Return A.")

Checkdoc complains that LIST should be described. I disagree with that,
list is a type, not a argument name.

I attach to this bug report a patch introducing test cases to
checkdoc-tests.el. If it's not possible to implement full support for CL
I prefer that checkdoc doesn't complain even if there is a bug in the
docstring rather than checkdoc complains when the docstring is correct.

Best,

-- 
Damien Cassou
http://damiencassou.seasidehosting.st

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-checkdoc-CL-tests.patch --]
[-- Type: text/x-patch, Size: 3192 bytes --]

From d7357884819f8921ff1a27dd3a25bdfd882d1c5a Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Fri, 16 Aug 2019 20:42:26 +0200
Subject: [PATCH] checkdoc CL tests

---
 test/lisp/emacs-lisp/checkdoc-tests.el | 61 ++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
index 1cefc4c366..ee4c1fb5a6 100644
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -37,6 +37,67 @@
     (insert "(defun foo())")
     (should-error (checkdoc-defun) :type 'user-error)))
 
+(ert-deftest checkdoc-cl-defmethod-ok ()
+  "Checkdoc should be happy with a simple correct cl-defmethod."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defmethod foo (a) \"Return A.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defmethod-with-types-ok ()
+  "Checkdoc should be happy with a cl-defmethod using types."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    ;; this method matches if A is the symbol `smthg' and if b is a list:
+    (insert "(cl-defmethod foo ((a (eql smthg)) (b list)) \"Return A+B.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-key-ok ()
+  "Checkdoc should be happy with a cl-defun using &key."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defun foo (&key a (b 27)) \"Return :A+:B.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-allow-other-keys-ok ()
+  "Checkdoc should be happy with a cl-defun using &allow-other-keys."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defun foo (&key a &allow-other-keys) \"Return :A.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-aux-ok ()
+  "Checkdoc should be happy with a cl-defun using &aux."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defun foo (a b &aux (c (+ a b))) \"Return A and B.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-default-optional-value-ok ()
+  "Checkdoc should be happy with a cl-defun using default values for optional args."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    ;; B is optional and equals 1+a if not provided. HAS-BS is non-nil
+    ;; if B was provided in the call:
+    (insert "(cl-defun foo (a &optional (b (1+ a) has-bs)) \"Return A + B.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-destructuring-ok ()
+  "Checkdoc should be happy with a cl-defun destructuring its arguments."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defun foo ((a b &optional c) d) \"Return A+B+C+D.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defmethod-with-context-ok ()
+  "Checkdoc should ignore context specializers in a cl-defmethod."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    ;; A context specializer is used to select the correct method but
+    ;; doesn't have to appear in the docstring:
+    (insert "(cl-defmethod foo (a &context (global-var (eql foo))) \"Return A.\")")
+    (checkdoc-defun)))
+
 (ert-deftest checkdoc-tests--next-docstring ()
   "Checks that the one-argument form of `defvar' works.
 See the comments in Bug#24998."
-- 
2.21.0

bug#37063: 26.2.90; Problems with recent CL support in checkdoc

[dick.r.chiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: checkdoc fix --]
[-- Type: text/x-diff, Size: 2160 bytes --]

From c97b08a1d6df46f674a18ab83ae86dc6a5ad3aa0 Mon Sep 17 00:00:00 2001
From: dickmao <none>
Date: Mon, 30 Sep 2019 10:04:49 -0400
Subject: [PATCH] ; checkdoc identifying formal args

* lisp/emacs-lisp/checkdoc.el (checkdoc-defun-info):
* test/lisp/emacs-lisp/checkdoc-tests.el: (Bug#37063)
---
 lisp/emacs-lisp/checkdoc.el            |  5 ++---
 test/lisp/emacs-lisp/checkdoc-tests.el | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 51fb75da69..6c40bdf632 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -1952,11 +1952,10 @@ checkdoc-defun-info
 	;; new obarray.
 	(if (not (listp lst)) (setq lst nil))
 	(unless is-advice
-          ;; lst here can be something like ((foo bar) baz) from
+          ;; (car lst) can be something like ((foo bar) baz) from
           ;; cl-lib methods; flatten it:
-          (setq lst (flatten-tree lst))
 	  (while lst
-	    (setq ret (cons (symbol-name (car lst)) ret)
+	    (setq ret (cons (symbol-name (car (flatten-tree (car lst)))) ret)
 		  lst (cdr lst)))))
       (nreverse ret))))
 
diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
index 1cefc4c366..b3cc943ac0 100644
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -50,4 +50,20 @@ checkdoc-tests--next-docstring
     (should (looking-at-p "\"baz\")"))
     (should-not (checkdoc-next-docstring))))
 
+(ert-deftest checkdoc-tests--cl-defun ()
+  "Identify formal arguments from arbitary lisp code."
+  (with-temp-buffer
+    (let ((checkdoc-autofix-flag 'never))
+      (emacs-lisp-mode)
+      (insert "(cl-defun foo(&key bar &aux (baz (baz bar))) \"BAR BAZ.\")")
+      (should-not (checkdoc-defun)))))
+
+(ert-deftest checkdoc-tests--cl-defmethod ()
+  "Identify formal arguments from object types."
+  (with-temp-buffer
+    (let ((checkdoc-autofix-flag 'never))
+      (emacs-lisp-mode)
+      (insert "(cl-defmethod foo((a list)) \"Return A.\")")
+      (should-not (checkdoc-defun)))))
+
 ;;; checkdoc-tests.el ends here
-- 
2.23.0

bug#37063: 26.2.90; Problems with recent CL support in checkdoc

[Lars Ingebrigtsen]

dick.r.chiang@gmail.com writes:

> diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
> index 51fb75da69..6c40bdf632 100644
> --- a/lisp/emacs-lisp/checkdoc.el
> +++ b/lisp/emacs-lisp/checkdoc.el
> @@ -1952,11 +1952,10 @@ checkdoc-defun-info
>  	;; new obarray.
>  	(if (not (listp lst)) (setq lst nil))
>  	(unless is-advice
> -          ;; lst here can be something like ((foo bar) baz) from
> +          ;; (car lst) can be something like ((foo bar) baz) from
>            ;; cl-lib methods; flatten it:
> -          (setq lst (flatten-tree lst))
>  	  (while lst
> -	    (setq ret (cons (symbol-name (car lst)) ret)
> +	    (setq ret (cons (symbol-name (car (flatten-tree (car lst)))) ret)
>  		  lst (cdr lst)))))
>        (nreverse ret))))

Thank you; I've now applied your patch here since it seems to fix the
use cases reported.  However, I didn't include the tests in your patch,
since they seemed to cover the same cases as Damien's (and they were
more extensive, so I applied those instead).

Besides, it doesn't look like you have copyright assignment papers on
file, and for larger patches we require copyright assignments to the
FSF.  If you plan on submitting further patches in the future, it might
be a good idea to do the paperwork now -- would you be willing to do so?

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

bug#37063: 26.2.90; Problems with recent CL support in checkdoc

[Lars Ingebrigtsen]

Damien Cassou <damien@cassou.me> writes:

> In the context of bug#37034, some initial support for CL functions and
> methods was added to checkdoc.  On my code, the changes make checkdoc
> trigger warnings on perfectly valid code IMO. For example,
>
>   (cl-defmethod foo ((a list)) "Return A.")
>
> Checkdoc complains that LIST should be described. I disagree with that,
> list is a type, not a argument name.
>
> I attach to this bug report a patch introducing test cases to
> checkdoc-tests.el. If it's not possible to implement full support for CL
> I prefer that checkdoc doesn't complain even if there is a bug in the
> docstring rather than checkdoc complains when the docstring is correct.

After a patch was submitted that fixed most of the cases (except the
&context and &aux ones, I believe), I've applied your checkdoc-tests.el
patch (except those two functions).

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

bug#37063: 26.2.90; Problems with recent CL support in checkdoc

[Damien Cassou]

Lars Ingebrigtsen <larsi@gnus.org> writes:
> After a patch was submitted that fixed most of the cases (except the
> &context and &aux ones, I believe), I've applied your checkdoc-tests.el
> patch (except those two functions).

thank you!

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#37063: 26.2.90; Problems with recent CL support in checkdoc

[Glenn Morris]

Test failures:

https://hydra.nixos.org/build/102805902

bug#37063: 26.2.90; Problems with recent CL support in checkdoc

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> Test failures:
>
> https://hydra.nixos.org/build/102805902

Should be fixed now.

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