From 776a88150784430dde6cc7925e73c3ac6dd1d386 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Mon, 18 Mar 2024 12:27:00 -0600 Subject: [PATCH 1/2] Add configuration for additional modules to exclude for alias lifting. Closes #139 --- lib/style/module_directives.ex | 12 +++++------ lib/styler.ex | 11 ++++++---- .../module_directives/alias_lifting_test.exs | 12 +++++++++++ test/support/style_case.ex | 20 ++++++++++--------- 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index b09a9c22..1af88c4e 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -102,7 +102,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__, _, [_, _ | _]} -> - {:skip, organize_directives(body_zipper, moduledoc), ctx} + {:skip, organize_directives(body_zipper, ctx, moduledoc), ctx} # 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 @@ -128,7 +128,7 @@ defmodule Styler.Style.ModuleDirectives do def 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} -> {:skip, zipper |> Zipper.up() |> organize_directives(ctx), ctx} # not actually a directive! carry on. :error -> {:cont, zipper, ctx} end @@ -173,7 +173,7 @@ defmodule Styler.Style.ModuleDirectives do # a dynamic module name, like `defmodule my_variable do ... end` defp moduledoc(_), do: nil - defp organize_directives(parent, moduledoc \\ nil) do + defp organize_directives(parent, ctx, moduledoc \\ nil) do {directives, nondirectives} = parent |> Zipper.children() @@ -201,7 +201,7 @@ defmodule Styler.Style.ModuleDirectives do aliases = expand_and_sort(directives[:alias] || []) requires = expand_and_sort(directives[:require] || []) - {aliases, requires, nondirectives} = lift_aliases(aliases, requires, nondirectives) + {aliases, requires, nondirectives} = lift_aliases(aliases, requires, nondirectives, ctx.config.lift_exclude) directives = [ @@ -229,9 +229,9 @@ defmodule Styler.Style.ModuleDirectives do end end - defp lift_aliases(aliases, requires, nondirectives) do + defp lift_aliases(aliases, requires, nondirectives, excluded_by_config) do excluded = - Enum.reduce(aliases, @stdlib, fn + Enum.reduce(aliases, MapSet.union(@stdlib, excluded_by_config), fn {:alias, _, [{:__aliases__, _, aliases}]}, excluded -> MapSet.put(excluded, List.last(aliases)) {:alias, _, [{:__aliases__, _, _}, [{_as, {:__aliases__, _, [as]}}]]}, excluded -> MapSet.put(excluded, as) # `alias __MODULE__` or other oddities diff --git a/lib/styler.ex b/lib/styler.ex index b04f4a34..034b9f4b 100644 --- a/lib/styler.ex +++ b/lib/styler.ex @@ -31,7 +31,9 @@ defmodule Styler do def style({ast, comments}, file, opts) do on_error = opts[:on_error] || :log zipper = Zipper.zip(ast) - context = %{comments: comments, file: file} + excludes = MapSet.new(List.wrap(opts[:alias_lifting_exclude])) + + context = %{comments: comments, file: file, config: %{lift_exclude: excludes}} {{ast, _}, %{comments: comments}} = Enum.reduce(@styles, {zipper, context}, fn style, {zipper, context} -> @@ -58,13 +60,14 @@ defmodule Styler do def features(_opts), do: [sigils: [], extensions: [".ex", ".exs"]] @impl Format - def format(input, formatter_opts, opts \\ []) do + def format(input, formatter_opts) do file = formatter_opts[:file] + styler_opts = formatter_opts[:styler] || [] {ast, comments} = input |> string_to_quoted_with_comments(to_string(file)) - |> style(file, opts) + |> style(file, styler_opts) quoted_to_string(ast, comments, formatter_opts) end @@ -81,7 +84,7 @@ defmodule Styler do end @doc false - def literal_encoder(a, b), do: {:ok, {:__block__, b, [a]}} + def literal_encoder(literal, meta), do: {:ok, {:__block__, meta, [literal]}} @doc false # Turns an ast and comments back into code, formatting it along the way. diff --git a/test/style/module_directives/alias_lifting_test.exs b/test/style/module_directives/alias_lifting_test.exs index d5c27880..c7ac7fc8 100644 --- a/test/style/module_directives/alias_lifting_test.exs +++ b/test/style/module_directives/alias_lifting_test.exs @@ -179,6 +179,18 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do end describe "it doesn't lift" do + test "collisions with configured modules" do + assert_style( + """ + alias Foo.Bar + + A.B.C + A.B.C + """, + alias_lifting_exclude: ~w(C)a + ) + end + test "collisions with std lib" do assert_style """ defmodule DontYouDare do diff --git a/test/support/style_case.ex b/test/support/style_case.ex index ef52deb3..ab6e2ea7 100644 --- a/test/support/style_case.ex +++ b/test/support/style_case.ex @@ -16,18 +16,16 @@ defmodule Styler.StyleCase do using do quote do - import unquote(__MODULE__), only: [assert_style: 1, assert_style: 2, style: 1] + import unquote(__MODULE__), only: [assert_style: 1, assert_style: 2, style: 1, style: 2] end end - defmacro assert_style(before, expected \\ nil) do - expected = expected || before - - quote bind_quoted: [before: before, expected: expected] do + defmacro assert_style(before, expected, options) do + quote bind_quoted: [before: before, expected: expected, options: options] do alias Styler.Zipper expected = String.trim(expected) - {styled_ast, styled, styled_comments} = style(before) + {styled_ast, styled, styled_comments} = style(before, options) if styled != expected and ExUnit.configuration()[:trace] do IO.puts("\n======Given=============\n") @@ -89,7 +87,7 @@ defmodule Styler.StyleCase do end) assert expected == styled - {_, restyled, _} = style(styled) + {_, restyled, _} = style(styled, options) assert restyled == styled, """ expected styling to be idempotent, but a second pass resulted in more changes. @@ -107,9 +105,13 @@ defmodule Styler.StyleCase do end end - def style(code) do + def assert_style(no_change), do: assert_style(no_change, no_change, []) + def assert_style(before, expected) when is_binary(expected), do: assert_style(before, expected, []) + def assert_style(no_change, opts) when is_list(opts), do: assert_style(no_change, no_change, opts) + + def style(code, options \\ []) do {ast, comments} = Styler.string_to_quoted_with_comments(code) - {styled_ast, comments} = Styler.style({ast, comments}, "testfile", on_error: :raise) + {styled_ast, comments} = Styler.style({ast, comments}, "testfile", [{:on_error, :raise} | options]) try do styled_code = styled_ast |> Styler.quoted_to_string(comments) |> String.trim_trailing("\n") From 8a53b51960a62997f1a4dd7fcbbb552ef97a62cd Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Tue, 19 Mar 2024 10:51:16 -0600 Subject: [PATCH 2/2] this time with persistent_term --- lib/style/module_directives.ex | 19 +++----- lib/styler.ex | 5 +- lib/styler/config.ex | 46 +++++++++++++++++++ .../module_directives/alias_lifting_test.exs | 17 +++---- test/support/style_case.ex | 24 +++++----- 5 files changed, 76 insertions(+), 35 deletions(-) create mode 100644 lib/styler/config.ex diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index 1af88c4e..5fd9585e 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -74,13 +74,6 @@ defmodule Styler.Style.ModuleDirectives do @attr_directives ~w(moduledoc shortdoc behaviour)a ++ @callback_attrs @defstruct ~w(schema embedded_schema defstruct)a - @stdlib MapSet.new(~w( - Access Agent Application Atom Base Behaviour Bitwise Code Date DateTime Dict Ecto Enum Exception - File Float GenEvent GenServer HashDict HashSet Integer IO Kernel Keyword List - Macro Map MapSet Module NaiveDateTime Node Oban OptionParser Path Port Process Protocol - Range Record Regex Registry Set Stream String StringIO Supervisor System Task Time Tuple URI Version - )a) - @moduledoc_false {:@, [line: nil], [{:moduledoc, [line: nil], [{:__block__, [line: nil], [false]}]}]} def run({{:defmodule, _, children}, _} = zipper, ctx) do @@ -102,7 +95,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__, _, [_, _ | _]} -> - {:skip, organize_directives(body_zipper, ctx, moduledoc), ctx} + {:skip, organize_directives(body_zipper, moduledoc), ctx} # 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 @@ -128,7 +121,7 @@ defmodule Styler.Style.ModuleDirectives do def 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), ctx} + {:ok, zipper} -> {:skip, zipper |> Zipper.up() |> organize_directives(), ctx} # not actually a directive! carry on. :error -> {:cont, zipper, ctx} end @@ -173,7 +166,7 @@ defmodule Styler.Style.ModuleDirectives do # a dynamic module name, like `defmodule my_variable do ... end` defp moduledoc(_), do: nil - defp organize_directives(parent, ctx, moduledoc \\ nil) do + defp organize_directives(parent, moduledoc \\ nil) do {directives, nondirectives} = parent |> Zipper.children() @@ -201,7 +194,7 @@ defmodule Styler.Style.ModuleDirectives do aliases = expand_and_sort(directives[:alias] || []) requires = expand_and_sort(directives[:require] || []) - {aliases, requires, nondirectives} = lift_aliases(aliases, requires, nondirectives, ctx.config.lift_exclude) + {aliases, requires, nondirectives} = lift_aliases(aliases, requires, nondirectives) directives = [ @@ -229,9 +222,9 @@ defmodule Styler.Style.ModuleDirectives do end end - defp lift_aliases(aliases, requires, nondirectives, excluded_by_config) do + defp lift_aliases(aliases, requires, nondirectives) do excluded = - Enum.reduce(aliases, MapSet.union(@stdlib, excluded_by_config), fn + Enum.reduce(aliases, Styler.Config.get(:lifting_excludes), fn {:alias, _, [{:__aliases__, _, aliases}]}, excluded -> MapSet.put(excluded, List.last(aliases)) {:alias, _, [{:__aliases__, _, _}, [{_as, {:__aliases__, _, [as]}}]]}, excluded -> MapSet.put(excluded, as) # `alias __MODULE__` or other oddities diff --git a/lib/styler.ex b/lib/styler.ex index 034b9f4b..7bfb104e 100644 --- a/lib/styler.ex +++ b/lib/styler.ex @@ -30,10 +30,9 @@ defmodule Styler do @doc false def style({ast, comments}, file, opts) do on_error = opts[:on_error] || :log + Styler.Config.set(opts) + context = %{comments: comments, file: file} zipper = Zipper.zip(ast) - excludes = MapSet.new(List.wrap(opts[:alias_lifting_exclude])) - - context = %{comments: comments, file: file, config: %{lift_exclude: excludes}} {{ast, _}, %{comments: comments}} = Enum.reduce(@styles, {zipper, context}, fn style, {zipper, context} -> diff --git a/lib/styler/config.ex b/lib/styler/config.ex new file mode 100644 index 00000000..c2aa4fab --- /dev/null +++ b/lib/styler/config.ex @@ -0,0 +1,46 @@ +# Copyright 2024 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + +defmodule Styler.Config do + @moduledoc false + @key __MODULE__ + + @stdlib MapSet.new(~w( + Access Agent Application Atom Base Behaviour Bitwise Code Date DateTime Dict Ecto Enum Exception + File Float GenEvent GenServer HashDict HashSet Integer IO Kernel Keyword List + Macro Map MapSet Module NaiveDateTime Node Oban OptionParser Path Port Process Protocol + Range Record Regex Registry Set Stream String StringIO Supervisor System Task Time Tuple URI Version + )a) + + def set(config) do + :persistent_term.get(@key) + :ok + rescue + ArgumentError -> set!(config) + end + + def set!(config) do + excludes = + config[:alias_lifting_exclude] + |> List.wrap() + |> MapSet.new() + |> MapSet.union(@stdlib) + + :persistent_term.put(@key, %{ + lifting_excludes: excludes + }) + end + + def get(key) do + @key + |> :persistent_term.get() + |> Map.fetch!(key) + end +end diff --git a/test/style/module_directives/alias_lifting_test.exs b/test/style/module_directives/alias_lifting_test.exs index c7ac7fc8..ce88d27e 100644 --- a/test/style/module_directives/alias_lifting_test.exs +++ b/test/style/module_directives/alias_lifting_test.exs @@ -180,15 +180,16 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do describe "it doesn't lift" do test "collisions with configured modules" do - assert_style( - """ - alias Foo.Bar + Styler.Config.set!(alias_lifting_exclude: ~w(C)a) - A.B.C - A.B.C - """, - alias_lifting_exclude: ~w(C)a - ) + assert_style """ + alias Foo.Bar + + A.B.C + A.B.C + """ + + Styler.Config.set!([]) end test "collisions with std lib" do diff --git a/test/support/style_case.ex b/test/support/style_case.ex index ab6e2ea7..79ac23d0 100644 --- a/test/support/style_case.ex +++ b/test/support/style_case.ex @@ -16,16 +16,22 @@ defmodule Styler.StyleCase do using do quote do - import unquote(__MODULE__), only: [assert_style: 1, assert_style: 2, style: 1, style: 2] + import unquote(__MODULE__), only: [assert_style: 1, assert_style: 2, style: 1] end end - defmacro assert_style(before, expected, options) do - quote bind_quoted: [before: before, expected: expected, options: options] do + setup_all do + Styler.Config.set([]) + end + + defmacro assert_style(before, expected \\ nil) do + expected = expected || before + + quote bind_quoted: [before: before, expected: expected] do alias Styler.Zipper expected = String.trim(expected) - {styled_ast, styled, styled_comments} = style(before, options) + {styled_ast, styled, styled_comments} = style(before) if styled != expected and ExUnit.configuration()[:trace] do IO.puts("\n======Given=============\n") @@ -87,7 +93,7 @@ defmodule Styler.StyleCase do end) assert expected == styled - {_, restyled, _} = style(styled, options) + {_, restyled, _} = style(styled) assert restyled == styled, """ expected styling to be idempotent, but a second pass resulted in more changes. @@ -105,13 +111,9 @@ defmodule Styler.StyleCase do end end - def assert_style(no_change), do: assert_style(no_change, no_change, []) - def assert_style(before, expected) when is_binary(expected), do: assert_style(before, expected, []) - def assert_style(no_change, opts) when is_list(opts), do: assert_style(no_change, no_change, opts) - - def style(code, options \\ []) do + def style(code) do {ast, comments} = Styler.string_to_quoted_with_comments(code) - {styled_ast, comments} = Styler.style({ast, comments}, "testfile", [{:on_error, :raise} | options]) + {styled_ast, comments} = Styler.style({ast, comments}, "testfile", on_error: :raise) try do styled_code = styled_ast |> Styler.quoted_to_string(comments) |> String.trim_trailing("\n")