lifting from within user-defined macros can break compilation

[novaugust]

Versions

currently on main, but 0.12 when it gets cut

Example Input

lifting is smart enough to not interact with quote children, but user-defined macros can hide that that's occurring.

defmodule Foo do
  @moduledoc false

  my_quote do
    alias AliasFor.In.MyQuote

    ... Foo.Bar.Baz... 
    ... Foo.Bar.Baz ...
  end
end

results in:

defmodule Foo do
  @moduledoc false
  alias Foo.Bar.Baz

  my_quote do
    alias AliasFor.In.MyQuote

    ... Baz... 
    ... Baz ...
  end
end

but as with quote, the compiler will warn that Baz is unused within Foo, and anything that uses my_quote will then error on Baz being undefined

as-is, the user will have to put the alias where it belongs inside my_quote, which styler is just fine with and will leave be. still, i'd like styler to not leave a codebase uncompilable, even if it just requires a simple human fix

Fix

I see two possible

1. never go into do blocks of unknown parents looking for liftable aliases (allowlist all kernel forms and nothing else)
2. when a do-block contains a module directive, assume that it's equivalent to a quote block and back out of it

there are pros and cons to each, but 2 probably gives the more correct result, with the downside being a more complex look-ahead implementation. actually, the real win might be doing a combination of both: when encounter directives in a nested scope, figure out if it's a known block parent. if it is, carry on. if it's not, assume this works as a quote does and don't lift

[maennchen]

The same applies to defmodule inside of ExUnit tests.

Example:

defmodule Oidcc.Plug.UtilsTest do
  use ExUnit.Case, async: false

  describe "get_refresh_jwks_fun/1" do
    test "returns client_store.refresh_jwks function when implemented" do
      defmodule ClientStoreWithRefresh do
        @behaviour Oidcc.Plug.ClientStore

        @impl Oidcc.Plug.ClientStore
        def get_client_context(_conn), do: {:ok, %{}}

        @impl Oidcc.Plug.ClientStore
        def refresh_jwks(arg), do: {:refreshed, arg}
      end
    end
  end
end

Lifting the Oidcc.Plug.ClientStore will result in an unused alias message and the wrong module name being used.

Full file: https://github.com/erlef/oidcc_plug/blob/main/test/oidcc/plug/utils_test.exs

[novaugust]

thanks for the report @maennchen.

i'm surprised to learn that there's an unused alias warning in that situation - i thought inner-modules had access to their parents' context. guess i'm learning something new about elixir

trying to create a minimal repro, i have this:

defmodule A.B.C do
  @callback foo() :: :ok
end

defmodule A do
  alias A.B.C

  defmodule B do
    @behaviour C
    @impl C
    def foo, do: :ok
  end
end

which i'd think should trigger the same warning, but doesn't. so i'm willing to bet that the warning you're getting is a consequence of the nested modules in an exunit testing script. (made an issue w/ elixir here)

there's two things you can do here:

1. add that alias to the alias_lifting_excludes configuration as a one-off fix for this unique situation
2. un-nest your modules so whatever weird compilation steps are happening in exunit stop generating warnings about it

i did 2 in this pr

styler could be updated to not lift aliases out of nested modules, but given that that works in normal situations i'd rather leave the behaviour in.

[maennchen]

Just a little FYI for some context

defmodule in tests are not uncommon. Even elixir does it: https://github.com/elixir-lang/elixir/blob/32f7646eb4d61d98df9dd9a929e5cf34e12fb33a/lib/iex/test/iex/interaction_test.exs#L263-L267

[novaugust]

from the issue w/ elixir, you can do alias A.B.C, warn: false if you like having your defmodules in your tests

defmodule in tests are not uncommon. Even elixir does it

for sure! there are lots of common patterns styler isn't compatible with; particularly patterns libraries engage with