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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Package: Emacs
Version: 30.0.50


Currently, ELisp defines

    (lambda (blabla) "Help!")

as a function that returns "Help!" *and* whose docstring is "Help!".
As seen in commit eeb89a5cb292bffe40ba7d0b0cf81f82f8452bf8, it can be
a source of annoyance as well.

I cannot remember finding source code which makes use of this "feature".
My impression is that our docs document this behavior simply because
that's how it happened to work rather than how it should work.
This is documented in the texinfo under "Function Documentation" where
it says:

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

I think we should change that to say that

    if the only body form is a string then it serves as the return value
    and not as the documentation.

This will/would require a few changes to `macroexp.el` and
`bytecomp.el`, but it should be minor.  It shouldn't introduce any
significant breakage either because the only effect will be to make it
so some functions won't have a docstring any more, but most (all?) of
those function never expected to have a docstring in the first place.


        Stefan

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

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca
> Date: Sun, 25 Feb 2024 11:29:45 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Currently, ELisp defines
> 
>     (lambda (blabla) "Help!")
> 
> as a function that returns "Help!" *and* whose docstring is "Help!".
> As seen in commit eeb89a5cb292bffe40ba7d0b0cf81f82f8452bf8, it can be
> a source of annoyance as well.
> 
> I cannot remember finding source code which makes use of this "feature".
> My impression is that our docs document this behavior simply because
> that's how it happened to work rather than how it should work.
> This is documented in the texinfo under "Function Documentation" where
> it says:
> 
>     [...
>     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.
> 
> I think we should change that to say that
> 
>     if the only body form is a string then it serves as the return value
>     and not as the documentation.
> 
> This will/would require a few changes to `macroexp.el` and
> `bytecomp.el`, but it should be minor.  It shouldn't introduce any
> significant breakage either because the only effect will be to make it
> so some functions won't have a docstring any more, but most (all?) of
> those function never expected to have a docstring in the first place.

Here again we are changing age-old (mis)features, which some code
somewhere probably relies upon.  Why? because a newly-introduced
byte-compilation warning erroneously flagged that as a mistake in the
doc string.

My vote is to leave this alone.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Stefan,

Stefan Monnier writes:

> Package: Emacs
> Version: 30.0.50
>
>
> Currently, ELisp defines
>
>     (lambda (blabla) "Help!")
>
> as a function that returns "Help!" *and* whose docstring is "Help!".
> As seen in commit eeb89a5cb292bffe40ba7d0b0cf81f82f8452bf8, it can be
> a source of annoyance as well.
>
> I cannot remember finding source code which makes use of this "feature".

vc-mode kind of abuses of this feature:

--8<---------------cut here---------------start------------->8---
(defun vc-mode (&optional _arg)
  ;; Dummy function for C-h m
  "Version Control minor mode.
This minor mode is automatically activated whenever you visit a file under
control of one of the revision control systems in `vc-handled-backends'.
VC commands are globally reachable under the prefix \\[vc-prefix-map]:
\\{vc-prefix-map}")
--8<---------------cut here---------------end--------------->8---

Here the assumption is that the string is used as a doc string, which
indeed leads to unexpected results, see Bug#65092.


Best,

Eshel

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Here again we are changing age-old (mis)features, which some code
> somewhere probably relies upon.  Why? because a newly-introduced
> byte-compilation warning erroneously flagged that as a mistake in the
> doc string.
>
> My vote is to leave this alone.

There is no benefit to this misfeature.
Leaving this alone will maintain a low-level of pain for the
foreseeable future.  Fixing it may introduce minor breakage (like
`C-h m` saying there's no docstring) in the short term but these are
easy to fix and they'll disappear quickly.


        Stefan

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

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:

> Hi Stefan,
>
> Stefan Monnier writes:
>
>> Package: Emacs
>> Version: 30.0.50
>>
>>
>> Currently, ELisp defines
>>
>>     (lambda (blabla) "Help!")
>>
>> as a function that returns "Help!" *and* whose docstring is "Help!".
>> As seen in commit eeb89a5cb292bffe40ba7d0b0cf81f82f8452bf8, it can be
>> a source of annoyance as well.
>>
>> I cannot remember finding source code which makes use of this "feature".
>
> vc-mode kind of abuses of this feature:
>
> (defun vc-mode (&optional _arg)
>   ;; Dummy function for C-h m
>   "Version Control minor mode.
> This minor mode is automatically activated whenever you visit a file under
> control of one of the revision control systems in `vc-handled-backends'.
> VC commands are globally reachable under the prefix \\[vc-prefix-map]:
> \\{vc-prefix-map}")

But cases like these can be easily resolved, e.g. just by adding a nil
to the end of the body.

> Here the assumption is that the string is used as a doc string, which
> indeed leads to unexpected results, see Bug#65092.
>
>
> Best,
>
> Eshel
>
>
>
>

-- 
	Philip Kaludercic on peregrine

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

[Mattias Engdegård]

> There is no benefit to this misfeature.
> Leaving this alone will maintain a low-level of pain for the
> foreseeable future.  Fixing it may introduce minor breakage (like
> `C-h m` saying there's no docstring) in the short term but these are
> easy to fix and they'll disappear quickly.

Indeed. Eli is right that we need to be careful, but a mechanised scan of our code really shows that this might be a source of bugs that we should consider doing something about. An amusing example:

> (defun semantic-mrub-read-history nil
>   "History of `semantic-mrub-completing-read'.")

... which is then not called but used as a variable by the code.

Or this one:

> ;; Don't alias this to `ignore', as that will cause the resulting
> ;; function to be interactive.
> (defun use-package-normalize/:disabled (_name _keyword _arg)
>   "Do nothing, return nil.")


Return nil my foot.

Would it be productive to warn, and about what? Not for pure lambdas, because there should be nothing wrong with

  (lambda () "string")

and it's extremely unlikely to be intended as a doc string (I found not a single instance).

The overwhelming majority of docstring-value `defun`s are functions that are placeholders, unfinished or unimplemented, sometimes with commented-out code. Often it's clear that the user didn't know how to proceed:

This indicates that maybe we should warn for `defun`; users can easily add a `nil` before or (more commonly) after the string depending on what they mean.

The docstring-result syndrome is particularly common in `cl-defgeneric` because it's not seen as code that will ever be used, so we should probably not warn in that case.

We could expand

 (defun (ARGS) "string") -> (defun (ARGS) "string" "string")

to preserve semantics (and the same for defmacro, cl-defun, etc).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> (defun semantic-mrub-read-history nil
>>   "History of `semantic-mrub-completing-read'.")
>
> ... which is then not called but used as a variable by the code.

Looks like a defvar-vs-defun typo.

> Or this one:
>
>> ;; Don't alias this to `ignore', as that will cause the resulting
>> ;; function to be interactive.
>> (defun use-package-normalize/:disabled (_name _keyword _arg)
>>   "Do nothing, return nil.")
> Return nil my foot.

:-)

> The docstring-result syndrome is particularly common in
> `cl-defgeneric` because it's not seen as code that will ever be used,
> so we should probably not warn in that case.

I don't understand: (cl-defgeneric FUN (ARGS) "DOCSTRING") should not
generate a function that returns "DOCSTRING" (it should just declare
FOO to be a generic function with no methods).

> We could expand
>
>  (defun (ARGS) "string") -> (defun (ARGS) "string" "string")
>
> to preserve semantics (and the same for defmacro, cl-defun, etc).

I still haven't seen any code that wants this behavior, so that's
a definite no for me.


        Stefan

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

[Mattias Engdegård]

26 feb. 2024 kl. 18.44 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> I don't understand: (cl-defgeneric FUN (ARGS) "DOCSTRING") should not
> generate a function that returns "DOCSTRING" (it should just declare
> FOO to be a generic function with no methods).

Sorry, you are quite right; there is no function generated there.

>> (defun (ARGS) "string") -> (defun (ARGS) "string" "string")
>> 
>> to preserve semantics (and the same for defmacro, cl-defun, etc).
> 
> I still haven't seen any code that wants this behavior, so that's
> a definite no for me.

I presume you mean that `defun`s with a single string always want it to be their doc string? That indeed seems to be the dominant use. These are the few counter-examples that I could find:

> (cl-defmethod gnus-search-transform ((_ gnus-search-imap)
> 				     (_query null))
>   "ALL")

> (cl-defmethod gnus-search-transform ((_engine gnus-search-notmuch)
> 				     (_query null))
>   "*")

> (cl-defmethod xref-location-group ((_ xref-bogus-location)) "(No location)")

> (defun xselect-convert-xm-special (_selection _type _value)
>   "")

There is also the concern that an otherwise empty function with only a doc string could be important for returning a non-nil value. It would take quite some digging to find out whether that occurs so I didn't do it now.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> (defun (ARGS) "string") -> (defun (ARGS) "string" "string")
>>> to preserve semantics (and the same for defmacro, cl-defun, etc).
>> I still haven't seen any code that wants this behavior, so that's
>> a definite no for me.
> I presume you mean that `defun`s with a single string always want it to be
> their doc string? That indeed seems to be the dominant use. These are the
> few counter-examples that I could find:

No, I mean that we should not keep the old dual-use semantics, even if
it means some docstrings get temporarily "lost" until the code is fixed.

As for whether (defun (ARGS) "string") should treat the string as
a docstring or a return value, I'd rather keep it as a return value, so
it's the same as for `lambda`.  Another reason is because, the kind of
failures it is likely to introduce is a lot more mild: it's much safer
to remove a docstring than to change the return value.

So far, the harder part of such a change (beside getting it accepted 🙂)
seems to be the font-lock highlighting.


        Stefan

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

[Mattias Engdegård]

26 feb. 2024 kl. 19.17 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> No, I mean that we should not keep the old dual-use semantics, even if
> it means some docstrings get temporarily "lost" until the code is fixed.

Yes, that's fine. Those aren't likely to have many avid readers.

> As for whether (defun (ARGS) "string") should treat the string as
> a docstring or a return value, I'd rather keep it as a return value, so
> it's the same as for `lambda`.  Another reason is because, the kind of
> failures it is likely to introduce is a lot more mild: it's much safer
> to remove a docstring than to change the return value.

I agree. I'll make a patch for the warning, just so that we can see what it would look like.

And will let you worry about the highlighting then!

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

[Mattias Engdegård]

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

> I agree. I'll make a patch for the warning, just so that we can see what it would look like.

Here, belatedly. It includes the lambda string change. The warning includes `defun` and `defmacro` but not plain lambdas.

Removing doc strings from ambiguous lambdas didn't break anything, as expected. I think the warning makes sense, because it does expose some mistakes where doc strings were returned but shouldn't be, or returned strings also exposed as (nonsensical) doc strings.

There are quite a few cl-defmethod forms with this syndrome but those definitions never were ambiguous (the strings  are always a return value, never doc string).


[-- Attachment #2: docstring-result-w.diff --]
[-- Type: application/octet-stream, Size: 2117 bytes --]

diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index cc176821026..42a25bcc0aa 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -263,6 +263,11 @@ '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)."
+      (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)
@@ -305,7 +310,7 @@ 'byte-run--parse-body
                         (t (setq interactive-form form)))
                        t))))
             (setq body (cdr body)))
-        (list docstring declare-form interactive-form body warnings))))
+          (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

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Mon, 4 Mar 2024 15:28:51 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  69387@debbugs.gnu.org,
>  Philip Kaludercic <philipk@posteo.net>
> 
> > I agree. I'll make a patch for the warning, just so that we can see what it would look like.
> 
> Here, belatedly. It includes the lambda string change. The warning includes `defun` and `defmacro` but not plain lambdas.
> 
> Removing doc strings from ambiguous lambdas didn't break anything, as expected. I think the warning makes sense, because it does expose some mistakes where doc strings were returned but shouldn't be, or returned strings also exposed as (nonsensical) doc strings.
> 
> There are quite a few cl-defmethod forms with this syndrome but those definitions never were ambiguous (the strings  are always a return value, never doc string).

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

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

[Mattias Engdegård]

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

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Tue, 5 Mar 2024 14:16:50 +0100
> Cc: monnier@iro.umontreal.ca,
>  69387@debbugs.gnu.org,
>  philipk@posteo.net
> 
> +** 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.

Thanks, but please always remember to end the heading lines in NEWS
with a period.

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

[Mattias Engdegård]

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

5 mars 2024 kl. 16.38 skrev Eli Zaretskii <eliz@gnu.org>:

> Thanks, but please always remember to end the heading lines in NEWS
> with a period.

Thank you, corrected in the patch.

I'm rowing back on the warning; it's probably more annoying than detecting actual errors, so it's been dropped from the patch.


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

From dd2aeed849d4f3c72ab37f942700f8f79f1439c2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 6 Mar 2024 12:03:06 +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).

* lisp/emacs-lisp/bytecomp.el (byte-compile-lambda):
Don't use a lone string literal as doc string.
* test/lisp/emacs-lisp/bytecomp-resources/warn-wide-docstring-defun.el
(foo): Update docstring warning test.
* doc/lispref/functions.texi (Function Documentation): Update manual.
* etc/NEWS: Announce the change.
---
 doc/lispref/functions.texi                       | 13 ++++++-------
 etc/NEWS                                         | 16 ++++++++++++++++
 lisp/emacs-lisp/bytecomp.el                      |  9 ++++-----
 .../warn-wide-docstring-defun.el                 |  3 ++-
 4 files changed, 28 insertions(+), 13 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 fd957fdb115..73faeb20126 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1805,6 +1805,22 @@ 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 bodies consist of a single string literal now
+only return that string; it is not used as a docstring.  Example:
+
+    (defun sing-a-song ()
+      "Sing a song.")
+
+The above function returns the string '"Sing a song."' but has no
+docstring.  Previously, that string was used as both a docstring and
+return value, which was never what the programmer wanted.  If you want
+the string to be a docstring, add an explicit return value.
+
+This change applies to 'defun', 'defsubst', 'defmacro' and 'lambda'
+forms; other defining forms such as 'cl-defun' already worked this way.
+
 ** New or changed byte-compilation warnings
 
 ---
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/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)
-- 
2.32.0 (Apple Git-132)

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

[Mattias Engdegård]

> I'm rowing back on the warning; it's probably more annoying than detecting actual errors, so it's been dropped from the patch.

This seems safe and desirable enough and has now been pushed to master.
It allowed us to revert the work-around in cc-defs.el that silenced a doc-string working.