From 8b90c3614776be287d1d0ca3575caa6f0d3d1dd5 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Fri, 19 Jun 2026 17:32:10 -0700 Subject: [PATCH 01/16] first step --- .gitignore | 1 + lib/style/configs.ex | 34 +++--- lib/style/pipes.ex | 8 +- test/style/blocks_test.exs | 29 +++++ test/style/comment_directives_test.exs | 28 +++++ test/style/configs_test.exs | 144 +++++++++++++++++++++++++ test/style/defs_test.exs | 1 + test/style/module_directives_test.exs | 19 ++++ test/style/pipes_test.exs | 31 ++++++ 9 files changed, 278 insertions(+), 17 deletions(-) diff --git a/.gitignore b/.gitignore index d0b2773e..742eff19 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,4 @@ formatter-*.tar # Temporary files, for example, from tests. /tmp/ +.tokensave diff --git a/lib/style/configs.ex b/lib/style/configs.ex index d83b4474..62d4085f 100644 --- a/lib/style/configs.ex +++ b/lib/style/configs.ex @@ -50,16 +50,6 @@ defmodule Styler.Style.Configs do def run({{:config, cfm, [_, _ | _]} = config, zm}, %{mix_config?: true, comments: comments} = ctx) do # all of these list are reversed due to the reduce {configs, assignments, rest} = accumulate(zm.r, [], []) - # @TODO - # okay so comments between nodes that we moved....... - # lets just push them out of the way (???). so - # 1. figure out first/last possible lines we're talking about here - # 2. only pass comments in that range off - # 3. split those comments into "moved, didn't move" - # 4. for any "didn't move" comments... move them to the top? - # - # also, should i just do a scan of the configs ++ assignments, and see if any of them have lines out of order, - # and decide from there whether or not i want to do set_lines configs = [config | configs] @@ -80,16 +70,30 @@ defmodule Styler.Style.Configs do |> Style.reset_newlines() |> Enum.concat(configs) - {nodes, comments} = + {nodes, comments, rest} = 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 {node_comments, _} = Style.comments_for_node(config, comments) first_line = min(List.first(node_comments)[:line] || cfm[:line], cfm[:line]) - Style.order_line_meta_and_comments(nodes, comments, first_line) + + # Sorting and re-spacing can make the block taller (config groups gain blank lines between them). + # `order_line_meta_and_comments` only moves the sorted nodes and their comments, so `rest` nodes and any + # trailing comments keep their original lines - if the block grows past them, the formatter pulls those + # comments up into our configs. Measure the growth and shift the trailing region down to match. + # `configs`/`assignments` are reverse-ordered (`accumulate` prepends), so we can't just take the last node. + block_end = [config | configs ++ assignments] |> Enum.map(&Style.max_line/1) |> Enum.max() + {block_comments, tail_comments} = Enum.split_with(comments, &(&1.line <= block_end)) + + {nodes, block_comments} = Style.order_line_meta_and_comments(nodes, block_comments, first_line) + + # order_line_meta_and_comments lays nodes out with increasing line numbers, so the block now ends at the last + delta = Style.max_line(nodes) - block_end + tail_comments = Enum.map(tail_comments, &%{&1 | line: &1.line + delta}) + rest = Enum.map(rest, &Style.shift_line(&1, delta)) + + {nodes, Enum.sort_by(block_comments ++ tail_comments, & &1.line), rest} else - {nodes, comments} + {nodes, comments, rest} end [config | left_siblings] = Enum.reverse(nodes, zm.l) diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index 9aceca19..c4915f8d 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -110,17 +110,21 @@ defmodule Styler.Style.Pipes do # 3 |> rhs(...args) # => # 1 var = rhs(lhs, ...args) + # everything collapses onto the `=` line, so hoist any interleaved comments above it + comments = Style.displace_comments(ctx.comments, vm[:line]..Style.max_line(rhs)) oneline_assignment = Style.set_line({:=, am, [var, {fun, rhs_meta, [lhs | args]}]}, vm[:line]) # skip so we don't re-traverse - {:cont, Zipper.replace(assignment_parent, oneline_assignment), ctx} + {:cont, Zipper.replace(assignment_parent, oneline_assignment), %{ctx | comments: comments}} _ -> # lhs # |> rhs(...args) # => # rhs(lhs, ...) + # everything collapses onto lhs_line, so hoist any interleaved comments above the call + comments = Style.displace_comments(ctx.comments, lhs_line..Style.max_line(rhs)) oneline_function_call = Style.set_line({fun, rhs_meta, [lhs | args]}, lhs_line) - {:cont, Zipper.replace(single_pipe_zipper, oneline_function_call), ctx} + {:cont, Zipper.replace(single_pipe_zipper, oneline_function_call), %{ctx | comments: comments}} end end end diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 1c0240af..9e8d721b 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -1284,4 +1284,33 @@ defmodule Styler.Style.BlocksTest do ) end end + + describe "comment movement regressions" do + test "comments survive swap when do body is multiple lines (negated if)" do + assert_style( + """ + if !a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # c + c + else + # b1 + b1 + # b2 + b2 + end + """ + ) + end + end end diff --git a/test/style/comment_directives_test.exs b/test/style/comment_directives_test.exs index 80cb4348..f55160f6 100644 --- a/test/style/comment_directives_test.exs +++ b/test/style/comment_directives_test.exs @@ -450,4 +450,32 @@ defmodule Styler.Style.CommentDirectivesTest do ) end end + + describe "comment movement regressions" do + test "interior orphan comment moves to the wrong item" do + assert_style( + """ + # styler:sort + [ + :d, + + # this comment describes apple + + :apple, + + :c + ] + """, + """ + # styler:sort + [ + # this comment describes apple + :apple, + :c, + :d + ] + """ + ) + end + end end diff --git a/test/style/configs_test.exs b/test/style/configs_test.exs index 05704acc..b439a7a7 100644 --- a/test/style/configs_test.exs +++ b/test/style/configs_test.exs @@ -430,4 +430,148 @@ defmodule Styler.Style.ConfigsTest do ) end end + + test "issue #244: phx.new config.exs comments survive sorting" do + assert_style( + """ + # This file is responsible for configuring your application + # and its dependencies with the aid of the Config module. + # + # This configuration file is loaded before any dependency and + # is restricted to this project. + + # General application configuration + import Config + + config :phx_new, + ecto_repos: [PhxNew.Repo], + generators: [timestamp_type: :utc_datetime] + + # Configures the endpoint + config :phx_new, PhxNewWeb.Endpoint, + url: [host: "localhost"], + adapter: Bandit.PhoenixAdapter, + pubsub_server: PhxNew.PubSub, + live_view: [signing_salt: "QdK6mkm/"] + + # Configures the mailer + # + # By default it uses the "Local" adapter which stores the emails + # locally. You can see the emails in your browser, at "/dev/mailbox". + # + # For production it's recommended to configure a different adapter + # at the `config/runtime.exs`. + config :phx_new, PhxNew.Mailer, adapter: Swoosh.Adapters.Local + + # Configure esbuild (the version is required) + config :esbuild, version: "0.25.4" + + # Configures Elixir's Logger + config :logger, :default_formatter, + format: "$time $metadata[$level] $message\\n", + metadata: [:request_id] + + # Use Jason for JSON parsing in Phoenix + config :phoenix, :json_library, Jason + + # Import environment specific config. This must remain at the bottom + # of this file so it overrides the configuration defined above. + import_config "\#{config_env()}.exs" + """, + """ + # This file is responsible for configuring your application + # and its dependencies with the aid of the Config module. + # + # This configuration file is loaded before any dependency and + # is restricted to this project. + + # General application configuration + import Config + + # Configure esbuild (the version is required) + config :esbuild, version: "0.25.4" + + # Configures Elixir's Logger + config :logger, :default_formatter, + format: "$time $metadata[$level] $message\\n", + metadata: [:request_id] + + # Use Jason for JSON parsing in Phoenix + config :phoenix, :json_library, Jason + + # Configures the mailer + # + # By default it uses the "Local" adapter which stores the emails + # locally. You can see the emails in your browser, at "/dev/mailbox". + # + # For production it's recommended to configure a different adapter + # at the `config/runtime.exs`. + config :phx_new, PhxNew.Mailer, adapter: Swoosh.Adapters.Local + + # Configures the endpoint + config :phx_new, PhxNewWeb.Endpoint, + url: [host: "localhost"], + adapter: Bandit.PhoenixAdapter, + pubsub_server: PhxNew.PubSub, + live_view: [signing_salt: "QdK6mkm/"] + + config :phx_new, + ecto_repos: [PhxNew.Repo], + generators: [timestamp_type: :utc_datetime] + + # Import environment specific config. This must remain at the bottom + # of this file so it overrides the configuration defined above. + import_config "\#{config_env()}.exs" + """ + ) + end + + test "issue #244: phx.new prod.exs trailing/dangling comments survive sorting" do + assert_style( + """ + import Config + + # Note we also include the path to a cache manifest + # containing the digested version of static files. This + # manifest is generated by the `mix assets.deploy` task, + # which you should run after static files are built and + # before starting your production server. + config :phx_new, PhxNewWeb.Endpoint, cache_static_manifest: "priv/static/cache_manifest.json" + + # Configures Swoosh API Client + config :swoosh, api_client: Swoosh.ApiClient.Req + + # Disable Swoosh Local Memory Storage + config :swoosh, local: false + + # Do not print debug messages in production + config :logger, level: :info + + # Runtime production configuration, including reading + # of environment variables, is done on config/runtime.exs. + """, + """ + import Config + + # Do not print debug messages in production + config :logger, level: :info + + # Note we also include the path to a cache manifest + # containing the digested version of static files. This + # manifest is generated by the `mix assets.deploy` task, + # which you should run after static files are built and + # before starting your production server. + config :phx_new, PhxNewWeb.Endpoint, cache_static_manifest: "priv/static/cache_manifest.json" + + # Configures Swoosh API Client + config :swoosh, api_client: Swoosh.ApiClient.Req + + # Disable Swoosh Local Memory Storage + config :swoosh, local: false + + # Runtime production configuration, including reading + # of environment variables, is done on config/runtime.exs. + """ + ) + end end diff --git a/test/style/defs_test.exs b/test/style/defs_test.exs index f7a9fab7..2981c77f 100644 --- a/test/style/defs_test.exs +++ b/test/style/defs_test.exs @@ -245,4 +245,5 @@ defmodule Styler.Style.DefsTest do assert_raise SyntaxError, fn -> assert_style("def foo(a) true") end end end + end diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs index 5fe9a262..c4f4d343 100644 --- a/test/style/module_directives_test.exs +++ b/test/style/module_directives_test.exs @@ -729,4 +729,23 @@ defmodule Styler.Style.ModuleDirectivesTest do ) end end + + describe "comment movement regressions" do + test "comment on hoisted import is stranded when an attribute is reordered above it" do + assert_style( + """ + @endpoint Foo + + # this comment belongs to the import + import Plug.Conn + """, + """ + # this comment belongs to the import + import Plug.Conn + + @endpoint Foo + """ + ) + end + end end diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 570f5b91..f515bb66 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -1058,4 +1058,35 @@ defmodule Styler.Style.PipesTest do end end end + + describe "comment movement regressions" do + test "unpiping a single pipe drops an interleaved comment below the expression" do + assert_style( + """ + foo + # comment + |> bar() + """, + """ + # comment + bar(foo) + """ + ) + end + + test "unpiping a single pipe into an assignment hoists an interleaved comment above it" do + assert_style( + """ + x = + foo + # comment + |> bar() + """, + """ + # comment + x = bar(foo) + """ + ) + end + end end From 38439ecf8b467e5cba74a8335d3408f67d213d9f Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Fri, 19 Jun 2026 17:48:30 -0700 Subject: [PATCH 02/16] update module_directives to use Style for its comment handling --- lib/style/module_directives.ex | 127 +++++++----------- test/style/defs_test.exs | 1 - .../module_directives/alias_lifting_test.exs | 1 + test/style/module_directives_test.exs | 7 +- 4 files changed, 50 insertions(+), 86 deletions(-) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index fac6189a..72222e6d 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -121,7 +121,8 @@ defmodule Styler.Style.ModuleDirectives do # we want only-child literal block to be handled in the only-child catch-all. it means someone did a weird # (that would be a literal, so best case someone wrote a string and forgot to put `@moduledoc` before it) {:__block__, _, [_, _ | _]} -> - {:skip, organize_directives(body_zipper, moduledoc), ctx} + {zipper, comments} = organize_directives(body_zipper, ctx.comments, moduledoc) + {:skip, zipper, %{ctx | comments: comments}} # a module whose only child is a moduledoc. nothing to do here! # seems weird at first blush but lots of projects/libraries do this with their root namespace module @@ -131,12 +132,12 @@ defmodule Styler.Style.ModuleDirectives do # There's only one child, and it's not a moduledoc. Conditionally add a moduledoc, then style the only_child only_child -> if moduledoc do - zipper = + {zipper, comments} = body_zipper |> Zipper.replace({:__block__, [], [moduledoc, only_child]}) - |> organize_directives() + |> organize_directives(ctx.comments) - {:skip, zipper, ctx} + {:skip, zipper, %{ctx | comments: comments}} else do_run(body_zipper, ctx) end @@ -148,9 +149,13 @@ defmodule Styler.Style.ModuleDirectives do defp do_run({{directive, _, children}, _} = zipper, ctx) when directive in @directives and is_list(children) do # Need to be careful that we aren't getting false positives on variables or fns like `def import(foo)` or `alias = 1` case Style.ensure_block_parent(zipper) do - {:ok, zipper} -> {:skip, zipper |> Zipper.up() |> organize_directives(), ctx} + {:ok, zipper} -> + {zipper, comments} = zipper |> Zipper.up() |> organize_directives(ctx.comments) + {:skip, zipper, %{ctx | comments: comments}} + # not actually a directive! carry on. - :error -> {:cont, zipper, ctx} + :error -> + {:cont, zipper, ctx} end end @@ -229,10 +234,11 @@ defmodule Styler.Style.ModuleDirectives do end end - defp organize_directives(parent, moduledoc \\ nil) do + defp organize_directives(parent, comments, moduledoc \\ nil) do + original_children = Zipper.children(parent) + acc = - parent - |> Zipper.children() + original_children |> Enum.reduce(@env, fn {:@, _, [{attr_directive, _, _}]} = ast, acc when attr_directive in @attr_directives -> # attr_directives are moved above aliases, so we need to expand them @@ -281,19 +287,39 @@ defmodule Styler.Style.ModuleDirectives do acc.require ] |> Stream.concat() - |> fix_line_numbers(List.first(nondirectives)) + |> Enum.to_list() + + nodes = directives ++ nondirectives + + # If we actually changed the ordering (sorted directives, hoisted them above nondirectives, expanded/added/removed + # one), re-lay the whole block via `order_line_meta_and_comments`. It moves each node *and the comments attached to + # it* together, so a directive's comment follows it when it changes position. When nothing moved (e.g. a config file + # whose `import Config` is already on top) we leave every line alone - other styles like Configs rely on that, and + # reflowing would only risk disturbing comments we have no reason to touch. + {nodes, comments} = + if nodes != [] and Style.without_meta(nodes) != Style.without_meta(original_children) do + first_line = nodes |> Enum.map(&Style.meta(&1)[:line]) |> Enum.min() + Style.order_line_meta_and_comments(nodes, comments, first_line) + else + {nodes, comments} + end + + {directives, nondirectives} = Enum.split(nodes, length(directives)) # 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 - Zipper.replace_children(parent, nondirectives) - else - # this ensures we continue the traversal _after_ any directives - parent - |> Zipper.replace_children(directives) - |> Zipper.down() - |> Zipper.rightmost() - |> Zipper.insert_siblings(nondirectives) - end + zipper = + if directives == [] do + Zipper.replace_children(parent, nondirectives) + else + # this ensures we continue the traversal _after_ any directives + parent + |> Zipper.replace_children(directives) + |> Zipper.down() + |> Zipper.rightmost() + |> Zipper.insert_siblings(nondirectives) + end + + {zipper, comments} end # alias_env have to be recomputed after we've sorted our `alias` nodes @@ -548,65 +574,4 @@ defmodule Styler.Style.ModuleDirectives do |> Enum.map(&elem(&1, 0)) |> Style.reset_newlines() end - - # "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/defs_test.exs b/test/style/defs_test.exs index 2981c77f..f7a9fab7 100644 --- a/test/style/defs_test.exs +++ b/test/style/defs_test.exs @@ -245,5 +245,4 @@ defmodule Styler.Style.DefsTest do assert_raise SyntaxError, fn -> assert_style("def foo(a) true") end end end - end diff --git a/test/style/module_directives/alias_lifting_test.exs b/test/style/module_directives/alias_lifting_test.exs index 721228b9..cc87660b 100644 --- a/test/style/module_directives/alias_lifting_test.exs +++ b/test/style/module_directives/alias_lifting_test.exs @@ -402,6 +402,7 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do """, """ alias A.B.C + # Foo is my fave require Foo diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs index c4f4d343..9381fec7 100644 --- a/test/style/module_directives_test.exs +++ b/test/style/module_directives_test.exs @@ -426,7 +426,7 @@ defmodule Styler.Style.ModuleDirectivesTest do end describe "with comments..." do - test "moving aliases up through non-directives doesn't move comments up" do + test "moving aliases up through non-directives moves their comments with them" do assert_style( """ defmodule Foo do @@ -450,9 +450,11 @@ defmodule Styler.Style.ModuleDirectivesTest do defmodule Foo do # mdf @moduledoc false + # A alias A.A # B alias B.B + # C alias C.C # foo @@ -460,9 +462,6 @@ defmodule Styler.Style.ModuleDirectivesTest do # ok :ok end - - # C - # A end """ ) From 100f17c946f386357209657f3ec8a1c174b00417 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Wed, 15 Jul 2026 08:10:56 -0700 Subject: [PATCH 03/16] tests --- test/style/blocks_test.exs | 24 ++++++++++++++++++++++ test/style/comment_directives_test.exs | 28 -------------------------- 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 9e8d721b..8f0bc8ef 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -314,6 +314,30 @@ defmodule Styler.Style.BlocksTest do ) end + test "block swapping comments - comments modify the bodies" do + assert_style( + """ + case foo do + false -> + # a + :error + true -> + # b + :ok + end + """, + """ + if foo do + # b + :ok + else + # a + :error + end + """ + ) + end + test "complex comments" do assert_style( """ diff --git a/test/style/comment_directives_test.exs b/test/style/comment_directives_test.exs index f55160f6..80cb4348 100644 --- a/test/style/comment_directives_test.exs +++ b/test/style/comment_directives_test.exs @@ -450,32 +450,4 @@ defmodule Styler.Style.CommentDirectivesTest do ) end end - - describe "comment movement regressions" do - test "interior orphan comment moves to the wrong item" do - assert_style( - """ - # styler:sort - [ - :d, - - # this comment describes apple - - :apple, - - :c - ] - """, - """ - # styler:sort - [ - # this comment describes apple - :apple, - :c, - :d - ] - """ - ) - end - end end From 5b31bd5dd3defefbae38c12c928402da314e1789 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Wed, 15 Jul 2026 10:31:58 -0700 Subject: [PATCH 04/16] if_ast bugs --- lib/style/blocks.ex | 25 ++- test/style/blocks_test.exs | 347 +++++++++++++++++++++++++++++++++++++ 2 files changed, 364 insertions(+), 8 deletions(-) diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index 216930ba..8d6cea16 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -204,7 +204,7 @@ defmodule Styler.Style.Blocks do if Style.max_line(do_) > Style.max_line(else_) do # we inverted the if/else blocks of this `if` statement in a previous pass (due to negators or unless) # shift comments etc to make it happy now - if_ast(zipper, head, do_, else_, ctx) + if_ast(zipper, head, do_, else_, ctx, false) else {:cont, zipper, ctx} end @@ -369,26 +369,32 @@ defmodule Styler.Style.Blocks do defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx) do do_ = {{:__block__, [line: nil], [:do]}, do_body} else_ = {{:__block__, [line: nil], [:else]}, else_body} - if_ast(zipper, head, do_, else_, ctx) + if_ast(zipper, head, do_, else_, ctx, true) end - defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx) do + defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx, conversion?) do line = meta[:line] + # `conversion?: true` means we're converting a non-if construct (case/cond) into an if for the first + # time - no `do`/`else` keyword line has ever been spent, so nothing needs to be reclaimed. + # `conversion?: false` means the node is already an if whose do/else content is sitting in the + # "wrong" slot (eg negated-if swapping do/else bodies in place) - the *other* keyword's line is + # being vacated and needs to be accounted for. + # ... why am i doing this again? hmm. do_body = Macro.update_meta(do_body, &Keyword.delete(&1, :end_of_expression)) else_body = Macro.update_meta(else_body, &Keyword.delete(&1, :end_of_expression)) max_do_line = Style.max_line(do_body) max_else_line = Style.max_line(else_body) - end_line = max(max_do_line, max_else_line) # Change ast meta and comment lines to fit the `if` ast - {do_, else_, comments} = + {do_, else_, comments, end_line} = if max_do_line >= max_else_line do # we're swapping the ordering of two blocks of code # and so must swap the lines of the ast & comments to keep comments where they belong! # the math is: move B up by the length of A, and move A down by the length of B plus one (for the else keyword) else_size = max_else_line - line + else_size = if conversion?, do: else_size, else: else_size + 1 do_size = max_do_line - max_else_line shifts = [ @@ -399,13 +405,16 @@ defmodule Styler.Style.Blocks do ] do_ = {Style.set_line(do_kw, line), Style.shift_line(do_body, -else_size)} - else_ = {Style.set_line(else_kw, max_else_line), Style.shift_line(else_body, do_size)} - {do_, else_, Style.shift_comments(ctx.comments, shifts)} + else_line = if conversion?, do: max_else_line, else: line + do_size + else_ = {Style.set_line(else_kw, else_line), Style.shift_line(else_body, do_size)} + end_line = if conversion?, do: max(max_do_line, max_else_line), else: max_do_line + 1 + {do_, else_, Style.shift_comments(ctx.comments, shifts), end_line} else # much simpler case -- just scootch things in the else down by 1 for the `else` keyword. do_ = {{:__block__, [line: line], [:do]}, do_body} else_ = Style.shift_line({{:__block__, [line: max_do_line], [:else]}, else_body}, 1) - {do_, else_, Style.shift_comments(ctx.comments, max_do_line..max_else_line, 1)} + end_line = max(max_do_line, max_else_line) + {do_, else_, Style.shift_comments(ctx.comments, max_do_line..max_else_line, 1), end_line} end zipper diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 8f0bc8ef..48d65656 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -1337,4 +1337,351 @@ defmodule Styler.Style.BlocksTest do ) end end + + describe "block swap comment coverage" do + test "unless with multi-line do/else bodies" do + assert_style( + """ + unless a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # c + c + else + # b1 + b1 + # b2 + b2 + end + """ + ) + end + + test "non-! negator (!=) with multi-line do/else bodies" do + assert_style( + """ + if a != b do + # d1 + d1 + # d2 + d2 + else + # e + e + end + """, + """ + if a == b do + # e + e + else + # d1 + d1 + # d2 + d2 + end + """ + ) + end + + test "3-statement do body swaps below a 1-statement else" do + assert_style( + """ + if !a do + # x1 + x1 + # x2 + x2 + # x3 + x3 + else + # y + y + end + """, + """ + if a do + # y + y + else + # x1 + x1 + # x2 + x2 + # x3 + x3 + end + """ + ) + end + + test "1-statement do body swaps above a 3-statement else" do + assert_style( + """ + if !a do + # p + p + else + # q1 + q1 + # q2 + q2 + # q3 + q3 + end + """, + """ + if a do + # q1 + q1 + # q2 + q2 + # q3 + q3 + else + # p + p + end + """ + ) + end + + test "equal-size (2 statement) do/else bodies swap" do + assert_style( + """ + if !a do + # m1 + m1 + # m2 + m2 + else + # n1 + n1 + # n2 + n2 + end + """, + """ + if a do + # n1 + n1 + # n2 + n2 + else + # m1 + m1 + # m2 + m2 + end + """ + ) + end + + test "dangling comment at the end of a swapped do body" do + assert_style( + """ + if !a do + # b + b + # dangling + else + # c + c + end + """, + """ + if a do + # c + c + else + # b + b + # dangling + end + """ + ) + end + + test "blank line between statements survives the swap" do + assert_style( + """ + if !a do + # b1 + b1 + + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # c + c + else + # b1 + b1 + + # b2 + b2 + end + """ + ) + end + + test "leading comment on the if itself is untouched by the swap" do + assert_style( + """ + # leading comment + if !a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """, + """ + # leading comment + if a do + # c + c + else + # b1 + b1 + # b2 + b2 + end + """ + ) + end + + test "double negator collapse leaves multi-line do/else bodies untouched" do + assert_style( + """ + if !!a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """ + ) + end + + test "case to if with multi-line do/else bodies (false first)" do + assert_style( + """ + case foo do + false -> + # d1 + d1 + # d2 + d2 + true -> + # e + e + end + """, + """ + if foo do + # e + e + else + # d1 + d1 + # d2 + d2 + end + """ + ) + end + + test "case to if with equal-size (2 statement) do/else bodies (false first)" do + assert_style( + """ + case foo do + false -> + # d1 + d1 + # d2 + d2 + true -> + # e1 + e1 + # e2 + e2 + end + """, + """ + if foo do + # e1 + e1 + # e2 + e2 + else + # d1 + d1 + # d2 + d2 + end + """ + ) + end + + test "cond to if with multi-line do body and 1-statement else" do + assert_style( + """ + cond do + a -> + # f1 + f1 + # f2 + f2 + true -> + # g + g + end + """, + """ + if a do + # f1 + f1 + # f2 + f2 + else + # g + g + end + """ + ) + end + end end From e4827e238808c9e5850d1fb0d05b7d42b170aad5 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Wed, 15 Jul 2026 10:53:42 -0700 Subject: [PATCH 05/16] use style! --- lib/style/blocks.ex | 60 ++++++++++++++------------------------------- 1 file changed, 18 insertions(+), 42 deletions(-) diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index 8d6cea16..5e5043a1 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -204,7 +204,7 @@ defmodule Styler.Style.Blocks do if Style.max_line(do_) > Style.max_line(else_) do # we inverted the if/else blocks of this `if` statement in a previous pass (due to negators or unless) # shift comments etc to make it happy now - if_ast(zipper, head, do_, else_, ctx, false) + if_ast(zipper, head, do_, else_, ctx) else {:cont, zipper, ctx} end @@ -366,56 +366,32 @@ defmodule Styler.Style.Blocks do defp nodes_equivalent?(a, b), do: Style.without_meta(a) == Style.without_meta(b) + defp ensure_line({tag, meta, children} = node) do + if meta[:line], do: node, else: {tag, Keyword.put(meta, :line, Style.meta(List.first(children))[:line]), children} + end + defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx) do do_ = {{:__block__, [line: nil], [:do]}, do_body} else_ = {{:__block__, [line: nil], [:else]}, else_body} - if_ast(zipper, head, do_, else_, ctx, true) + if_ast(zipper, head, do_, else_, ctx) end - defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx, conversion?) do + defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx) do line = meta[:line] - # `conversion?: true` means we're converting a non-if construct (case/cond) into an if for the first - # time - no `do`/`else` keyword line has ever been spent, so nothing needs to be reclaimed. - # `conversion?: false` means the node is already an if whose do/else content is sitting in the - # "wrong" slot (eg negated-if swapping do/else bodies in place) - the *other* keyword's line is - # being vacated and needs to be accounted for. - - # ... why am i doing this again? hmm. do_body = Macro.update_meta(do_body, &Keyword.delete(&1, :end_of_expression)) else_body = Macro.update_meta(else_body, &Keyword.delete(&1, :end_of_expression)) - max_do_line = Style.max_line(do_body) - max_else_line = Style.max_line(else_body) - - # Change ast meta and comment lines to fit the `if` ast - {do_, else_, comments, end_line} = - if max_do_line >= max_else_line do - # we're swapping the ordering of two blocks of code - # and so must swap the lines of the ast & comments to keep comments where they belong! - # the math is: move B up by the length of A, and move A down by the length of B plus one (for the else keyword) - else_size = max_else_line - line - else_size = if conversion?, do: else_size, else: else_size + 1 - do_size = max_do_line - max_else_line - - shifts = [ - # move comments in the `else_body` down by the size of the `do_body` - {line..max_else_line, do_size}, - # move comments in `do_body` up by the size of the `else_body` - {(max_else_line + 1)..max_do_line, -else_size} - ] - - do_ = {Style.set_line(do_kw, line), Style.shift_line(do_body, -else_size)} - else_line = if conversion?, do: max_else_line, else: line + do_size - else_ = {Style.set_line(else_kw, else_line), Style.shift_line(else_body, do_size)} - end_line = if conversion?, do: max(max_do_line, max_else_line), else: max_do_line + 1 - {do_, else_, Style.shift_comments(ctx.comments, shifts), end_line} - else - # much simpler case -- just scootch things in the else down by 1 for the `else` keyword. - do_ = {{:__block__, [line: line], [:do]}, do_body} - else_ = Style.shift_line({{:__block__, [line: max_do_line], [:else]}, else_body}, 1) - end_line = max(max_do_line, max_else_line) - {do_, else_, Style.shift_comments(ctx.comments, max_do_line..max_else_line, 1), end_line} - end + # lay `do_body` then `else_body` out one after the other starting right after the `if`'s own line, + # carrying each one's comments along with it - same mechanism `Configs`/`ModuleDirectives` use to + # reorder nodes without stranding their comments. `order_line_meta_and_comments` needs each node's + # own top-level `:line`, but a multi-statement body is a `__block__` with no line of its own - + # borrow its first child's line for that purpose. + {[do_body, else_body], comments} = + Style.order_line_meta_and_comments([ensure_line(do_body), ensure_line(else_body)], ctx.comments, line) + + do_ = {Style.set_line(do_kw, line), do_body} + else_ = {Style.set_line(else_kw, Style.max_line(do_body)), else_body} + end_line = Style.max_line(else_body) + 1 zipper |> Zipper.replace({:if, [do: [line: line], end: [line: end_line], line: line], [head, [do_, else_]]}) From f74eb22bd3dbb1c3fa0a3ec3718c81313f4ac400 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 16 Jul 2026 09:22:34 -0700 Subject: [PATCH 06/16] my goodness --- lib/style/blocks.ex | 107 +++++++++++++++++++++++++++++-------- test/style/blocks_test.exs | 3 +- 2 files changed, 86 insertions(+), 24 deletions(-) diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index 5e5043a1..57263685 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -34,11 +34,21 @@ defmodule Styler.Style.Blocks do # case statement with exactly 2 `->` cases # rewrite to `if` if it's any of 3 trivial cases - def run({{:case, _, [head, [{_, [{:->, _, [[lhs_a], a]}, {:->, _, [[lhs_b], b]}]}]]}, _} = zipper, ctx) do + def run({{:case, m, [head, [{_, [{:->, am, [[lhs_a], a]}, {:->, bm, [[lhs_b], b]}]}]]}, _} = zipper, ctx) do + comments = + ctx.comments + |> pull_leading_comment(am[:line], body_start_line(a)) + |> pull_leading_comment(bm[:line], body_start_line(b)) + + ctx = %{ctx | comments: comments} + + # whichever clause ends up as `else` trails right up to either the *other* clause's own line (if it's + # first) or this `case`'s own `end` (if it's last) - use that real boundary so a dangling/trailing + # comment moves along with its content instead of getting stranded. case {lhs_a, lhs_b} do - {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx) - {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx) - {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx) + {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx, m[:end][:line]) + {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx, m[:end][:line]) + {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx, bm[:line]) _ -> {:cont, zipper, ctx} end end @@ -78,7 +88,7 @@ defmodule Styler.Style.Blocks do {:cont, zipper, ctx} end - def run({{:cond, _, [[{do_, clauses}]]}, _} = zipper, ctx) do + def run({{:cond, m, [[{do_, clauses}]]}, _} = zipper, ctx) do # ensure all final `atom -> final_clause` use `true` for consistency. # `:else` is cute but consistency is all. rewrite_literal_to_true = fn @@ -96,8 +106,18 @@ defmodule Styler.Style.Blocks do case List.update_at(clauses, -1, rewrite_literal_to_true) do # # Credo.Check.Refactor.CondStatements - [{:->, _, [[head], a]}, {:->, _, [[{:__block__, _, [true]}], b]}] -> if_ast(zipper, head, a, b, ctx) - clauses -> {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} + # `b` (the final clause, going into `else`) trails right up to this `cond`'s own `end` - use that + # real boundary so a dangling/trailing comment moves along with its content instead of getting stranded. + [{:->, am, [[head], a]}, {:->, bm, [[{:__block__, _, [true]}], b]}] -> + comments = + ctx.comments + |> pull_leading_comment(am[:line], body_start_line(a)) + |> pull_leading_comment(bm[:line], body_start_line(b)) + + if_ast(zipper, head, a, b, %{ctx | comments: comments}, m[:end][:line]) + + clauses -> + {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} end end @@ -200,11 +220,13 @@ defmodule Styler.Style.Blocks do [head, [do_block, {_, {:__block__, _, [nil]}}]] -> {:cont, Zipper.replace(zipper, {:if, m, [head, [do_block]]}), ctx} - [head, [do_, else_]] -> + [head, [do_, {else_kw, _} = else_]] -> if Style.max_line(do_) > Style.max_line(else_) do # we inverted the if/else blocks of this `if` statement in a previous pass (due to negators or unless) - # shift comments etc to make it happy now - if_ast(zipper, head, do_, else_, ctx) + # shift comments etc to make it happy now. `else_`'s content used to be paired with `do_kw`, and + # `else_kw`'s real (unmodified) line still marks exactly where that content used to trail off to - + # use it so a dangling/trailing comment moves along with its content instead of getting stranded. + if_ast(zipper, head, do_, else_, ctx, Style.meta(else_kw)[:line]) else {:cont, zipper, ctx} end @@ -370,29 +392,70 @@ defmodule Styler.Style.Blocks do if meta[:line], do: node, else: {tag, Keyword.put(meta, :line, Style.meta(List.first(children))[:line]), children} end - defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx) do + defp body_start_line({:__block__, meta, [child | _]}), do: meta[:line] || Style.meta(child)[:line] + defp body_start_line({_, meta, _}), do: meta[:line] + + # A leading comment on a `case`/`cond` clause's own `->` line (eg `# a` directly above `false ->`) can sit + # a line or more above the clause body's own content once that body is multi-line - too far for the + # (unwidened, on purpose - see `bound_trailing`) adjacency check in `order_line_meta_and_comments` to find. + # Pull any such leading comment down to sit directly adjacent to the body's real first line instead, so + # normal adjacency finds it. Leaves the body's own `:line` (and thus its rendered position) untouched. + defp pull_leading_comment(comments, header_line, body_line) do + {mine, rest} = Style.comments_for_lines(comments, header_line, header_line) + delta = body_line - header_line + if delta == 0, do: comments, else: Enum.sort_by(rest ++ Enum.map(mine, &%{&1 | line: &1.line + delta}), & &1.line) + end + + # Widens `else_body`'s own trailing boundary out to `bound` (exclusive - the real line of whatever comes + # right after it, eg the `if`'s own `end` or the next `case`/`cond` clause) by faking an `end_of_expression`, + # so a dangling/trailing comment after its last statement gets claimed by it instead of getting stranded. + # `do_body` never needs this: it's always the first of the two laid out below, so any trailing comment of + # its own that's genuinely adjacent to `else_body` gets claimed by `else_body`'s own (unwidened) leading- + # comment check anyway. `nil` bound means there's nothing real to bound it by - leave the body untouched. + defp bound_trailing(node, nil), do: node + + defp bound_trailing({tag, meta, children}, bound), + do: {tag, Keyword.put(meta, :end_of_expression, newlines: 1, line: bound - 1), children} + + defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx, else_bound) do do_ = {{:__block__, [line: nil], [:do]}, do_body} else_ = {{:__block__, [line: nil], [:else]}, else_body} - if_ast(zipper, head, do_, else_, ctx) + if_ast(zipper, head, do_, else_, ctx, else_bound) end - defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx) do + defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx, else_bound) do line = meta[:line] - do_body = Macro.update_meta(do_body, &Keyword.delete(&1, :end_of_expression)) - else_body = Macro.update_meta(else_body, &Keyword.delete(&1, :end_of_expression)) + + do_body = + do_body + |> Macro.update_meta(&Keyword.delete(&1, :end_of_expression)) + |> ensure_line() + + else_body = + else_body + |> Macro.update_meta(&Keyword.delete(&1, :end_of_expression)) + |> bound_trailing(else_bound) + |> ensure_line() # lay `do_body` then `else_body` out one after the other starting right after the `if`'s own line, # carrying each one's comments along with it - same mechanism `Configs`/`ModuleDirectives` use to - # reorder nodes without stranding their comments. `order_line_meta_and_comments` needs each node's - # own top-level `:line`, but a multi-statement body is a `__block__` with no line of its own - - # borrow its first child's line for that purpose. - {[do_body, else_body], comments} = - Style.order_line_meta_and_comments([ensure_line(do_body), ensure_line(else_body)], ctx.comments, line) + # reorder nodes without stranding their comments. + {[do_body, else_body], comments} = Style.order_line_meta_and_comments([do_body, else_body], ctx.comments, line) - do_ = {Style.set_line(do_kw, line), do_body} - else_ = {Style.set_line(else_kw, Style.max_line(do_body)), else_body} + # `else_body`'s (still-widened) end reserves room past any dangling comment `bound_trailing` claimed for + # it, so `end` doesn't collide with that comment's line - compute positions from it before dropping it. + else_line = Style.max_line(do_body) end_line = Style.max_line(else_body) + 1 + # the fake `end_of_expression` from `bound_trailing` did its job above (widening the comment search and + # the position calculations just above); its shifted value would otherwise mean something different once + # do_/else_ are actually adjacent siblings, so drop it and embed the real (shifted) content instead. + do_body = Macro.update_meta(do_body, &Keyword.delete(&1, :end_of_expression)) + else_body = Macro.update_meta(else_body, &Keyword.delete(&1, :end_of_expression)) + + do_ = {Style.set_line(do_kw, line), do_body} + else_ = {Style.set_line(else_kw, else_line), else_body} + zipper |> Zipper.replace({:if, [do: [line: line], end: [line: end_line], line: line], [head, [do_, else_]]}) |> run(%{ctx | comments: comments}) diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 48d65656..8884d6d5 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -208,9 +208,8 @@ defmodule Styler.Style.BlocksTest do if foo do # a :ok + # b end - - # b """ ) From 1a0ed2cacec0df5a059a53bc75c1a196b54988e7 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 16 Jul 2026 14:02:18 -0700 Subject: [PATCH 07/16] ew --- lib/style/blocks.ex | 48 ++++++---- test/style/blocks_test.exs | 183 +++++++++++++++++++++++++++++++++++++ 2 files changed, 211 insertions(+), 20 deletions(-) diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index 57263685..f0767e41 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -44,11 +44,12 @@ defmodule Styler.Style.Blocks do # whichever clause ends up as `else` trails right up to either the *other* clause's own line (if it's # first) or this `case`'s own `end` (if it's last) - use that real boundary so a dangling/trailing - # comment moves along with its content instead of getting stranded. + # comment moves along with its content instead of getting stranded. Same deal for `do_` when *it* ends + # up as the last clause (only the `false`/`true` ordering below flips do/else relative to source order). case {lhs_a, lhs_b} do - {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx, m[:end][:line]) - {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx, m[:end][:line]) - {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx, bm[:line]) + {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx, nil, m[:end][:line]) + {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx, nil, m[:end][:line]) + {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx, m[:end][:line], bm[:line]) _ -> {:cont, zipper, ctx} end end @@ -114,7 +115,7 @@ defmodule Styler.Style.Blocks do |> pull_leading_comment(am[:line], body_start_line(a)) |> pull_leading_comment(bm[:line], body_start_line(b)) - if_ast(zipper, head, a, b, %{ctx | comments: comments}, m[:end][:line]) + if_ast(zipper, head, a, b, %{ctx | comments: comments}, nil, m[:end][:line]) clauses -> {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} @@ -223,10 +224,12 @@ defmodule Styler.Style.Blocks do [head, [do_, {else_kw, _} = else_]] -> if Style.max_line(do_) > Style.max_line(else_) do # we inverted the if/else blocks of this `if` statement in a previous pass (due to negators or unless) - # shift comments etc to make it happy now. `else_`'s content used to be paired with `do_kw`, and + # shift comments etc to make it happy now. `do_`'s content used to be paired with `else_kw` and always + # trailed right up to this `if`'s own `end`; `else_`'s content used to be paired with `do_kw`, and # `else_kw`'s real (unmodified) line still marks exactly where that content used to trail off to - - # use it so a dangling/trailing comment moves along with its content instead of getting stranded. - if_ast(zipper, head, do_, else_, ctx, Style.meta(else_kw)[:line]) + # use those real boundaries so a dangling/trailing comment moves along with its content instead of + # getting stranded. + if_ast(zipper, head, do_, else_, ctx, m[:end][:line], Style.meta(else_kw)[:line]) else {:cont, zipper, ctx} end @@ -406,29 +409,31 @@ defmodule Styler.Style.Blocks do if delta == 0, do: comments, else: Enum.sort_by(rest ++ Enum.map(mine, &%{&1 | line: &1.line + delta}), & &1.line) end - # Widens `else_body`'s own trailing boundary out to `bound` (exclusive - the real line of whatever comes - # right after it, eg the `if`'s own `end` or the next `case`/`cond` clause) by faking an `end_of_expression`, - # so a dangling/trailing comment after its last statement gets claimed by it instead of getting stranded. - # `do_body` never needs this: it's always the first of the two laid out below, so any trailing comment of - # its own that's genuinely adjacent to `else_body` gets claimed by `else_body`'s own (unwidened) leading- - # comment check anyway. `nil` bound means there's nothing real to bound it by - leave the body untouched. + # Widens a body's own trailing boundary out to `bound` (exclusive - the real line of whatever comes right + # after it, eg the `if`'s own `end` or the next `case`/`cond` clause) by faking an `end_of_expression`, so + # a dangling/trailing comment after its last statement gets claimed by it instead of getting stranded. Only + # relevant for whichever of `do_body`/`else_body` is genuinely the *last* real clause/keyword before that + # boundary - widening the other one risks reaching into territory that rightfully belongs to its neighbor's + # own leading-comment claim (that neighbor still gets a natural, unwidened claim on anything truly adjacent + # to it). `nil` bound means this body isn't the last one - leave it untouched. defp bound_trailing(node, nil), do: node defp bound_trailing({tag, meta, children}, bound), do: {tag, Keyword.put(meta, :end_of_expression, newlines: 1, line: bound - 1), children} - defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx, else_bound) do + defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx, do_bound, else_bound) do do_ = {{:__block__, [line: nil], [:do]}, do_body} else_ = {{:__block__, [line: nil], [:else]}, else_body} - if_ast(zipper, head, do_, else_, ctx, else_bound) + if_ast(zipper, head, do_, else_, ctx, do_bound, else_bound) end - defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx, else_bound) do + defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx, do_bound, else_bound) do line = meta[:line] do_body = do_body |> Macro.update_meta(&Keyword.delete(&1, :end_of_expression)) + |> bound_trailing(do_bound) |> ensure_line() else_body = @@ -442,9 +447,12 @@ defmodule Styler.Style.Blocks do # reorder nodes without stranding their comments. {[do_body, else_body], comments} = Style.order_line_meta_and_comments([do_body, else_body], ctx.comments, line) - # `else_body`'s (still-widened) end reserves room past any dangling comment `bound_trailing` claimed for - # it, so `end` doesn't collide with that comment's line - compute positions from it before dropping it. - else_line = Style.max_line(do_body) + # a still-widened end reserves room past any dangling comment `bound_trailing` claimed - compute + # positions from it before dropping it. `end_line` can use it as-is (nothing follows `end`, so landing + # exactly on the reserved line is fine), but `else_kw` has `else_body` coming right after it, so it + # needs to land one line *past* the reservation instead of on top of it - but only when `do_body` was + # actually widened; otherwise this would just be an unnecessary (and possibly wrong) nudge forward. + else_line = Style.max_line(do_body) + if(do_bound, do: 1, else: 0) end_line = Style.max_line(else_body) + 1 # the fake `end_of_expression` from `bound_trailing` did its job above (widening the comment search and diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 8884d6d5..ec35b10b 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -1683,4 +1683,187 @@ defmodule Styler.Style.BlocksTest do ) end end + + describe "block swap comment coverage - suspected gaps" do + test "blank line breaks natural adjacency to the next clause" do + assert_style( + """ + if !a do + b + # dangling + + else + c + end + """, + """ + if a do + c + else + b + # dangling + end + """ + ) + end + + test "multi-line leading comment block on a clause header" do + assert_style( + """ + case foo do + # line 1 + # line 2 + false -> :error + true -> :ok + end + """, + """ + if foo do + :ok + else + # line 1 + # line 2 + :error + end + """ + ) + end + + test "wildcard else clause with multi-line do body and a leading comment" do + assert_style( + """ + case foo do + # a + true -> + b1 + b2 + _ -> + # c + c + end + """, + """ + if foo do + # a + b1 + b2 + else + # c + c + end + """ + ) + end + + test "with rewritten through case to if, with comments" do + assert_style( + """ + with true <- foo() do + # a + bar() + else + false -> + # b + baz() + end + """, + """ + if foo() do + # a + bar() + else + # b + baz() + end + """ + ) + end + + test "comment directly before the case, no blank line before the first clause" do + assert_style( + """ + # leading + case foo do + false -> :error + true -> :ok + end + """, + """ + # leading + if foo do + :ok + else + :error + end + """ + ) + end + + test "nested if inside a swapped multi-statement body" do + assert_style( + """ + if !a do + # b1 + b1 + + if x do + # nested + y + end + + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # c + c + else + # b1 + b1 + + if x do + # nested + y + end + + # b2 + b2 + end + """ + ) + end + + test "asymmetric case with leading comments on both clauses and a trailing dangling comment" do + assert_style( + """ + case foo do + # a + false -> + d1 + d2 + # b + true -> + e + # dangling + end + """, + """ + if foo do + # b + e + # dangling + else + # a + d1 + d2 + end + """ + ) + end + end end From 006f6221963cfab95b397a81d325c3310fdd8d71 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 23 Jul 2026 08:11:52 -0700 Subject: [PATCH 08/16] humanize if_ast --- lib/style/blocks.ex | 93 +++++++++++++++++---------------------------- 1 file changed, 35 insertions(+), 58 deletions(-) diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index f0767e41..312af2cb 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -35,21 +35,17 @@ defmodule Styler.Style.Blocks do # case statement with exactly 2 `->` cases # rewrite to `if` if it's any of 3 trivial cases def run({{:case, m, [head, [{_, [{:->, am, [[lhs_a], a]}, {:->, bm, [[lhs_b], b]}]}]]}, _} = zipper, ctx) do - comments = - ctx.comments - |> pull_leading_comment(am[:line], body_start_line(a)) - |> pull_leading_comment(bm[:line], body_start_line(b)) + end_line = m[:end][:line] - ctx = %{ctx | comments: comments} + ctx = + ctx + |> Map.update!(:comments, &pull_leading_comment(&1, am[:line], body_start_line(a))) + |> Map.update!(:comments, &pull_leading_comment(&1, bm[:line], body_start_line(b))) - # whichever clause ends up as `else` trails right up to either the *other* clause's own line (if it's - # first) or this `case`'s own `end` (if it's last) - use that real boundary so a dangling/trailing - # comment moves along with its content instead of getting stranded. Same deal for `do_` when *it* ends - # up as the last clause (only the `false`/`true` ordering below flips do/else relative to source order). case {lhs_a, lhs_b} do - {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx, nil, m[:end][:line]) - {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx, nil, m[:end][:line]) - {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx, m[:end][:line], bm[:line]) + {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx, else: end_line) + {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx, else: end_line) + {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx, do: end_line, else: bm[:line]) _ -> {:cont, zipper, ctx} end end @@ -115,7 +111,7 @@ defmodule Styler.Style.Blocks do |> pull_leading_comment(am[:line], body_start_line(a)) |> pull_leading_comment(bm[:line], body_start_line(b)) - if_ast(zipper, head, a, b, %{ctx | comments: comments}, nil, m[:end][:line]) + if_ast(zipper, head, a, b, %{ctx | comments: comments}, else: m[:end][:line]) clauses -> {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} @@ -229,7 +225,7 @@ defmodule Styler.Style.Blocks do # `else_kw`'s real (unmodified) line still marks exactly where that content used to trail off to - # use those real boundaries so a dangling/trailing comment moves along with its content instead of # getting stranded. - if_ast(zipper, head, do_, else_, ctx, m[:end][:line], Style.meta(else_kw)[:line]) + if_ast(zipper, head, do_, else_, ctx, do: m[:end][:line], else: Style.meta(else_kw)[:line]) else {:cont, zipper, ctx} end @@ -391,10 +387,6 @@ defmodule Styler.Style.Blocks do defp nodes_equivalent?(a, b), do: Style.without_meta(a) == Style.without_meta(b) - defp ensure_line({tag, meta, children} = node) do - if meta[:line], do: node, else: {tag, Keyword.put(meta, :line, Style.meta(List.first(children))[:line]), children} - end - defp body_start_line({:__block__, meta, [child | _]}), do: meta[:line] || Style.meta(child)[:line] defp body_start_line({_, meta, _}), do: meta[:line] @@ -409,55 +401,40 @@ defmodule Styler.Style.Blocks do if delta == 0, do: comments, else: Enum.sort_by(rest ++ Enum.map(mine, &%{&1 | line: &1.line + delta}), & &1.line) end - # Widens a body's own trailing boundary out to `bound` (exclusive - the real line of whatever comes right - # after it, eg the `if`'s own `end` or the next `case`/`cond` clause) by faking an `end_of_expression`, so - # a dangling/trailing comment after its last statement gets claimed by it instead of getting stranded. Only - # relevant for whichever of `do_body`/`else_body` is genuinely the *last* real clause/keyword before that - # boundary - widening the other one risks reaching into territory that rightfully belongs to its neighbor's - # own leading-comment claim (that neighbor still gets a natural, unwidened claim on anything truly adjacent - # to it). `nil` bound means this body isn't the last one - leave it untouched. - defp bound_trailing(node, nil), do: node - - defp bound_trailing({tag, meta, children}, bound), - do: {tag, Keyword.put(meta, :end_of_expression, newlines: 1, line: bound - 1), children} - - defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx, do_bound, else_bound) do + # When we're coming in from here, we know we're coming in for a transformation from a different block + defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx, bounds) do do_ = {{:__block__, [line: nil], [:do]}, do_body} else_ = {{:__block__, [line: nil], [:else]}, else_body} - if_ast(zipper, head, do_, else_, ctx, do_bound, else_bound) + if_ast(zipper, head, do_, else_, ctx, bounds) end - defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx, do_bound, else_bound) do + defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx, bounds) do line = meta[:line] - do_body = - do_body - |> Macro.update_meta(&Keyword.delete(&1, :end_of_expression)) - |> bound_trailing(do_bound) - |> ensure_line() - - else_body = - else_body - |> Macro.update_meta(&Keyword.delete(&1, :end_of_expression)) - |> bound_trailing(else_bound) - |> ensure_line() - - # lay `do_body` then `else_body` out one after the other starting right after the `if`'s own line, - # carrying each one's comments along with it - same mechanism `Configs`/`ModuleDirectives` use to - # reorder nodes without stranding their comments. + # When converting different blocks to if statements, the comment algo can miss dangling comments for different shapes. + # Setting a fake end_of_expression helps clue it in to grab those comments. + # this is a dirty hack - the end_of_expression gets nixed before this function returns + [do_body, else_body] = + for {kw, {node, meta, children}} <- [do: do_body, else: else_body] do + meta = + if line = bounds[kw], + do: Keyword.put(meta, :end_of_expression, [newlines: 1, line: line - 1]), + else: Keyword.delete(meta, :end_of_expression) + + meta = + if meta[:line], + do: meta, + else: Keyword.put(meta, :line, Style.meta(hd(children))[:line]) + + {node, meta, children} + end + {[do_body, else_body], comments} = Style.order_line_meta_and_comments([do_body, else_body], ctx.comments, line) - # a still-widened end reserves room past any dangling comment `bound_trailing` claimed - compute - # positions from it before dropping it. `end_line` can use it as-is (nothing follows `end`, so landing - # exactly on the reserved line is fine), but `else_kw` has `else_body` coming right after it, so it - # needs to land one line *past* the reservation instead of on top of it - but only when `do_body` was - # actually widened; otherwise this would just be an unnecessary (and possibly wrong) nudge forward. - else_line = Style.max_line(do_body) + if(do_bound, do: 1, else: 0) + # the lines for the else and end keywords. not sure why the bounds... + else_line = Style.max_line(do_body) + if(bounds[:do], do: 1, else: 0) end_line = Style.max_line(else_body) + 1 - - # the fake `end_of_expression` from `bound_trailing` did its job above (widening the comment search and - # the position calculations just above); its shifted value would otherwise mean something different once - # do_/else_ are actually adjacent siblings, so drop it and embed the real (shifted) content instead. + # clean up the dangling comments hack do_body = Macro.update_meta(do_body, &Keyword.delete(&1, :end_of_expression)) else_body = Macro.update_meta(else_body, &Keyword.delete(&1, :end_of_expression)) From 63b7c393ed69b1256381df075b64367d2ecc8556 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 23 Jul 2026 13:26:41 -0700 Subject: [PATCH 09/16] humanize if_ast bounds nonsense --- lib/style.ex | 8 ++- lib/style/blocks.ex | 103 ++++++++++++++++--------------------- test/style/blocks_test.exs | 100 +++++++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 60 deletions(-) diff --git a/lib/style.ex b/lib/style.ex index 63e5d815..9a6d0d1d 100644 --- a/lib/style.ex +++ b/lib/style.ex @@ -170,6 +170,9 @@ defmodule Styler.Style do {directive, updated_meta, children} end + def first_line({:__block__, m, [{_, cm, _} | _]}), do: m[:line] || cm[:line] + def first_line(node), do: meta(node)[:line] + def max_line([_ | _] = list), do: list |> List.last() |> max_line() def max_line(ast) do @@ -198,7 +201,7 @@ defmodule Styler.Style do {nodes, shifted_comments, comments, _line} = Enum.reduce(nodes, {[], [], comments, first_line}, fn node, {n_acc, c_acc, comments, move_to_line} -> meta = meta(node) - line = meta[:line] + line = first_line(node) last_line = max_line(node) {mine, comments} = comments_for_lines(comments, line, last_line) @@ -223,7 +226,7 @@ defmodule Styler.Style do @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: comments_for_lines(comments, m[:line], max_line(node)) + def comments_for_node(node, comments), do: comments_for_lines(comments, first_line(node), max_line(node)) @doc """ Gets all comments in range start_line..last_line, and any comments immediately before start_line.s @@ -239,6 +242,7 @@ defmodule Styler.Style do here, comments_for_lines(comments, 4, 6) is "a", "b", "c", "d" """ def comments_for_lines(comments, start_line, last_line) do + if !start_line, do: raise "nil" comments |> Enum.reverse() |> comments_for_lines(start_line, last_line, [], []) end diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index 312af2cb..8a431416 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -35,17 +35,15 @@ defmodule Styler.Style.Blocks do # case statement with exactly 2 `->` cases # rewrite to `if` if it's any of 3 trivial cases def run({{:case, m, [head, [{_, [{:->, am, [[lhs_a], a]}, {:->, bm, [[lhs_b], b]}]}]]}, _} = zipper, ctx) do - end_line = m[:end][:line] - - ctx = - ctx - |> Map.update!(:comments, &pull_leading_comment(&1, am[:line], body_start_line(a))) - |> Map.update!(:comments, &pull_leading_comment(&1, bm[:line], body_start_line(b))) + # @TODO shouldn't be shifting if we aren't doing if_ast rewrites. + # try to put shift into the if_ast header that matches transformations? + ctx = shift_arrow_comments_into_body(ctx, {am, a}, {bm, b}) + b = Macro.update_meta(b, &Keyword.put(&1, :end_of_expression, [line: m[:end][:line], newlines: 1])) case {lhs_a, lhs_b} do - {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx, else: end_line) - {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx, else: end_line) - {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx, do: end_line, else: bm[:line]) + {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx) + {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx) + {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx) _ -> {:cont, zipper, ctx} end end @@ -106,12 +104,9 @@ defmodule Styler.Style.Blocks do # `b` (the final clause, going into `else`) trails right up to this `cond`'s own `end` - use that # real boundary so a dangling/trailing comment moves along with its content instead of getting stranded. [{:->, am, [[head], a]}, {:->, bm, [[{:__block__, _, [true]}], b]}] -> - comments = - ctx.comments - |> pull_leading_comment(am[:line], body_start_line(a)) - |> pull_leading_comment(bm[:line], body_start_line(b)) - - if_ast(zipper, head, a, b, %{ctx | comments: comments}, else: m[:end][:line]) + ctx = shift_arrow_comments_into_body(ctx, {am, a}, {bm, b}) + b = Macro.update_meta(b, &Keyword.put(&1, :end_of_expression, [line: m[:end][:line], newlines: 1])) + if_ast(zipper, head, a, b, ctx) clauses -> {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} @@ -207,6 +202,11 @@ defmodule Styler.Style.Blocks do # Credo.Check.Refactor.NegatedConditionsWithElse # if !x, do: y, else: z => if x, do: z, else: y [negator, [{do_, do_body}, {else_, else_body}]] when is_negator(negator) -> + # end of expression hacks ensure that these bodies keep dangling comments in their blocks. + # someday we might find a better way! + # ohhhhhhh probably i need to fix the line numbers on the do and else to match the lines.... HMM + do_body = Macro.update_meta(do_body, &Keyword.put(&1, :end_of_expression, [line: Style.meta(else_)[:line], newlines: 1])) + else_body = Macro.update_meta(else_body, &Keyword.put(&1, :end_of_expression, [line: m[:end][:line], newlines: 1])) zipper |> Zipper.replace({:if, m, [invert(negator), [{do_, else_body}, {else_, do_body}]]}) |> run(ctx) # drop `else end` @@ -217,15 +217,9 @@ defmodule Styler.Style.Blocks do [head, [do_block, {_, {:__block__, _, [nil]}}]] -> {:cont, Zipper.replace(zipper, {:if, m, [head, [do_block]]}), ctx} - [head, [do_, {else_kw, _} = else_]] -> + [head, [do_, else_]] -> if Style.max_line(do_) > Style.max_line(else_) do - # we inverted the if/else blocks of this `if` statement in a previous pass (due to negators or unless) - # shift comments etc to make it happy now. `do_`'s content used to be paired with `else_kw` and always - # trailed right up to this `if`'s own `end`; `else_`'s content used to be paired with `do_kw`, and - # `else_kw`'s real (unmodified) line still marks exactly where that content used to trail off to - - # use those real boundaries so a dangling/trailing comment moves along with its content instead of - # getting stranded. - if_ast(zipper, head, do_, else_, ctx, do: m[:end][:line], else: Style.meta(else_kw)[:line]) + if_ast(zipper, head, do_, else_, ctx) else {:cont, zipper, ctx} end @@ -387,52 +381,45 @@ defmodule Styler.Style.Blocks do defp nodes_equivalent?(a, b), do: Style.without_meta(a) == Style.without_meta(b) - defp body_start_line({:__block__, meta, [child | _]}), do: meta[:line] || Style.meta(child)[:line] - defp body_start_line({_, meta, _}), do: meta[:line] - - # A leading comment on a `case`/`cond` clause's own `->` line (eg `# a` directly above `false ->`) can sit - # a line or more above the clause body's own content once that body is multi-line - too far for the - # (unwidened, on purpose - see `bound_trailing`) adjacency check in `order_line_meta_and_comments` to find. - # Pull any such leading comment down to sit directly adjacent to the body's real first line instead, so - # normal adjacency finds it. Leaves the body's own `:line` (and thus its rendered position) untouched. - defp pull_leading_comment(comments, header_line, body_line) do - {mine, rest} = Style.comments_for_lines(comments, header_line, header_line) - delta = body_line - header_line - if delta == 0, do: comments, else: Enum.sort_by(rest ++ Enum.map(mine, &%{&1 | line: &1.line + delta}), & &1.line) + # shifts comments sitting directly on arrows into the body. + # ideally this gets rolled into comment management, but because we're removing the arrows, we can't see the gaps + # in the bodies of these things. + # maybe an alternative is to hack the bodies to have a start line equal to the arrow's start line, thereby + # leaving all comment manip to our comment manip function + # yeah, the main problem seems to be that the body of an arrow can have a line number much higher than the arrow itself, and so we lose comments modifying the arrow. + # we need to encode that arrow line number somehow. essentially, that's the line number of our `do` keyword or whatever + defp shift_arrow_comments_into_body(ctx, {am, a}, {bm, b}) do + ctx + |> Map.update!(:comments, &do_shift_arrow_comments_into_body(&1, am, a)) + |> Map.update!(:comments, &do_shift_arrow_comments_into_body(&1, bm, b)) + end + + defp do_shift_arrow_comments_into_body(comments, arrow_meta, body) do + arrow_line = arrow_meta[:line] + body_line = Style.first_line(body) + + if body_line == arrow_line do + comments + else + {mine, rest} = Style.comments_for_lines(comments, arrow_line, arrow_line) + mine = Enum.map(mine, &%{&1 | line: &1.line + 1}) + Enum.sort_by(rest ++ mine, & &1.line) + end end # When we're coming in from here, we know we're coming in for a transformation from a different block - defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx, bounds) do + defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx) do do_ = {{:__block__, [line: nil], [:do]}, do_body} else_ = {{:__block__, [line: nil], [:else]}, else_body} - if_ast(zipper, head, do_, else_, ctx, bounds) + if_ast(zipper, head, do_, else_, ctx) end - defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx, bounds) do + defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx) do line = meta[:line] - # When converting different blocks to if statements, the comment algo can miss dangling comments for different shapes. - # Setting a fake end_of_expression helps clue it in to grab those comments. - # this is a dirty hack - the end_of_expression gets nixed before this function returns - [do_body, else_body] = - for {kw, {node, meta, children}} <- [do: do_body, else: else_body] do - meta = - if line = bounds[kw], - do: Keyword.put(meta, :end_of_expression, [newlines: 1, line: line - 1]), - else: Keyword.delete(meta, :end_of_expression) - - meta = - if meta[:line], - do: meta, - else: Keyword.put(meta, :line, Style.meta(hd(children))[:line]) - - {node, meta, children} - end - {[do_body, else_body], comments} = Style.order_line_meta_and_comments([do_body, else_body], ctx.comments, line) - # the lines for the else and end keywords. not sure why the bounds... - else_line = Style.max_line(do_body) + if(bounds[:do], do: 1, else: 0) + else_line = Style.max_line(do_body) end_line = Style.max_line(else_body) + 1 # clean up the dangling comments hack do_body = Macro.update_meta(do_body, &Keyword.delete(&1, :end_of_expression)) diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index ec35b10b..b4ea8605 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -1844,10 +1844,12 @@ defmodule Styler.Style.BlocksTest do case foo do # a false -> + # foo d1 d2 # b true -> + # bar e # dangling end @@ -1855,15 +1857,113 @@ defmodule Styler.Style.BlocksTest do """ if foo do # b + # bar e # dangling else # a + # foo d1 d2 end """ ) end + + test "cond do rewrite with dangler" do + assert_style( + """ + cond do + # a + foo? -> + # foo + d1 + d2 + # b + true -> + # bar + e + # dangling + end + """, + """ + if foo? do + # a + # foo + d1 + d2 + else + # b + # bar + e + # dangling + end + """ + ) + end + + test "another" do + assert_style( + """ + case foo do + # a + false -> + # foo + d1 + # b + true -> + # bar + e + f + # dangling + end + """, + """ + if foo do + # b + # bar + e + f + # dangling + else + # a + # foo + d1 + end + """ + ) + end + + test "yet another" do + assert_style( + """ + case foo do + # a + true -> + # foo + d1 + # b + false -> + # bar + e + f + # dangling + end + """, + """ + if foo do + # a + # foo + d1 + else + # b + # bar + e + f + # dangling + end + """ + ) + end end end From 8941179e02cb721d78dc4b9d4eb87e5dc2e967b5 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 23 Jul 2026 14:26:39 -0700 Subject: [PATCH 10/16] humanize if_ast duplications into arrows_to_if --- lib/style/blocks.ex | 91 ++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 50 deletions(-) diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index 8a431416..b3a20588 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -34,16 +34,13 @@ defmodule Styler.Style.Blocks do # case statement with exactly 2 `->` cases # rewrite to `if` if it's any of 3 trivial cases - def run({{:case, m, [head, [{_, [{:->, am, [[lhs_a], a]}, {:->, bm, [[lhs_b], b]}]}]]}, _} = zipper, ctx) do - # @TODO shouldn't be shifting if we aren't doing if_ast rewrites. - # try to put shift into the if_ast header that matches transformations? - ctx = shift_arrow_comments_into_body(ctx, {am, a}, {bm, b}) - b = Macro.update_meta(b, &Keyword.put(&1, :end_of_expression, [line: m[:end][:line], newlines: 1])) + def run({{:case, m, [head, [{_, [{:->, _, [[lhs_a], _]} = a, {:->, _, [[lhs_b], _]} = b]}]]}, _} = zipper, ctx) do + end_line = m[:end][:line] case {lhs_a, lhs_b} do - {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx) - {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx) - {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx) + {{_, _, [true]}, {_, _, [false]}} -> arrows_to_if(zipper, head, a, b, end_line, ctx) + {{_, _, [true]}, {:_, _, _}} -> arrows_to_if(zipper, head, a, b, end_line, ctx) + {{_, _, [false]}, {_, _, [true]}} -> arrows_to_if(zipper, head, b, a, end_line, ctx) _ -> {:cont, zipper, ctx} end end @@ -100,16 +97,9 @@ defmodule Styler.Style.Blocks do end case List.update_at(clauses, -1, rewrite_literal_to_true) do - # # Credo.Check.Refactor.CondStatements - # `b` (the final clause, going into `else`) trails right up to this `cond`'s own `end` - use that - # real boundary so a dangling/trailing comment moves along with its content instead of getting stranded. - [{:->, am, [[head], a]}, {:->, bm, [[{:__block__, _, [true]}], b]}] -> - ctx = shift_arrow_comments_into_body(ctx, {am, a}, {bm, b}) - b = Macro.update_meta(b, &Keyword.put(&1, :end_of_expression, [line: m[:end][:line], newlines: 1])) - if_ast(zipper, head, a, b, ctx) - - clauses -> - {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} + # Credo.Check.Refactor.CondStatements + [{:->, _, [[head], _]} = a, {:->, _, [[{:__block__, _, [true]}], _]} = b] -> arrows_to_if(zipper, head, a, b, m[:end][:line], ctx) + clauses -> {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} end end @@ -202,11 +192,8 @@ defmodule Styler.Style.Blocks do # Credo.Check.Refactor.NegatedConditionsWithElse # if !x, do: y, else: z => if x, do: z, else: y [negator, [{do_, do_body}, {else_, else_body}]] when is_negator(negator) -> - # end of expression hacks ensure that these bodies keep dangling comments in their blocks. - # someday we might find a better way! - # ohhhhhhh probably i need to fix the line numbers on the do and else to match the lines.... HMM + # end of expression hack ensure that the else body keeps dangling comments its block do_body = Macro.update_meta(do_body, &Keyword.put(&1, :end_of_expression, [line: Style.meta(else_)[:line], newlines: 1])) - else_body = Macro.update_meta(else_body, &Keyword.put(&1, :end_of_expression, [line: m[:end][:line], newlines: 1])) zipper |> Zipper.replace({:if, m, [invert(negator), [{do_, else_body}, {else_, do_body}]]}) |> run(ctx) # drop `else end` @@ -219,7 +206,7 @@ defmodule Styler.Style.Blocks do [head, [do_, else_]] -> if Style.max_line(do_) > Style.max_line(else_) do - if_ast(zipper, head, do_, else_, ctx) + organize_if(zipper, head, do_, else_, ctx) else {:cont, zipper, ctx} end @@ -381,24 +368,35 @@ defmodule Styler.Style.Blocks do defp nodes_equivalent?(a, b), do: Style.without_meta(a) == Style.without_meta(b) - # shifts comments sitting directly on arrows into the body. - # ideally this gets rolled into comment management, but because we're removing the arrows, we can't see the gaps - # in the bodies of these things. - # maybe an alternative is to hack the bodies to have a start line equal to the arrow's start line, thereby - # leaving all comment manip to our comment manip function - # yeah, the main problem seems to be that the body of an arrow can have a line number much higher than the arrow itself, and so we lose comments modifying the arrow. - # we need to encode that arrow line number somehow. essentially, that's the line number of our `do` keyword or whatever - defp shift_arrow_comments_into_body(ctx, {am, a}, {bm, b}) do - ctx - |> Map.update!(:comments, &do_shift_arrow_comments_into_body(&1, am, a)) - |> Map.update!(:comments, &do_shift_arrow_comments_into_body(&1, bm, b)) + # hacks comments above the arrows to have the same line number as the start of the body, + # and hacks the body of the last of a/b to have an end of expression equal to where the `end` keyword is to make sure + # dangling comments get caught + # would be lovely to not hack things so hard but c'est la vie for now + defp arrows_to_if(zipper, head, {:->, am, [_, a]}, {:->, bm, [_, b]}, end_line, ctx) do + ctx = + ctx + |> Map.update!(:comments, &lower_arrow_comments_to_body(&1, am, a)) + |> Map.update!(:comments, &lower_arrow_comments_to_body(&1, bm, b)) + + # hacking the end_of_expression helps ensure that the (previously) last clause catches dangling comments + [a, b] = + if Style.first_line(a) < Style.first_line(b) do + b = Macro.update_meta(b, &Keyword.put(&1, :end_of_expression, [line: end_line, newlines: 1])) + [a, b] + else + a = Macro.update_meta(a, &Keyword.put(&1, :end_of_expression, [line: end_line, newlines: 1])) + [a, b] + end + + do_ = {{:__block__, [line: nil], [:do]}, a} + else_ = {{:__block__, [line: nil], [:else]}, b} + organize_if(zipper, head, do_, else_, ctx) end - defp do_shift_arrow_comments_into_body(comments, arrow_meta, body) do + defp lower_arrow_comments_to_body(comments, arrow_meta, body) do arrow_line = arrow_meta[:line] - body_line = Style.first_line(body) - if body_line == arrow_line do + if Style.first_line(body) == arrow_line do comments else {mine, rest} = Style.comments_for_lines(comments, arrow_line, arrow_line) @@ -407,29 +405,22 @@ defmodule Styler.Style.Blocks do end end - # When we're coming in from here, we know we're coming in for a transformation from a different block - defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx) do - do_ = {{:__block__, [line: nil], [:do]}, do_body} - else_ = {{:__block__, [line: nil], [:else]}, else_body} - if_ast(zipper, head, do_, else_, ctx) - end - - defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx) do - line = meta[:line] + defp organize_if(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx) do + head_line = meta[:line] - {[do_body, else_body], comments} = Style.order_line_meta_and_comments([do_body, else_body], ctx.comments, line) + {[do_body, else_body], comments} = Style.order_line_meta_and_comments([do_body, else_body], ctx.comments, head_line) else_line = Style.max_line(do_body) end_line = Style.max_line(else_body) + 1 - # clean up the dangling comments hack + # clean up the dangling comments hack if this was a conversion do_body = Macro.update_meta(do_body, &Keyword.delete(&1, :end_of_expression)) else_body = Macro.update_meta(else_body, &Keyword.delete(&1, :end_of_expression)) - do_ = {Style.set_line(do_kw, line), do_body} + do_ = {Style.set_line(do_kw, head_line), do_body} else_ = {Style.set_line(else_kw, else_line), else_body} zipper - |> Zipper.replace({:if, [do: [line: line], end: [line: end_line], line: line], [head, [do_, else_]]}) + |> Zipper.replace({:if, [do: [line: head_line], end: [line: end_line], line: head_line], [head, [do_, else_]]}) |> run(%{ctx | comments: comments}) end From 545cf5883379d5f1e32716741c4cd20c800fc122 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 23 Jul 2026 15:21:24 -0700 Subject: [PATCH 11/16] pare down configs comments etc --- lib/style/configs.ex | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/style/configs.ex b/lib/style/configs.ex index 62d4085f..c74f6447 100644 --- a/lib/style/configs.ex +++ b/lib/style/configs.ex @@ -77,19 +77,17 @@ defmodule Styler.Style.Configs do first_line = min(List.first(node_comments)[:line] || cfm[:line], cfm[:line]) # Sorting and re-spacing can make the block taller (config groups gain blank lines between them). - # `order_line_meta_and_comments` only moves the sorted nodes and their comments, so `rest` nodes and any - # trailing comments keep their original lines - if the block grows past them, the formatter pulls those - # comments up into our configs. Measure the growth and shift the trailing region down to match. + # reodering means the block can grow past `rest` and its comments, causing the comments for `rest` to get sucked up into our block. + # `configs`/`assignments` are reverse-ordered (`accumulate` prepends), so we can't just take the last node. - block_end = [config | configs ++ assignments] |> Enum.map(&Style.max_line/1) |> Enum.max() - {block_comments, tail_comments} = Enum.split_with(comments, &(&1.line <= block_end)) + max_before_ordering = [config | configs ++ assignments] |> Enum.map(&Style.max_line/1) |> Enum.max() + {block_comments, tail_comments} = Enum.split_while(comments, &(&1.line <= max_before_ordering)) {nodes, block_comments} = Style.order_line_meta_and_comments(nodes, block_comments, first_line) - # order_line_meta_and_comments lays nodes out with increasing line numbers, so the block now ends at the last - delta = Style.max_line(nodes) - block_end + delta = Style.max_line(nodes) - max_before_ordering tail_comments = Enum.map(tail_comments, &%{&1 | line: &1.line + delta}) - rest = Enum.map(rest, &Style.shift_line(&1, delta)) + rest = Style.shift_line(rest, delta) {nodes, Enum.sort_by(block_comments ++ tail_comments, & &1.line), rest} else From 7d6f06a20158d7d6237d58752416652776e79267 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 23 Jul 2026 15:48:24 -0700 Subject: [PATCH 12/16] humanize module directives --- lib/style/configs.ex | 2 -- lib/style/module_directives.ex | 47 +++++++++++++--------------------- 2 files changed, 18 insertions(+), 31 deletions(-) diff --git a/lib/style/configs.ex b/lib/style/configs.ex index c74f6447..3583c850 100644 --- a/lib/style/configs.ex +++ b/lib/style/configs.ex @@ -78,8 +78,6 @@ defmodule Styler.Style.Configs do # Sorting and re-spacing can make the block taller (config groups gain blank lines between them). # reodering means the block can grow past `rest` and its comments, causing the comments for `rest` to get sucked up into our block. - - # `configs`/`assignments` are reverse-ordered (`accumulate` prepends), so we can't just take the last node. max_before_ordering = [config | configs ++ assignments] |> Enum.map(&Style.max_line/1) |> Enum.max() {block_comments, tail_comments} = Enum.split_while(comments, &(&1.line <= max_before_ordering)) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index 72222e6d..b0a9d627 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -121,8 +121,7 @@ defmodule Styler.Style.ModuleDirectives do # we want only-child literal block to be handled in the only-child catch-all. it means someone did a weird # (that would be a literal, so best case someone wrote a string and forgot to put `@moduledoc` before it) {:__block__, _, [_, _ | _]} -> - {zipper, comments} = organize_directives(body_zipper, ctx.comments, moduledoc) - {:skip, zipper, %{ctx | comments: comments}} + organize_directives(body_zipper, ctx, moduledoc) # a module whose only child is a moduledoc. nothing to do here! # seems weird at first blush but lots of projects/libraries do this with their root namespace module @@ -132,12 +131,9 @@ defmodule Styler.Style.ModuleDirectives do # There's only one child, and it's not a moduledoc. Conditionally add a moduledoc, then style the only_child only_child -> if moduledoc do - {zipper, comments} = - body_zipper - |> Zipper.replace({:__block__, [], [moduledoc, only_child]}) - |> organize_directives(ctx.comments) - - {:skip, zipper, %{ctx | comments: comments}} + body_zipper + |> Zipper.replace({:__block__, [], [moduledoc, only_child]}) + |> organize_directives(ctx) else do_run(body_zipper, ctx) end @@ -149,13 +145,9 @@ defmodule Styler.Style.ModuleDirectives do defp do_run({{directive, _, children}, _} = zipper, ctx) when directive in @directives and is_list(children) do # Need to be careful that we aren't getting false positives on variables or fns like `def import(foo)` or `alias = 1` case Style.ensure_block_parent(zipper) do - {:ok, zipper} -> - {zipper, comments} = zipper |> Zipper.up() |> organize_directives(ctx.comments) - {:skip, zipper, %{ctx | comments: comments}} - + {:ok, zipper} -> zipper |> Zipper.up() |> organize_directives(ctx) # not actually a directive! carry on. - :error -> - {:cont, zipper, ctx} + :error -> {:cont, zipper, ctx} end end @@ -234,8 +226,9 @@ defmodule Styler.Style.ModuleDirectives do end end - defp organize_directives(parent, comments, moduledoc \\ nil) do + defp organize_directives(parent, ctx, moduledoc \\ nil) do original_children = Zipper.children(parent) + comments = ctx.comments acc = original_children @@ -291,24 +284,20 @@ defmodule Styler.Style.ModuleDirectives do nodes = directives ++ nondirectives - # If we actually changed the ordering (sorted directives, hoisted them above nondirectives, expanded/added/removed - # one), re-lay the whole block via `order_line_meta_and_comments`. It moves each node *and the comments attached to - # it* together, so a directive's comment follows it when it changes position. When nothing moved (e.g. a config file - # whose `import Config` is already on top) we leave every line alone - other styles like Configs rely on that, and - # reflowing would only risk disturbing comments we have no reason to touch. - {nodes, comments} = - if nodes != [] and Style.without_meta(nodes) != Style.without_meta(original_children) do - first_line = nodes |> Enum.map(&Style.meta(&1)[:line]) |> Enum.min() - Style.order_line_meta_and_comments(nodes, comments, first_line) + {{directives, nondirectives}, comments} = + # not happy with the `without_meta` comparison but not sure what can be done. + if Enum.empty?(nodes) or Style.without_meta(nodes) == Style.without_meta(original_children) do + {{directives, nondirectives}, comments} else - {nodes, comments} + first_line = nodes |> Enum.map(&Style.meta(&1)[:line]) |> Enum.min() + {nodes, comments} = Style.order_line_meta_and_comments(nodes, comments, first_line) + # This is shameful. I'm sorry + {Enum.split(nodes, length(directives)), comments} end - {directives, nondirectives} = Enum.split(nodes, length(directives)) - # the # of aliases can be decreased during sorting - if there were any, we need to be sure to write the deletion zipper = - if directives == [] do + if Enum.empty?(directives) do Zipper.replace_children(parent, nondirectives) else # this ensures we continue the traversal _after_ any directives @@ -319,7 +308,7 @@ defmodule Styler.Style.ModuleDirectives do |> Zipper.insert_siblings(nondirectives) end - {zipper, comments} + {:skip, zipper, %{ctx | comments: comments}} end # alias_env have to be recomputed after we've sorted our `alias` nodes From 693803be9e60ad32f0b36de44f9c1754c644bb82 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Fri, 24 Jul 2026 12:42:21 -0700 Subject: [PATCH 13/16] humanize pipes --- .gitignore | 1 - lib/style/pipes.ex | 3 --- 2 files changed, 4 deletions(-) diff --git a/.gitignore b/.gitignore index 742eff19..d0b2773e 100644 --- a/.gitignore +++ b/.gitignore @@ -24,4 +24,3 @@ formatter-*.tar # Temporary files, for example, from tests. /tmp/ -.tokensave diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index c4915f8d..ad908d28 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -110,10 +110,8 @@ defmodule Styler.Style.Pipes do # 3 |> rhs(...args) # => # 1 var = rhs(lhs, ...args) - # everything collapses onto the `=` line, so hoist any interleaved comments above it comments = Style.displace_comments(ctx.comments, vm[:line]..Style.max_line(rhs)) oneline_assignment = Style.set_line({:=, am, [var, {fun, rhs_meta, [lhs | args]}]}, vm[:line]) - # skip so we don't re-traverse {:cont, Zipper.replace(assignment_parent, oneline_assignment), %{ctx | comments: comments}} _ -> @@ -121,7 +119,6 @@ defmodule Styler.Style.Pipes do # |> rhs(...args) # => # rhs(lhs, ...) - # everything collapses onto lhs_line, so hoist any interleaved comments above the call comments = Style.displace_comments(ctx.comments, lhs_line..Style.max_line(rhs)) oneline_function_call = Style.set_line({fun, rhs_meta, [lhs | args]}, lhs_line) {:cont, Zipper.replace(single_pipe_zipper, oneline_function_call), %{ctx | comments: comments}} From 7aa025554495da019eaa844dab28e08f64f5baf5 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Fri, 24 Jul 2026 12:44:52 -0700 Subject: [PATCH 14/16] humanize configs test names --- test/style/configs_test.exs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/style/configs_test.exs b/test/style/configs_test.exs index b439a7a7..1f4340be 100644 --- a/test/style/configs_test.exs +++ b/test/style/configs_test.exs @@ -352,7 +352,7 @@ defmodule Styler.Style.ConfigsTest do ) end - test "big block regression #230" do + test "big block regression" do # The nodes are in reverse order assert_style( """ @@ -390,7 +390,7 @@ defmodule Styler.Style.ConfigsTest do ) end - test "phx config" do + test "phx.new config.exs" do assert_style( """ import Config @@ -431,7 +431,7 @@ defmodule Styler.Style.ConfigsTest do end end - test "issue #244: phx.new config.exs comments survive sorting" do + test "phx.new config.exs" do assert_style( """ # This file is responsible for configuring your application @@ -526,7 +526,7 @@ defmodule Styler.Style.ConfigsTest do ) end - test "issue #244: phx.new prod.exs trailing/dangling comments survive sorting" do + test "phx.new prod.exs" do assert_style( """ import Config From bfa219787d705e51ebf8f952d6b671c2c32ec808 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Fri, 24 Jul 2026 13:04:18 -0700 Subject: [PATCH 15/16] nix duplicative tests --- test/style/blocks_test.exs | 204 ------------------------------------- 1 file changed, 204 deletions(-) diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index b4ea8605..1b3b62ac 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -313,30 +313,6 @@ defmodule Styler.Style.BlocksTest do ) end - test "block swapping comments - comments modify the bodies" do - assert_style( - """ - case foo do - false -> - # a - :error - true -> - # b - :ok - end - """, - """ - if foo do - # b - :ok - else - # a - :error - end - """ - ) - end - test "complex comments" do assert_style( """ @@ -1283,58 +1259,6 @@ defmodule Styler.Style.BlocksTest do ) end end - - test "comments and flips" do - assert_style( - """ - if !a do - # b - b - else - # c - c - end - """, - """ - if a do - # c - c - else - # b - b - end - """ - ) - end - end - - describe "comment movement regressions" do - test "comments survive swap when do body is multiple lines (negated if)" do - assert_style( - """ - if !a do - # b1 - b1 - # b2 - b2 - else - # c - c - end - """, - """ - if a do - # c - c - else - # b1 - b1 - # b2 - b2 - end - """ - ) - end end describe "block swap comment coverage" do @@ -1423,68 +1347,6 @@ defmodule Styler.Style.BlocksTest do ) end - test "1-statement do body swaps above a 3-statement else" do - assert_style( - """ - if !a do - # p - p - else - # q1 - q1 - # q2 - q2 - # q3 - q3 - end - """, - """ - if a do - # q1 - q1 - # q2 - q2 - # q3 - q3 - else - # p - p - end - """ - ) - end - - test "equal-size (2 statement) do/else bodies swap" do - assert_style( - """ - if !a do - # m1 - m1 - # m2 - m2 - else - # n1 - n1 - # n2 - n2 - end - """, - """ - if a do - # n1 - n1 - # n2 - n2 - else - # m1 - m1 - # m2 - m2 - end - """ - ) - end - test "dangling comment at the end of a swapped do body" do assert_style( """ @@ -1623,38 +1485,6 @@ defmodule Styler.Style.BlocksTest do ) end - test "case to if with equal-size (2 statement) do/else bodies (false first)" do - assert_style( - """ - case foo do - false -> - # d1 - d1 - # d2 - d2 - true -> - # e1 - e1 - # e2 - e2 - end - """, - """ - if foo do - # e1 - e1 - # e2 - e2 - else - # d1 - d1 - # d2 - d2 - end - """ - ) - end - test "cond to if with multi-line do body and 1-statement else" do assert_style( """ @@ -1682,9 +1512,7 @@ defmodule Styler.Style.BlocksTest do """ ) end - end - describe "block swap comment coverage - suspected gaps" do test "blank line breaks natural adjacency to the next clause" do assert_style( """ @@ -1902,38 +1730,6 @@ defmodule Styler.Style.BlocksTest do ) end - test "another" do - assert_style( - """ - case foo do - # a - false -> - # foo - d1 - # b - true -> - # bar - e - f - # dangling - end - """, - """ - if foo do - # b - # bar - e - f - # dangling - else - # a - # foo - d1 - end - """ - ) - end - test "yet another" do assert_style( """ From 8e7b67aa80d56e4bdd74eb1311ec6681e9d80a79 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Fri, 24 Jul 2026 13:09:31 -0700 Subject: [PATCH 16/16] format and nix another test --- lib/style.ex | 1 - lib/style/blocks.ex | 15 ++++++++++----- lib/style/module_directives.ex | 2 +- test/style/blocks_test.exs | 22 ---------------------- 4 files changed, 11 insertions(+), 29 deletions(-) diff --git a/lib/style.ex b/lib/style.ex index 9a6d0d1d..4d9264c9 100644 --- a/lib/style.ex +++ b/lib/style.ex @@ -242,7 +242,6 @@ defmodule Styler.Style do here, comments_for_lines(comments, 4, 6) is "a", "b", "c", "d" """ def comments_for_lines(comments, start_line, last_line) do - if !start_line, do: raise "nil" comments |> Enum.reverse() |> comments_for_lines(start_line, last_line, [], []) end diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index b3a20588..63317454 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -98,8 +98,11 @@ defmodule Styler.Style.Blocks do case List.update_at(clauses, -1, rewrite_literal_to_true) do # Credo.Check.Refactor.CondStatements - [{:->, _, [[head], _]} = a, {:->, _, [[{:__block__, _, [true]}], _]} = b] -> arrows_to_if(zipper, head, a, b, m[:end][:line], ctx) - clauses -> {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} + [{:->, _, [[head], _]} = a, {:->, _, [[{:__block__, _, [true]}], _]} = b] -> + arrows_to_if(zipper, head, a, b, m[:end][:line], ctx) + + clauses -> + {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} end end @@ -193,7 +196,9 @@ defmodule Styler.Style.Blocks do # if !x, do: y, else: z => if x, do: z, else: y [negator, [{do_, do_body}, {else_, else_body}]] when is_negator(negator) -> # end of expression hack ensure that the else body keeps dangling comments its block - do_body = Macro.update_meta(do_body, &Keyword.put(&1, :end_of_expression, [line: Style.meta(else_)[:line], newlines: 1])) + else_line = Style.meta(else_)[:line] + do_body = Macro.update_meta(do_body, &Keyword.put(&1, :end_of_expression, line: else_line, newlines: 1)) + zipper |> Zipper.replace({:if, m, [invert(negator), [{do_, else_body}, {else_, do_body}]]}) |> run(ctx) # drop `else end` @@ -381,10 +386,10 @@ defmodule Styler.Style.Blocks do # hacking the end_of_expression helps ensure that the (previously) last clause catches dangling comments [a, b] = if Style.first_line(a) < Style.first_line(b) do - b = Macro.update_meta(b, &Keyword.put(&1, :end_of_expression, [line: end_line, newlines: 1])) + b = Macro.update_meta(b, &Keyword.put(&1, :end_of_expression, line: end_line, newlines: 1)) [a, b] else - a = Macro.update_meta(a, &Keyword.put(&1, :end_of_expression, [line: end_line, newlines: 1])) + a = Macro.update_meta(a, &Keyword.put(&1, :end_of_expression, line: end_line, newlines: 1)) [a, b] end diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index b0a9d627..ad44b86b 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -284,8 +284,8 @@ defmodule Styler.Style.ModuleDirectives do nodes = directives ++ nondirectives + # have to compare without meta due to newlines being reset while grouping directives {{directives, nondirectives}, comments} = - # not happy with the `without_meta` comparison but not sure what can be done. if Enum.empty?(nodes) or Style.without_meta(nodes) == Style.without_meta(original_children) do {{directives, nondirectives}, comments} else diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 1b3b62ac..bf8b8c9f 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -1513,28 +1513,6 @@ defmodule Styler.Style.BlocksTest do ) end - test "blank line breaks natural adjacency to the next clause" do - assert_style( - """ - if !a do - b - # dangling - - else - c - end - """, - """ - if a do - c - else - b - # dangling - end - """ - ) - end - test "multi-line leading comment block on a clause header" do assert_style( """