bug#21343: 24.5; parse-partial-sexp mistakes string for a comment

[npostavs@users.sourceforge.net]

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

tags 21343 patch
quit

npostavs@users.sourceforge.net writes:

> A solution could be to use syntax-ppss to go to start of string before
> parsing (though I wonder if syntax-ppss should be used more for the
> parsing itself in that case, it seems like there's a lot of work going
> that probably duplicates what's already being done):

Actually, it turns out using syntax-ppss in the loop is slower, at least
in the case of calling `pp' on a big list (which is a pessimal case for
a cache based parser).

Here is a patch (the first one just simplifies the code).


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 11487 bytes --]

From bada5e84d79e38ff386685f9b6b80fdf4e3b0988 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 5 Mar 2017 00:16:13 -0500
Subject: [PATCH v1 1/2] * lisp/emacs-lisp/lisp-mode.el (indent-sexp):
 Simplify.

* test/lisp/emacs-lisp/lisp-mode-tests.el (indent-sexp):
(indent-subsexp, indent-sexp-in-string): New tests.
---
 lisp/emacs-lisp/lisp-mode.el            | 169 ++++++++++++++------------------
 test/lisp/emacs-lisp/lisp-mode-tests.el |  92 +++++++++++++++++
 2 files changed, 165 insertions(+), 96 deletions(-)
 create mode 100644 test/lisp/emacs-lisp/lisp-mode-tests.el

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index d720e0bc57..de0e66039a 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1056,103 +1056,80 @@ indent-sexp
 If optional arg ENDPOS is given, indent each line, stopping when
 ENDPOS is encountered."
   (interactive)
-  (let ((indent-stack (list nil))
-	(next-depth 0)
-	;; If ENDPOS is non-nil, use nil as STARTING-POINT
-	;; so that calculate-lisp-indent will find the beginning of
-	;; the defun we are in.
-	;; If ENDPOS is nil, it is safe not to scan before point
-	;; since every line we indent is more deeply nested than point is.
-	(starting-point (if endpos nil (point)))
-	(last-point (point))
-	last-depth bol outer-loop-done inner-loop-done state this-indent)
-    (or endpos
-	;; Get error now if we don't have a complete sexp after point.
-	(save-excursion (forward-sexp 1)))
+  (let* ((indent-stack (list nil))
+         ;; If ENDPOS is non-nil, use beginning of defun as STARTING-POINT.
+         ;; If ENDPOS is nil, it is safe not to scan before point
+         ;; since every line we indent is more deeply nested than point is.
+         (starting-point (save-excursion (if endpos (beginning-of-defun))
+                                         (point)))
+         (state nil)
+         (init-depth 0)
+         (next-depth 0)
+         (last-depth 0)
+         (last-syntax-point (point)))
+    (unless endpos
+      ;; Get error now if we don't have a complete sexp after point.
+      (save-excursion (forward-sexp 1)
+                      ;; We need a marker because we modify the buffer
+                      ;; text preceding endpos.
+                      (setq endpos (point-marker))))
     (save-excursion
-      (setq outer-loop-done nil)
-      (while (if endpos (< (point) endpos)
-	       (not outer-loop-done))
-	(setq last-depth next-depth
-	      inner-loop-done nil)
-	;; Parse this line so we can learn the state
-	;; to indent the next line.
-	;; This inner loop goes through only once
-	;; unless a line ends inside a string.
-	(while (and (not inner-loop-done)
-		    (not (setq outer-loop-done (eobp))))
-	  (setq state (parse-partial-sexp (point) (progn (end-of-line) (point))
-					  nil nil state))
-	  (setq next-depth (car state))
-	  ;; If the line contains a comment other than the sort
-	  ;; that is indented like code,
-	  ;; indent it now with indent-for-comment.
-	  ;; Comments indented like code are right already.
-	  ;; In any case clear the in-comment flag in the state
-	  ;; because parse-partial-sexp never sees the newlines.
-	  (if (car (nthcdr 4 state))
-	      (progn (indent-for-comment)
-		     (end-of-line)
-		     (setcar (nthcdr 4 state) nil)))
-	  ;; If this line ends inside a string,
-	  ;; go straight to next line, remaining within the inner loop,
-	  ;; and turn off the \-flag.
-	  (if (car (nthcdr 3 state))
-	      (progn
-		(forward-line 1)
-		(setcar (nthcdr 5 state) nil))
-	    (setq inner-loop-done t)))
-	(and endpos
-	     (<= next-depth 0)
-	     (progn
-	       (setq indent-stack (nconc indent-stack
-					 (make-list (- next-depth) nil))
-		     last-depth (- last-depth next-depth)
-		     next-depth 0)))
-	(forward-line 1)
-	;; Decide whether to exit.
-	(if endpos
-	    ;; If we have already reached the specified end,
-	    ;; give up and do not reindent this line.
-	    (if (<= endpos (point))
-		(setq outer-loop-done t))
-	  ;; If no specified end, we are done if we have finished one sexp.
-	  (if (<= next-depth 0)
-	      (setq outer-loop-done t)))
-	(unless outer-loop-done
-	  (while (> last-depth next-depth)
-	    (setq indent-stack (cdr indent-stack)
-		  last-depth (1- last-depth)))
-	  (while (< last-depth next-depth)
-	    (setq indent-stack (cons nil indent-stack)
-		  last-depth (1+ last-depth)))
-	  ;; Now indent the next line according
-	  ;; to what we learned from parsing the previous one.
-	  (setq bol (point))
-	  (skip-chars-forward " \t")
-	  ;; But not if the line is blank, or just a comment
-	  ;; (except for double-semi comments; indent them as usual).
-	  (if (or (eobp) (looking-at "\\s<\\|\n"))
-	      nil
-	    (if (and (car indent-stack)
-		     (>= (car indent-stack) 0))
-		(setq this-indent (car indent-stack))
-	      (let ((val (calculate-lisp-indent
-			  (if (car indent-stack) (- (car indent-stack))
-			    starting-point))))
-		(if (null val)
-		    (setq this-indent val)
-		  (if (integerp val)
-		      (setcar indent-stack
-			      (setq this-indent val))
-		    (setcar indent-stack (- (car (cdr val))))
-		    (setq this-indent (car val))))))
-	    (if (and this-indent (/= (current-column) this-indent))
-		(progn (delete-region bol (point))
-		       (indent-to this-indent)))))
-	(or outer-loop-done
-	    (setq outer-loop-done (= (point) last-point))
-	    (setq last-point (point)))))))
+      (while (< (point) endpos)
+        ;; Parse this line so we can learn the state to indent the
+        ;; next line.
+        (setq state (parse-partial-sexp
+                     last-syntax-point (progn (end-of-line) (point))
+                     nil nil state))
+        ;; If a string continues past end of line, skip to its end.
+        (when (nth 3 state)
+          (setq state (parse-partial-sexp (point) (point-max)
+                                          nil nil state 'syntax-table)))
+        (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))
+        (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))
+          ;; Now indent the next line according
+          ;; to what we learned from parsing the previous one.
+          (let ((bol (point)))
+            (skip-chars-forward " \t")
+            ;; But not if the line is blank, or just a comment (we
+            ;; already called `indent-for-comment' above).
+            (unless (or (eolp) (eq (char-syntax (char-after)) ?<))
+              (let ((this-indent (car indent-stack)))
+                (when (listp this-indent)
+                  (let ((val (calculate-lisp-indent
+                              (or (car this-indent) starting-point))))
+                    (setq
+                     this-indent
+                     (cond ((integerp val)
+                            (setf (car indent-stack) val))
+                           ((consp val) ; (COLUMN CONTAINING-SEXP-START)
+                            (setf (car indent-stack) (cdr val))
+                            (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!"))))))
+                (when (/= (current-column) this-indent)
+                  (delete-region bol (point))
+                  (indent-to this-indent))))))))))
 
 (defun indent-pp-sexp (&optional arg)
   "Indent each line of the list starting just after point, or prettyprint it.
diff --git a/test/lisp/emacs-lisp/lisp-mode-tests.el b/test/lisp/emacs-lisp/lisp-mode-tests.el
new file mode 100644
index 0000000000..1972396112
--- /dev/null
+++ b/test/lisp/emacs-lisp/lisp-mode-tests.el
@@ -0,0 +1,92 @@
+;;; lisp-mode-tests.el --- Test Lisp editing commands  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2017 Free Software Foundation, Inc.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'cl-lib)
+(require 'lisp-mode)
+
+(ert-deftest indent-sexp ()
+  "Test basics of \\[indent-sexp]."
+  (with-temp-buffer
+    (insert "\
+\(a
+ (prog1
+     (prog1
+         1
+       2)
+   2)
+ (1
+  \"string
+ending\"
+  2)                                    ; comment
+ ;; comment
+ b)")
+    (goto-char (point-min))
+    (let ((indent-tabs-mode nil)
+          (correct (buffer-string)))
+      (dolist (mode '(fundamental-mode emacs-lisp-mode))
+        (funcall mode)
+        (indent-sexp)
+        ;; 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-sexp)
+        (should (equal (buffer-string) correct))
+        ;; Correctly remove indentation.
+        (save-excursion
+          (let ((n 0))
+            (while (not (eobp))
+              (insert (make-string n ?\s))
+              (cl-incf n)
+              (back-to-indentation)
+              (forward-line (if (eq (char-after) ?\") 2 1)))))
+        (indent-sexp)
+        (should (equal (buffer-string) correct))))))
+
+(ert-deftest indent-subsexp ()
+  "Make sure calling `indent-sexp' inside a sexp works."
+  (with-temp-buffer
+    (insert "\
+\(d1 xx
+    (d2 yy
+	zz)
+    11)")
+    (let ((correct (buffer-string)))
+      (search-backward "d2")
+      (up-list -1)
+      (indent-sexp)
+      (should (equal (buffer-string) correct)))))
+
+(ert-deftest indent-sexp-in-string ()
+  "Make sure calling `indent-sexp' inside a string works."
+  ;; See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21343.
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "\";\"")
+    (let ((correct (buffer-string)))
+      (search-backward ";")
+      (indent-sexp)
+      (should (equal (buffer-string) correct)))))
+
+(provide 'lisp-mode-tests)
+;;; lisp-mode-tests.el ends here
-- 
2.11.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch --]
[-- Type: text/x-diff, Size: 1482 bytes --]

From b318e8c0bdd0f08b49f7f8c635bf886d967bc742 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 5 Mar 2017 00:53:58 -0500
Subject: [PATCH v1 2/2] Fix indent-sexp when called from inside a string
 (Bug#21343)

* lisp/emacs-lisp/lisp-mode.el (indent-sexp): Get initial syntax parse
state from `syntax-ppss'.
---
 lisp/emacs-lisp/lisp-mode.el | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index de0e66039a..ce61c69f3c 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1062,10 +1062,14 @@ indent-sexp
          ;; since every line we indent is more deeply nested than point is.
          (starting-point (save-excursion (if endpos (beginning-of-defun))
                                          (point)))
-         (state nil)
-         (init-depth 0)
-         (next-depth 0)
-         (last-depth 0)
+         ;; 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)))
     (unless endpos
       ;; Get error now if we don't have a complete sexp after point.
-- 
2.11.1