Add option to formatter to rewrite unless #13769

sabiwara · 2024-08-09T06:00:16Z

This is still a draft in order to explore the possibility, not ready yet.

I haven't started to work on the --migrate flag in mix format yet, this is focusing on the unless rewrite itself.

Commits are organized to help the review:

4dd0d08 is the rewrite itself (esp. see the tests)
e970466 shows what it did when run on the codebase
2c89d85 is the only breaking change that needed to be fixed (defmacro unless)
the rest is manual tweaks I did to improve over the auto-generated ones

lib/mix/lib/mix/tasks/deps.loadpaths.ex

lib/mix/lib/mix/tasks/escript.build.ex

lib/mix/lib/mix/tasks/test.ex

lib/mix/lib/mix/utils.ex

lib/mix/test/mix/release_test.exs

lib/mix/test/test_helper.exs

lib/mix/lib/mix/tasks/run.ex

lib/mix/lib/mix/tasks/deps.compile.ex

lib/elixir/test/elixir/file_test.exs

josevalim · 2024-08-09T07:49:56Z

Thank you @sabiwara. ❤️ I am generally ok with this change. I think the automatically rewriting will definitely leave some gaps for improvements, but I really can't see any loss of readability in any of these changes (before or after adjustments).

I'd love to hear folks thoughs on this one.

PS: Also a separate discussion, but we should probably pick a default name for all "rewrite" options. The two that exist today are called normalize_. But that's another PR.

Co-authored-by: José Valim <jose.valim@gmail.com>

sabiwara · 2024-08-24T04:45:47Z

Rebased and opening for reviews. Opinions welcome.

If we agree on this one, the next step would be to add the --migrate flag to mix format which would include rewrite options, and soft deprecate unless.
I'd also like to revisit #11845 since it would be a good candidate for the migrate flag.

I'm still on the fence if we should include the existing normalize_ options in the --migrate flag and rename them, since this would be a small breaking change - but at least we're not validating format opts so it might be OK.

josevalim · 2024-08-24T07:47:57Z

I think changing normalize is fine. If you are setting it today, is to set it to false, which will be the new behavior anyway. Plus if you ran it once, you don’t have to run it again, which makes it a perfect candidate for migrate.

sabiwara · 2024-08-27T04:32:57Z

Sounds good.
Naming-wise, I think it would be good to have all the --migrate options prefixed by migrate_, e.g. migrate_unless.
And we could even have a meta-option migrate defaulting to false, which would itself be the default for all migrate_ options. WDYT?

If we are OK with this current PR, perhaps I can merge it and work on the flags in a follow-up PR?

novaugust · 2024-08-29T16:17:12Z

I'd love to hear folks thoughs on this one.

I'm all for it! unless can be useful for natural language reading for native speakers, but (imo) is a somewhat surprising programming construct for folks who haven't seen it in other languages. lots of style guides recommend avoiding it in various cases.

the only thing i'll really miss is the succinctness of using it to raise exceptions (unless all_is_well?, do: raise "boom"). but, fewer equivalent choices for developers + standardization across teams/codebases is a big win.

styler rewrites some forms to unless, but changes lots of unless to if instead. happy to just if everything

that said, here's a very convoluted code snippet (people have done stranger) that i'd be curious to see a rewrite for:

unless x = false, do: !x

neeeeevermind, i saw it gets negated (didn't know you could wrap assignments in !, TIL)

iex(1)> unless x = false, do: !x
true
iex(2)> if  !(x = false), do: !x
true

novaugust · 2024-08-29T16:34:46Z

lib/elixir/test/elixir/file_test.exs

@@ -1794,7 +1794,7 @@ defmodule FileTest do
        stat = File.stat!(fixture)
        assert stat.mode == 0o100666

-        unless windows?() do
+        if not windows?() do


these are manual ! to not tweaks right?

Correct (you can see individual commits to see what is automatic or tweaked)

novaugust · 2024-08-29T16:36:18Z

lib/elixir/lib/code/formatter.ex

+    :>,
+    :>=,
+    :<,
+    :<=,
+    :in


and and or could be added here?

I wish we could, but unfortunately this wouldn't be safe and might break code that is working today, since false or 42 is valid (also arguably pretty bad) but not (false or 42) would raise.

ahhh right, i always think that those are bool, bool -> bool, forgot it's bool, any -> any
i'll just join you in wishing we could then

I guess we could try harder in these cases, checking if the right handside itself is a guard/comparison... but I'm not sure it's worth the extra complexity and it won't capture cases like and enabled? anyway.

alternatively, only use not with in, and for the 4 comparisons rewrite them to their opposite like with the equality operators:

{:>, meta, [left, right]} -> {:<=, meta, [left, right]} {:>=, meta, [left, right]} -> {:<, meta, [left, right]} ...

I hesitated between the two approaches, not sure which would be better. Maybe yours is more natural indeed.

Actually not might be better in this case, if we consider that the user explicitly wrote:

unless x > @limit do

over

if x <= @limit do

which they could have done in the first place, then

if not x > @limit do

might read better to them for some reason?

It is not critical though, people might see the diff and fix it to their preference eventually.
So I'm fine with either approach.

yeah, i think you're right. if not is a simpler change to reason over than if $swapped_equality_operator. thanks for spelling that scenario out.

sabiwara · 2024-08-29T21:35:34Z

neeeeevermind, i saw it gets negated (didn't know you could wrap assignments in !, TIL)

Yes it is quite unreadable and people might want to do it differently, but it feels quite a rare case since assignment is mostly useful for if rather than unless.

sabiwara · 2024-09-06T12:49:43Z

Closing this one for now, merged #13808 and will reopen when I have something more ready with the full implementation.

josevalim · 2024-09-06T16:04:34Z

@sabiwara we also don't need to rewrite all of them. If we can rewrite most, that will already help many codebases migrate.

novaugust · 2024-09-06T19:41:54Z

May I ask for the context I'm missing on the motivations for this work? is unless on the way out for elixir 2.0?

sabiwara · 2024-09-06T22:14:49Z

@novaugust yes the idea is to eventually deprecate unless, but to offer the tools to make the transition more smooth.

novaugust · 2024-09-09T16:08:56Z

cool -- if that's for sure happening i'll steal what you've started here and have styler drop unless as well then =)

novaugust · 2024-09-09T17:49:47Z

lib/elixir/lib/code/formatter.ex

+      {op, _, [_, _]} when op in @bool_operators -> {:not, [], [condition]}
+      {guard, _, [_ | _]} when guard in @guards -> {:not, [], [condition]}


a thought i had while implementing this for styler -- being smart and using not here might be dangerous, because it's possible the user did something whacky like

import Kernel, except: [is_atom: 1] def is_atom(...), do: :ok

better to just always use ! and leave it to the user to translate to not as they want?

The idea behind migrate option is that they rewrite the AST and cannot guarantee to work in edge cases where metaprogramming assumes a given AST or Kernel overwrites are done (after all people can have re-defined unless itself, or ==, etc...).

This particular one feels like an unlikely case, the need to overwriting is_atom feels low, the convention that only guards returning booleans should start with is_ should also discourage it... I think we're fine.