Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/elixir/lib/code.ex
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,11 @@ defmodule Code do
`'foo'` becomes `~c"foo"`.
Defaults to the value of the `:migrate` option. This option changes the AST.

* `:migrate_unless` (since v1.18.0) - when `true`,
rewrites `unless` expressions using `if` with a negated condition, for example
`unless foo, do:` becomes `if !foo, do:`.
Defaults to the value of the `:migrate` option. This option changes the AST.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the corner cases we discussed in the past, such as unless x = expr() do, do we handle those as well? Or we don't? If we don't, we must say not all cases can be automatically migrated. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do handle these, it becomes if !(x = expr()) which is terrible code, but it does work fine and I expect unless x = expr() to be a very rare need anyway since x could only be false or nil. WDYT?

#13769 (comment)
#13769 (comment)


## Design principles

The formatter was designed under three principles.
Expand Down
66 changes: 66 additions & 0 deletions lib/elixir/lib/code/formatter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ defmodule Code.Formatter do
migrate = Keyword.get(opts, :migrate, false)
migrate_bitstring_modifiers = Keyword.get(opts, :migrate_bitstring_modifiers, migrate)
migrate_charlists_as_sigils = Keyword.get(opts, :migrate_charlists_as_sigils, migrate)
migrate_unless = Keyword.get(opts, :migrate_unless, migrate)
syntax_colors = Keyword.get(opts, :syntax_colors, [])

sigils =
Expand All @@ -218,6 +219,7 @@ defmodule Code.Formatter do
file: file,
migrate_bitstring_modifiers: migrate_bitstring_modifiers,
migrate_charlists_as_sigils: migrate_charlists_as_sigils,
migrate_unless: migrate_unless,
inspect_opts: %Inspect.Opts{syntax_colors: syntax_colors}
}
end
Expand Down Expand Up @@ -485,6 +487,27 @@ defmodule Code.Formatter do
binary_op_to_algebra(:in, "not in", meta, left, right, context, state)
end

# rewrite unless as if!
defp quoted_to_algebra(
{:unless, meta, [condition, block]},
context,
%{migrate_unless: true} = state
) do
quoted_to_algebra({:if, meta, [negate_condition(condition), block]}, context, state)
end

defp quoted_to_algebra(
{:|>, meta1, [condition, {:unless, meta2, [block]}]},
context,
%{migrate_unless: true} = state
) do
quoted_to_algebra(
{:|>, meta1, [negate_condition(condition), {:if, meta2, [block]}]},
context,
state
)
end

# ..
defp quoted_to_algebra({:.., _meta, []}, context, state) do
if context in [:no_parens_arg, :no_parens_one_arg] do
Expand Down Expand Up @@ -2450,4 +2473,47 @@ defmodule Code.Formatter do
defp has_double_quote?(chunk) do
is_binary(chunk) and chunk =~ @double_quote
end

# Migration rewrites

@bool_operators [
:>,
:>=,
:<,
:<=,
:in
]
@guards [
:is_atom,
:is_boolean,
:is_nil,
:is_number,
:is_integer,
:is_float,
:is_binary,
:is_map,
:is_struct,
:is_non_struct_map,
:is_exception,
:is_list,
:is_tuple,
:is_function,
:is_reference,
:is_pid,
:is_port
]

defp negate_condition(condition) do
case condition do
{neg, _, [condition]} when neg in [:!, :not] -> condition
{:|>, _, _} -> {:|>, [], [condition, {{:., [], [Kernel, :!]}, [closing: []], []}]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we found the migrations more readable not doing this particular transformation, as this hides the most impactful part of the statement. compare:

# given:
unless Widget |> where(foo: ^bar) |> Repo.exists?(), do: ...
# rewritten to if with pipe
if Widget |> where(foo: ^bar) |> Repo.exists?() |> Kernel.!(), do: ...
# treated as any other statement
if !(Widget |> where(foo: ^bar) |> Repo.exists?()), do:

our team asked for the latter as it has a much more natural-language read (not to mention that Kernel.!() is unlikely to be written by a human in any case)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion 💜 Even if I'm the one who originally implemented it as |> Kernel.!(), now that you mention it I have to agree.

Plus this is removing complexity so I'm all for it.

Do you want to send a PR @novaugust?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to, i'll kick one out tomorrow

{op, _, [_, _]} when op in @bool_operators -> {:not, [], [condition]}
{guard, _, [_ | _]} when guard in @guards -> {:not, [], [condition]}
{:==, meta, [left, right]} -> {:!=, meta, [left, right]}
{:===, meta, [left, right]} -> {:!==, meta, [left, right]}
{:!=, meta, [left, right]} -> {:==, meta, [left, right]}
{:!==, meta, [left, right]} -> {:===, meta, [left, right]}
_ -> {:!, [], [condition]}
end
end
end
78 changes: 78 additions & 0 deletions lib/elixir/test/elixir/code_formatter/migration_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,76 @@ defmodule Code.Formatter.MigrationTest do
end
end

describe "migrate_unless: true" do
@opts [migrate_unless: true]

test "rewrites unless as an if with negated condition" do
bad = "unless x, do: y"

good = "if !x, do: y"

assert_format bad, good, @opts

bad = """
unless x do
y
else
z
end
"""

good = """
if !x do
y
else
z
end
"""

assert_format bad, good, @opts
end

test "rewrites pipelines with negated condition" do
bad = "x |> unless(do: y)"

good = "!x |> if(do: y)"

assert_format bad, good, @opts

bad = "x |> foo() |> unless(do: y)"

good = "x |> foo() |> Kernel.!() |> if(do: y)"

assert_format bad, good, @opts
end

test "rewrites in as not in" do
assert_format "unless x in y, do: 1", "if x not in y, do: 1", @opts
end

test "rewrites equality operators" do
assert_format "unless x == y, do: 1", "if x != y, do: 1", @opts
assert_format "unless x === y, do: 1", "if x !== y, do: 1", @opts
assert_format "unless x != y, do: 1", "if x == y, do: 1", @opts
assert_format "unless x !== y, do: 1", "if x === y, do: 1", @opts
end

test "rewrites boolean or is_* conditions with not" do
assert_format "unless x > 0, do: 1", "if not (x > 0), do: 1", @opts
assert_format "unless is_atom(x), do: 1", "if not is_atom(x), do: 1", @opts
end

test "removes ! or not in condition" do
assert_format "unless not x, do: 1", "if x, do: 1", @opts
assert_format "unless !x, do: 1", "if x, do: 1", @opts
end

test "does nothing without the migrate_unless option" do
assert_same "unless x, do: y"
assert_same "unless x, do: y, else: z"
end
end

describe "migrate: true" do
test "enables :migrate_bitstring_modifiers" do
assert_format "<<foo::binary()>>", "<<foo::binary>>", migrate: true
Expand All @@ -209,5 +279,13 @@ defmodule Code.Formatter.MigrationTest do
test "enables :migrate_charlists_as_sigils" do
assert_format ~S['abc'], ~S[~c"abc"], migrate: true
end

test "enables :migrate_unless" do
bad = "unless x, do: y"

good = "if !x, do: y"

assert_format bad, good, migrate: true
end
end
end
Loading