Lift module attributes to variables when used by directives (#168)

Fixes the longstanding compilation-breaking bug that occurs when module attributes are moved below directives that reference them.

Closes #156, Ref #65 and #43

CHANGELOG.md

@@ -69,6 +69,7 @@ See the moduledoc for `Styler.Style.Configs` for more.
 ### Breaking Changes
 
 * drop support for elixir `1.14`
+* ModuleDirectives: group callback attributes (`before_compile after_compile after_verify`) with nondirectives (previously, were grouped with `use`, their relative order maintained). to keep the desired behaviour, you can make new `use` macros that wrap these callbacks. Apologies if this makes using Styler untenable for your codebase, but it's probably not a good tool for macro-heavy libraries.
 * sorting configs for the first time can change your configuration; see `Styler.Style.Configs` moduledoc for more
 
 ## v0.11.9

lib/style/module_directives.ex

@@ -93,7 +93,6 @@ defmodule Styler.Style.ModuleDirectives do
   alias Styler.Zipper
 
   @directives ~w(alias import require use)a
-  @callback_attrs ~w(before_compile after_compile after_verify)a
   @attr_directives ~w(moduledoc shortdoc behaviour)a
   @defstruct ~w(schema embedded_schema defstruct)a
 
@@ -191,65 +190,119 @@ defmodule Styler.Style.ModuleDirectives do
   defp moduledoc(_), do: nil
 
   @acc %{
-    "@shortdoc": [],
-    "@moduledoc": [],
-    "@behaviour": [],
+    shortdoc: [],
+    moduledoc: [],
+    behaviour: [],
     use: [],
     import: [],
     alias: [],
     require: [],
     nondirectives: [],
-    dealiases: %{}
+    dealiases: %{},
+    attrs: MapSet.new(),
+    attr_lifts: []
   }
 
+  defp lift_module_attrs({node, _, _} = ast, %{attrs: attrs} = acc) do
+    if Enum.empty?(attrs) do
+      {ast, acc}
+    else
+      use? = node == :use
+
+      Macro.prewalk(ast, acc, fn
+        {:@, m, [{attr, _, _} = var]} = ast, acc ->
+          if attr in attrs do
+            replacement =
+              if use?,
+                do: {:unquote, [closing: [line: m[:line]], line: m[:line]], [var]},
+                else: var
+
+            {replacement, %{acc | attr_lifts: [attr | acc.attr_lifts]}}
+          else
+            {ast, acc}
+          end
+
+        ast, acc ->
+          {ast, acc}
+      end)
+    end
+  end
+
   defp organize_directives(parent, moduledoc \\ nil) do
     acc =
       parent
       |> Zipper.children()
       |> Enum.reduce(@acc, fn
+        {:@, _, [{attr_directive, _, _}]} = ast, acc when attr_directive in @attr_directives ->
+          # attr_directives are moved above aliases, so we need to dealias them
+          {ast, acc} = acc.dealiases |> Dealias.apply(ast) |> lift_module_attrs(acc)
+          %{acc | attr_directive => [ast | acc[attr_directive]]}
+
         {:@, _, [{attr, _, _}]} = ast, acc ->
-          key =
-            cond do
-              # TODO drop for a 1.0 release?
-              # the order of callbacks relative to use can matter if the use is also doing callbacks
-              # looking back, this is probably a hack to support one person's weird hackery 🤣
-              attr in @callback_attrs -> :use
-              attr in @attr_directives -> :"@#{attr}"
-              true -> :nondirectives
-            end
-
-          # both callback and attr_directives are moved above aliases, so we need to dealias them
-          ast = if key == :nondirectives, do: ast, else: Dealias.apply(acc.dealiases, ast)
-          %{acc | key => [ast | acc[key]]}
+          %{acc | nondirectives: [ast | acc.nondirectives], attrs: MapSet.put(acc.attrs, attr)}
 
         {directive, _, _} = ast, acc when directive in @directives ->
+          {ast, acc} = lift_module_attrs(ast, acc)
           ast = expand(ast)
           # import and used get hoisted above aliases, so need to dealias
           ast = if directive in ~w(import use)a, do: Dealias.apply(acc.dealiases, ast), else: ast
           dealiases = if directive == :alias, do: Dealias.put(acc.dealiases, ast), else: acc.dealiases
+
           # the reverse accounts for `expand` putting things in reading order, whereas we're accumulating in reverse
-          %{acc | directive => Enum.reverse(ast, acc[directive]), :dealiases => dealiases}
+          %{acc | directive => Enum.reverse(ast, acc[directive]), dealiases: dealiases}
 
         ast, acc ->
           %{acc | nondirectives: [ast | acc.nondirectives]}
       end)
       # Reversing once we're done accumulating since `reduce`ing into list accs means you're reversed!
       |> Map.new(fn
-        {:"@moduledoc", []} -> {:"@moduledoc", List.wrap(moduledoc)}
+        {:moduledoc, []} -> {:moduledoc, List.wrap(moduledoc)}
         {:use, uses} -> {:use, uses |> Enum.reverse() |> Style.reset_newlines()}
-        {directive, to_sort} when directive in ~w(@behaviour import alias require)a -> {directive, sort(to_sort)}
+        {directive, to_sort} when directive in ~w(behaviour import alias require)a -> {directive, sort(to_sort)}
         {:dealiases, d} -> {:dealiases, d}
         {k, v} -> {k, Enum.reverse(v)}
       end)
       |> lift_aliases()
 
+    # Not happy with it, but this does the work to move module attribute assignments above the module or quote or whatever
+    # Given that it'll only be run once and not again, i'm okay with it being inefficient
+    {acc, parent} =
+      if Enum.any?(acc.attr_lifts) do
+        lifts = acc.attr_lifts
+
+        nondirectives =
+          Enum.map(acc.nondirectives, fn
+            {:@, m, [{attr, am, _}]} = ast -> if attr in lifts, do: {:@, m, [{attr, am, [{attr, am, nil}]}]}, else: ast
+            ast -> ast
+          end)
+
+        assignments =
+          Enum.flat_map(acc.nondirectives, fn
+            {:@, m, [{attr, am, [val]}]} -> if attr in lifts, do: [{:=, m, [{attr, am, nil}, val]}], else: []
+            _ -> []
+          end)
+
+        {past, _} = parent
+
+        parent =
+          parent
+          |> Zipper.up()
+          |> Style.find_nearest_block()
+          |> Zipper.prepend_siblings(assignments)
+          |> Zipper.find(&(&1 == past))
+
+        {%{acc | nondirectives: nondirectives}, parent}
+      else
+        {acc, parent}
+      end
+
     nondirectives = acc.nondirectives
 
     directives =
       [
-        acc."@shortdoc",
-        acc."@moduledoc",
-        acc."@behaviour",
+        acc.shortdoc,
+        acc.moduledoc,
+        acc.behaviour,
         acc.use,
         acc.import,
         acc.alias,

test/style/module_directives_test.exs

@@ -388,17 +388,6 @@ defmodule Styler.Style.ModuleDirectivesTest do
       )
     end
 
-    test "groups module callbacks with uses, keeping ordering" do
-      assert_style """
-      defmacro __using__(_) do
-        quote do
-          @after_compile Foo
-          use Bar
-        end
-      end
-      """
-    end
-
     test "interwoven directives w/o the context of a module" do
       assert_style(
         """
@@ -584,4 +573,50 @@ defmodule Styler.Style.ModuleDirectivesTest do
       """
     )
   end
+
+  describe "module attribute lifting" do
+    test "replaces uses in other attributes and `use` correctly" do
+      assert_style(
+        """
+        defmodule MyGreatLibrary do
+          @library_options [...]
+          @moduledoc make_pretty_docs(@library_options)
+          use OptionsMagic, my_opts: @library_options
+        end
+        """,
+        """
+        library_options = [...]
+
+        defmodule MyGreatLibrary do
+          @moduledoc make_pretty_docs(library_options)
+          use OptionsMagic, my_opts: unquote(library_options)
+
+          @library_options library_options
+        end
+        """
+      )
+    end
+
+    test "works with `quote`" do
+      assert_style(
+        """
+        quote do
+          @library_options [...]
+          @moduledoc make_pretty_docs(@library_options)
+          use OptionsMagic, my_opts: @library_options
+        end
+        """,
+        """
+        library_options = [...]
+
+        quote do
+          @moduledoc make_pretty_docs(library_options)
+          use OptionsMagic, my_opts: unquote(library_options)
+
+          @library_options library_options
+        end
+        """
+      )
+    end
+  end
 end