bug#65790: 29.1; "docstring wider than 80 characters" when there is no docstring

[Damien Cassou <damien@cassou.me>]

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

Thank you for your feedback. 3 patch files are attached to this
email. My answers below:

Stefan Kangas <stefankangas@gmail.com> writes:
> Damien Cassou <damien@cassou.me> writes:
> I think this also could use some tests, see bytecomp-tests.el:964 and
> onwards.


I've added tests to bytecomp-tests.el as suggested. (why not
cl-macs-tests.el instead?)


>> * lisp/emacs-lisp/cl-macs.el (cl-defsubst): Reduce likelihood of
>> "docstring wider than 80 characters" errors in generated code.
>
> Please remember to include the bug number when it is known.


done

>> -             ,(format "compiler-macro for inlining `%s'." name)
>> […]
>> +             ,(internal--format-docstring-line "compiler-macro for `%s'." name)
>
> Why drop the word "inlining"?


I felt this word was not really important because (1) this is generated
code and (2) the docstring starts with "compiler-macro". Removing it
reduces the likelihood of the error message. I added an explanation in
the commit message. If the word is important I can add it back.


> (This comment might need reflowing.)


done


> Could we keep the old format when possible, and use the new one only
> when needed?


I tried to do that, is that what you want? I feel the added complexity
isn't worth it so I'm fine changing it back to the simpler version if
you change your mind.


-- 
Damien Cassou

"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-bytecomp-tests.el-Add-new-helper-function.patch --]
[-- Type: text/x-patch, Size: 1436 bytes --]

From 28c90e4c3371501f2965ff5aac0d5108572fa674 Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Mon, 11 Sep 2023 21:59:45 +0200
Subject: [PATCH 1/3] bytecomp-tests.el: Add new helper function

* test/lisp/emacs-lisp/bytecomp-tests.el
(bytecomp--without-warning-test): Add helper function.
(bytecomp-warn--ignore): Use the helper.
---
 test/lisp/emacs-lisp/bytecomp-tests.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index cd1bda3ec55..c0adab75269 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -937,14 +937,17 @@ bytecomp--with-warning-test
            (should (re-search-forward
                     (string-replace " " "[ \n]+" re-warning)))))))
 
+(defun bytecomp--without-warning-test (form)
+  (bytecomp--with-warning-test "\\`\\'" form))
+
 (ert-deftest bytecomp-warn--ignore ()
   (bytecomp--with-warning-test "unused"
     '(lambda (y) 6))
-  (bytecomp--with-warning-test "\\`\\'" ;No warning!
+  (bytecomp--without-warning-test
     '(lambda (y) (ignore y) 6))
   (bytecomp--with-warning-test "assq"
     '(lambda (x y) (progn (assq x y) 5)))
-  (bytecomp--with-warning-test "\\`\\'" ;No warning!
+  (bytecomp--without-warning-test
     '(lambda (x y) (progn (ignore (assq x y)) 5))))
 
 (ert-deftest bytecomp-warn-wrong-args ()
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Shorten-docstrings-generated-by-cl-defsubst-bug-6579.patch --]
[-- Type: text/x-patch, Size: 2795 bytes --]

From 4be1620fe344ebacd707bfb6c69d34eaa1a27df5 Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Tue, 12 Sep 2023 08:28:37 +0200
Subject: [PATCH 2/3] Shorten docstrings generated by cl-defsubst (bug#65790)

* lisp/emacs-lisp/cl-macs.el (cl-defsubst): Remove the word "inlining"
in the generated docstring as it is not very useful and increases the
likelihood of "docstring wider than 80 characters"
errors. Additionally, split the first line of the docstring into 2
lines if the function name is very long.
---
 lisp/emacs-lisp/cl-macs.el             |  9 ++++++++-
 test/lisp/emacs-lisp/bytecomp-tests.el | 11 +++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index c8e2610c8b0..dc17f992660 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2931,7 +2931,14 @@ cl-defsubst
              ,(if (memq '&key args)
                   `(&whole cl-whole &cl-quote ,@args)
                 (cons '&cl-quote args))
-             ,(format "compiler-macro for inlining `%s'." name)
+             ;; NB.  This will produce incorrect results in some
+             ;; cases, as our coding conventions says that the first
+             ;; line must be a full sentence.  However, if we don't
+             ;; word wrap we will have byte-compiler warnings about
+             ;; overly long docstrings.  So we can't have a perfect
+             ;; result here, and choose to avoid the byte-compiler
+             ;; warnings.
+             ,(internal--format-docstring-line "compiler-macro for `%s'." name)
              (cl--defsubst-expand
               ',argns '(cl-block ,name ,@(cdr (macroexp-parse-body body)))
               nil
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index c0adab75269..102616c9bb7 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -972,6 +972,17 @@ bytecomp-warn-wide-docstring/defvar
   (bytecomp--with-warning-test "defvar.*foo.*wider than.*characters"
     `(defvar foo t ,bytecomp-tests--docstring)))
 
+(ert-deftest bytecomp-warn-wide-docstring/cl-defsubst ()
+  (bytecomp--without-warning-test
+   `(cl-defsubst short-name ()
+      "Do something."))
+  (bytecomp--without-warning-test
+   `(cl-defsubst long-name-with-less-80-characters-but-still-quite-a-bit ()
+      "Do something."))
+  (bytecomp--with-warning-test "wider than.*characters"
+   `(cl-defsubst long-name-with-more-than-80-characters-yes-this-is-a-very-long-name-but-why-not!! ()
+      "Do something.")))
+
 (ert-deftest bytecomp-warn-quoted-condition ()
   (bytecomp--with-warning-test
       "Warning: `condition-case' condition should not be quoted: 'arith-error"
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Shorten-docstrings-generated-by-cl-defstruct-bug-657.patch --]
[-- Type: text/x-patch, Size: 5660 bytes --]

From 23e8316ef7659b361157c0659bdb96a4bc9cb1e7 Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Tue, 12 Sep 2023 08:28:37 +0200
Subject: [PATCH 3/3] Shorten docstrings generated by cl-defstruct (bug#65790)

* lisp/emacs-lisp/cl-macs.el (cl-defstruct): Split the first line of
generated docstrings if either the struct name or a field name is very
long. This reduces the likelihood of "docstring wider than 80
characters" errors.
---
 lisp/emacs-lisp/cl-macs.el             | 44 ++++++++++++++++++--------
 test/lisp/emacs-lisp/bytecomp-tests.el | 17 ++++++++++
 2 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index dc17f992660..ef00f9e4fe4 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -3187,18 +3187,27 @@ cl-defstruct
               ;; The arg "cl-x" is referenced by name in e.g. pred-form
 	      ;; and pred-check, so changing it is not straightforward.
 	      (push `(,defsym ,accessor (cl-x)
-                       ,(concat
-                         ;; NB.  This will produce incorrect results
-                         ;; in some cases, as our coding conventions
-                         ;; says that the first line must be a full
-                         ;; sentence.  However, if we don't word wrap
-                         ;; we will have byte-compiler warnings about
-                         ;; overly long docstrings.  So we can't have
-                         ;; a perfect result here, and choose to avoid
-                         ;; the byte-compiler warnings.
-                         (internal--format-docstring-line
-                          "Access slot \"%s\" of `%s' struct CL-X." slot name)
-                         (if doc (concat "\n" doc) ""))
+                       ,(let ((long-docstring
+                               (format "Access slot \"%s\" of `%s' struct CL-X." slot name)))
+                          (concat
+                           ;; NB.  This will produce incorrect results
+                           ;; in some cases, as our coding conventions
+                           ;; says that the first line must be a full
+                           ;; sentence.  However, if we don't word
+                           ;; wrap we will have byte-compiler warnings
+                           ;; about overly long docstrings.  So we
+                           ;; can't have a perfect result here, and
+                           ;; choose to avoid the byte-compiler
+                           ;; warnings.
+                           (if (>= (length long-docstring) byte-compile-docstring-max-column)
+                               (concat
+                                (internal--format-docstring-line
+                                 "Access slot \"%s\" of CL-X." slot)
+                                "\n"
+                                (internal--format-docstring-line
+                                 "Struct CL-X is a `%s'." name))
+                             (internal--format-docstring-line long-docstring))
+                           (if doc (concat "\n" doc) "")))
                        (declare (side-effect-free t))
                        ,access-body)
                     forms)
@@ -3274,7 +3283,16 @@ cl-defstruct
 	(push `(,cldefsym ,cname
                    (&cl-defs (nil ,@descs) ,@args)
                  ,(if (stringp doc) doc
-                    (format "Constructor for objects of type `%s'." name))
+                    ;; NB.  This will produce incorrect results in
+                    ;; some cases, as our coding conventions says that
+                    ;; the first line must be a full sentence.
+                    ;; However, if we don't word wrap we will have
+                    ;; byte-compiler warnings about overly long
+                    ;; docstrings.  So we can't have a perfect result
+                    ;; here, and choose to avoid the byte-compiler
+                    ;; warnings.
+                    (internal--format-docstring-line
+                     "Constructor for objects of type `%s'." name))
                  ,@(if (cl--safe-expr-p `(progn ,@(mapcar #'cl-second descs)))
                        '((declare (side-effect-free t))))
                  (,con-fun ,@make))
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index 102616c9bb7..03aed5263b6 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -983,6 +983,23 @@ bytecomp-warn-wide-docstring/cl-defsubst
    `(cl-defsubst long-name-with-more-than-80-characters-yes-this-is-a-very-long-name-but-why-not!! ()
       "Do something.")))
 
+(ert-deftest bytecomp-warn-wide-docstring/cl-defstruct ()
+  (bytecomp--without-warning-test
+   `(cl-defstruct short-name
+      field))
+  (bytecomp--without-warning-test
+   `(cl-defstruct short-name
+      long-name-with-less-80-characters-but-still-quite-a-bit))
+  (bytecomp--without-warning-test
+   `(cl-defstruct long-name-with-less-80-characters-but-still-quite-a-bit
+      field))
+  (bytecomp--with-warning-test "wider than.*characters"
+    `(cl-defstruct short-name
+       long-name-with-more-than-80-characters-yes-this-is-a-very-long-name-but-why-not!!))
+  (bytecomp--with-warning-test "wider than.*characters"
+    `(cl-defstruct long-name-with-more-than-80-characters-yes-this-is-a-very-long-name-but-why-not!!
+       field)))
+
 (ert-deftest bytecomp-warn-quoted-condition ()
   (bytecomp--with-warning-test
       "Warning: `condition-case' condition should not be quoted: 'arith-error"
-- 
2.41.0