bug#26619: 26.0.50; Wrong indentation in emacs-lisp-mode

[npostavs]

tags 26619 patch
quit

Tino Calancha <tino.calancha@gmail.com> writes:

> Write a file /tmp/test.el with contents:
> (defun test ()
>   "This is a test.
> Test indentation in emacs-lisp-mode"
>   (message "Hi!"))
>
> emacs -Q /tmp/test.el
> C-x h TAB ; Wrong indentation.
>
> This happens after commit 6fa9cc0593150a318f0e08e69ec10672d548a7c1
> ; Merge: improve indent-sexp and lisp-indent-region performance

Oops, right.  I made lisp-indent-region keep a running parse state like
indent-sexp, but I should have taken the indent-stack too.  Here's a
patch:

>From bb640d2f1570b38d98950fb0c374f66ef0988bd2 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 23 Apr 2017 10:43:05 -0400
Subject: [PATCH v1] Fix lisp-indent-region with multiline string literals
 (Bug#26619)

* lisp/emacs-lisp/lisp-mode.el (lisp-mode--indent-cache): New
function, extracted from `indent-sexp'.
(lisp-indent-region, indent-sexp): Use it.
* test/lisp/emacs-lisp/lisp-mode-tests.el
(lisp-mode-tests--correctly-indented-sexp): Extract literal to constant.
(lisp-indent-region, lisp-indent-region-defun-with-docstring): New
tests.
---
 lisp/emacs-lisp/lisp-mode.el            | 80 ++++++++++++++++++---------------
 test/lisp/emacs-lisp/lisp-mode-tests.el | 57 ++++++++++++++++++++---
 2 files changed, 97 insertions(+), 40 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index fa931e76ad..d619dd8f4c 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -764,6 +764,36 @@ lisp-ppss
         (parse-partial-sexp (car (last (nth 9 pss))) pos)
       pss)))
 
+(defun lisp-mode--indent-cache (init-depth)
+  "Returns a closure that computes indentation, caching by depth."
+  (let ((indent-stack (list nil))
+        (last-depth init-depth))
+    (lambda (&optional depth-or-state)
+      "Pass depth to update cache, or parse state for indentation."
+      (if (listp depth-or-state)        ; It's a parse state.
+          (let ((val (if (car indent-stack) indent-stack
+                       (calculate-lisp-indent depth-or-state))))
+            (cond ((nth 3 depth-or-state) nil) ; Inside a string.
+                  ((integerp val)
+                   (setf (car indent-stack) val))
+                  ((consp val)    ; (COLUMN CONTAINING-SEXP-START)
+                   (car val))
+                  ;; This only happens if we're in a string.
+                  (t (error "This shouldn't happen"))))
+        (let ((depth depth-or-state))   ; It's a depth.
+          (when (< depth init-depth)
+            (setq indent-stack (nconc indent-stack
+                                      (make-list (- init-depth depth) nil))
+                  last-depth (- last-depth depth)
+                  depth init-depth))
+          (let ((depth-delta (- depth last-depth)))
+            (cond ((< depth-delta 0)
+                   (setq indent-stack (nthcdr (- depth-delta) indent-stack)))
+                  ((> depth-delta 0)
+                   (setq indent-stack (nconc (make-list depth-delta nil)
+                                             indent-stack))))
+            (setq last-depth depth)))))))
+
 (defun lisp-indent-region (start end)
   "Indent region as Lisp code, efficiently."
   (save-excursion
@@ -773,12 +803,13 @@ lisp-indent-region
     ;; parse state, which forces each indent call to reparse from the
     ;; beginning.  That has O(n^2) complexity.
     (let* ((parse-state (lisp-ppss start))
+           (calc-indent (lisp-mode--indent-cache (car parse-state)))
            (last-syntax-point start)
            (pr (unless (minibufferp)
                  (make-progress-reporter "Indenting region..." (point) end))))
       (while (< (point) end)
         (unless (and (bolp) (eolp))
-          (lisp-indent-line parse-state))
+          (lisp-indent-line (funcall calc-indent parse-state)))
         (forward-line 1)
         (let ((last-sexp (nth 2 parse-state)))
           (setq parse-state (parse-partial-sexp last-syntax-point (point)
@@ -788,16 +819,18 @@ lisp-indent-region
           (unless (nth 2 parse-state)
             (setf (nth 2 parse-state) last-sexp))
           (setq last-syntax-point (point)))
+        ;; Update cache's depth stack.
+        (funcall calc-indent (car parse-state))
         (and pr (progress-reporter-update pr (point))))
       (and pr (progress-reporter-done pr))
       (move-marker end nil))))
 
-(defun lisp-indent-line (&optional parse-state)
+(defun lisp-indent-line (&optional indent)
   "Indent current line as Lisp code."
   (interactive)
   (let ((pos (- (point-max) (point)))
         (indent (progn (beginning-of-line)
-                       (calculate-lisp-indent (or parse-state (lisp-ppss))))))
+                       (or indent (calculate-lisp-indent (lisp-ppss))))))
     (skip-chars-forward " \t")
     (if (or (null indent) (looking-at "\\s<\\s<\\s<"))
        ;; Don't alter indentation of a ;;; comment line
@@ -1117,15 +1150,12 @@ indent-sexp
 If optional arg ENDPOS is given, indent each line, stopping when
 ENDPOS is encountered."
   (interactive)
-  (let* ((indent-stack (list nil))
-         ;; Use `syntax-ppss' to get initial state so we don't get
+  (let* (;; Use `syntax-ppss' to get initial state so we don't get
          ;; confused by starting inside a string.  We don't use
          ;; `syntax-ppss' in the loop, because this is measurably
          ;; slower when we're called on a long list.
          (state (syntax-ppss))
-         (init-depth (car state))
-         (next-depth init-depth)
-         (last-depth init-depth)
+         (calc-indent (lisp-mode--indent-cache (car state)))
          (last-syntax-point (point)))
     ;; We need a marker because we modify the buffer
     ;; text preceding endpos.
@@ -1152,48 +1182,28 @@ indent-sexp
             (setq last-sexp (or (nth 2 state) last-sexp))
             (setq last-syntax-point (point)))
           (setf (nth 2 state) last-sexp))
-        (setq next-depth (car state))
+        ;; Update cache's depth stack.
+        (funcall calc-indent (car state))
         ;; If the line contains a comment indent it now with
         ;; `indent-for-comment'.
         (when (nth 4 state)
           (indent-for-comment)
           (end-of-line))
         (setq last-syntax-point (point))
-        (when (< next-depth init-depth)
-          (setq indent-stack (nconc indent-stack
-                                    (make-list (- init-depth next-depth) nil))
-                last-depth (- last-depth next-depth)
-                next-depth init-depth))
         ;; Now indent the next line according to what we learned from
         ;; parsing the previous one.
         (forward-line 1)
         (when (< (point) endpos)
-          (let ((depth-delta (- next-depth last-depth)))
-            (cond ((< depth-delta 0)
-                   (setq indent-stack (nthcdr (- depth-delta) indent-stack)))
-                  ((> depth-delta 0)
-                   (setq indent-stack (nconc (make-list depth-delta nil)
-                                             indent-stack))))
-            (setq last-depth next-depth))
           ;; But not if the line is blank, or just a comment (we
           ;; already called `indent-for-comment' above).
           (skip-chars-forward " \t")
           (unless (or (eolp) (eq (char-syntax (char-after)) ?<))
             (indent-line-to
-             (or (car indent-stack)
-                 ;; The state here is actually to the end of the
-                 ;; previous line, but that's fine for our purposes.
-                 ;; And parsing over the newline would only destroy
-                 ;; element 2 (last sexp position).
-                 (let ((val (calculate-lisp-indent state)))
-                   (cond ((integerp val)
-                          (setf (car indent-stack) val))
-                         ((consp val) ; (COLUMN CONTAINING-SEXP-START)
-                          (car val))
-                         ;; `calculate-lisp-indent' only returns nil
-                         ;; when we're in a string, but this won't
-                         ;; happen because we skip strings above.
-                         (t (error "This shouldn't happen!"))))))))))
+             ;; The state here is actually to the end of the
+             ;; previous line, but that's fine for our purposes.
+             ;; And parsing over the newline would only destroy
+             ;; element 2 (last sexp position).
+             (funcall calc-indent state))))))
     (move-marker endpos nil)))
 
 (defun indent-pp-sexp (&optional arg)
diff --git a/test/lisp/emacs-lisp/lisp-mode-tests.el 
b/test/lisp/emacs-lisp/lisp-mode-tests.el
index 27f0bb5ec1..3c6364acc8 100644
--- a/test/lisp/emacs-lisp/lisp-mode-tests.el
+++ b/test/lisp/emacs-lisp/lisp-mode-tests.el
@@ -21,10 +21,7 @@
 (require 'cl-lib)
 (require 'lisp-mode)
 
-(ert-deftest indent-sexp ()
-  "Test basics of \\[indent-sexp]."
-  (with-temp-buffer
-    (insert "\
+(defconst lisp-mode-tests--correctly-indented-sexp "\
 \(a
  (prog1
      (prog1
@@ -42,9 +39,14 @@
   2)                                    ; comment
  ;; comment
  b)")
+
+(ert-deftest indent-sexp ()
+  "Test basics of \\[indent-sexp]."
+  (with-temp-buffer
+    (insert lisp-mode-tests--correctly-indented-sexp)
     (goto-char (point-min))
     (let ((indent-tabs-mode nil)
-          (correct (buffer-string)))
+          (correct lisp-mode-tests--correctly-indented-sexp))
       (dolist (mode '(fundamental-mode emacs-lisp-mode))
         (funcall mode)
         (indent-sexp)
@@ -97,5 +99,50 @@
       (indent-sexp)
       (should (equal (buffer-string) correct)))))
 
+(ert-deftest lisp-indent-region ()
+  "Test basics of `lisp-indent-region'."
+  (with-temp-buffer
+    (insert lisp-mode-tests--correctly-indented-sexp)
+    (goto-char (point-min))
+    (let ((indent-tabs-mode nil)
+          (correct lisp-mode-tests--correctly-indented-sexp))
+      (emacs-lisp-mode)
+      (indent-region (point-min) (point-max))
+      ;; Don't mess up correctly indented code.
+      (should (string= (buffer-string) correct))
+      ;; Correctly add indentation.
+      (save-excursion
+        (while (not (eobp))
+          (delete-horizontal-space)
+          (forward-line)))
+      (indent-region (point-min) (point-max))
+      (should (equal (buffer-string) correct))
+      ;; Correctly remove indentation.
+      (save-excursion
+        (let ((n 0))
+          (while (not (eobp))
+            (unless (looking-at "noindent\\|^[[:blank:]]*$")
+              (insert (make-string n ?\s)))
+            (cl-incf n)
+            (forward-line))))
+      (indent-region (point-min) (point-max))
+      (should (equal (buffer-string) correct)))))
+
+
+(ert-deftest lisp-indent-region-defun-with-docstring ()
+  "Test Bug#26619."
+  (with-temp-buffer
+    (insert "\
+\(defun test ()
+  \"This is a test.
+Test indentation in emacs-lisp-mode\"
+  (message \"Hi!\"))")
+    (let ((indent-tabs-mode nil)
+          (correct (buffer-string)))
+      (emacs-lisp-mode)
+      (indent-region (point-min) (point-max))
+      (should (equal (buffer-string) correct)))))
+
+
 (provide 'lisp-mode-tests)
 ;;; lisp-mode-tests.el ends here
-- 
2.11.1