Fix overriding with Elixir. prefixed defmodule

[sabiwara]

Attempt at fixing #12456

@josevalim is this the kind of fix you had in mind?
It might be a bit hacky but fixes it for now, opening the PR to move the discussion forward.

[josevalim]

I think you also want to skip the alias if there is nesting and in other cases, so you would probably end-up with this:

  defp alias_defmodule({:__aliases__, _, [h | t]}, module, %{module: env_module})
       when is_atom(h) and h != :"Elixir" and is_atom(module) and env_module != nil do
    module = :elixir_aliases.concat([env_module, h])
    alias = String.to_atom("Elixir." <> Atom.to_string(h))
    case t do
      [] -> {module, module, alias}
      _ -> {String.to_atom(Enum.join([module | t], ".")), module, alias}
    end
  end

  defp alias_defmodule(_ast, _module, _env) do
    false
  end

This way you can also get rid of the with.

[sabiwara]

Oh you're right in the case of nesting, I misunderstood.

Ignoring in all cases including the defp alias_defmodule(_raw, module, _env) do was my original approach, but it was emitting warnings so I went for something more precise:

I should be able to replace the with by a simple case though.

[sabiwara]

https://github.com/elixir-lang/elixir/blob/main/lib/elixir/test/elixir/module_test.exs#L317-L334

raw is ModuleTest.RawModule or {:__aliases__, [line: 328, column: 25], [{:__MODULE__, [line: 328, column: 15], nil}, :NonAtomAlias]}

[sabiwara]

How about 9c33fef?

[josevalim]

LGTM, I would just return false instead of nil but it is minor. :)

[josevalim]

Actually, nevermind, the current fix is wrong. I can add a test similar to the other warnings that show it:

  test "does not leak alias from Elixir root alias" do
    defmodule Elixir.ModuleTest.ElixirRootAlias do
      def hello, do: :world
    end

    refute __ENV__.aliases[Elixir.ModuleTest]
    refute __ENV__.aliases[Elixir.ElixirRootAlias]
    assert Elixir.ModuleTest.ElixirRootAlias.hello() == :world
  end

[josevalim]

And I think the proper fix is this one (you only need to change alias_defmodule):

  defp alias_defmodule({:__aliases__, _, [h | t]}, module, %{module: env_module})
       when is_atom(h) and h != :"Elixir" and is_atom(module) and env_module != nil do
    module = :elixir_aliases.concat([env_module, h])
    alias = String.to_atom("Elixir." <> Atom.to_string(h))

    case t do
      [] -> {module, module, alias}
      _ -> {String.to_atom(Enum.join([module | t], ".")), module, alias}
    end
  end

  defp alias_defmodule(_ast, module, _env) do
    {module, module, nil}
  end

That's because we are using the alias to define a context module which is used by the compiler to avoid warnings. Perhaps we should stop using the alias like this, but it is how it is right now.

[sabiwara]

I would just return false instead of nil but it is minor. :)

Just for my reference, what would have been the rationale to pick false in this case?
I don't recall seeing tuple | false very frequently except some erlang bifs, so I simply wondered what's the rule of thumb to pick false over nil.

[josevalim]

I find false more intentional. There are some constructs that return nil by default (for example if without else), so you may have accidental nils.

[sabiwara]

Thanks for clarifying!

[josevalim]

I will go ahead and merge this one and merge my new test and implementation after because I want to refactor the bulk of this code. :)

[josevalim]

💚 💙 💜 💛 ❤️

[sabiwara]

I will go ahead and merge this one and merge my new test and implementation after because I want to refactor the bulk of this code. :)

Thanks a lot for stepping in on this one! It was much more involved than I thought and I didn't know aliases well enough to understand it properly 😅

[josevalim]

@sabiwara yeah, it looked deceptively simple. :)