[lenst@lysator.liu.se: Bug in lisp indentation]

[lenst@lysator.liu.se: Bug in lisp indentation]

[Richard Stallman]

[Resending due to lack of response]

Would someone please fix this in the trunk, then ack?

------- Start of forwarded message -------
X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY 
	autolearn=failed version=3.1.0
DKIM-Signature: a=rsa-sha1; c=relaxed/relaxed; d=gmail.com; s=beta;
	h=domainkey-signature:received:received:mime-version:content-transfer-encoding:message-id:content-type:to:from:subject:date:x-mailer:sender;
	b=OC5yF4sSxx2DzwOlTT1lwVVsV620vibxKp7n8hBcCcDSuiejCUplwZibXyU4l38sYjBVbQ+H118HFm6tgOY8Nqwu2mNPzge/0MGH6u6Z4LPwq94dmNzBe430V9sPoNNQSlmrFIoPLcSrARb4G7ipKPxMs69aSAw9H3obW7LIevI=
Mime-Version: 1.0 (Apple Message framework v752.2)
Content-Type: text/plain; charset=US-ASCII; format=flowed
To: emacs-pretest-bug@gnu.org
From: Lennart Staflin <lenst@lysator.liu.se>
Date: Thu, 31 May 2007 11:44:46 +0200
Subject: Bug in lisp indentation

In lisp-mode the indentation of the following expression is wrong:

(foo
:bar :a12
:sie sume )

If I press TAB at the beginning of the last line I get:

(foo
:bar :a12
       :sie sume )

I think this is wrong, the indentation should be as the first example.
This was not the behaviour in emacs 21, neither is the beaviour of
indent-region consistent with TAB.


In GNU Emacs 22.0.990.1 (powerpc-apple-darwin8.9.0, Carbon Version
1.6.0) of 2007-05-31 on sin.local Windowing system distributor `Apple
Inc.', version 10.4.9 configured using `configure
'--enable-carbon-app' '--enable-maintainer-mode' '--without-x'
'--with-jpeg' '--with-gif' '--with-png''

Important settings:
   value of $LC_ALL: nil
   value of $LC_COLLATE: nil
   value of $LC_CTYPE: en_US.UTF-8
   value of $LC_MESSAGES: nil
   value of $LC_MONETARY: nil
   value of $LC_NUMERIC: nil
   value of $LC_TIME: nil
   value of $LANG: nil
   locale-coding-system: utf-8
   default-enable-multibyte-characters: t

Major mode: Lisp

Minor modes in effect:
   encoded-kbd-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   unify-8859-on-encoding-mode: t
   utf-translate-cjk-mode: t
   auto-compression-mode: t
   line-number-mode: t

Recent input:
C-x C-f t e s TAB RET C-n C-n TAB C-x u C-p C-p ESC
C-@ C-x C-x C-x C-x ESC x e DEL i n d e SPC - r e SPC
g SPC RET C-n C-n C-n C-n C-n C-n C-n C-n C-p DEL C-a
C-p C-p C-n C-n C-n C-x C-s ESC x r e p o TAB r t TAB
RET

Recent messages:
For information about the GNU Project and its goals, type C-h C-p.
Undo!
Mark set
Making completion list...
Loading help-mode...done
Wrote /Users/lenst/test.lisp
Making completion list...
Loading emacsbug...
Loading regexp-opt...done
Loading emacsbug...done



_______________________________________________
emacs-pretest-bug mailing list
emacs-pretest-bug@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-pretest-bug
------- End of forwarded message -------

Re: Bug in lisp indentation

[Thien-Thi Nguyen]

() Lennart Staflin <lenst@lysator.liu.se>
() Thu, 31 May 2007 11:44:46 +0200

   In lisp-mode the indentation of the following expression is wrong:

   (foo
   :bar :a12
   :sie sume )

   If I press TAB at the beginning of the last line I get:

   (foo
   :bar :a12
         :sie sume )

   I think this is wrong, the indentation should be as the first example.
   This was not the behaviour in emacs 21, neither is the beaviour of
   indent-region consistent with TAB.

does the following patch give good results?
here is a description of the change:

        * emacs-lisp/lisp-mode.el (calculate-lisp-indent): Use the
        last sexp that begins a line, instead of the truly last sexp.

can someone suggest a better implementation?

thi

_______________________________________________________________________
diff -c -r1.202 lisp-mode.el
*** lisp-mode.el	6 Jul 2007 17:26:29 -0000	1.202
--- lisp-mode.el	14 Jul 2007 19:48:07 -0000
***************
*** 854,859 ****
--- 854,866 ----
                    (> (setq paren-depth (elt state 0)) 0))
          (setq retry nil)
          (setq calculate-lisp-indent-last-sexp (elt state 2))
+         ;; The last sexp may not be the first sexp on the line
+         ;; where it begins, so find that one, instead.
+         (when calculate-lisp-indent-last-sexp
+           (goto-char calculate-lisp-indent-last-sexp)
+           (beginning-of-line)
+           (skip-chars-forward " \t")
+           (setq calculate-lisp-indent-last-sexp (point)))
          (setq containing-sexp (elt state 1))
          ;; Position following last unclosed open.
          (goto-char (1+ containing-sexp))

Re: Bug in lisp indentation

[Stefan Monnier]

> +         ;; The last sexp may not be the first sexp on the line
> +         ;; where it begins, so find that one, instead.
> +         (when calculate-lisp-indent-last-sexp
> +           (goto-char calculate-lisp-indent-last-sexp)
> +           (beginning-of-line)
> +           (skip-chars-forward " \t")

That's wrong when you have things like

       (sdfgsdg dfgsd
                asfgasfg) (dfg)
     <TAB>sfgsfg

You want something along the lines of

   (while (not (at-indentation-p)) (forward-sexp -1))


-- Stefan

Re: Bug in lisp indentation

[Thien-Thi Nguyen]

() Stefan Monnier <monnier@iro.umontreal.ca>
() Sat, 14 Jul 2007 16:04:18 -0400

   That's wrong when you have things like

          (sdfgsdg dfgsd
                   asfgasfg) (dfg)
        <TAB>sfgsfg

   You want something along the lines of

      (while (not (at-indentation-p)) (forward-sexp -1))

indeed.  i've reworked the adjustment to be more specific.
below is try #3.  it does not address the case above, however
it does not change the current behavior for it, either.

	* emacs-lisp/lisp-mode.el (calculate-lisp-indent):
	In the case of constant symbol alignment, consider the
	sexp actually at indentation to be the "last sexp".

thi


______________________________________________________
diff -c -r1.202 lisp-mode.el
*** lisp-mode.el	6 Jul 2007 17:26:29 -0000	1.202
--- lisp-mode.el	14 Jul 2007 20:38:40 -0000
***************
*** 930,935 ****
--- 930,945 ----
                         (goto-char indent-point)
                         (skip-chars-forward " \t")
                         (looking-at ":"))
+                      ;; The last sexp may not be the first sexp on the line
+                      ;; where it begins, so find that one, instead.
+                      (save-excursion
+                        (goto-char calculate-lisp-indent-last-sexp)
+                        (while (and (not (looking-back "^[ \t]*"))
+                                    (or (not containing-sexp)
+                                        (< (1+ containing-sexp) (point))))
+                          (forward-sexp -1)
+                          (backward-prefix-chars))
+                        (setq calculate-lisp-indent-last-sexp (point)))
                       (> calculate-lisp-indent-last-sexp
                          (save-excursion
                            (goto-char (1+ containing-sexp))

Re: Bug in lisp indentation

[Lennart Staflin]

Thien-Thi Nguyen wrote:
> () Lennart Staflin <lenst@lysator.liu.se>
> () Thu, 31 May 2007 11:44:46 +0200
>
>    In lisp-mode the indentation of the following expression is wrong:
>
>    (foo
>    :bar :a12
>    :sie sume )
>
>    If I press TAB at the beginning of the last line I get:
>
>    (foo
>    :bar :a12
>          :sie sume )
>
>    I think this is wrong, the indentation should be as the first example.
>    This was not the behaviour in emacs 21, neither is the beaviour of
>    indent-region consistent with TAB.
>
> does the following patch give good results?
> here is a description of the change:
>
>         * emacs-lisp/lisp-mode.el (calculate-lisp-indent): Use the
>         last sexp that begins a line, instead of the truly last sexp.
>
> can someone suggest a better implementation?
>
> thi
>
> _______________________________________________________________________
> diff -c -r1.202 lisp-mode.el
> *** lisp-mode.el	6 Jul 2007 17:26:29 -0000	1.202
> --- lisp-mode.el	14 Jul 2007 19:48:07 -0000
> ***************
> *** 854,859 ****
> --- 854,866 ----
>                     (> (setq paren-depth (elt state 0)) 0))
>           (setq retry nil)
>           (setq calculate-lisp-indent-last-sexp (elt state 2))
> +         ;; The last sexp may not be the first sexp on the line
> +         ;; where it begins, so find that one, instead.
> +         (when calculate-lisp-indent-last-sexp
> +           (goto-char calculate-lisp-indent-last-sexp)
> +           (beginning-of-line)
> +           (skip-chars-forward " \t")
> +           (setq calculate-lisp-indent-last-sexp (point)))
>           (setq containing-sexp (elt state 1))
>           ;; Position following last unclosed open.
>           (goto-char (1+ containing-sexp))
>   

This might align the :sie line with the :bar line, but it introduces 
other indentation bugs. Also the whitespace in the mail is mangled 
slightly. The :bar line should not be indented under the left paren, but 
under the foo symbol.

As for a simpler change, I think there is a chunk of code that was added 
during the pretest to align keyword symbols, that I think is misguided. 
Removing that code fixes this bug.

//Lennart Staflin

Re: Bug in lisp indentation

[Thien-Thi Nguyen]

() Lennart Staflin <lenst@lysator.liu.se>
() Sun, 15 Jul 2007 16:43:22 +0200

   This might align the :sie line with the :bar line,
   but it introduces other indentation bugs. Also the
   whitespace in the mail is mangled slightly. The :bar
   line should not be indented under the left paren,
   but under the foo symbol.

   As for a simpler change, I think there is a chunk of
   code that was added during the pretest to align
   keyword symbols, that I think is misguided. Removing
   that code fixes this bug.

please disregard tries #1 and #2, which are buggy.
i'm interested in how try #3 performs for you.

if you don't receive try #3 (on this thread) in the
next day or so, let me know and i'll send it to you
personally.

thi

Re: Bug in lisp indentation

[Lennart Staflin]

Thien-Thi Nguyen wrote:
> please disregard tries #1 and #2, which are buggy.
> i'm interested in how try #3 performs for you.
>
> if you don't receive try #3 (on this thread) in the
> next day or so, let me know and i'll send it to you
> personally.
>   
Yes, #3 works.

//Lennart

Re: Bug in lisp indentation

[Thien-Thi Nguyen]

() Lennart Staflin <lenst@lysator.liu.se>
() Mon, 16 Jul 2007 16:04:30 +0200

   Yes, #3 works.

thanks for confirming.
installed on the trunk.

thi

Re: Bug in lisp indentation

[Thien-Thi Nguyen]

oops, upon further testing i see the first try was buggy.
below is try #2.  please discard try #1.

thi

____________________________________________
diff -c -r1.202 lisp-mode.el
*** lisp-mode.el	6 Jul 2007 17:26:29 -0000	1.202
--- lisp-mode.el	14 Jul 2007 20:03:25 -0000
***************
*** 855,860 ****
--- 855,869 ----
          (setq retry nil)
          (setq calculate-lisp-indent-last-sexp (elt state 2))
          (setq containing-sexp (elt state 1))
+         ;; The last sexp may not be the first sexp on the line
+         ;; where it begins, so find that one, instead.
+         (when calculate-lisp-indent-last-sexp
+           (goto-char calculate-lisp-indent-last-sexp)
+           (when (and containing-sexp
+                      (< containing-sexp (line-beginning-position)))
+             (beginning-of-line)
+             (skip-chars-forward " \t")
+             (setq calculate-lisp-indent-last-sexp (point))))
          ;; Position following last unclosed open.
          (goto-char (1+ containing-sexp))
          ;; Is there a complete sexp since then?

Re: Fwd: Bug in lisp indentation

[Stefan Monnier]

> neither is the beaviour of indent-region consistent with TAB.

This is very easy to fix.  And I think it's a good idea in any case.
See patch below,


        Stefan


--- lisp-mode.el	08 jui 2007 14:44:28 -0400	1.202
+++ lisp-mode.el	15 jui 2007 21:49:08 -0400	
@@ -214,8 +214,6 @@
   ;;(set (make-local-variable 'adaptive-fill-mode) nil)
   (make-local-variable 'indent-line-function)
   (setq indent-line-function 'lisp-indent-line)
-  (make-local-variable 'indent-region-function)
-  (setq indent-region-function 'lisp-indent-region)
   (make-local-variable 'parse-sexp-ignore-comments)
   (setq parse-sexp-ignore-comments t)
   (make-local-variable 'outline-regexp)
@@ -1206,16 +1204,6 @@
 	    (setq outer-loop-done (= (point) last-point))
 	    (setq last-point (point)))))))
 
-(defun lisp-indent-region (start end)
-  "Indent every line whose first char is between START and END inclusive."
-  (save-excursion
-    (let ((endmark (copy-marker end)))
-      (goto-char start)
-      (and (bolp) (not (eolp))
-	   (lisp-indent-line))
-      (indent-sexp endmark)
-      (set-marker endmark nil))))
-
 (defun indent-pp-sexp (&optional arg)
   "Indent each line of the list starting just after point, or prettyprint it.
 A prefix argument specifies pretty-printing."

Re: Fwd: Bug in lisp indentation

[Richard Stallman]

The main purpose of lisp-indent-region was to optimize reindenting
of regions.  This optimization made a big difference, way back when.
Maybe it isn't necessary any more, but I'd like someone to try it
on a not very fast computer and see.

Re: Fwd: Bug in lisp indentation

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

> The main purpose of lisp-indent-region was to optimize reindenting
> of regions.  This optimization made a big difference, way back when.
> Maybe it isn't necessary any more, but I'd like someone to try it
> on a not very fast computer and see.

Parsing arbitrarily far backward for every line is always going to
make for quadratic behavior.

I think we should try to come up with a general scheme where parsing
data can get cached and interpolated easily, so that
lisp-indent-region would basically be

(let ((indent-cache (make-indent-cache)))
  (loop through region
    (indent-line)))

Similar things would be nice for font locking and other stuff: some
cache that shortcircuits some specifical search/scan requests.

-- 
David Kastrup

Re: Fwd: Bug in lisp indentation

[Stefan Monnier]

> Parsing arbitrarily far backward for every line is always going to
> make for quadratic behavior.

Actually, that's not necessarily the case.
I've done some tests with my sml-mode (which requires a fairly costly
indentation algorithm) and while any particular line-indent does have
a worst case cost of about O(n), the worst case cost for "reasonable code"
when applied to a region was still O(n).  I.e. most lines take O(1), and
only O(1/n) lines take O(n) time.


        Stefan

Re: Fwd: Bug in lisp indentation

[Richard Stallman]

    > The main purpose of lisp-indent-region was to optimize reindenting
    > of regions.  This optimization made a big difference, way back when.
    > Maybe it isn't necessary any more, but I'd like someone to try it
    > on a not very fast computer and see.

    Parsing arbitrarily far backward for every line is always going to
    make for quadratic behavior.

I think what takes the time is parsing _down_ from the top of the
function.  Finding the start of the defun is very fast by comparison,
at least for Lisp.

The parsing down is quadratic too.  The question is whether this is
a noticeable delay in practice for common cases.  Can someone who
uses a slow machine please check that?

    I think we should try to come up with a general scheme where parsing
    data can get cached and interpolated easily, so that
    lisp-indent-region would basically be

    (let ((indent-cache (make-indent-cache)))
      (loop through region
	(indent-line)))

It is a good idea in principle.  One would need to design
this caching system.  Would you like to try?

If that isn't done, and if just deleting lisp-indent-region leads to
annoying slowdowns, then the only other choice is to fix the bug in
lisp-indent-region.

Would someone please try that?

Re: Fwd: Bug in lisp indentation

[Stefan Monnier]

> I think what takes the time is parsing _down_ from the top of the
> function.  Finding the start of the defun is very fast by comparison,
> at least for Lisp.

None of the indentation algorithms I've ever implemented does any
forward parsing.  It's always going backward.  Going backward leads to
better behavior as well: it only looks at nearby code to decide how to
indent, so its behavior is more local and hence easier for the user
to understand.


        Stefan

Re: Fwd: Bug in lisp indentation

[Richard Stallman]

    None of the indentation algorithms I've ever implemented does any
    forward parsing.  It's always going backward.

The Lisp mode indentation I implemented worked by using
parse-partial-sexp from the start of the defun.
Maybe it has been changed since.