bug#69387: 30.0.50; A string shouldn't be both a docstring and a return value

["Mattias Engdegård" <mattias.engdegard@gmail.com>]

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

4 mars 2024 kl. 15.46 skrev Eli Zaretskii <eliz@gnu.org>:

> Do we need to say something about this change in NEWS?

Yes, here is a complete patch.

The cl-lib definition forms (cl-defun, cl-defsubst, cl-defmacro etc) are unaffected by the change as they don't have the dual-use doc-string problem.

The doc-string mechanism change should be sound and safe. The warning is only an annoyance for people writing code like

(defun my-constant-fun ()
  "some-useful-string")

which isn't common but not necessarily bad style either, which would be the main argument against the warning -- the programmer needs to add a doc string or `nil` before the string to silence the compiler.

It's more common to see no-op functions like

(defun my-no-op-fun ()
  "This function is unfinished.")

and here it's probably a good idea to make the programmer be more explicit about the return value.


[-- Attachment #2: 0001-Use-single-string-literal-as-return-value-not-doc-st.patch --]
[-- Type: application/octet-stream, Size: 12944 bytes --]

From 16e3e27d26ab5fcfd61c0e8fbd4aa2a5498058a0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 28 Feb 2024 12:05:19 +0100
Subject: [PATCH] Use single string literal as return value, not doc string

A function or macro body consisting of a single string literal now only
uses it as a return value.  Previously, it was used as a doc string as
well which was never what the programmer intended and had some
inconvenient consequences (bug#69387).

This change also adds a warning for uses of `defun` and `defmacro` (but
not plain `lambda`) that stand to lose their doc string this way, to
alert the programmer of a possible mistake.

* lisp/emacs-lisp/bytecomp.el (byte-compile-lambda):
Don't use a lone string literal as doc string.
* lisp/emacs-lisp/byte-run.el (byte-run--parse-body): Add warning
for `defun` and `defmacro`.
* doc/lispref/functions.texi (Function Documentation): Update manual.
* etc/NEWS: Announce the change and the warning.
---
 doc/lispref/functions.texi                    | 13 ++-
 etc/NEWS                                      | 28 ++++++
 lisp/emacs-lisp/byte-run.el                   | 92 ++++++++++---------
 lisp/emacs-lisp/bytecomp.el                   |  9 +-
 .../bytecomp-resources/fun-attr-warn.el       | 11 ++-
 .../warn-wide-docstring-defun.el              |  3 +-
 test/lisp/emacs-lisp/bytecomp-tests.el        |  4 +-
 7 files changed, 101 insertions(+), 59 deletions(-)

diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
index 344b3ff3a50..ff635fc54b2 100644
--- a/doc/lispref/functions.texi
+++ b/doc/lispref/functions.texi
@@ -498,13 +498,12 @@ Function Documentation
 nice in the source code will look ugly when displayed by the help
 commands.
 
-  You may wonder how the documentation string could be optional, since
-there are required components of the function that follow it (the body).
-Since evaluation of a string returns that string, without any side effects,
-it has no effect if it is not the last form in the body.  Thus, in
-practice, there is no confusion between the first form of the body and the
-documentation string; if the only body form is a string then it serves both
-as the return value and as the documentation.
+  A documentation string must always be followed by at least one Lisp
+expression; otherwise, it is not a documentation string at all but the
+single expression of the body and used as the return value.
+When there is no meaningful value to return from a function, it is
+standard practice to return @code{nil} by adding it after the
+documentation string.
 
   The last line of the documentation string can specify calling
 conventions different from the actual function arguments.  Write
diff --git a/etc/NEWS b/etc/NEWS
index 06856602ea8..c31ebaea74b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1801,6 +1801,17 @@ Tree-sitter conditionally sets 'forward-sexp-function' for major modes
 that have defined 'sexp' in 'treesit-thing-settings' to enable
 sexp-related motion commands.
 
++++
+** Returned strings are never docstrings
+Functions and macros whose body consists of a single string literal now
+only return that string; it is not used as a docstring.
+Previously, that string was used as both a docstring and return value,
+which was never what the programmer wanted.  This change applies to
+'defun', 'defsubst', 'defmacro' and 'lambda' forms.
+If you want the string to be a docstring, add an explicit return value.
+
+See related warning below.
+
 ** New or changed byte-compilation warnings
 
 ---
@@ -1954,6 +1965,23 @@ name 'ignored-return-value'.
 The warning will only be issued for calls to functions declared
 'important-return-value' or 'side-effect-free' (but not 'error-free').
 
+---
+*** Warn about strings that look like docstrings but are just returned
+The compiler now warns about functions and macros where the body
+consists of a single string literal, which is now used as a return value
+only, not as a docstring.  Example:
+
+    (defun sing-a-song ()
+      "Sing a song.")
+
+The above function returns the string '"Sing a song."' but has no
+docstring.  This warning is emitted for 'defun', 'defsubst' and
+'defmacro' declarations, not 'lambda' forms.
+
+To silence the warning, assuming a string literal should be returned,
+add an explicit docstring (or 'nil') before the returned string.
+If the string should be a doc string, return something else (like 'nil').
+
 ---
 *** Warn about docstrings that contain control characters.
 The compiler now warns about docstrings with control characters other
diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index cc176821026..663f405cecd 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -263,49 +263,57 @@ 'byte-run--set-no-font-lock-keyword
 (defalias 'byte-run--parse-body
   #'(lambda (body allow-interactive)
       "Decompose BODY into (DOCSTRING DECLARE INTERACTIVE BODY-REST WARNINGS)."
-      (let* ((top body)
-             (docstring nil)
-             (declare-form nil)
-             (interactive-form nil)
-             (warnings nil)
-             (warn #'(lambda (msg form)
-                       (push (macroexp-warn-and-return
-                              (format-message msg) nil nil t form)
-                             warnings))))
-        (while
-            (and body
-                 (let* ((form (car body))
-                        (head (car-safe form)))
-                   (cond
-                    ((or (and (stringp form) (cdr body))
-                         (eq head :documentation))
-                     (cond
-                      (docstring (funcall warn "More than one doc string" top))
-                      (declare-form
-                       (funcall warn "Doc string after `declare'" declare-form))
-                      (interactive-form
-                       (funcall warn "Doc string after `interactive'"
-                                interactive-form))
-                      (t (setq docstring form)))
-                     t)
-                    ((eq head 'declare)
-                     (cond
-                      (declare-form
-                       (funcall warn "More than one `declare' form" form))
-                      (interactive-form
-                       (funcall warn "`declare' after `interactive'" form))
-                      (t (setq declare-form form)))
-                     t)
-                    ((eq head 'interactive)
+      (if (and (null (cdr body)) (stringp (car body)))
+          (list nil nil nil body
+                (list (macroexp-warn-and-return
+                       "Single string treated as return value, not doc string"
+                       nil nil t body)))
+        (let* ((top body)
+               (docstring nil)
+               (declare-form nil)
+               (interactive-form nil)
+               (warnings nil)
+               (warn #'(lambda (msg form)
+                         (push (macroexp-warn-and-return
+                                (format-message msg) nil nil t form)
+                               warnings))))
+          (while
+              (and body
+                   (let* ((form (car body))
+                          (head (car-safe form)))
                      (cond
-                      ((not allow-interactive)
-                       (funcall warn "No `interactive' form allowed here" form))
-                      (interactive-form
-                       (funcall warn "More than one `interactive' form" form))
-                      (t (setq interactive-form form)))
-                     t))))
-          (setq body (cdr body)))
-        (list docstring declare-form interactive-form body warnings))))
+                      ((or (and (stringp form) (cdr body))
+                           (eq head :documentation))
+                       (cond
+                        (docstring
+                         (funcall warn "More than one doc string" top))
+                        (declare-form
+                         (funcall warn "Doc string after `declare'"
+                                  declare-form))
+                        (interactive-form
+                         (funcall warn "Doc string after `interactive'"
+                                  interactive-form))
+                        (t (setq docstring form)))
+                       t)
+                      ((eq head 'declare)
+                       (cond
+                        (declare-form
+                         (funcall warn "More than one `declare' form" form))
+                        (interactive-form
+                         (funcall warn "`declare' after `interactive'" form))
+                        (t (setq declare-form form)))
+                       t)
+                      ((eq head 'interactive)
+                       (cond
+                        ((not allow-interactive)
+                         (funcall warn "No `interactive' form allowed here"
+                                  form))
+                        (interactive-form
+                         (funcall warn "More than one `interactive' form" form))
+                        (t (setq interactive-form form)))
+                       t))))
+            (setq body (cdr body)))
+          (list docstring declare-form interactive-form body warnings)))))
 
 (defalias 'byte-run--parse-declarations
   #'(lambda (name arglist clauses construct declarations-alist)
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index c3355eedd75..cf0e6d600dd 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -3061,12 +3061,11 @@ byte-compile-lambda
 	  (append (if (not lexical-binding) arglistvars)
                   byte-compile-bound-variables))
 	 (body (cdr (cdr fun)))
-	 (doc (if (stringp (car body))
+         ;; Treat a final string literal as a value, not a doc string.
+	 (doc (if (and (cdr body) (stringp (car body)))
                   (prog1 (car body)
-                    ;; Discard the doc string from the body
-                    ;; unless it is the last element of the body.
-                    (if (cdr body)
-                        (setq body (cdr body))))))
+                    ;; Discard the doc string from the body.
+                    (setq body (cdr body)))))
 	 (int (assq 'interactive body))
          command-modes)
     (when lexical-binding
diff --git a/test/lisp/emacs-lisp/bytecomp-resources/fun-attr-warn.el b/test/lisp/emacs-lisp/bytecomp-resources/fun-attr-warn.el
index be907b32f47..9bec23a81e3 100644
--- a/test/lisp/emacs-lisp/bytecomp-resources/fun-attr-warn.el
+++ b/test/lisp/emacs-lisp/bytecomp-resources/fun-attr-warn.el
@@ -42,9 +42,6 @@ faw-doc-decl-int-code
 
 ;; Correct (last string is return value)
 
-(defun faw-str ()
-  "something")
-
 (defun faw-decl-str ()
   (declare (pure t))
   "something")
@@ -63,6 +60,9 @@ faw-doc-str
   "something else")
 
 
+
+
+
 ;; Incorrect (bad order)
 
 (defun faw-int-decl-code (x)
@@ -264,3 +264,8 @@ faw-doc-int-decl-int-code
   (declare (pure t))
   (interactive "p")
   (print x))
+
+;; Incorrect (ambiguous string)
+
+(defun faw-str ()
+  "something")
diff --git a/test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defun.el b/test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defun.el
index 94b0e80c979..571f7f6f095 100644
--- a/test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defun.el
+++ b/test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defun.el
@@ -1,3 +1,4 @@
 ;;; -*- lexical-binding: t -*-
 (defun foo ()
-  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+  nil)
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index 26408e8685a..f95c4084bf4 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -2052,7 +2052,9 @@ bytecomp-fun-attr-warn
                "258:4: Warning: More than one `interactive' form"
                "257:4: Warning: `declare' after `interactive'"
                "265:4: Warning: More than one `interactive' form"
-               "264:4: Warning: `declare' after `interactive'")))
+               "264:4: Warning: `declare' after `interactive'"
+               "270:2: Warning: Single string treated as return value, not doc string"
+               )))
         (goto-char (point-min))
         (let ((actual nil))
           (while (re-search-forward
-- 
2.32.0 (Apple Git-132)