Repository: adobe/elixir-styler (elixir)

Issue:
feat: comment directive "sort"
Often there's a series of ast siblings that codebase authors want to keep sorted. We know this to be the case for things like aliases, imports, etc, but what about arbitrary lists within the code? The author just has to put a comment like "please keep this sorted :(" and hope their coworkers play along.

But the whole point of styler is saving you from having to think about your coworkers preferences! (this is a joke. sort of.)

So maybe it could maintain this for us?

## Example Input

```elixir
# given
@my_list [
  # styler:sort
  :a,
  :quick,
  :brown,
  :fox
]
# get
@my_list [
  # styler:sort
  :a,
  :brown,
  :fox,
  :quick
]
```

could be neat!


Pull request description:
Maintain comment-node relations when applying `#styler:sort` directive
Closes #167 

Patch:
diff --git a/lib/style.ex b/lib/style.ex
index d58ad1c..80c50b2 100644
--- a/lib/style.ex
+++ b/lib/style.ex
@@ -170,69 +170,6 @@ defmodule Styler.Style do
     {directive, updated_meta, children}
   end
 
-  @doc """
-  "Fixes" the line numbers of nodes who have had their orders changed via sorting or other methods.
-  This "fix" simply ensures that comments don't get wrecked as part of us moving AST nodes willy-nilly.
-
-  The fix is rather naive, and simply enforces the following property on the code:
-  A given node must have a line number less than the following node.
-  Et voila! Comments behave much better.
-
-  ## In Detail
-
-  For example, given document
-
-    1: defmodule ...
-    2: alias B
-    3: # this is foo
-    4: def foo ...
-    5: alias A
-
-  Sorting aliases the ast node for  would put `alias A` (line 5) before `alias B` (line 2).
-
-    1: defmodule ...
-    5: alias A
-    2: alias B
-    3: # this is foo
-    4: def foo ...
-
-  Elixir's document algebra would then encounter `line: 5` and immediately dump all comments with `line <= 5`,
-  meaning after running through the formatter we'd end up with
-
-    1: defmodule
-    2: # hi
-    3: # this is foo
-    4: alias A
-    5: alias B
-    6:
-    7: def foo ...
-
-  This function fixes that by seeing that `alias A` has a higher line number than its following sibling `alias B` and so
-  updates `alias A`'s line to be preceding `alias B`'s line.
-
-  Running the results of this function through the formatter now no longer dumps the comments prematurely
-
-    1: defmodule ...
-    2: alias A
-    3: alias B
-    4: # this is foo
-    5: def foo ...
-  """
-  def fix_line_numbers(nodes, nil), do: fix_line_numbers(nodes, 999_999)
-  def fix_line_numbers(nodes, {_, meta, _}), do: fix_line_numbers(nodes, meta[:line])
-  def fix_line_numbers(nodes, max), do: nodes |> Enum.reverse() |> do_fix_lines(max, [])
-
-  defp do_fix_lines([], _, acc), do: acc
-
-  defp do_fix_lines([{_, meta, _} = node | nodes], max, acc) do
-    line = meta[:line]
-
-    # the -2 is just an ugly hack to leave room for one-liner comments and not hijack them.
-    if line > max,
-      do: do_fix_lines(nodes, max, [shift_line(node, max - line - 2) | acc]),
-      else: do_fix_lines(nodes, line, [node | acc])
-  end
-
   def max_line([_ | _] = list), do: list |> List.last() |> max_line()
 
   def max_line(ast) do
@@ -257,4 +194,88 @@ defmodule Styler.Style do
       max_line
     end
   end
+
+  def order_line_meta_and_comments(nodes, comments, first_line) do
+    {nodes, comments, node_comments} = fix_lines(nodes, comments, first_line, [], [])
+    {nodes, Enum.sort_by(comments ++ node_comments, & &1.line)}
+  end
+
+  defp fix_lines([], comments, _, node_acc, c_acc), do: {Enum.reverse(node_acc), comments, c_acc}
+
+  defp fix_lines([{_, meta, _} = node | nodes], comments, start_line, n_acc, c_acc) do
+    line = meta[:line]
+    last_line = meta[:end_of_expression][:line] || max_line(node)
+
+    {node, node_comments, comments} =
+      if start_line == line do
+        {node, [], comments}
+      else
+        {mine, comments} = comments_for_lines(comments, line, last_line)
+        line_with_comments = (List.first(mine)[:line] || line) - (List.first(mine)[:previous_eol_count] || 1) + 1
+
+        if line_with_comments == start_line do
+          {node, mine, comments}
+        else
+          shift = start_line - line_with_comments
+          # fix the node's line
+          node = shift_line(node, shift)
+          # fix the comment's line
+          mine = Enum.map(mine, &%{&1 | line: &1.line + shift})
+          {node, mine, comments}
+        end
+      end
+
+    {_, meta, _} = node
+    # @TODO what about comments that were free floating between blocks? i'm just ignoring them and maybe always will...
+    # kind of just want to shove them to the end though, so that they don't interrupt existing stanzas.
+    # i think that's accomplishable by doing a final call above that finds all comments in the comments list that weren't moved
+    # and which are in the range of start..finish and sets their lines to finish!
+    last_line = meta[:end_of_expression][:line] || max_line(node)
+    last_line = (meta[:end_of_expression][:newlines] || 1) + last_line
+    fix_lines(nodes, comments, last_line, [node | n_acc], node_comments ++ c_acc)
+  end
+
+  @doc """
+  Returns all comments "for" a node, including on the line before it.
+  see `comments_for_lines` for more
+  """
+  def comments_for_node({_, m, _} = node, comments) do
+    last_line = m[:end_of_expression][:line] || max_line(node)
+    comments_for_lines(comments, m[:line], last_line)
+  end
+
+  @doc """
+  Gets all comments in range start_line..last_line, and any comments immediately before start_line.s
+
+    1. code
+    2. # a
+    3. # b
+    4. code # c
+    5. # d
+    6. code
+    7. # e
+
+  here, comments_for_lines(comments, 4, 6) is "a", "b", "c", "d"
+  """
+  def comments_for_lines(comments, start_line, last_line) do
+    comments |> Enum.reverse() |> comments_for_lines(start_line, last_line, [], [])
+  end
+
+  defp comments_for_lines(reversed_comments, start, last, match, acc)
+
+  defp comments_for_lines([], _, _, match, acc), do: {Enum.reverse(match), acc}
+
+  defp comments_for_lines([%{line: line} = comment | rev_comments], start, last, match, acc) do
+    cond do
+      # after our block - no match
+      line > last -> comments_for_lines(rev_comments, start, last, match, [comment | acc])
+      # after start, before last -- it's a match!
+      line >= start -> comments_for_lines(rev_comments, start, last, [comment | match], acc)
+      # this is a comment immediately before start, which means it's modifying this block...
+      # we count that as a match, and look above it to see if it's a multiline comment
+      line == start - 1 -> comments_for_lines(rev_comments, start - 1, last, [comment | match], acc)
+      # comment before start - we've thus iterated through all comments which could be in our range
+      true -> {match, Enum.reverse(rev_comments, [comment | acc])}
+    end
+  end
 end
diff --git a/lib/style/comment_directives.ex b/lib/style/comment_directives.ex
index 7b1ccb7..34e59b8 100644
--- a/lib/style/comment_directives.ex
+++ b/lib/style/comment_directives.ex
@@ -17,14 +17,15 @@ defmodule Styler.Style.CommentDirectives do
 
   @behaviour Styler.Style
 
+  alias Styler.Style
   alias Styler.Zipper
 
   def run(zipper, ctx) do
-    zipper =
+    {zipper, comments} =
       ctx.comments
       |> Enum.filter(&(&1.text == "# styler:sort"))
       |> Enum.map(& &1.line)
-      |> Enum.reduce(zipper, fn line, zipper ->
+      |> Enum.reduce({zipper, ctx.comments}, fn line, {zipper, comments} ->
         found =
           Zipper.find(zipper, fn
             {_, meta, _} -> Keyword.get(meta, :line, -1) >= line
@@ -32,20 +33,30 @@ defmodule Styler.Style.CommentDirectives do
           end)
 
         if found do
-          # @TODO fix line numbers, move comments
-          Zipper.update(found, &sort/1)
+          {node, _} = found
+          {sorted, comments} = sort(node, ctx.comments)
+          {Zipper.replace(found, sorted), comments}
         else
-          zipper
+          {zipper, comments}
         end
       end)
 
-    {:halt, zipper, ctx}
+    {:halt, zipper, %{ctx | comments: comments}}
   end
 
-  defp sort({:__block__, meta, [list]}) when is_list(list), do: {:__block__, meta, [sort(list)]}
-  defp sort(list) when is_list(list), do: Enum.sort_by(list, &Macro.to_string/1)
+  defp sort({:__block__, meta, [list]} = node, comments) when is_list(list) do
+    list = Enum.sort_by(list, &Macro.to_string/1)
+    line = meta[:line]
+    # no need to fix line numbers if it's a single line structure
+    {list, comments} =
+      if line == Style.max_line(node),
+        do: {list, comments},
+        else: Style.order_line_meta_and_comments(list, comments, line)
 
-  defp sort({:sigil_w, sm, [{:<<>>, bm, [string]}, modifiers]}) do
+    {{:__block__, meta, [list]}, comments}
+  end
+
+  defp sort({:sigil_w, sm, [{:<<>>, bm, [string]}, modifiers]}, comments) do
     # ew. gotta be a better way.
     # this keeps indentation for the sigil via joiner, while prepend and append are the bookending whitespace
     {prepend, joiner, append} =
@@ -61,10 +72,18 @@ defmodule Styler.Style.CommentDirectives do
       end
 
     string = string |> String.split() |> Enum.sort() |> Enum.join(joiner)
-    {:sigil_w, sm, [{:<<>>, bm, [prepend, string, append]}, modifiers]}
+    {{:sigil_w, sm, [{:<<>>, bm, [prepend, string, append]}, modifiers]}, comments}
+  end
+
+  defp sort({:=, m, [lhs, rhs]}, comments) do
+    {rhs, comments} = sort(rhs, comments)
+    {{:=, m, [lhs, rhs]}, comments}
+  end
+
+  defp sort({:@, m, [{a, am, [assignment]}]}, comments) do
+    {assignment, comments} = sort(assignment, comments)
+    {{:@, m, [{a, am, [assignment]}]}, comments}
   end
 
-  defp sort({:=, m, [lhs, rhs]}), do: {:=, m, [lhs, sort(rhs)]}
-  defp sort({:@, m, [{a, am, [assignment]}]}), do: {:@, m, [{a, am, [sort(assignment)]}]}
-  defp sort(x), do: x
+  defp sort(x, comments), do: {x, comments}
 end
diff --git a/lib/style/configs.ex b/lib/style/configs.ex
index ab110a0..75a0d75 100644
--- a/lib/style/configs.ex
+++ b/lib/style/configs.ex
@@ -80,19 +80,14 @@ defmodule Styler.Style.Configs do
       |> Style.reset_newlines()
       |> Enum.concat(configs)
 
-    # `set_lines` performs better than `fix_line_numbers` for a large number of nodes moving, as it moves their comments with them
-    # however, it will also move any comments not associated with a node, causing wildly unpredictable sad times!
-    # so i'm trying to guess which change will be less damaging.
-    # moving >=3 nodes hints that this is an initial run, where `set_lines` definitely outperforms.
     {nodes, comments} =
       if changed?(nodes) do
         # after running, this block should take up the same # of lines that it did before
         # the first node of `rest` is greater than the highest line in configs, assignments
         # config line is the first line to be used as part of this block
-        # that will change when we consider preceding comments
-        {node_comments, _} = comments_for_node(config, comments)
+        {node_comments, _} = Style.comments_for_node(config, comments)
         first_line = min(List.last(node_comments)[:line] || cfm[:line], cfm[:line])
-        set_lines(nodes, comments, first_line)
+        Style.order_line_meta_and_comments(nodes, comments, first_line)
       else
         {nodes, comments}
       end
@@ -121,73 +116,6 @@ defmodule Styler.Style.Configs do
 
   defp changed?(_), do: false
 
-  defp set_lines(nodes, comments, first_line) do
-    {nodes, comments, node_comments} = set_lines(nodes, comments, first_line, [], [])
-    # @TODO if there are dangling comments between the nodes min/max, push them somewhere?
-    # likewise deal with conflicting line comments?
-    {nodes, Enum.sort_by(comments ++ node_comments, & &1.line)}
-  end
-
-  def set_lines([], comments, _, node_acc, c_acc), do: {Enum.reverse(node_acc), comments, c_acc}
-
-  def set_lines([{_, meta, _} = node | nodes], comments, start_line, n_acc, c_acc) do
-    line = meta[:line]
-    last_line = meta[:end_of_expression][:line] || Style.max_line(node)
-
-    {node, node_comments, comments} =
-      if start_line == line do
-        {node, [], comments}
-      else
-        {mine, comments} = comments_for_lines(comments, line, last_line)
-        line_with_comments = (List.first(mine)[:line] || line) - (List.first(mine)[:previous_eol_count] || 1) + 1
-
-        if line_with_comments == start_line do
-          {node, mine, comments}
-        else
-          shift = start_line - line_with_comments
-          node = Style.shift_line(node, shift)
-
-          mine = Enum.map(mine, &%{&1 | line: &1.line + shift})
-          {node, mine, comments}
-        end
-      end
-
-    {_, meta, _} = node
-    # @TODO what about comments that were free floating between blocks? i'm just ignoring them and maybe always will...
-    # kind of just want to shove them to the end though, so that they don't interrupt existing stanzas.
-    # i think that's accomplishable by doing a final call above that finds all comments in the comments list that weren't moved
-    # and which are in the range of start..finish and sets their lines to finish!
-    last_line = meta[:end_of_expression][:line] || Style.max_line(node)
-    last_line = (meta[:end_of_expression][:newlines] || 1) + last_line
-    set_lines(nodes, comments, last_line, [node | n_acc], node_comments ++ c_acc)
-  end
-
-  defp comments_for_node({_, m, _} = node, comments) do
-    last_line = m[:end_of_expression][:line] || Style.max_line(node)
-    comments_for_lines(comments, m[:line], last_line)
-  end
-
-  defp comments_for_lines(comments, start_line, last_line) do
-    comments
-    |> Enum.reverse()
-    |> comments_for_lines(start_line, last_line, [], [])
-  end
-
-  defp comments_for_lines(reversed_comments, start, last, match, acc)
-
-  defp comments_for_lines([], _, _, match, acc), do: {Enum.reverse(match), acc}
-
-  defp comments_for_lines([%{line: line} = comment | rev_comments], start, last, match, acc) do
-    cond do
-      line > last -> comments_for_lines(rev_comments, start, last, match, [comment | acc])
-      line >= start -> comments_for_lines(rev_comments, start, last, [comment | match], acc)
-      # @TODO bug: match line looks like `x = :foo # comment for x`
-      # could account for that by pre-running the formatter on config files :/
-      line == start - 1 -> comments_for_lines(rev_comments, start - 1, last, [comment | match], acc)
-      true -> {match, Enum.reverse(rev_comments, [comment | acc])}
-    end
-  end
-
   defp accumulate([{:config, _, [_, _ | _]} = c | siblings], cs, as), do: accumulate(siblings, [c | cs], as)
   defp accumulate([{:=, _, [_lhs, _rhs]} = a | siblings], cs, as), do: accumulate(siblings, cs, [a | as])
   defp accumulate(rest, configs, assignments), do: {configs, assignments, rest}
diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex
index 623ce5f..eb10761 100644
--- a/lib/style/module_directives.ex
+++ b/lib/style/module_directives.ex
@@ -263,7 +263,7 @@ defmodule Styler.Style.ModuleDirectives do
         acc.require
       ]
       |> Stream.concat()
-      |> Style.fix_line_numbers(List.first(nondirectives))
+      |> 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
     if Enum.empty?(directives) do
@@ -419,4 +419,66 @@ defmodule Styler.Style.ModuleDirectives do
     |> Enum.map(&elem(&1, 0))
     |> Style.reset_newlines()
   end
+
+  # TODO investigate removing this in favor of the Style.post_sort_cleanup(node, comments)
+  # "Fixes" the line numbers of nodes who have had their orders changed via sorting or other methods.
+  # This "fix" simply ensures that comments don't get wrecked as part of us moving AST nodes willy-nilly.
+  #
+  # The fix is rather naive, and simply enforces the following property on the code:
+  # A given node must have a line number less than the following node.
+  # Et voila! Comments behave much better.
+  #
+  # ## In Detail
+  #
+  # For example, given document
+  #
+  #   1: defmodule ...
+  #   2: alias B
+  #   3: # this is foo
+  #   4: def foo ...
+  #   5: alias A
+  #
+  # Sorting aliases the ast node for  would put `alias A` (line 5) before `alias B` (line 2).
+  #
+  #   1: defmodule ...
+  #   5: alias A
+  #   2: alias B
+  #   3: # this is foo
+  #   4: def foo ...
+  #
+  # Elixir's document algebra would then encounter `line: 5` and immediately dump all comments with `line <= 5`,
+  # meaning after running through the formatter we'd end up with
+  #
+  #   1: defmodule
+  #   2: # hi
+  #   3: # this is foo
+  #   4: alias A
+  #   5: alias B
+  #   6:
+  #   7: def foo ...
+  #
+  # This function fixes that by seeing that `alias A` has a higher line number than its following sibling `alias B` and so
+  # updates `alias A`'s line to be preceding `alias B`'s line.
+  #
+  # Running the results of this function through the formatter now no longer dumps the comments prematurely
+  #
+  #   1: defmodule ...
+  #   2: alias A
+  #   3: alias B
+  #   4: # this is foo
+  #   5: def foo ...
+  defp fix_line_numbers(nodes, nil), do: fix_line_numbers(nodes, 999_999)
+  defp fix_line_numbers(nodes, {_, meta, _}), do: fix_line_numbers(nodes, meta[:line])
+  defp fix_line_numbers(nodes, max), do: nodes |> Enum.reverse() |> do_fix_lines(max, [])
+
+  defp do_fix_lines([], _, acc), do: acc
+
+  defp do_fix_lines([{_, meta, _} = node | nodes], max, acc) do
+    line = meta[:line]
+
+    # the -2 is just an ugly hack to leave room for one-liner comments and not hijack them.
+    if line > max,
+      do: do_fix_lines(nodes, max, [Style.shift_line(node, max - line - 2) | acc]),
+      else: do_fix_lines(nodes, line, [node | acc])
+  end
 end


Test patch:
diff --git a/test/style/comment_directives_test.exs b/test/style/comment_directives_test.exs
index 487be6c..def6af5 100644
--- a/test/style/comment_directives_test.exs
+++ b/test/style/comment_directives_test.exs
@@ -182,5 +182,56 @@ defmodule Styler.Style.CommentDirectivesTest do
         """
       )
     end
+
+    test "treats comments nicely" do
+      assert_style(
+        """
+        # pre-amble comment
+        # styler:sort
+        [
+          {:phoenix, "~> 1.7"},
+          # hackney comment
+          {:hackney, "1.18.1", override: true},
+          {:styler, "~> 1.2", only: [:dev, :test], runtime: false},
+          {:credo, "~> 1.7", only: [:dev, :test], runtime: false},
+          {:excellent_migrations, "~> 0.1", only: [:dev, :test], runtime: false},
+          # ecto
+          {:ecto, "~> 3.12"},
+          {:ecto_sql, "~> 3.12"},
+          # genstage comment 1
+          # genstage comment 2
+          {:gen_stage, "~> 1.0", override: true},
+          # telemetry
+          {:telemetry, "~> 1.0", override: true},
+          # dangling comment
+        ]
+
+        # some other comment
+        """,
+        """
+        # pre-amble comment
+        # styler:sort
+        [
+          {:credo, "~> 1.7", only: [:dev, :test], runtime: false},
+          # ecto
+          {:ecto, "~> 3.12"},
+          {:ecto_sql, "~> 3.12"},
+          {:excellent_migrations, "~> 0.1", only: [:dev, :test], runtime: false},
+          # genstage comment 1
+          # genstage comment 2
+          {:gen_stage, "~> 1.0", override: true},
+          # hackney comment
+          {:hackney, "1.18.1", override: true},
+          {:phoenix, "~> 1.7"},
+          {:styler, "~> 1.2", only: [:dev, :test], runtime: false},
+          # telemetry
+          {:telemetry, "~> 1.0", override: true}
+          # dangling comment
+        ]
+
+        # some other comment
+        """
+      )
+    end
   end
 end
diff --git a/test/style_test.exs b/test/style_test.exs
index 0f7f678..43bc8b2 100644
--- a/test/style_test.exs
+++ b/test/style_test.exs
@@ -3,8 +3,6 @@ defmodule Styler.StyleTest do
 
   import Styler.Style, only: [displace_comments: 2, shift_comments: 3]
 
-  alias Styler.Style
-
   @code """
   # Above module
   defmodule Foo do
@@ -117,16 +115,4 @@ defmodule Styler.StyleTest do
       end
     end
   end
-
-  describe "fix_line_numbers" do
-    test "returns ast list with increasing line numbers" do
-      nodes = for n <- [1, 2, 999, 1000, 5, 6], do: {:node, [line: n], [n]}
-      fixed = Style.fix_line_numbers(nodes, 7)
-
-      Enum.scan(fixed, fn {_, [line: this_line], _} = this_node, {_, [line: previous_line], _} ->
-        assert this_line >= previous_line
-        this_node
-      end)
-    end
-  end
 end