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 2 commits
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
59 changes: 39 additions & 20 deletions lib/elixir/test/elixir/kernel/alias_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@ Code.require_file("../test_helper.exs", __DIR__)

alias Kernel.AliasTest.Nested, as: Nested

defmodule Nested do
def value, do: 1
end
Module.create(
Nested,
quote do
def value, do: 1
end,
file: __ENV__.file
)

defmodule Kernel.AliasTest do
use ExUnit.Case, async: true
Expand Down Expand Up @@ -80,20 +84,24 @@ end
defmodule Kernel.AliasNestingTest do
use ExUnit.Case, async: true

require Kernel.AliasNestingGenerator
Kernel.AliasNestingGenerator.create()
test "aliases nesting with previous alias" do
Code.eval_string("""
defmodule Testing do
import ExUnit.Assertions

test "aliases nesting" do
assert Parent.a() == :a
assert Parent.Child.b() == :a
end
require Kernel.AliasNestingGenerator
Kernel.AliasNestingGenerator.create()

defmodule Nested do
def value, do: 2
end
assert Parent.a() == :a
assert Parent.Child.b() == :a

test "aliases nesting with previous alias" do
assert Nested.value() == 2
defmodule Nested do
def value, do: 2
end

assert Nested.value() == 2
end
""")
end
end

Expand Down Expand Up @@ -129,14 +137,25 @@ defmodule Macro.AliasTest.Aliaser do
end
end

defmodule Macro.AliasTest.User do
defmodule Macro.AliasTest do
Copy link
Member

Choose a reason for hiding this comment

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

Is most likely no longer needs to be a separate module, we can fold into the previous one?

use ExUnit.Case, async: true

use Macro.AliasTest.Definer
use Macro.AliasTest.Aliaser

test "has a struct defined from after compile" do
assert is_map(struct(Macro.AliasTest.User.First, []))
assert is_map(struct(Macro.AliasTest.User.Second, []).baz)
ExUnit.CaptureIO.capture_io(:stderr, fn ->
Code.eval_string("""
defmodule Macro.AliasTest.User do
import ExUnit.Assertions
use Macro.AliasTest.Definer
use Macro.AliasTest.Aliaser

def run_test do
assert is_map(struct(Macro.AliasTest.User.First, []))
assert is_map(struct(Macro.AliasTest.User.Second, []).baz)
end
end
""")

apply(Macro.AliasTest.User, :run_test, [])
end)
end
end
40 changes: 40 additions & 0 deletions lib/elixir/test/elixir/kernel/warning_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1954,6 +1954,46 @@ defmodule Kernel.WarningTest do
"extra parentheses on a remote function capture &System.pid()/0 have been deprecated. Please remove the parentheses: &System.pid/0"
end

test "nested defmodule when 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 do
end
end
""")
end)

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

test "nested defmodule with multiple name components when 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, Child])
end

defp assert_compile_error(messages, string) do
captured =
capture_err(fn ->
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