Add :normalize_calls_on_pipe to Code.Formatter

[josevalim]

It should rewrite both:

foo |> bar
foo |> bar.baz

to:

foo |> bar()
foo |> bar.baz()

Then we can deprecate piping into a variable.

[sabiwara]

Hi, I was starting to look into this, quick question: Should we hard-code :|> here, or should it be a :normalize_calls_on_pipe_operators defaulting to [:|>]?

[josevalim]

I like the list version, good call!

[sabiwara]

I just noticed after implementing this is something I already implemented in the past and we ended up closing it because of the problem of defmacro 😅

-defmacro left |> right do
+defmacro left |> right() do

I'm starting to think that this feature is a bit too nice to give up though. I'm trying to find a way we could proceed without making the formatter aware of defmacro, for instance by having an extra config list (e.g. never_normalize_args_of: [:defmacro], which would prevent the effect of :normalize_calls_on_pipe_operators or :enforce_calls_on_right_hand_side_of. Thoughts? :)

[KiKoS0]

Hey 👋 Just wanted to mention that I was looking into this task, trying to get familiar with the codebase and I gave up as soon as I messed up the pipe defmacro 😄 So I'm really curious on how this gets solved.

[josevalim]

This is tricky because this change will definitely be a breaking change. So I think, once again, we will have to give up on it :( (and I hope we will remind it next time, haha). Thank you @KiKoS0 and @sabiwara!

[sabiwara]

and I hope we will remind it next time, haha

Haha, I hope so too, I can't believe I reimplemented almost the same thing twice without remembering 🤣

This is tricky because this change will definitely be a breaking change

I'm not sure, but maybe if there are options to disable it and if default settings don't break regular elixir code, it might not be considered a breaking change?

I have another proposal: I can open a PR that doesn't modify the formatter but just applies formatting to existing pipes throughout the elixir repo. Since the elixir codebase is one of the go-to examples people check for, it might be good to adopt the style we want to encourage even if we don't automate it. WDYT?

[josevalim]

The goal of this would be to automate so we can deprecate pipes without parens but if we can't automate, we can't deprecate, so there isn't much value in adding the parens I think. :)

[sabiwara]

I think now that we have the --migrate flag we can re-open this one? Will start working on it if we agree.

[josevalim]

There is a downside here: every time we run --migrate in Elixir, it will make the code invalid, and we will have to manually fix it. But maybe we can get around this by setting the :migrate_... options directly on our .formatters.exs? Should we have all of them, except the pipe one, enabled by default?

[lukaszsamson]

Other tools use a separate flag for unsafe source modifications e.g. ruff uses —unsafe-fixes

[josevalim]

Yes, our --migrate is meant to be things that change AST and are definitely unsafe. So we have added the functionality, the question now is which transformations to bake in.

[sabiwara]

There is a downside here: every time we run --migrate in Elixir, it will make the code invalid, and we will have to manually fix it.

This is already the case for migrate_unless, so it shouldn't be an issue?

But maybe we can get around this by setting the :migrate_... options directly on our .formatters.exs?

We could indeed, if we feel we need to run --migrate on the elixir codebase. Otherwise if we run it without the option it doesn't really matter I think?

[josevalim]

Does migrate_unless currently make Elixir code itself invalid like pipe does? I guess the same Kernel.defmacro issue?

[sabiwara]

Does migrate_unless currently make Elixir code itself invalid like pipe does? I guess the same Kernel.defmacro issue?

Correct!

defmacro if(!condition, clauses) do

[sabiwara]

Although there is an argument that could be made that --migrate being best effort, and for unless we already have some Kernel-aware rewrite rules (guards, comparison operators...), so we could make these rules smarter to disable them within defmacro?

[josevalim]

so we could make these rules smarter to disable them within defmacro?

💯

[sabiwara]

Then we can deprecate piping into a variable.

We ended up deciding against this, since there is no ambiguity we don't need the warning here.