bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Dmitry Gutov]

Try this example with ruby-mode:

foo(
  a,
  b => [
    1, 3, 4
  ],
  c => [
    5, 6
  ])

Currently, it will indent the last line to the 0th column, which doesn't
look right to me.

The following patch fixes that.  Would it be all right to install it?

=== modified file 'lisp/emacs-lisp/smie.el'
--- lisp/emacs-lisp/smie.el	2013-11-04 20:45:36 +0000
+++ lisp/emacs-lisp/smie.el	2013-12-12 01:39:59 +0000
@@ -1423,8 +1423,7 @@
   (save-excursion
     ;; (forward-comment (point-max))
     (when (looking-at "\\s)")
-      (while (not (zerop (skip-syntax-forward ")")))
-        (skip-chars-forward " \t"))
+      (forward-char 1)
       (condition-case nil
           (progn
             (backward-sexp 1)


In GNU Emacs 24.3.50.8 (x86_64-unknown-linux-gnu, GTK+ Version 3.8.6)
 of 2013-12-09 on axl
Bzr revision: 115440 dmantipov@yandex.ru-20131209163052-oess75ps2o5tt61q
Windowing system distributor `The X.Org Foundation', version 11.0.11403000
System Description:	Ubuntu 13.10

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Stefan Monnier]

> Currently, it will indent the last line to the 0th column, which doesn't
> look right to me.

Well, it does look right to me, because that line closes the whole construct.


        Stefan

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Dmitry Gutov]

On 12.12.2013 14:55, Stefan Monnier wrote:
> Well, it does look right to me, because that line closes the whole construct.

Here are some arguments on why it's wrong:

- It deforms the last array literal and breaks the symmetry with the 
previous one.

- If it emphasizes the fact that the construct is closed, why doesn't it 
align the opening and closing round parens, and aligns delimiters from 
different sexps instead?

- What about the following example? The last line likewise closes the 
whole construct, but we don't indent it to the 0th column, right?

foo(
   a,
   b,
   c)

- Likewise, this example:

foo([a,
      b])

- Indentation functions usually don't care about tokens that go after 
the first one after indentation. I'm used to that. For example, take 
js-mode. It handles the initial example differently, but compare this 
one under both modes:

foo([a,
      b
     ])

js-mode indents it as I would expect.

- If I want to emphasize the closing of the whole construct, I can put 
the closing paren on a separate line.

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Steve Purcell]

I agree strongly with Dmitry here. Column zero is fine if the paren is on its own, but if there are any preceding characters on the line which have a different logical indent level, as is the case with “]” here, the indentation for those should dominate.

As another example, in the case of javascript, there’s the idiomatic function case:

   something(function() {
      … blah
   });

and I don’t think this is inconsistent with the above: unindenting the “}” relative to the function body will have the right effect without even needing to consider the “)”.

-Steve

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Stefan Monnier]

> I agree strongly with Dmitry here.

No need to convince anyone here.  I was just pointing out that what is
right will depend, so the right fix is to let the rules-function
control it, rather than to replace one hard-coded choice with another.


        Stefan

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Dmitry Gutov]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> No need to convince anyone here.  I was just pointing out that what is
> right will depend

Sorry, I didn't get that.

> so the right fix is to let the rules-function
> control it, rather than to replace one hard-coded choice with another.

That won't work. ruby-smie-rules is never called because
smie-indent-close doesn't use smie-indent--rule, and it goes before the
functions that do in smie-indent-functions. Unless you're suggesting to
change either of those points.

I thought rather to add a defvar to switch between the two behaviors in
smie-indent-close, or make smie-indent-functions buffer-local and
replace smie-indent-close there with a modified implementation.

Still inclined toward the latter.

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Stefan Monnier]

> Unless you're suggesting to change either of those points.

That's exactly what I'm suggesting, yes.

> I thought rather to add a defvar to switch between the two behaviors in
> smie-indent-close,

That could be acceptable, tho so far all the indentation is controlled
by the rules-function, so it makes sense to keep it that way rather than
to introduce a variable.  Also, if it's done in the rules-function, the
function may return different results depending on context.

> or make smie-indent-functions buffer-local and replace
> smie-indent-close there with a modified implementation.

This approach would work if the requirement is very specific to one
particular major mode, but in this case, it seems to be a fairly
common one.


        Stefan

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Dmitry Gutov]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> That's exactly what I'm suggesting, yes.

So, call `smie-indent--rule' from `smie-indent-close'? That should work.

>> I thought rather to add a defvar to switch between the two behaviors in
>> smie-indent-close,
>
> That could be acceptable, tho so far all the indentation is controlled
> by the rules-function, so it makes sense to keep it that way rather than
> to introduce a variable.

Maybe so.

> Also, if it's done in the rules-function, the
> function may return different results depending on context.

True, but so far I don't see a situation in Ruby where it'd depend on
the context.

>> or make smie-indent-functions buffer-local and replace
>> smie-indent-close there with a modified implementation.
>
> This approach would work if the requirement is very specific to one
> particular major mode, but in this case, it seems to be a fairly
> common one.

I believe this argument also works against doing it in the rules
function, and in favor of adding a defvar.

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Stefan Monnier]

>> Also, if it's done in the rules-function, the
>> function may return different results depending on context.
> True, but so far I don't see a situation in Ruby where it'd depend on
> the context.

Agreed, but we're talking about a change in smie.el, not in ruby-mode.el ;-)

>>> or make smie-indent-functions buffer-local and replace
>>> smie-indent-close there with a modified implementation.
>> This approach would work if the requirement is very specific to one
>> particular major mode, but in this case, it seems to be a fairly
>> common one.
> I believe this argument also works against doing it in the rules
> function, and in favor of adding a defvar.

I don't think so: setting the var is a one-liner, adding the rule to the
rule-function is also a one-liner.  So either way is just as easy for
the major-mode.

By contrast, setting smie-indent-functions buffer-locally, then
removing smie-indent-close from it and adding some other function
requires a lot more code, and a lot more brittle as well (the ordering
in smie-indent-functions is important, the set of functions in there and
their order is not guaranteed to stay unchanged in future versions, the
replacement function needs to be written (delegating to
smie-indent-close seems like it might not work), ...


        Stefan

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Dmitry Gutov]

On 15.12.2013 03:50, Stefan Monnier wrote:
>> I believe this argument also works against doing it in the rules
>> function, and in favor of adding a defvar.
>
> I don't think so: setting the var is a one-liner, adding the rule to the
> rule-function is also a one-liner.  So either way is just as easy for
> the major-mode.

I mean in terms of code reuse: the rules function is also specific to a 
major mode. The rule itself doesn't look like it'll take just one line 
to me, AFAICS it'll have to duplicate most of the code in 
`smie-indent-close':

     (`(:before . ,(or `")" `"]" `"}"))
      (save-excursion
        (forward-char 1)
        (condition-case nil
           (progn
             (backward-sexp 1)
             (cons 'column . (smie-indent-virtual)))
          (scan-error nil))))

If any other major mode wants to do the same, they have to duplicate 
this, or extract this code to a helper function in smie.el.

Using the rules function will also add 2-3 lines to `smie-indent-close'.

> By contrast, setting smie-indent-functions buffer-locally, then
> removing smie-indent-close from it and adding some other function
> requires a lot more code, and a lot more brittle as well (the ordering
> in smie-indent-functions is important, the set of functions in there and
> their order is not guaranteed to stay unchanged in future versions, the
> replacement function needs to be written (delegating to
> smie-indent-close seems like it might not work), ...

Looks like three lines to me. :)

(setq-local smie-indent-functions (copy-sequence smie-indent-functions))
(setcar (memq 'smie-indent-close smie-indent-functions)
         'ruby--smie-indent-close)

And the replacement function wouldn't be much longer than the added rule.

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Stefan Monnier]

>>> I believe this argument also works against doing it in the rules
>>> function, and in favor of adding a defvar.
>> I don't think so: setting the var is a one-liner, adding the rule to the
>> rule-function is also a one-liner.  So either way is just as easy for
>> the major-mode.
> I mean in terms of code reuse: the rules function is also specific
> to a major mode. The rule itself doesn't look like it'll take just one line
> to me, AFAICS it'll have to duplicate most of the code in
> smie-indent-close':

No, the rule should be something like

   (`(:close-all . ,_) t)

I.e. equivalent to

  (setq-local smie-indent-close-all t)


-- Stefan

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Dmitry Gutov]

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

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> No, the rule should be something like
>
>    (`(:close-all . ,_) t)

Ah, ok. How does this look to you?

Note that it makes the new behavior the default one, because "close all"
sounds more fitting to characterize the current behavior ("closing" all
sexps ending on the current line).

If that's not what you meant, maybe a different method name is in order.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: smie-close-all.diff --]
[-- Type: text/x-diff, Size: 3044 bytes --]

=== modified file 'lisp/emacs-lisp/smie.el'
--- lisp/emacs-lisp/smie.el	2013-12-11 15:59:27 +0000
+++ lisp/emacs-lisp/smie.el	2013-12-15 23:09:35 +0000
@@ -1135,6 +1135,10 @@
 - :list-intro, in which case ARG is a token and the function should return
   non-nil if TOKEN is followed by a list of expressions (not separated by any
   token) rather than an expression.
+- :close-all, in which case ARG is a close-paren token at indentation and
+  the function should return non-nil if it should be aligned with the opener
+  of the last close-paren token on the same line, if there are multiple.
+  Otherwise, it will be aligned with its own opener.
 
 When ARG is a token, the function is called with point just before that token.
 A return value of nil always means to fallback on the default behavior, so the
@@ -1316,8 +1320,8 @@
 (defun smie-indent--rule (method token
                           ;; FIXME: Too many parameters.
                           &optional after parent base-pos)
-  "Compute indentation column according to `indent-rule-functions'.
-METHOD and TOKEN are passed to `indent-rule-functions'.
+  "Compute indentation column according to `smie-rules-function'.
+METHOD and TOKEN are passed to `smie-rules-function'.
 AFTER is the position after TOKEN, if known.
 PARENT is the parent info returned by `smie-backward-sexp', if known.
 BASE-POS is the position relative to which offsets should be applied."
@@ -1330,11 +1334,7 @@
   ;; - :after tok, where
   ;;                  ; after is set; parent=nil; base-pos=point;
   (save-excursion
-    (let ((offset
-           (let ((smie--parent parent)
-                 (smie--token token)
-                 (smie--after after))
-             (funcall smie-rules-function method token))))
+    (let ((offset (smie-indent--rule-1 method token after parent)))
       (cond
        ((not offset) nil)
        ((eq (car-safe offset) 'column) (cdr offset))
@@ -1355,6 +1355,12 @@
                  (smie-indent-virtual) (current-column)))))
        (t (error "Unknown indentation offset %s" offset))))))
 
+(defun smie-indent--rule-1 (method token &optional after parent)
+  (let ((smie--parent parent)
+        (smie--token token)
+        (smie--after after))
+    (funcall smie-rules-function method token)))
+
 (defun smie-indent-forward-token ()
   "Skip token forward and return it, along with its levels."
   (let ((tok (funcall smie-forward-token-function)))
@@ -1423,8 +1429,13 @@
   (save-excursion
     ;; (forward-comment (point-max))
     (when (looking-at "\\s)")
-      (while (not (zerop (skip-syntax-forward ")")))
-        (skip-chars-forward " \t"))
+      (if (smie-indent--rule-1 :close-all
+                               (buffer-substring-no-properties
+                                (point) (1+ (point)))
+                               (1+ (point)))
+          (while (not (zerop (skip-syntax-forward ")")))
+            (skip-chars-forward " \t"))
+        (forward-char 1))
       (condition-case nil
           (progn
             (backward-sexp 1)

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Stefan Monnier]

> Ah, ok.  How does this look to you?

I think it's OK.

> If that's not what you meant, maybe a different method name is in order.

Preserving the old behavior would maybe be better, but ... let's try it
as is.


        Stefan

bug#16116: 24.3.50; smie-indent-close aligns inner closing paren with the outer opening paren

[Dmitry Gutov]

On 16.12.2013 16:23, Stefan Monnier wrote:
> I think it's OK.

Thanks for looking, applied.

> Preserving the old behavior would maybe be better, but ... let's try it
> as is.

I do believe the new behavior is a better default.

Octave, Prolog, etc, examples in test/indent seem unaffected, but there 
is one affected example in elpa/packages/sml-mode/testcases.sml (I'm 
also getting other mismatches there, but those are probably unrelated).