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

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

[Damien Cassou]

I have this piece of code:

(cl-defstruct (json-process-client-application
               (:constructor json-process-client--application-create)
               (:conc-name json-process-client--application-))
  message-callbacks)

In GNU Emacs 29.1, "Warning: docstring wider than 80 characters" even
though I wrote no docstring here. I understand the problem lies in the
generated code and I agree the lisp names in my code are long. But, I
don't think the byte compiler should annoy me about docstrings I didn't
write.

What do you think?

-- 
Damien Cassou

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

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

[Eli Zaretskii]

> From: Damien Cassou <damien@cassou.me>
> Date: Wed, 06 Sep 2023 21:08:29 +0200
> 
> I have this piece of code:
> 
> (cl-defstruct (json-process-client-application
>                (:constructor json-process-client--application-create)
>                (:conc-name json-process-client--application-))
>   message-callbacks)
> 
> In GNU Emacs 29.1, "Warning: docstring wider than 80 characters" even
> though I wrote no docstring here. I understand the problem lies in the
> generated code and I agree the lisp names in my code are long. But, I
> don't think the byte compiler should annoy me about docstrings I didn't
> write.
> 
> What do you think?

How else will we/you know there's a potential problem there?

If you think you can do nothing about this problem, just ignore the
warning.  It's just a warning.

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

[Damien Cassou]

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:
> How else will we/you know there's a potential problem there?


my opinion is that if more than 80 characters in a docstring is a
problem then the macro should make sure not to generate one.


> If you think you can do nothing about this problem, just ignore the
> warning.  It's just a warning.


I hate warnings: either a problem is important and I want the build to
fail or there is no problem and I don't want to see anything. If I let
warnings exist then the ones I want to ignore will slowly be too
numerous for me to notice those I would like to fix.

This is why in my tool chain (makel) I'm always setting
`byte-compile-error-on-warn' to t and I do similar things in non
elisp-projects.


This is just my opinion.

Best

-- 
Damien Cassou

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

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

[Stefan Kangas]

Damien Cassou <damien@cassou.me> writes:

> I have this piece of code:
>
> (cl-defstruct (json-process-client-application
>                (:constructor json-process-client--application-create)
>                (:conc-name json-process-client--application-))
>   message-callbacks)
>
> In GNU Emacs 29.1, "Warning: docstring wider than 80 characters" even
> though I wrote no docstring here. I understand the problem lies in the
> generated code and I agree the lisp names in my code are long. But, I
> don't think the byte compiler should annoy me about docstrings I didn't
> write.
>
> What do you think?

Yes, it would be nice not to get that warning.  I fixed many such
problems when I first added the warning about long docstrings.

Patches welcome.

PS. As a work-around, you can set `byte-compile-docstring-max-column'.

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

[Damien Cassou]

Stefan Kangas <stefankangas@gmail.com> writes:
> Yes, it would be nice not to get that warning.  I fixed many such
> problems when I first added the warning about long docstrings.
>
> Patches welcome.


The only patch I can think of would change the macro so that the
docstring references each name on a line of its own. This way, we would
only get a warning if the name itself is more than 80 characters. Would
that be a reasonable patch for you?


> PS. As a work-around, you can set `byte-compile-docstring-max-column'.

Thank you for this.

-- 
Damien Cassou

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

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

[Stefan Kangas]

Damien Cassou <damien@cassou.me> writes:

> The only patch I can think of would change the macro so that the
> docstring references each name on a line of its own. This way, we would
> only get a warning if the name itself is more than 80 characters. Would
> that be a reasonable patch for you?

That's basically the solution, yes, but we probably want to wrap the
lines only when it's necessary.  See `internal--fill-string-single-line'
and where it is used for inspiration.

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

[Damien Cassou]

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

Stefan Kangas <stefankangas@gmail.com> writes:
> That's basically the solution, yes, but we probably want to wrap the
> lines only when it's necessary.  See `internal--fill-string-single-line'
> and where it is used for inspiration.

What do you think about attached patch? The patch replaces one usage of
`format' with `internal--format-docstring-line' and also slightly change
2 docstring texts.


-- 
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-Shorten-docstrings-generated-in-cl-macs.el.patch --]
[-- Type: text/x-patch, Size: 2190 bytes --]

From 093c76caa8ac551868565d0e690b9979593cf94d Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Sun, 10 Sep 2023 08:40:52 +0200
Subject: [PATCH] Shorten docstrings generated in cl-macs.el

* lisp/emacs-lisp/cl-macs.el (cl-defsubst): Reduce likelihood of
"docstring wider than 80 characters" errors in generated code.
---
 lisp/emacs-lisp/cl-macs.el | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index c8e2610c8b0..0f142b87e07 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2931,7 +2931,15 @@ 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
@@ -3190,7 +3198,10 @@ cl-defstruct
                          ;; 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)
+                          "Access slot \"%s\" of CL-X." slot)
+                         "\n"
+                         (internal--format-docstring-line
+                          "Struct CL-X is a `%s'." name)
                          (if doc (concat "\n" doc) ""))
                        (declare (side-effect-free t))
                        ,access-body)
-- 
2.41.0

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

[Stefan Kangas]

Damien Cassou <damien@cassou.me> writes:

> What do you think about attached patch? The patch replaces one usage of
> `format' with `internal--format-docstring-line' and also slightly change
> 2 docstring texts.

Thanks, some comments below.

I think this also could use some tests, see bytecomp-tests.el:964 and
onwards.

> From 093c76caa8ac551868565d0e690b9979593cf94d Mon Sep 17 00:00:00 2001
> From: Damien Cassou <damien@cassou.me>
> Date: Sun, 10 Sep 2023 08:40:52 +0200
> Subject: [PATCH] Shorten docstrings generated in cl-macs.el
>
> * 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.

> ---
>  lisp/emacs-lisp/cl-macs.el | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
> index c8e2610c8b0..0f142b87e07 100644
> --- a/lisp/emacs-lisp/cl-macs.el
> +++ b/lisp/emacs-lisp/cl-macs.el
> @@ -2931,7 +2931,15 @@ 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)

Why drop the word "inlining"?

(This comment might need reflowing.)

>               (cl--defsubst-expand
>                ',argns '(cl-block ,name ,@(cdr (macroexp-parse-body body)))
>                nil
> @@ -3190,7 +3198,10 @@ cl-defstruct
>                           ;; 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)
> +                          "Access slot \"%s\" of CL-X." slot)
> +                         "\n"
> +                         (internal--format-docstring-line
> +                          "Struct CL-X is a `%s'." name)

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

>                           (if doc (concat "\n" doc) ""))
>                         (declare (side-effect-free t))
>                         ,access-body)
> --
> 2.41.0

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

[Damien Cassou]

[-- 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

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

[Stefan Kangas]

Version: 30.1

Damien Cassou <damien@cassou.me> writes:

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

Good question.  Perhaps we should move all of those tests to more
relevant locations.  OTOH, the macro for this is in bytecomp-tests.el
currently, so I guess we'd have to move that.  Maybe it would fit in
ert-x.el.  It seems outside the scope of this bug report though.

>> 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.

The code is less nice obviously, but I think not changing the
autogenerated docstrings in the typical cases is also important.
Therefore, I think the complexity is ultimately worth it.  Perhaps
someone should do a round over all of this to see if it can be cleaned
up a bit, though...

I've now installed your patches on master.  Feel free to post followups
if you want to fix any of the above, or if you spot anything else that
could be improved in relation to this.

Thanks again for your efforts.