Function indentation about tuareg HOT 12 CLOSED

I personally like the new behavior. But a configuration option would be nice I guess.

from tuareg.

The behaviour changed at commit 3933dc4, when SMIE became the default, but nowadays disabling SMIE doesn't revert to the old indentation (which is a good thing of course)

from tuareg.

A parameter to set a maximum indentation with respect to the previous non-empty line would work great for me. I'm used to the following style:

with_connection (fun conn ->
  do_something conn x;
  ...
)

and

logf `Info "User %s has %i new messages"
  (Uid.to_string uid)
  (List.length new_messages);

How can I achieve this? If the feature seems reasonable but needs to be added, what part of the code should I look at?

from tuareg.

This new behaviour is very annoying when working on a code base that has been indented using the old behaviour. Also not everyone likes to use indentation for alignment. So some configurable options would be really nice here. Or would the "proper" solution be to just go with ocp-indent?

from tuareg.

This new behaviour is very annoying when working on a code base that
has been indented using the old behaviour. Also not everyone likes to
use indentation for alignment. So some configurable options would be
really nice here.

There's clearly some interest in providing some way for SMIE modes to
use a slightly different kind of indentation style (for sh-mode, this
would look like a notion of "continued line").

I'd be happy to provide it, but I generally find it difficult to find
an actual rule that works acceptably for "all" cases.

Or would the "proper" solution be to just go with ocp-indent?

E.g. let's take Martin's example:

let x = with_connection (fun conn ->
do_something conn x;
...
)

and let's pass it to ocp-indent:

let x = with_connection (fun conn ->
do_something conn x;
...
)

OK, it looks like ocp-indent found a set of rules that cover this case
in the way you expect. Let's try to tweak the example by adding another
argument to that function:

let x = with_connection (fun conn ->
do_something conn x;
...
)
toto

gets indented by ocp-indent as:

let x = with_connection (fun conn ->
do_something conn x;
...
)
toto

I don't know about you, but I consider the above indentation as a bug.
Not to imply that ocp-indent is crap (Tuareg's SMIE indentation has its
share of bugs), but the problem is that I don't know how to come up with
an indentation rule that gets you what you want without suffering from
such bad indentation cases.

And ocp-indent has an "easier" problem than we do, because by design, it
has the complete parse tree at hand, so it could trivially check if
there's an additional argument after the "(fun conn -> ...)" and use
another rule in that case. Tuareg's SMIE code could also look past the
(fun conn -> ...) to decide how to indent, but it's generally considered
undesirable to change the indentation depending on code in
subsequent lines.

Martin's suggestion to just "bound" the additional indentation could
work, but it's still not great. It would probably look like:

let x = with_connection (fun conn ->
do_something conn x;
...
)
toto

in the best case, and if it's not supported by additional changes it
could very well look like:

let x = with_connection (fun conn ->
do_something conn x;
...
)
toto

which I find rather unpalatable.

    Stefan

from tuareg.

For the record, I installed ocp-indent via opam, and added a

(require 'ocp-indent)

in my .emacs to avoid the problem. I agree though that the example given by Stefan is not well handled by ocp-indent, but I still prefer it to the current SMIE behaviour. Of course that's a matter of taste.

from tuareg.

[ Somehow my first email reply was included above, but my second was ignored. So here it is, inserted via the web interface instead. ]

Regarding indentation of:

logf `Info "User %s has %i new messages"
  (Uid.to_string uid)
  (List.length new_messages);

Could you try the patch below, then (setq smie-indent-align-args nil)
and see if you like the result?

-- Stefan

=== modified file 'lisp/emacs-lisp/smie.el'
--- lisp/emacs-lisp/smie.el 2014-07-21 01:58:43 +0000
+++ lisp/emacs-lisp/smie.el 2014-10-26 20:10:29 +0000
@@ -1177,6 +1177,7 @@
            (not (eobp))
            ;; Could be an open-paren.
            (forward-char 1))
+               ;; FIXME: Add better hook here!
           (funcall smie--hanging-eolp-function)
                (point))))))

@@ -1666,6 +1667,9 @@
         (+ (smie-indent-virtual) (smie-indent--offset 'basic))) ;
        (t (smie-indent-virtual))))))                            ;An infix.

+(defvar smie-indent-align-args t
+  "Non-nil if indentation should try to align arguments.")
+
 (defun smie-indent-exps ()
   ;; Indentation of sequences of simple expressions without
   ;; intervening keywords or operators.  E.g. "a b c" or "g (balbla) f".
@@ -1700,12 +1704,12 @@
         ;; We're the first expression of the list.  In that case, the
         ;; indentation should be (have been) determined by its context.
         nil)
-       (arg
+       ((and arg smie-indent-align-args)
         ;; There's a previous element, and it's not special (it's not
         ;; the function), so let's just align with that one.
         (goto-char (car positions))
         (current-column))
-       ((cdr positions)
+       ((and (cdr positions) smie-indent-align-args)
         ;; We skipped some args plus the function and bumped into something.
         ;; Align with the first arg.
         (goto-char (cadr positions))

from tuareg.

OK, I have another work-in-progress patch which (combined with the previous (setq smie-indent-align-args nil)) can do the following:

let quux list = List.map list ~f:(fun item ->
                    print_item item
                  )

This is still not what ocp-indent does, but it's a step in that direction. I think I can get pretty close to ocp-indent's behaviour with one more change (specifically when indenting args in "blabla = fun args").

from tuareg.

@monnier I think your comment just ended up here #24.

With the patch applied (on top of Emacs 24.4) and smie-indent-align-args set to nil the example you give indents like this. This looks broken to me. But maybe I'm just asking for different things than the original reporter.

let () =
  logf `Info "User %s has %i new messages"
    (Uid.to_string uid)
      (List.length new_messages)

I guess it will not be feasible to reproduce tuareg-mode's old indentation behavior exactly with smie.

from tuareg.

btw, having unit tests could help for this kinds of issues. See for instance the ocp-indent ones (and the failing ones)

from tuareg.

I installed a patch that introduces a new config var tuareg-indent-align-with-first-arg which should let you express your preference for the "logf" example above.

from tuareg.

Could you guys try the latest code and see how you like the result? Some of the changes depend on a new (and undocumented :-( ) feature in Emacs-24.5, so make sure you try it in Emacs-24.5 (it should not break in older Emacsen, but it won't give the same indentation results).

I'll close this issue now, so if the result is not "good enough", please open a new issue.

from tuareg.