diff --git a/lib/style.ex b/lib/style.ex index d58ad1cb..80c50b23 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 7b1ccb70..34e59b84 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 ab110a0b..75a0d750 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 623ce5f0..eb10761e 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 diff --git a/test/style/comment_directives_test.exs b/test/style/comment_directives_test.exs index 487be6c1..def6af55 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 0f7f6785..43bc8b27 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