From 8746cdee57239e59c68f9265221a6797d6ebf349 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Mon, 8 Apr 2024 15:36:43 -0600 Subject: [PATCH 1/5] de-alias when sorting module directives. closes #137 --- README.md | 37 +--------------- lib/style/module_directives.ex | 43 ++++++++++++++++++- .../module_directives/alias_lifting_test.exs | 35 +++++++-------- test/style/module_directives_test.exs | 34 ++++++++++++++- 4 files changed, 93 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 69dc6229..6eea95cf 100644 --- a/README.md +++ b/README.md @@ -108,42 +108,7 @@ Once styled the first time, future styling formats shouldn't take noticeably mor ### Troubleshooting: Compilation broke due to Module Directive rearrangement -Sometimes naively moving Module Directives around can break compilation. - -Here's helpers on how to manually fix that and have a happy styling for the rest of -your codebase's life. - -#### Alias dependency - -If you have an alias that, for example, a `@behaviour` relies on, compilation will break after your first run. -This requires one-time manual fixing to get your repo in line with Styler's standards. - -For example, if your code was this: -```elixir -defmodule MyModule do - @moduledoc "Implements MyBehaviour!" - alias Deeply.Nested.MyBehaviour - @behaviour MyBehaviour - ... -end -``` - -then Styler will style the file like this, which cannot compile due to `MyBehaviour` not being defined. - -```elixir -defmodule MyModule do - @moduledoc "Implements MyBehaviour!" - @behaviour MyBehaviour # <------ compilation error, MyBehaviour is not defined! - - alias Deeply.Nested.MyBehaviour - - ... -end -``` - -A simple solution is to manually expand the alias with a find-replace-all like: -`@behaviour MyBehaviour` -> `@behaviour Deeply.Nested.MyBehaviour`. It's important to specify that you only want to -find-replace with the `@behaviour` prefix or you'll unintentially expand `MyBehaviour` everywhere in the codebase. +Styler naively moves module attributes, which can break compilation. For now, the only fix is some elbow grease. #### Module Attribute dependency diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index d46cccec..22dcbbca 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -31,7 +31,7 @@ defmodule Styler.Style.ModuleDirectives do **This can break your code.** - ### Strict Layout: alias referents + ### Strict Layout Modules directives are sorted into the following order: @@ -229,6 +229,7 @@ defmodule Styler.Style.ModuleDirectives do aliases, requires ] + |> Stream.map(&dealias(&1, aliases)) |> Enum.concat() |> Style.fix_line_numbers(List.first(nondirectives)) @@ -367,6 +368,46 @@ defmodule Styler.Style.ModuleDirectives do |> Zipper.node() end + # these are just lazy hacks so i can Enum.map the list of directives while not doing weird stuff to the aliases themselves + defp dealias([], _), do: [] + defp dealias([{:alias, _, _} | _] = aliases, _), do: aliases + defp dealias([{:require, _, _} | _] = requires, _), do: requires + + defp dealias(non_aliases, aliases) do + latest_non_aliases = non_aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.max(fn -> -1 end) + earliest_alias = aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.min(fn -> 999_999 end) + + if earliest_alias >= latest_non_aliases do + non_aliases + else + Enum.map(non_aliases, fn {_, meta, _} = ast -> + line = meta[:line] + + if line < earliest_alias do + ast + else + dealiases = + aliases + |> Enum.filter(fn {_, meta, _} -> meta[:line] < line end) + |> Map.new(fn {:alias, _, [{:__aliases__, _, aliases}]} -> {List.last(aliases), aliases} end) + + ast + |> Zipper.zip() + |> Zipper.traverse(fn + {{:__aliases__, meta, [first | rest]}, _} = zipper -> + if dealias = dealiases[first], + do: Zipper.replace(zipper, {:__aliases__, meta, dealias ++ rest}), + else: zipper + + zipper -> + zipper + end) + |> Zipper.node() + end + end) + end + end + defp expand_and_sort(directives) do # sorting is done with `downcase` to match Credo directives diff --git a/test/style/module_directives/alias_lifting_test.exs b/test/style/module_directives/alias_lifting_test.exs index 1c56cbdd..67f016e5 100644 --- a/test/style/module_directives/alias_lifting_test.exs +++ b/test/style/module_directives/alias_lifting_test.exs @@ -156,23 +156,24 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do test "lifts in modules with only-child bodies" do assert_style """ - defmodule A do - def lift_me() do - A.B.C.foo() - A.B.C.baz() - end - end - """,""" - defmodule A do - @moduledoc false - alias A.B.C - - def lift_me do - C.foo() - C.baz() - end - end - """ + defmodule A do + def lift_me() do + A.B.C.foo() + A.B.C.baz() + end + end + """, + """ + defmodule A do + @moduledoc false + alias A.B.C + + def lift_me do + C.foo() + C.baz() + end + end + """ end describe "comments stay put" do diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs index 8e0ac2b1..19030def 100644 --- a/test/style/module_directives_test.exs +++ b/test/style/module_directives_test.exs @@ -224,9 +224,9 @@ defmodule Styler.Style.ModuleDirectivesTest do @behaviour Lawful use B - use A + use A.A - import A + import A.A import C alias A.A @@ -548,4 +548,34 @@ defmodule Styler.Style.ModuleDirectivesTest do assert_style "@derive Inspect" end + + test "de-aliases use/behaviour/import/moduledoc" do + assert_style """ + defmodule MyModule do + alias A.B.C + @moduledoc "Implements \#{C.foo()}!" + alias D.F.C + import C + alias G.H.C + @behaviour C + alias Z.X.C + use SomeMacro, with: C + end + """, + """ + defmodule MyModule do + @moduledoc "Implements \#{A.B.C.foo()}!" + @behaviour G.H.C + + use SomeMacro, with: Z.X.C + + import D.F.C + + alias A.B.C + alias D.F.C + alias G.H.C + alias Z.X.C + end + """ + end end From fcb8243b86addc5451e16ac2dc81086af95209d1 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Mon, 8 Apr 2024 15:38:56 -0600 Subject: [PATCH 2/5] hmm --- lib/style/module_directives.ex | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index 22dcbbca..12101174 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -372,10 +372,11 @@ defmodule Styler.Style.ModuleDirectives do defp dealias([], _), do: [] defp dealias([{:alias, _, _} | _] = aliases, _), do: aliases defp dealias([{:require, _, _} | _] = requires, _), do: requires + defp dealias(directives, []), do: directives defp dealias(non_aliases, aliases) do - latest_non_aliases = non_aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.max(fn -> -1 end) - earliest_alias = aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.min(fn -> 999_999 end) + latest_non_aliases = non_aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.max() + earliest_alias = aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.min() if earliest_alias >= latest_non_aliases do non_aliases From 588167c36dbd6cf154939c0a0595cb5f35f81759 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Mon, 8 Apr 2024 15:43:02 -0600 Subject: [PATCH 3/5] dont try so hard --- lib/style/module_directives.ex | 72 +++++++++++++++------------------- 1 file changed, 32 insertions(+), 40 deletions(-) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index 12101174..620600c0 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -218,6 +218,7 @@ defmodule Styler.Style.ModuleDirectives do requires = expand_and_sort(directives[:require] || []) {aliases, requires, nondirectives} = lift_aliases(aliases, requires, nondirectives) + earliest_alias_line = aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.min(fn -> nil end) directives = [ @@ -229,8 +230,7 @@ defmodule Styler.Style.ModuleDirectives do aliases, requires ] - |> Stream.map(&dealias(&1, aliases)) - |> Enum.concat() + |> Enum.flat_map(&dealias(&1, aliases, earliest_alias_line)) |> Style.fix_line_numbers(List.first(nondirectives)) # the # of aliases can be decreased during sorting - if there were any, we need to be sure to write the deletion @@ -369,44 +369,36 @@ defmodule Styler.Style.ModuleDirectives do end # these are just lazy hacks so i can Enum.map the list of directives while not doing weird stuff to the aliases themselves - defp dealias([], _), do: [] - defp dealias([{:alias, _, _} | _] = aliases, _), do: aliases - defp dealias([{:require, _, _} | _] = requires, _), do: requires - defp dealias(directives, []), do: directives - - defp dealias(non_aliases, aliases) do - latest_non_aliases = non_aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.max() - earliest_alias = aliases |> Stream.map(fn {_, meta, _} -> meta[:line] end) |> Enum.min() - - if earliest_alias >= latest_non_aliases do - non_aliases - else - Enum.map(non_aliases, fn {_, meta, _} = ast -> - line = meta[:line] - - if line < earliest_alias do - ast - else - dealiases = - aliases - |> Enum.filter(fn {_, meta, _} -> meta[:line] < line end) - |> Map.new(fn {:alias, _, [{:__aliases__, _, aliases}]} -> {List.last(aliases), aliases} end) - - ast - |> Zipper.zip() - |> Zipper.traverse(fn - {{:__aliases__, meta, [first | rest]}, _} = zipper -> - if dealias = dealiases[first], - do: Zipper.replace(zipper, {:__aliases__, meta, dealias ++ rest}), - else: zipper - - zipper -> - zipper - end) - |> Zipper.node() - end - end) - end + defp dealias([{:alias, _, _} | _] = aliases, _, _), do: aliases + defp dealias([{:require, _, _} | _] = requires, _, _), do: requires + defp dealias(directives, [], _), do: directives + + defp dealias(directives, aliases, earliest_alias_line) do + Enum.map(directives, fn {_, meta, _} = ast -> + line = meta[:line] + + if line < earliest_alias_line do + ast + else + dealiases = + aliases + |> Enum.filter(fn {_, meta, _} -> meta[:line] < line end) + |> Map.new(fn {:alias, _, [{:__aliases__, _, aliases}]} -> {List.last(aliases), aliases} end) + + ast + |> Zipper.zip() + |> Zipper.traverse(fn + {{:__aliases__, meta, [first | rest]}, _} = zipper -> + if dealias = dealiases[first], + do: Zipper.replace(zipper, {:__aliases__, meta, dealias ++ rest}), + else: zipper + + zipper -> + zipper + end) + |> Zipper.node() + end + end) end defp expand_and_sort(directives) do From cff10d7b44e13d08ace68677d5daff87f62cc2b8 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Mon, 8 Apr 2024 15:54:03 -0600 Subject: [PATCH 4/5] yeah, thatll do --- lib/style/module_directives.ex | 62 ++++++++++++++++------------------ 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index 620600c0..ff32714b 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -230,7 +230,8 @@ defmodule Styler.Style.ModuleDirectives do aliases, requires ] - |> Enum.flat_map(&dealias(&1, aliases, earliest_alias_line)) + |> Stream.concat() + |> Enum.map(&dealias(&1, aliases, earliest_alias_line)) |> Style.fix_line_numbers(List.first(nondirectives)) # the # of aliases can be decreased during sorting - if there were any, we need to be sure to write the deletion @@ -368,37 +369,34 @@ defmodule Styler.Style.ModuleDirectives do |> Zipper.node() end - # these are just lazy hacks so i can Enum.map the list of directives while not doing weird stuff to the aliases themselves - defp dealias([{:alias, _, _} | _] = aliases, _, _), do: aliases - defp dealias([{:require, _, _} | _] = requires, _, _), do: requires - defp dealias(directives, [], _), do: directives - - defp dealias(directives, aliases, earliest_alias_line) do - Enum.map(directives, fn {_, meta, _} = ast -> - line = meta[:line] - - if line < earliest_alias_line do - ast - else - dealiases = - aliases - |> Enum.filter(fn {_, meta, _} -> meta[:line] < line end) - |> Map.new(fn {:alias, _, [{:__aliases__, _, aliases}]} -> {List.last(aliases), aliases} end) - - ast - |> Zipper.zip() - |> Zipper.traverse(fn - {{:__aliases__, meta, [first | rest]}, _} = zipper -> - if dealias = dealiases[first], - do: Zipper.replace(zipper, {:__aliases__, meta, dealias ++ rest}), - else: zipper - - zipper -> - zipper - end) - |> Zipper.node() - end - end) + # if a behaviour/use/import/whatever relied on an `alias` above it, de-aliasing + # fixes that node to have the full reference again - which is necessary since the alias will be moving below it + defp dealias(node, aliases, earliest_alias_line) + # skip alias/require nodes, as they don't need dealiasing + defp dealias({:alias, _, _} = ast, _, _), do: ast + defp dealias({:require, _, _} = ast, _, _), do: ast + # no aliases, so nothing to de-alias + defp dealias(ast, [], _), do: ast + + defp dealias({_, meta, _} = ast, aliases, earliest_alias_line) do + line = meta[:line] + + if line < earliest_alias_line do + ast + else + dealiases = + aliases + |> Enum.filter(fn {_, meta, _} -> meta[:line] < line end) + |> Map.new(fn {:alias, _, [{:__aliases__, _, aliases}]} -> {List.last(aliases), aliases} end) + + Macro.prewalk(ast, fn + {:__aliases__, meta, [first | rest]} = ast -> + if dealias = dealiases[first], do: {:__aliases__, meta, dealias ++ rest}, else: ast + + ast -> + ast + end) + end end defp expand_and_sort(directives) do From 1de03952df01cd5f462a25d511ca4b62e6affe37 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Mon, 8 Apr 2024 15:57:31 -0600 Subject: [PATCH 5/5] format tests --- .../module_directives/alias_lifting_test.exs | 40 ++++++------- test/style/module_directives_test.exs | 56 ++++++++++--------- 2 files changed, 50 insertions(+), 46 deletions(-) diff --git a/test/style/module_directives/alias_lifting_test.exs b/test/style/module_directives/alias_lifting_test.exs index 67f016e5..7f34f157 100644 --- a/test/style/module_directives/alias_lifting_test.exs +++ b/test/style/module_directives/alias_lifting_test.exs @@ -155,25 +155,27 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do end test "lifts in modules with only-child bodies" do - assert_style """ - defmodule A do - def lift_me() do - A.B.C.foo() - A.B.C.baz() - end - end - """, - """ - defmodule A do - @moduledoc false - alias A.B.C - - def lift_me do - C.foo() - C.baz() - end - end - """ + assert_style( + """ + defmodule A do + def lift_me() do + A.B.C.foo() + A.B.C.baz() + end + end + """, + """ + defmodule A do + @moduledoc false + alias A.B.C + + def lift_me do + C.foo() + C.baz() + end + end + """ + ) end describe "comments stay put" do diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs index 19030def..299d22d1 100644 --- a/test/style/module_directives_test.exs +++ b/test/style/module_directives_test.exs @@ -550,32 +550,34 @@ defmodule Styler.Style.ModuleDirectivesTest do end test "de-aliases use/behaviour/import/moduledoc" do - assert_style """ - defmodule MyModule do - alias A.B.C - @moduledoc "Implements \#{C.foo()}!" - alias D.F.C - import C - alias G.H.C - @behaviour C - alias Z.X.C - use SomeMacro, with: C - end - """, - """ - defmodule MyModule do - @moduledoc "Implements \#{A.B.C.foo()}!" - @behaviour G.H.C - - use SomeMacro, with: Z.X.C - - import D.F.C - - alias A.B.C - alias D.F.C - alias G.H.C - alias Z.X.C - end - """ + assert_style( + """ + defmodule MyModule do + alias A.B.C + @moduledoc "Implements \#{C.foo()}!" + alias D.F.C + import C + alias G.H.C + @behaviour C + alias Z.X.C + use SomeMacro, with: C + end + """, + """ + defmodule MyModule do + @moduledoc "Implements \#{A.B.C.foo()}!" + @behaviour G.H.C + + use SomeMacro, with: Z.X.C + + import D.F.C + + alias A.B.C + alias D.F.C + alias G.H.C + alias Z.X.C + end + """ + ) end end