Make byte-compiler warn about wide docstrings

* lisp/emacs-lisp/bytecomp.el (byte-compile--wide-docstring-p):
(byte-compile-docstring-length-warn): New defuns.
(byte-compile-docstring-max-column): New defcustom.
(byte-compile--wide-docstring-substitution-len): New variable.
(byte-compile-warning-types, byte-compile-warnings): New value
'docstrings'.
(byte-compile-file-form-autoload, byte-compile-file-form-defvar):
(byte-compile-file-form-defvar-function, byte-compile-lambda):
(byte-compile-defvar, byte-compile-file-form-defalias): Warn about too
wide docstrings.  (Bug#44858)

* test/lisp/emacs-lisp/bytecomp-tests.el
(bytecomp-warn-wide-docstring/defconst)
(bytecomp-warn-wide-docstring/defvar): New tests.
(bytecomp--define-warning-file-test): New macro.
(bytecomp/warn-wide-docstring-autoload\.el)
(bytecomp/warn-wide-docstring-custom-declare-variable\.el)
(bytecomp/warn-wide-docstring-defalias\.el)
(bytecomp/warn-wide-docstring-defconst\.el)
(bytecomp/warn-wide-docstring-define-abbrev-table\.el)
(bytecomp/warn-wide-docstring-define-obsolete-function-alias\.el)
(bytecomp/warn-wide-docstring-define-obsolete-variable-alias\.el)
(bytecomp/warn-wide-docstring-defun\.el)
(bytecomp/warn-wide-docstring-defvar\.el)
(bytecomp/warn-wide-docstring-defvaralias\.el)
(bytecomp/warn-wide-docstring-ignore-fill-column\.el)
(bytecomp/warn-wide-docstring-ignore-override\.el)
(bytecomp/warn-wide-docstring-ignore\.el)
(bytecomp/warn-wide-docstring-multiline-first\.el)
(bytecomp/warn-wide-docstring-multiline\.el): New tests.
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-autoload.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-custom-declare-variable.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defalias.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defconst.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-define-abbrev-table.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-define-obsolete-function-alias.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-define-obsolete-variable-alias.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defun.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defvar.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defvaralias.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-ignore-fill-column.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-ignore-override.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-ignore.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-multiline-first.el:
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-multiline.el:
New files.

etc/NEWS

@@ -2200,17 +2200,24 @@ menu handling.
 +++
 ** 'inhibit-nul-byte-detection' is renamed to 'inhibit-null-byte-detection'.
 
+** Byte compiler
+
 +++
-** New byte-compiler check for missing dynamic variable declarations.
+*** New byte-compiler check for missing dynamic variable declarations.
 It is meant as an (experimental) aid for converting Emacs Lisp code
 to lexical binding, where dynamic (special) variables bound in one
 file can affect code in another.  For details, see the manual section
 "(Elisp) Converting to Lexical Binding".
 
 +++
-** 'byte-recompile-directory' can now compile symlinked ".el" files.
+*** 'byte-recompile-directory' can now compile symlinked ".el" files.
 This is achieved by giving a non-nil FOLLOW-SYMLINKS parameter.
 
+*** The byte-compiler now warns about too wide documentation strings.
+By default, it will warn if a documentation string is wider than the
+largest of 80 characters or 'fill-column'.  This is controlled by the
+new user option 'byte-compile-docstring-max-column'.
+
 ---
 ** 'unload-feature' now also tries to undo additions to buffer-local hooks.
 

etc/TODO

@@ -638,8 +638,6 @@ Do this for some or all errors associated with using subprocesses.
 ** Maybe reinterpret 'parse-error' as a category of errors
 Put some other errors under it.
 
-** Make byte-compiler warn when a doc string is too wide
-
 ** Make byte-optimization warnings issue accurate line numbers
 
 ** Record the sxhash of the default value for customized variables

lisp/emacs-lisp/bytecomp.el

@@ -299,7 +299,8 @@ The information is logged to `byte-compile-log-buffer'."
 (defconst byte-compile-warning-types
   '(redefine callargs free-vars unresolved
              obsolete noruntime interactive-only
-	     make-local mapcar constants suspicious lexical lexical-dynamic)
+             make-local mapcar constants suspicious lexical lexical-dynamic
+             docstrings)
   "The list of warning types used when `byte-compile-warnings' is t.")
 (defcustom byte-compile-warnings t
   "List of warnings that the byte-compiler should issue (t for all).
@@ -322,6 +323,8 @@ Elements of the list may be:
   make-local  calls to make-variable-buffer-local that may be incorrect.
   mapcar      mapcar called for effect.
   constants   let-binding of, or assignment to, constants/nonvariables.
+  docstrings  docstrings that are too wide (longer than 80 characters,
+              or `fill-column', whichever is bigger)
   suspicious  constructs that usually don't do what the coder wanted.
 
 If the list begins with `not', then the remaining elements specify warnings to
@@ -1563,6 +1566,81 @@ extra args."
            (if (equal sig1 '(1 . 1)) "argument" "arguments")
            (byte-compile-arglist-signature-string sig2)))))))
 
+(defvar byte-compile--wide-docstring-substitution-len 3
+  "Substitution width used in `byte-compile--wide-docstring-p'.
+This is a heuristic for guessing the width of a documentation
+string: `byte-compile--wide-docstring-p' assumes that any
+`substitute-command-keys' command substitutions are this long.")
+
+(defun byte-compile--wide-docstring-p (docstring col)
+  "Return t if string DOCSTRING is wider than COL.
+Ignore all `substitute-command-keys' substitutions, except for
+the `\\\\=[command]' ones that are assumed to be of length
+`byte-compile--wide-docstring-substitution-len'.  Also ignore
+URLs."
+  (string-match
+   (format "^.\\{%s,\\}$" (int-to-string (1+ col)))
+   (replace-regexp-in-string
+    (rx (or
+         ;; Ignore some URLs.
+         (seq "http" (? "s") "://" (* anychar))
+         ;; Ignore these `substitute-command-keys' substitutions.
+         (seq "\\" (or "="
+                       (seq "<" (* (not ">")) ">")
+                       (seq "{" (* (not "}")) "}")))))
+    ""
+    ;; Heuristic: assume these substitutions are of some length N.
+    (replace-regexp-in-string
+     (rx "\\" (or (seq "[" (* (not "]")) "]")))
+     (make-string byte-compile--wide-docstring-substitution-len ?x)
+     docstring))))
+
+(defcustom byte-compile-docstring-max-column 80
+  "Recommended maximum width of doc string lines.
+The byte-compiler will emit a warning for documentation strings
+containing lines wider than this.  If `fill-column' has a larger
+value, it will override this variable."
+  :group 'bytecomp
+  :type 'integer
+  :safe #'integerp
+  :version "28.1")
+
+(defun byte-compile-docstring-length-warn (form)
+  "Warn if documentation string of FORM is too wide.
+It is too wide if it has any lines longer than the largest of
+`fill-column' and `byte-compile-docstring-max-column'."
+  ;; This has some limitations that it would be nice to fix:
+  ;; 1. We don't try to handle defuns.  It is somewhat tricky to get
+  ;;    it right since `defun' is a macro.  Also, some macros
+  ;;    themselves produce defuns (e.g. `define-derived-mode').
+  ;; 2. We assume that any `subsititute-command-keys' command replacement has a
+  ;;    given length.  We can't reliably do these replacements, since the value
+  ;;    of the keymaps in general can't be known at compile time.
+  (when (byte-compile-warning-enabled-p 'docstrings)
+    (let ((col (max byte-compile-docstring-max-column fill-column))
+          kind name docs)
+      (pcase (car form)
+        ((or 'autoload 'custom-declare-variable 'defalias
+             'defconst 'define-abbrev-table
+             'defvar 'defvaralias)
+         (setq kind (nth 0 form))
+         (setq name (nth 1 form))
+         (setq docs (nth 3 form)))
+        ;; Here is how one could add lambda's here:
+        ;; ('lambda
+        ;;   (setq kind "")   ; can't be "function", unfortunately
+        ;;   (setq docs (and (stringp (nth 2 form))
+        ;;                   (nth 2 form))))
+        )
+      (when (and (consp name) (eq (car name) 'quote))
+        (setq name (cadr name)))
+      (setq name (if name (format " `%s'" name) ""))
+      (when (and kind docs (stringp docs)
+                 (byte-compile--wide-docstring-p docs col))
+        (byte-compile-warn "%s%s docstring wider than %s characters"
+                           kind name col))))
+  form)
+
 (defun byte-compile-print-syms (str1 strn syms)
   (when syms
     (byte-compile-set-symbol-position (car syms) t))
@@ -2410,7 +2488,8 @@ list that represents a doc string reference.
              (delq (assq funsym byte-compile-unresolved-functions)
                    byte-compile-unresolved-functions)))))
   (if (stringp (nth 3 form))
-      form
+      (prog1 form
+        (byte-compile-docstring-length-warn form))
     ;; No doc string, so we can compile this as a normal form.
     (byte-compile-keep-pending form 'byte-compile-normal-call)))
 
@@ -2438,6 +2517,7 @@ list that represents a doc string reference.
   (if (and (null (cddr form))		;No `value' provided.
            (eq (car form) 'defvar))     ;Just a declaration.
       nil
+    (byte-compile-docstring-length-warn form)
     (cond ((consp (nth 2 form))
            (setq form (copy-sequence form))
            (setcar (cdr (cdr form))
@@ -2461,6 +2541,7 @@ list that represents a doc string reference.
        (if (byte-compile-warning-enabled-p 'suspicious)
            (byte-compile-warn
             "Alias for `%S' should be declared before its referent" newname)))))
+  (byte-compile-docstring-length-warn form)
   (byte-compile-keep-pending form))
 
 (put 'custom-declare-variable 'byte-hunk-handler
@@ -2844,6 +2925,7 @@ for symbols generated by the byte compiler itself."
     (unless (eq 'lambda (car-safe fun))
       (error "Not a lambda list: %S" fun))
     (byte-compile-set-symbol-position 'lambda))
+  (byte-compile-docstring-length-warn fun)
   (byte-compile-check-lambda-list (nth 1 fun))
   (let* ((arglist (nth 1 fun))
          (arglistvars (byte-compile-arglist-vars arglist))
@@ -4624,6 +4706,7 @@ binding slots have been popped."
              (byte-compile-warning-enabled-p 'lexical (nth 1 form)))
     (byte-compile-warn "global/dynamic var `%s' lacks a prefix"
                        (nth 1 form)))
+  (byte-compile-docstring-length-warn form)
   (let ((fun (nth 0 form))
 	(var (nth 1 form))
 	(value (nth 2 form))
@@ -4698,6 +4781,7 @@ binding slots have been popped."
       ;; - `arg' is the expression to which it is defined.
       ;; - `rest' is the rest of the arguments.
       (`(,_ ',name ,arg . ,rest)
+       (byte-compile-docstring-length-warn form)
        (pcase-let*
            ;; `macro' is non-nil if it defines a macro.
            ;; `fun' is the function part of `arg' (defaults to `arg').

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-autoload.el

@@ -0,0 +1,3 @@
+;;; -*- lexical-binding: t -*-
+(autoload 'foox "foo"
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-custom-declare-variable.el

@@ -0,0 +1,4 @@
+;;; -*- lexical-binding: t -*-
+(custom-declare-variable
+ 'foo t
+ "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defalias.el

@@ -0,0 +1,3 @@
+;;; -*- lexical-binding: t -*-
+(defalias 'foo #'ignore
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defconst.el

@@ -0,0 +1,3 @@
+;;; -*- lexical-binding: t -*-
+(defconst foo-bar nil
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-define-abbrev-table.el

@@ -0,0 +1,3 @@
+;;; -*- lexical-binding: t -*-
+(define-abbrev-table 'foo ()
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-define-obsolete-function-alias.el

@@ -0,0 +1,3 @@
+;;; -*- lexical-binding: t -*-
+(define-obsolete-function-alias 'foo #'ignore "99.1"
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-define-obsolete-variable-alias.el

@@ -0,0 +1,3 @@
+;;; -*- lexical-binding: t -*-
+(define-obsolete-variable-alias 'foo 'ignore "99.1"
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defun.el

@@ -0,0 +1,3 @@
+;;; -*- lexical-binding: t -*-
+(defun foo ()
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defvar.el

@@ -0,0 +1,6 @@
+;;; -*- lexical-binding: t -*-
+(defvar foo-bar nil
+  "multiline
+foo
+xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+bar")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defvaralias.el

@@ -0,0 +1,3 @@
+;;; -*- lexical-binding: t -*-
+(defvaralias 'foo-bar #'ignore
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-ignore-fill-column.el

@@ -0,0 +1,7 @@
+;;; -*- lexical-binding: t -*-
+(defvar foo-bar nil
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+
+;; Local Variables:
+;; fill-column: 100
+;; End:

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-ignore-override.el

@@ -0,0 +1,8 @@
+;;; -*- lexical-binding: t -*-
+(defvar foo-bar nil
+  "123456789012345")
+
+;; Local Variables:
+;; byte-compile-docstring-max-column: 10
+;; fill-column: 20
+;; End:

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-ignore.el

@@ -0,0 +1,7 @@
+;;; -*- lexical-binding: t -*-
+(defvar foo-bar nil
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+
+;; Local Variables:
+;; byte-compile-docstring-max-column: 100
+;; End:

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-multiline-first.el

@@ -0,0 +1,5 @@
+;;; -*- lexical-binding: t -*-
+(defvar foo-bar nil
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+This is a multiline docstring where the first line is long.
+foobar")

test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-multiline.el

@@ -0,0 +1,6 @@
+;;; -*- lexical-binding: t -*-
+(defvar foo-bar nil
+  "This is a multiline docstring.
+But it's not the first line that is long.
+xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+foobar")

test/lisp/emacs-lisp/bytecomp-tests.el

@@ -540,6 +540,16 @@ Subtests signal errors if something goes wrong."
   (bytecomp--with-warning-test "foo.*lacks a prefix"
     '(defvar foo nil)))
 
+(defvar bytecomp-tests--docstring (make-string 100 ?x))
+
+(ert-deftest bytecomp-warn-wide-docstring/defconst ()
+  (bytecomp--with-warning-test "defconst.*foo.*wider than.*characters"
+    `(defconst foo t ,bytecomp-tests--docstring)))
+
+(ert-deftest bytecomp-warn-wide-docstring/defvar ()
+  (bytecomp--with-warning-test "defvar.*foo.*wider than.*characters"
+    `(defvar foo t ,bytecomp-tests--docstring)))
+
 (defmacro bytecomp--define-warning-file-test (file re-warning &optional reverse)
   `(ert-deftest ,(intern (format "bytecomp/%s" file)) ()
      :expected-result ,(if reverse :failed :passed)
@@ -639,6 +649,67 @@ Subtests signal errors if something goes wrong."
 (bytecomp--define-warning-file-test "warn-variable-set-nonvariable.el"
                             "variable reference to nonvariable")
 
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-autoload.el"
+ "autoload.*foox.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-custom-declare-variable.el"
+ "custom-declare-variable.*foo.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-defalias.el"
+ "defalias.*foo.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-defconst.el"
+ "defconst.*foo.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-define-abbrev-table.el"
+ "define-abbrev.*foo.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-define-obsolete-function-alias.el"
+ "defalias.*foo.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-define-obsolete-variable-alias.el"
+ "defvaralias.*foo.*wider than.*characters")
+
+;; TODO: We don't yet issue warnings for defuns.
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-defun.el"
+ "wider than.*characters" 'reverse)
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-defvar.el"
+ "defvar.*foo.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-defvaralias.el"
+ "defvaralias.*foo.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-ignore-fill-column.el"
+ "defvar.*foo.*wider than.*characters" 'reverse)
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-ignore-override.el"
+ "defvar.*foo.*wider than.*characters" 'reverse)
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-ignore.el"
+ "defvar.*foo.*wider than.*characters" 'reverse)
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-multiline-first.el"
+ "defvar.*foo.*wider than.*characters")
+
+(bytecomp--define-warning-file-test
+ "warn-wide-docstring-multiline.el"
+ "defvar.*foo.*wider than.*characters")
+
 
 ;;;; Macro expansion.