Fix lisp-indent-region and indent-sexp (Bug#26619)

The new lisp-indent-region introduced in 2017-04-22 "Add new
`lisp-indent-region' that doesn't reparse the code." is broken because
it doesn't save the calculated indent amounts for already seen sexp
depths.  Fix this by unifying the indent-sexp and lisp-indent-region
code.  Furthermore, only preserve position 2 of the running parse
when the depth doesn't change.
* lisp/emacs-lisp/lisp-mode.el (lisp-ppss): Use an OLDSTATE that
corresponds with the start point when calling parse-partial-sexp.
(lisp-indent-state): New struct.
(lisp-indent-calc-next): New function, extracted from indent-sexp.
(indent-sexp, lisp-indent-region): Use it.
(lisp-indent-line): Take indentation, instead of parse state.
* test/lisp/emacs-lisp/lisp-mode-tests.el
(lisp-mode-tests--correctly-indented-sexp): New constant.
(lisp-indent-region, lisp-indent-region-defun-with-docstring):
(lisp-indent-region-open-paren, lisp-indent-region-in-sexp): New
tests.

2 files changed, 171 insertions, 90 deletions

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 7448864ff99..6287f27b139 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -754,49 +754,108 @@ function is `common-lisp-indent-function'."
 
 (defun lisp-ppss (&optional pos)
   "Return Parse-Partial-Sexp State at POS, defaulting to point.
-Like to `syntax-ppss' but includes the character address of the
-last complete sexp in the innermost containing list at position
+Like `syntax-ppss' but includes the character address of the last
+complete sexp in the innermost containing list at position
 2 (counting from 0).  This is important for lisp indentation."
   (unless pos (setq pos (point)))
   (let ((pss (syntax-ppss pos)))
     (if (nth 9 pss)
-        (parse-partial-sexp (car (last (nth 9 pss))) pos)
+        (let ((sexp-start (car (last (nth 9 pss)))))
+          (parse-partial-sexp sexp-start pos nil nil (syntax-ppss sexp-start)))
       pss)))
 
+(cl-defstruct (lisp-indent-state
+               (:constructor nil)
+               (:constructor lisp-indent-initial-state
+                             (&aux (ppss (lisp-ppss))
+                                   (ppss-point (point))
+                                   (depth (car ppss))
+                                   (stack (make-list (1+ depth) nil)))))
+  stack ;; Cached indentation, per depth.
+  ppss
+  depth
+  ppss-point)
+
+(defun lisp-indent-calc-next (state)
+  "Move to next line and return calculated indent for it.
+STATE is updated by side effect, the first state should be
+created by `lisp-indent-initial-state'.  This function may move
+by more than one line to cross a string literal."
+  (pcase-let (((cl-struct lisp-indent-state
+                          (stack indent-stack) ppss depth ppss-point)
+               state))
+    ;; Parse this line so we can learn the state to indent the
+    ;; next line.
+    (while (let ((last-sexp (nth 2 ppss)))
+             (setq ppss (parse-partial-sexp
+                         ppss-point (progn (end-of-line) (point))
+                         nil nil ppss))
+             ;; Preserve last sexp of state (position 2) for
+             ;; `calculate-lisp-indent', if we're at the same depth.
+             (if (and (not (nth 2 ppss)) (= depth (car ppss)))
+                 (setf (nth 2 ppss) last-sexp)
+               (setq last-sexp (nth 2 ppss)))
+             ;; Skip over newlines within strings.
+             (nth 3 ppss))
+      (let ((string-start (nth 8 ppss)))
+       (setq ppss (parse-partial-sexp (point) (point-max)
+                                      nil nil ppss 'syntax-table))
+       (setf (nth 2 ppss) string-start)) ; Finished a complete string.
+      (setq ppss-point (point)))
+    (setq ppss-point (point))
+    (let* ((next-depth (car ppss))
+           (depth-delta (- next-depth 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 depth next-depth))
+    (prog1
+        (let (indent)
+          (cond ((= (forward-line 1) 1) nil)
+                ((car indent-stack))
+                ((integerp (setq indent (calculate-lisp-indent ppss)))
+                 (setf (car indent-stack) indent))
+                ((consp indent)       ; (COLUMN CONTAINING-SEXP-START)
+                 (car indent))
+                ;; This only happens if we're in a string.
+                (t (error "This shouldn't happen"))))
+      (setf (lisp-indent-state-stack state) indent-stack)
+      (setf (lisp-indent-state-depth state) depth)
+      (setf (lisp-indent-state-ppss-point state) ppss-point)
+      (setf (lisp-indent-state-ppss state) ppss))))
+
 (defun lisp-indent-region (start end)
   "Indent region as Lisp code, efficiently."
   (save-excursion
     (setq end (copy-marker end))
     (goto-char start)
+    (beginning-of-line)
     ;; The default `indent-region-line-by-line' doesn't hold a running
     ;; parse state, which forces each indent call to reparse from the
     ;; beginning.  That has O(n^2) complexity.
-    (let* ((parse-state (lisp-ppss start))
-           (last-syntax-point start)
+    (let* ((parse-state (lisp-indent-initial-state))
            (pr (unless (minibufferp)
                  (make-progress-reporter "Indenting region..." (point) end))))
-      (while (< (point) end)
-        (unless (and (bolp) (eolp))
-          (lisp-indent-line parse-state))
-        (forward-line 1)
-        (let ((last-sexp (nth 2 parse-state)))
-          (setq parse-state (parse-partial-sexp last-syntax-point (point)
-                                                nil nil parse-state))
-          ;; It's important to preserve last sexp location for
-          ;; `calculate-lisp-indent'.
-          (unless (nth 2 parse-state)
-            (setf (nth 2 parse-state) last-sexp))
-          (setq last-syntax-point (point)))
-        (and pr (progress-reporter-update pr (point))))
+      (let ((ppss (lisp-indent-state-ppss parse-state)))
+        (unless (or (and (bolp) (eolp)) (nth 3 ppss))
+          (lisp-indent-line (calculate-lisp-indent ppss))))
+      (let ((indent nil))
+        (while (progn (setq indent (lisp-indent-calc-next parse-state))
+                      (< (point) end))
+          (unless (or (and (bolp) (eolp)) (not indent))
+            (lisp-indent-line indent))
+          (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
@@ -1116,16 +1175,7 @@ Lisp function does not specify a special indentation."
 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
-         ;; 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)
-         (last-syntax-point (point)))
+  (let* ((parse-state (lisp-indent-initial-state)))
     ;; We need a marker because we modify the buffer
     ;; text preceding endpos.
     (setq endpos (copy-marker
@@ -1135,64 +1185,20 @@ ENDPOS is encountered."
                     (save-excursion (forward-sexp 1) (point)))))
     (save-excursion
       (while (< (point) endpos)
-        ;; Parse this line so we can learn the state to indent the
-        ;; next line.  Preserve element 2 of the state (last sexp) for
-        ;; `calculate-lisp-indent'.
-        (let ((last-sexp (nth 2 state)))
-          (while (progn
-                   (setq state (parse-partial-sexp
-                                last-syntax-point (progn (end-of-line) (point))
-                                nil nil state))
-                   (setq last-sexp (or (nth 2 state) last-sexp))
-                   ;; Skip over newlines within strings.
-                   (nth 3 state))
-            (setq state (parse-partial-sexp (point) (point-max)
-                                            nil nil state 'syntax-table))
-            (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))
-        ;; 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))
+        (let ((indent (lisp-indent-calc-next parse-state)))
+          ;; If the line contains a comment indent it now with
+          ;; `indent-for-comment'.
+          (when (nth 4 (lisp-indent-state-ppss parse-state))
+            (save-excursion
+              (goto-char (lisp-indent-state-ppss-point parse-state))
+              (indent-for-comment)
+              (setf (lisp-indent-state-ppss-point parse-state)
+                    (line-end-position))))
           ;; 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!"))))))))))
+          (unless (or (eolp) (eq (char-syntax (char-after)) ?<) (not indent))
+            (indent-line-to indent)))))
     (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 27f0bb5ec13..1f78eb30105 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 @@ noindent\" 3
   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,78 @@ noindent\" 3
       (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)))))
+
+(ert-deftest lisp-indent-region-open-paren ()
+  (with-temp-buffer
+    (insert "\
+\(with-eval-after-load 'foo
+  (setq bar `(
+              baz)))")
+    (let ((indent-tabs-mode nil)
+          (correct (buffer-string)))
+      (emacs-lisp-mode)
+      (indent-region (point-min) (point-max))
+      (should (equal (buffer-string) correct)))))
+
+(ert-deftest lisp-indent-region-in-sexp ()
+  (with-temp-buffer
+    (insert "\
+\(when t
+  (when t
+    (list 1 2 3)
+    'etc)
+  (quote etc)
+  (quote etc))")
+    (let ((indent-tabs-mode nil)
+          (correct (buffer-string)))
+      (emacs-lisp-mode)
+      (search-backward "1")
+      (indent-region (point) (point-max))
+      (should (equal (buffer-string) correct)))))
+
+
 (provide 'lisp-mode-tests)
 ;;; lisp-mode-tests.el ends here