bug#56197: lisp-fill-paragraph behavior changed in Emacs 28

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

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

Hi Eli et al.,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

[...]

>> I don't see how a user option to control this could be useful, since
>> the preferred behavior is not only buffer-local, but also specific to
>> certain syntactic constructs.  But I won't object to having such an
>> option.
>
> Having the behavior defined per-project or even globally (reverting to
> the the pre-Emacs 28 behavior) via a simple option seems like it'd
> simplify things, and make them discoverable.

I tried fixing this generally, as it seems to me that something in
lisp-mode should be meet the needs of all lisp-derived languages such as
Scheme and not just Elisp.  I first added two tests, one of which
ensures no regression to the original bug that lead to this current
behavioral change (bug#28937) and the other one that should pass once
the issue reported here (bug#56197) is resolved.

The last patch is a WIP that didn't work; I was hoping that inserting
spaces corresponding to the width of the indent in the narrowed string
would cause the indent to be preserved only for the first line.  I don't
have other ideas at the moment; I'd appreciate if someone could tip in.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-test-lisp-Add-new-test-for-filling-behavior-change.patch --]
[-- Type: text/x-patch, Size: 2115 bytes --]

From aacf65440070df2ba356af1d118a50993fc8f865 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Mon, 30 Dec 2024 12:02:36 +0900
Subject: [PATCH 1/3] test/lisp: Add new test for filling behavior change.

Starting with Emacs 28, filling strings now happens in a narrowed scope,
and looses the leading indentation and can cause the string to extend
past the fill-column value (see bug#56197).

* test/lisp/emacs-lisp/lisp-mode-tests.el
(lisp-fill-paragraph-respects-fill-column): New test.
---
 test/lisp/emacs-lisp/lisp-mode-tests.el | 27 +++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/test/lisp/emacs-lisp/lisp-mode-tests.el b/test/lisp/emacs-lisp/lisp-mode-tests.el
index da02be65d03..9a2b1ea4654 100644
--- a/test/lisp/emacs-lisp/lisp-mode-tests.el
+++ b/test/lisp/emacs-lisp/lisp-mode-tests.el
@@ -308,6 +308,33 @@ lisp-indent-defun
       (indent-region (point-min) (point-max))
       (should (equal (buffer-string) orig)))))
 
+\f
+;;; Filling
+
+(ert-deftest lisp-fill-paragraph-respects-fill-column ()
+  "Test bug#56197 -- more specifically, validate that a leading indentation
+for a string is preserved in the filled string."
+  (with-temp-buffer
+    ;; The following is a contrived example that demonstrates the
+    ;; fill-column problem when the string to fill is indented.
+    (insert "\
+\(defun test ()
+                         \"This is a long string which is indented by a considerable value,
+causing it to protrude from the configured `fill-column' since
+lisp-fill-paragraph was refactored in version 28.\"
+  (message \"Hi!\"))")
+    (emacs-lisp-mode)
+    (search-backward "This is a long string")
+    (fill-paragraph)                    ;function under test
+    (goto-char (point-min))
+    (let ((i 1)
+          (lines-count (count-lines (point-min) (point-max))))
+      (while (< i lines-count)
+        (beginning-of-line i)
+        (end-of-line)
+        (should (<= (current-column) fill-column))
+        (setq i (1+ i))))))
+
 \f
 ;;; Fontification
 

base-commit: e32484547d0813665334bfd34b741492dde0d374
-- 
2.46.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-test-lisp-Add-a-test-checking-docstring-boundaries-w.patch --]
[-- Type: text/x-patch, Size: 1436 bytes --]

From e5685497111ef71e57948f023f8d2a03647c3d69 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Mon, 30 Dec 2024 14:26:46 +0900
Subject: [PATCH 2/3] test/lisp: Add a test checking docstring boundaries when
 filling.

* test/lisp/emacs-lisp/lisp-mode-tests.el
(lisp-fill-paragraph-docstring-boundaries): New test.
---
 test/lisp/emacs-lisp/lisp-mode-tests.el | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/test/lisp/emacs-lisp/lisp-mode-tests.el b/test/lisp/emacs-lisp/lisp-mode-tests.el
index 9a2b1ea4654..eac2763c595 100644
--- a/test/lisp/emacs-lisp/lisp-mode-tests.el
+++ b/test/lisp/emacs-lisp/lisp-mode-tests.el
@@ -311,6 +311,25 @@ lisp-indent-defun
 \f
 ;;; Filling
 
+(ert-deftest lisp-fill-paragraph-docstring-boundaries ()
+  "Test bug#28937, ensuring filling the docstring filled is properly
+bounded."
+  (with-temp-buffer
+    (insert "\
+(defun test ()
+  \"This is a test docstring.
+Here is some more text.\"
+  1
+  2
+  3
+  4
+  5)")
+    (let ((correct (buffer-string)))
+      (emacs-lisp-mode)
+      (search-backward "This is a test docstring")
+      (fill-paragraph)                  ;function under test
+      (should (equal (buffer-string) correct)))))
+
 (ert-deftest lisp-fill-paragraph-respects-fill-column ()
   "Test bug#56197 -- more specifically, validate that a leading indentation
 for a string is preserved in the filled string."
-- 
2.46.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-wip-work-on-solution.patch --]
[-- Type: text/x-patch, Size: 3778 bytes --]

From 5bc5de2fc6f7783b0cd71c5945755fc98431aa60 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Tue, 31 Dec 2024 10:18:30 +0900
Subject: [PATCH 3/3] wip: work on solution

---
 lisp/emacs-lisp/lisp-mode.el | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index d0c32d238bc..6c6d88f73a5 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1,6 +1,6 @@
 ;;; lisp-mode.el --- Lisp mode, and its idiosyncratic commands  -*- lexical-binding:t -*-
 
-;; Copyright (C) 1985-1986, 1999-2024 Free Software Foundation, Inc.
+;; Copyright (C) 1985-1986, 1999-2025 Free Software Foundation, Inc.
 
 ;; Maintainer: emacs-devel@gnu.org
 ;; Keywords: lisp, languages
@@ -1480,30 +1480,34 @@ lisp-fill-paragraph
                                   (derived-mode-p 'emacs-lisp-mode))
                              emacs-lisp-docstring-fill-column
                            fill-column)))
-        (let ((ppss (syntax-ppss))
-              (start (point))
-              ;; Avoid recursion if we're being called directly with
-              ;; `M-x lisp-fill-paragraph' in an `emacs-lisp-mode' buffer.
-              (fill-paragraph-function t))
+        (let* ((ppss (syntax-ppss))
+               (start (point))
+               ;; Avoid recursion if we're being called directly with
+               ;; `M-x lisp-fill-paragraph' in an `emacs-lisp-mode' buffer.
+               (fill-paragraph-function t)
+               (string-start (ppss-comment-or-string-start ppss))
+               (indent (save-excursion
+                         (goto-char string-start)
+                         (current-column))))
           (save-excursion
             (save-restriction
               ;; If we're not inside a string, then do very basic
               ;; filling.  This avoids corrupting embedded strings in
               ;; code.
-              (if (not (ppss-comment-or-string-start ppss))
+              (if (not string-start)
                   (lisp--fill-line-simple)
                 ;; If we're in a string, then narrow (roughly) to that
                 ;; string before filling.  This avoids filling Lisp
                 ;; statements that follow the string.
                 (when (ppss-string-terminator ppss)
-                  (goto-char (ppss-comment-or-string-start ppss))
+                  (goto-char string-start)
                   ;; The string may be unterminated -- in that case, don't
                   ;; narrow.
                   (when (ignore-errors
                           (progn
                             (forward-sexp 1)
                             t))
-                    (narrow-to-region (1+ (ppss-comment-or-string-start ppss))
+                    (narrow-to-region (1+ string-start)
                                       (1- (point)))))
                 ;; Move back to where we were.
                 (goto-char start)
@@ -1516,7 +1520,16 @@ lisp-fill-paragraph
                     (goto-char (point-min))
                     (forward-line 1)
                     (narrow-to-region (point) (point-max))))
-	        (fill-paragraph justify)))))))
+                ;; Insert spaces to reproduce the same leading indent
+                ;; for the string inside the narrowed region, to avoid
+                ;; bug#56197.
+                (save-excursion
+                  (goto-char (point-min))
+                  (insert (make-string indent ?\s)))
+	        (fill-paragraph justify)
+                (save-excursion         ;clean up
+                  (goto-char (point-min))
+                  (delete-char indent))))))))
   ;; Never return nil.
   t)
 
-- 
2.46.0


[-- Attachment #5: Type: text/plain, Size: 443 bytes --]


If that's not easily fixable, perhaps I'll submit a patch to the paredit
project so that they stop relying on lisp-paragraph-fill [0] and instead
use the default (which is fill-paragraph).  That's how I came to notice
this behavior change in GNU Guix, which is written in Scheme; using
scheme-mode doesn't exhibit the issue since it's just using
fill-paragraph.

[0]  https://paredit.org/cgit/paredit/tree/paredit.el#n1066

-- 
Thanks,
Maxim