fenollp / erlang-formatter

Format Erlang code using Emacs' erlang-mode
Apache License 2.0
45 stars 5 forks source link

Wrong indentation in binary pattern match #45

Closed bjosv closed 2 years ago

bjosv commented 2 years ago

When formatting code we get some strange results from the erlang-formatter. Indifferent of using the makefile-method or via rebar3 fmt the following code example:

-module(parser).

parse(Token) ->
    case Token of
        <<"$", Rest/binary>> -> {done, Rest};
        <<"1", Rest/binary>> -> {done, Rest};
        <<"2", Rest/binary>> -> {done, Rest}
    end.

results in a bit skewed output:

-module(parser).

parse(Token) ->
    case Token of
        <<"$", Rest/binary>> -> {done, Rest};
        <<"1", Rest/binary>> -> {done, Rest};
          <<"2", Rest/binary>> -> {done, Rest}
        end.

By experimenting a bit it seems that "$" is to be blamed for the skewed lines further down (see line with <<"2" and end).

When opening the same file in emacs manually the fomatting can be restored by either the menu Erlang->Indent->Indent buffer or by M-x erlang-indent-current-buffer. Using tab will also correct the lines in emacs. So there seem to be a difference using erlang-formatter and doing it manually.

Another finding is that when trying to break down what fmt.el was doing and reproducing it via an own test function in my emacs, I found that the following function resulted in the same skewed output as from erlang-formatter.

(defun my-erlang-format ()
  (interactive)
  (erlang-mode)
  (erlang-indent-current-buffer))

while removing (erlang-mode) in the above function resulted in an ok output!

Any ideas what is happening?

Tested using GNU Emacs 28.1 and Emacs 26.3 emacs-nox 1:26.3+1, with and without newer erlang.el

fenollp commented 2 years ago

I've created https://github.com/fenollp/erlang-formatter/pull/46 that reproduces the bug you're describing. I'm fairly certain this is https://github.com/erlang/otp/issues/3482 again.

WRT erlang-mode I am not sure what's going on either. Yes tests pass with it removed. Maybe fmt is even a bit faster without it!

zuiderkwast commented 2 years ago

Hi Pierre! We met in Stockholm.

What are the risks of removing (erlang-mode) if erlang-mode is not installed? Will it silently indent using some other language instead of failing?

fenollp commented 2 years ago

Hi Viktor! Wow it's been a long time. Way too long! Nice work on Redis :)

Neither https://www.erlang.org/doc/apps/tools/erlang_mode_chapter.html nor https://www.erlang.org/doc/man/erlang.el.html seem to require erlang-mode. erlang-indent-current-buffer is defined and added to the menu in erlang.el. If the goal is to save some time processing files I'd say to find a way to skip erlang-start.el usage and go straight to erlang.el. See it called here https://github.com/fenollp/erlang-formatter/blob/252cb19f13aa078be852e4dc1ec868a9d9999484/priv/fmt.el#L1 But then that's only a constant gain.

Taking a step back and actually answering your question: erlang-start loads erlang-mode so by the time we're mapping fmt-file the mode has already been loaded IIUC. Good point looking into possible backwards incompatibilities!

zuiderkwast commented 2 years ago

Great! Then I think your PR solves it.

fenollp commented 2 years ago

Hm the PR doesn't solve the $ issue but I gather you meant it saves calling erlang-mode in a loop?

zuiderkwast commented 2 years ago

I thought it soves the $ issue. Sorry I didn't follow properly.

bjosv commented 2 years ago

As an update: I did some debugging using the own defined function my-erlang-format described above. I have one function with and one without (erlang-mode). The difference seems to be that, with erlang-mode set, the parsing seems to have the wrong understanding of the balanced expressions. The function: https://github.com/fenollp/erlang-formatter/blob/252cb19f13aa078be852e4dc1ec868a9d9999484/priv/erlang.el#L2812 moves current (point) far to long with the belief that it's still within a string. We get the same result with erlang-formatter. When it works it moves (point) just past"$" and continues parsing the line.

The balanced expressions seems to be an Emacs feature that determines how things are parsed and it's setup by the mode. Somehow this differs depending on how we start emacs, or the mode.

forward-sexp should use use a syntax table and when looking at this I found the comments that might be the same thing: https://github.com/fenollp/erlang-formatter/blob/252cb19f13aa078be852e4dc1ec868a9d9999484/priv/erlang.el#L1473-L1475 Weird that it works in GUI mode thought.. So, the otp issue you mentioned is probably the right place where to fix this issue.