Indentation bug in function INDENT-SEXP (from CVS sources)

Indentation bug in function INDENT-SEXP (from CVS sources)

[sand]

This is with a version of GNU Emacs compiled from the CVS mainline sources of (roughly) August 11th, 2006.  I have applied the multi-tty patch, so I'm limited in the CVS versions I can use.  I'm confident that the problem has not been fixed since, as I describe below.

Here's an Emacs Lisp definition with bad indentation:

(defadvice yank (after after-yank-indent-region activate)
  (when (memq major-mode '(c++-mode
                           c-mode
                           emacs-lisp-mode
                       erlang-mode
                       java-mode
                       lisp-mode
                       scheme-mode
                           scheme48-mode))
    (let ((mark-even-if-inactive t))
      (indent-region (region-beginning) (region-end) nil))))

Place mark at the beginning of the "erlang-mode" line and point at the beginning of the "lisp-mode" line and run the "indent-region" command.  The erlang, java *and* lisp lines get indented.  The "scheme-mode" line does not:

(defadvice yank (after after-yank-indent-region activate)
  (when (memq major-mode '(c++-mode
                           c-mode
                           emacs-lisp-mode
                           erlang-mode
                           java-mode
                           lisp-mode
                       scheme-mode
                           scheme48-mode))
    (let ((mark-even-if-inactive t))
      (indent-region (region-beginning) (region-end) nil))))

I would expect the "lisp-mode" line to not move, based on the chosen region.  One can argue that the seen behavior is the correct behavior, as point is on the beginning of the "lisp-mode" line.  However, the same three lines change if you put mark at the beginning of the "erlang-mode" line and point at the *end* of the "java-mode" line (before the newline).  No part of the "lisp-mode" line is part of the region, so that is definitely the wrong behavior.  And in any case, I belive that the "lisp-mode" line should not have changed in the first case, following the principle of least surprise.

[WARNING: Description ends, analysis and patch follow!]

The bug is in the underlying INDENT-SEXP function.  It does test against the ENDPOS argument (marking the end of the region), but this test occurs at the top of the loop, before we have advanced to the following line, so we get the off-by-one error we see above.  There have been two revisions to lisp-mode.el since I compiled from CVS, neither of which deal with INDENT-SEXP, so the problem should still be in the most recent sources.

I have attached a patch that appears to fix the problem.  With it, both tests indent only the erlang and java lines.  The change hoists the FORWARD-LINE call up and earlier in the code, and compares ENDPOS against point immediately afterwards, setting OUTER-LOOP-DONE to true if we have reached ENDPOS.  (Whether or not this is the right solution long-term is another matter; INDENT-SEXP is already rather hairy.)

Derek

--
sand@blarg.net

------------------------------ cut here ------------------------------

--- lisp-mode.el	2007-01-30 10:37:45.000000000 -0800
+++ mod-lisp-mode.el	2007-01-30 10:37:39.000000000 -0800
@@ -1128,8 +1128,11 @@
 		     next-depth 0)))
 	(or outer-loop-done endpos
 	    (setq outer-loop-done (<= next-depth 0)))
+        (forward-line 1)
+        (if (and endpos (<= endpos (point)))
+            (setq outer-loop-done t))
 	(if outer-loop-done
-	    (forward-line 1)
+            nil
 	  (while (> last-depth next-depth)
 	    (setq indent-stack (cdr indent-stack)
 		  last-depth (1- last-depth)))
@@ -1138,7 +1141,6 @@
 		  last-depth (1+ last-depth)))
 	  ;; Now go to the next line and indent it according
 	  ;; to what we learned from parsing the previous one.
-	  (forward-line 1)
 	  (setq bol (point))
 	  (skip-chars-forward " t")
 	  ;; But not if the line is blank, or just a comment

Re: Indentation bug in function INDENT-SEXP (from CVS sources)

[Richard Stallman]

Thanks for fixing this bug.  I will install a variant of your fix.

This is a cleaned up version, and seems to work.  Does it work well for you?

*** lisp-mode.el	21 Jan 2007 01:36:10 -0500	1.196
--- lisp-mode.el	03 Feb 2007 06:10:11 -0500	
***************
*** 1128,1146 ****
  					 (make-list (- next-depth) nil))
  		     last-depth (- last-depth next-depth)
  		     next-depth 0)))
! 	(or outer-loop-done endpos
! 	    (setq outer-loop-done (<= next-depth 0)))
! 	(if outer-loop-done
! 	    (forward-line 1)
  	  (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 go to the next line and indent it according
  	  ;; to what we learned from parsing the previous one.
- 	  (forward-line 1)
  	  (setq bol (point))
  	  (skip-chars-forward " \t")
  	  ;; But not if the line is blank, or just a comment
--- 1128,1152 ----
  					 (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

Re: Indentation bug in function INDENT-SEXP (from CVS sources)

[sand]

Richard Stallman writes:
> Thanks for fixing this bug.  I will install a variant of your fix.
> 
> This is a cleaned up version, and seems to work.  Does it work well
> for you?

It works correctly with me test case.  Thanks.

Derek