elixir-lang / elixir

Elixir is a dynamic, functional language for building scalable and maintainable applications
https://elixir-lang.org/
Apache License 2.0
24.28k stars 3.34k forks source link

Macro.to_string adds unnecessary parentheses in 1.18-dev #13743

Closed hissssst closed 1 month ago

hissssst commented 1 month ago

Elixir and Erlang/OTP versions

Erlang/OTP 27 [erts-15.0] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit:ns]

Interactive Elixir (1.18.0-dev) - press Ctrl+C to exit (type h() ENTER for help)

Tried on recent msater 220c228f758469935589b09bb45bbf45e4800bca

Operating system

linux

Current behavior

iex(1)> Macro.to_string quote do: 1 ~> 2 &&& 3
"(1 ~> 2) &&& 3"

Expected behavior

As in 1.17.1

Erlang/OTP 25 [erts-13.2.2.9] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit:ns]

Interactive Elixir (1.17.1) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> Macro.to_string quote do: 1 ~> 2 &&& 3
"1 ~> 2 &&& 3"
josevalim commented 1 month ago

Can you provide more context why this is undesired? We are adding more parentheses around operators since it was requested for clarity.

hissssst commented 1 month ago

Can you provide more context why this is undesired?

First of all, it was already established behavior for many years. This is the main reason: I update elixir and tests fail.

Second, I've just found that it ignores no_parens metadata.

Third, it is partial, since some operators will have it, some won't, which is strange, because if operators will have parentheses in every place, they'll be no different from function calls

iex(17)> Macro.to_string quote do: 1 + 2 + 3
"1 + 2 + 3"
iex(18)> Macro.to_string quote do: 1 + 2 - 3
"1 + 2 - 3"
iex(19)> Macro.to_string quote do: 1 + 2 ~> 3
"(1 + 2) ~> 3"
iex(20)> Macro.to_string quote do: 1 ~> 2 &&& 3
"(1 ~> 2) &&& 3"
iex(21)> Macro.to_string quote do: 1 + 2 * 3
"1 + 2 * 3"
iex(22)> Macro.to_string quote do: 1 ~> 2 - 3
"1 ~> (2 - 3)"
iex(23)> Macro.to_string quote do: 1 * 2 - 3
"1 * 2 - 3"
sabiwara commented 1 month ago

For context: this has been introduced to address this issue, which originally was mostly focused on right-associativity operators, esp. --.

Third, it is partial, since some operators will have it, some won't, which is strange

I think it is a bit unfair to compare + - * and ~>: everybody has a good idea of the precedence of regular arithmetic operators, but custom ones can be less so, so I think it can improve clarity. That being said, there is often a clear equivalent (&&& -> &&), so I agree this feel inconsistent:

iex(1)> Macro.to_string quote do: 1 ~> 2 &&& 3
"(1 ~> 2) &&& 3"
iex(2)> Macro.to_string quote do: 1 ~> 2 && 3
"1 ~> 2 && 3"

@josevalim do you think we could be more conservative and add parentheses only for right-assoc stuff like -- / ---?

hissssst commented 1 month ago

For context: this has been introduced to address this https://github.com/elixir-lang/elixir/issues/13710, which originally was mostly focused on right-associativity operators, esp. --.

Yes, I agree that associativity of operators can be hard to get right without reading the doc. This decision is always up to developers of the language. For example, Python for some reason has ** with right associativity, so there's no consistency for custom operators, that's why every language needs to be clear about associativity and operator precedence.

My opinion: operators are not functions, they're not for writing readable code, but for writing formulas and mathematical expressions. You need to learn their associativity and priority before using them, but once you do, you'll be able to express complex things in a compact way. And to be honest, Elixir already has very clear, short and easily accessible documentation about operators and a limited set of operators, so I would keep the pre-1.18 behavior, this is not Coq or Haskell situation, where operators overuse makes code unreadable even for experienced developers

josevalim commented 1 month ago

To be clear, I was asking for more context on how the operators are used in your scenario.

The formatter rules may change over time if we believe it improves some scenarios and in this case it was meant to improve bit wise operators. Unless there are counter examples, then the change will remain.

hissssst commented 1 month ago

The formatter rules may change over time

To be precise, this is not about formatter rule, but a Macro.to_string change, which was never returning always-formatted strings. Formatting is applied by Code.format_string!, but I am talking about Macro.to_string

josevalim commented 1 month ago

Oh, and we also improved ambiguity around pipeline operators, as foo + bar |> baz was ambiguous.

but I am talking about Macro.to_string

The argument is the same. The focus will be on readability of the code. You are welcome to implement your own version of Macro.to_string if you want minimal parens.

hissssst commented 1 month ago

To be clear, I was asking for more context on how the operators are used in your scenario.

For writing lenses. Lenses have their own algebra and branching rules, and operators reduce amount of code written and improve readability.

For example, lens path(:x) ||| path(:y) takes value by :x key, and if it is not present, takes by :y key

https://hexdocs.pm/pathex

josevalim commented 1 month ago

Thank you. Can you provide the actual examples from pathex affected by this change?

hissssst commented 1 month ago

And lens path(:response) ~> matching(%{status: 200}) &&& path(:data) will take data out of response if status is okay

josevalim commented 1 month ago

Fixed in b799e9eda4613a1bc40fd0824fb08d5df3b3e24b.