tuareg icon indicating copy to clipboard operation
tuareg copied to clipboard

Are the “new” indentation functions ready?

Open foretspaisibles opened this issue 10 years ago • 14 comments

Tuareg mode completely changed its auto indentation functions, resulting in a quite different user experience. Taste matters left aside, I do not feel that these “new” indentation functions are ready.

There is two serious problems with these new indentation functions:

  • They seem to ignore the tuareg-*-indent variables.
  • They are stubbornly automatic.

About tuareg-*-indent variables. If the variables are not used anymore, they should be removed, their mere existence induces the user in error. If these variables are currently not used anymore but should soon be used again, it is an objective sign that the feature is not terminated and should live in a development branch.

About stubbornness. It seems that the RETURN key calls the automatic indentation functions on the current line. In the cases where these functions are wrong, this is very annoying, as the user must quote most of its input. There is an important case where these functions are almost always wrong: when editing a codebase prepared with other indentation settings. In this case, it is important to preserve the structure to avoid introducing artificial code changes in the repository.


Suggested resolution

  • Revert the change bringing new indentation functions in in the main branch.
  • Start a development branch using the new indentation functions.
  • Prepare an automatic test suite to help debugging indentation functions.
  • Arrange so that a user can choose between the two indentation function sets.

I know very little elisp, but I think I can help for the three first points above.

foretspaisibles avatar Jan 22 '15 08:01 foretspaisibles

The SMIE indentation does not affect whether return auto-indents or not, IOW this is an orthogonal issue. You probably simply have electric-indent-mode enabled (it's a new global default in Emacs-24.4).

monnier avatar Jan 22 '15 13:01 monnier

Touché! You quite made my day! :)

Nevertheless, how do you feel about the tuareg-*-indent part of the issue? Wouldn't it make sense to remove them as they are (as far as I understand) not used any more?

foretspaisibles avatar Jan 22 '15 17:01 foretspaisibles

As long as the old indentation is included (and accessible by customizing tuareg-use-smie) we can't remove them. Also, wherever possible the new SMIE-based code should obey them (but IIRC, many of them can't really be obeyed). But yes, in the mean time, those vars which only apply to the old indentation code should be documented accordingly. Also, if there's a particular indentation variable which the SMIE code doesn't obey but which you'd like to see obeyed, I suggest you report this explicitly with some sample code showing the difference.

monnier avatar Jan 22 '15 22:01 monnier

I moved the configuration variables that SMIE does not use to tuareg_indent.el — that should make clearer which ones are supposed to have an effect.

Chris00 avatar Jan 30 '15 22:01 Chris00

Hello all Maybe only partly related, but I wanted to report frustration with the new SMIE-based tuareg. The problem has to do with indenting anonymous functions. This is what I want:

List.iter (fun x ->
  do something; (* notice: indented by 2 characters *)
) xs;

Tuareg with tuareg-use-smie set to nil gives me the desired behavior. If tuareg-use-smie is set to t, then the function body and the closing paren are aligned on the "fun" keyword, like this:

List.iter (fun x ->
           do something;
          ) xs;

Is this a bug? Is it intentional? If it is intentional, I believe it should be configurable. Thanks, Francois.

fpottier avatar Jul 03 '15 21:07 fpottier

@fpottier This is a common complain whose roots lies in SMIE engine. I agree the behavior should be configurable. The problem is addressed in another issue where a fix—to SMIE—is suggested. Would you mind to try it and report?

@monnier Any news about your work-in-progress patch?

Chris00 avatar Jul 03 '15 23:07 Chris00

I haven't worked on this any further, no. As for François's request, I'm not exactly sure what should be the general behavior. E.g. what should happen for

foo bar (fun x ->
             do something
            )

and

foo (fun x ->
             do something
       )
       xs

and

foo (fun x ->
             do something
       ) xs
       ys
       zs

As for whether it's intention or a bug: it's "intentional" in that SMIE is designed to have the indentation reflect the AST's structure and the general rules for function calls want the arguments to be indented deeper that the function that's called, and ideally that all args of the same funcall should be indented at the same depth, but

foo (fun ->
  do something
)
xs

then has the problem that "xs" is an arg to "foo" but if you indented deeper than "foo" then the close paren looks misindented, whereas if you indented like the close paren, then it's misindented because it's no deeper than "foo". IOW the indentation you want is "wrong", and is only acceptable because it's a very special case: we avoid misindenting the args after the close paren simply because they're not on their own line, and we accept the "misindentation" of the close paren because it's a closer and we're not bothered by the fact that, strictly speaking, it doesn't close the "foo" call but only one of its arguments.

monnier avatar Jul 04 '15 18:07 monnier

Since one must make sure that additional arguments are not to be confused with subsequent instructions, I suggest that the following indentation be used (if the user requests the alternative indentation):

foo (fun x ->
    do something
  ) xs
  ys

To be coherent, I suppose that standard arguments should then be indented as

long_function x y
  z t

@fpottier Any thoughts on this?

Chris00 avatar Jul 04 '15 18:07 Chris00

Maybe only partly related, but I wanted to report frustration with the new SMIE-based tuareg. The problem has to do with indenting anonymous functions.

@fpotier I switched to ocp-indent integration under tuareg and I find it very convenient, both fast and effective.

In ocp-indent they have an automatic test input/expected output, do we want to have something like that on tuareg ? I know too little elisp to handle it all by myself but I could contribute shell scripts to handle the surrounding logic (run the tests, report results) and prepare Travis continuous integration.

For this purpose, does it seem possible to write an elisp function so that we can call something like

emacs --run-that-elisp-code '(load-tuareg)(save "sample_1.got" (tuareg-indent (read "sample_1.ml")))'

(This is pseudo-code, just telling the intent! ☺)

Such automatic testing would probably save a lot of time for the developers, right?

foretspaisibles avatar Jul 05 '15 08:07 foretspaisibles

automatic test input/expected output,

This is not yet in place but this is desirable indeed. It is just lack of time... :-(

Chris00 avatar Jul 05 '15 08:07 Chris00

On Sat, Jul 04, 2015 at 11:18:43AM -0700, monnier wrote:

I haven't worked on this any further, no. As for François's request, I'm not exactly sure what should be the general behavior. E.g. what should happen for

foo bar (fun x ->
             do something
            )

The "traditional" rule for indenting anonymous functions is, I believe:

1- indent the function body by +2 with respect to previous indentation level (not with respect to the "fun" keyword!)

2- indent the closing parenthesis, if alone on its own line, by +0 with respect to the previous indentation level

So the result would be:

foo bar (fun x ->
  do something
)

Now, concerning your further questions, where extra arguments appear after the closing parenthesis, I would like to see this an independent issue: how to indent function calls when the actual parameters are listed on several lines?

For instance, if you like function calls to be indented like this:

f xs
  ys zs

then you should get this:

foo (fun x ->
  do something
)
  xs

and this:

foo (fun x ->
  do something
) xs
  ys
  zs

fpottier avatar Jul 06 '15 13:07 fpottier

and this: foo (fun x -> do something ) xs ys zs

And IIUC, to take an even more general case, you'd like

foo ab cd
  ef gh
  (fun x ->
  do something
) xs
  ys
  zs

?

    Stefan

monnier avatar Jul 06 '15 15:07 monnier

Hi Stefan,

And IIUC, to take an even more general case, you'd like

foo ab cd
  ef gh
  (fun x ->
  do something
) xs
  ys
  zs

Well, the way I interpret my own very informal rules, the current indentation level should increase by 2 for "function arguments beyond the first line", then should again increase by 2 for "an anonymous function body". So I suppose I would expect this:

foo ab cd
  ef gh
  (fun x ->
    do something
  ) xs
  ys
  zs

fpottier avatar Jul 06 '15 16:07 fpottier

Indeed, you'd be welcome to use the indentation tests put in place for ocp-indent. There are two kinds:

  • unit tests (tuned to ocp-indent's specific and options) https://github.com/OCamlPro/ocp-indent/tree/master/tests
  • runs on real code bases with statistics on numbers of correctly indented lines. This already does batch-runs of tuareg, so you could use it already. In a different repo at https://github.com/AltGr/ocp-indent-tests

Both can generate diffs and html summaries

AltGr avatar Oct 01 '15 05:10 AltGr