Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn about name conflicts in nested modules #12451

Closed
wants to merge 4 commits into from
Closed

Warn about name conflicts in nested modules #12451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b3597c
Warn about name conflicts in nested modules
whatyouhide Mar 6, 2023
4afbd8f
Improve tests
whatyouhide Mar 7, 2023
2e62e8a
Fix message
whatyouhide Mar 8, 2023
3589107
FIX
whatyouhide Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions lib/elixir/lib/kernel.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4981,6 +4981,28 @@ defmodule Kernel do
module = :elixir_aliases.concat([env.module, h])
alias = String.to_atom("Elixir." <> Atom.to_string(h))

case :lists.keyfind(alias, 1, env.aliases) do
# If the alias is the same as the module being defined, it means we're redefining
# the same nested module. In that case, let's not warn specifically about "shadowed"
# aliases here.
{^alias, ^module} ->
:ok

{^alias, other_parent} ->
defmod_name = Enum.join([h | t], ".")

IO.warn(
"the nested call to \"defmodule #{defmod_name}\" will not implicitly alias " <>
"#{inspect(alias)} here because there is already an alias #{inspect(other_parent)} " <>
"that resolves to the same alias. To fix this, either remove the " <>
"#{inspect(other_parent)} alias, or define the nested module outside of the parent",
Copy link
Member

@josevalim josevalim Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to think about the error message. What do you think about:

you are defining a module named Child.Comment, inside module Post,
but the module will be defined as RootChild.Comment instead of Post.Child.Comment
due to the "alias RootChild, as: Child". You must address this ambiguity
either by removing the alias Child or by defining the module outside of the parent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's not what happens though, because there is technically no ambiguity, is just that we don't set up an implicit alias. I updated the error message, I like it a lot better now. If you're good with it, I'll merge.

env
)

false ->
:ok
end

case t do
[] -> {module, module, alias}
_ -> {String.to_atom(Enum.join([module | t], ".")), module, alias}
Expand Down
42 changes: 42 additions & 0 deletions lib/elixir/test/elixir/kernel_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,48 @@ defmodule KernelTest do
defmodule :"foo\\bar", do: :ok
end
end

test "warns when nested and there is an alias with the same name" do
whatyouhide marked this conversation as resolved.
Show resolved Hide resolved
output =
ExUnit.CaptureIO.capture_io(:stderr, fn ->
Code.eval_string("""
defmodule Parent do
alias OtherParent.Child

defmodule Child do
end
end
""")
end)

assert output =~ ~s(the nested call to "defmodule Child" will not implicitly alias)
after
purge(Parent)
purge(Child)
end

test "warns when nested with multiple name components and there is an alias with the same name" do
output =
ExUnit.CaptureIO.capture_io(:stderr, fn ->
Code.eval_string("""
defmodule Parent do
alias OtherParent.Child

defmodule Child.One do
end

defmodule Child.Two do
end
end
""")
end)

assert output =~
~s(the nested call to "defmodule Child.One" will not implicitly alias Child)
after
purge(Parent)
purge(Child)
end
end

describe "access" do
Expand Down
7 changes: 3 additions & 4 deletions lib/elixir/test/elixir/typespec_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ Code.require_file("test_helper.exs", __DIR__)
# Holds tests for both Kernel.Typespec and Code.Typespec
defmodule TypespecTest do
use ExUnit.Case, async: true
alias TypespecTest.TypespecSample

defstruct [:hello]

Expand Down Expand Up @@ -120,7 +119,7 @@ defmodule TypespecTest do

test "redefined type" do
assert_raise Kernel.TypespecError,
~r"type foo/0 is already defined in .*test/elixir/typespec_test.exs:126",
~r"type foo/0 is already defined in .*test/elixir/typespec_test.exs:125",
fn ->
test_module do
@type foo :: atom
Expand All @@ -129,7 +128,7 @@ defmodule TypespecTest do
end

assert_raise Kernel.TypespecError,
~r"type foo/2 is already defined in .*test/elixir/typespec_test.exs:136",
~r"type foo/2 is already defined in .*test/elixir/typespec_test.exs:135",
fn ->
test_module do
@type foo :: atom
Expand All @@ -139,7 +138,7 @@ defmodule TypespecTest do
end

assert_raise Kernel.TypespecError,
~r"type foo/0 is already defined in .*test/elixir/typespec_test.exs:145",
~r"type foo/0 is already defined in .*test/elixir/typespec_test.exs:144",
fn ->
test_module do
@type foo :: atom
Expand Down