Warn about name conflicts in nested modules #12451

whatyouhide · 2023-03-06T14:22:30Z

I’m not sure what to do for this particular warning in this particular test, since we're testing exactly the behaviour that warns here 😕

warning: the nested call to "defmodule First" will not implicitly alias First here because there is already an alias Some.First that resolves to the same alias. To fix this, either remove the Some.First alias, or define the nested module outside of the parent
  test/elixir/kernel/alias_test.exs:132: Macro.AliasTest.User (module)

Closes #11130.

lib/elixir/test/elixir/kernel_test.exs

josevalim · 2023-03-06T14:29:32Z

I’m not sure what to do for this particular warning in this particular test, since we're testing exactly the behaviour that warns here 😕

Move whatever that module is checking into the same test that checks for the warning :)

whatyouhide · 2023-03-07T01:03:34Z

@josevalim done and done ✅

josevalim · 2023-03-07T08:53:31Z

lib/elixir/test/elixir/kernel/alias_test.exs

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

-defmodule Macro.AliasTest.User do
+defmodule Macro.AliasTest do


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

josevalim · 2023-03-07T09:00:50Z

lib/elixir/lib/kernel.ex

+          "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",


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

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.

josevalim

Minor nits and ship it!

lukaszsamson · 2023-03-08T06:19:16Z

Either I fail to understand what's the real issue here or those error messages are not correct.
Tested in iex on master
Code without conflicts:

defmodule Parent do
        alias OtherParent.OtherChild
        def a1, do: IO.puts "Parent " <> inspect(__ENV__.aliases)
        defmodule Child do
          def a2, do: IO.puts "Parent.Child " <> inspect(__ENV__.aliases)
        end
        def a3, do: IO.puts "Parent " <> inspect(__ENV__.aliases)
end

gives

iex(2)> Parent.a1                                                                
Parent [{OtherChild, OtherParent.OtherChild}]
:ok
iex(3)> Parent.Child.a2
Parent.Child [{OtherChild, OtherParent.OtherChild}, {Child, Parent.Child}]
:ok
iex(4)> Parent.a3      
Parent [{OtherChild, OtherParent.OtherChild}, {Child, Parent.Child}]
:ok

and code with conflicts

defmodule Parent do
        alias OtherParent.Child
        def a1, do: IO.puts "Parent " <> inspect(__ENV__.aliases)
        defmodule Child do
          def a2, do: IO.puts "Parent.Child " <> inspect(__ENV__.aliases)
        end
        def a3, do: IO.puts "Parent " <> inspect(__ENV__.aliases)
end

gives

iex(2)> Parent.a1
Parent [{Child, OtherParent.Child}]
:ok
iex(3)> Parent.Child.a2
Parent.Child [{Child, Parent.Child}]
:ok
iex(4)> Parent.a3      
Parent [{Child, Parent.Child}]
:ok

I'm seeing that the implicit alias Parent.Child, as: Child is set both in the submodule and in the parent module. The only difference is that alias OtherParent.Child, as: Child gets overwritten.

Regarding the messages, it's not clear where this implicit alias is not being set. Is it in the parent module body or in the child module body or both

josevalim · 2023-03-08T09:41:10Z

The only difference is that alias OtherParent.Child, as: Child gets overwritten.

The implicit overriding is what we are warning about.

josevalim · 2023-03-08T09:45:17Z

Ah, I see your point, the original report says the alias is not defined, but the alias is always defined and overrides. Which I would say is the correct behaviour! So I think we can go ahead and close this?

Thanks for sanity checking @lukaszsamson!

lukaszsamson · 2023-03-08T09:48:22Z

The original problem from the discussion list was aboutan ecto issue. Maybe ecto macros make some assumptions about aliases that are not fully correct

josevalim · 2023-03-08T09:48:50Z

If we want, we can add a warning that a previously defined alias is being overridden by the module.

module Child.Comment, inside module Post, is redefining the alias Child to point
to Post.Child instead of RootChild. Either remove the alias to RootChild or define
the module outside of its parent

But it feels like it is expected new aliases override old aliases, so I would close it.

josevalim · 2023-03-08T09:52:34Z

Yes, Ecto is showing the behaviour we thought we were seeing in this pull request. But not Elixir.

josevalim · 2023-03-08T09:56:36Z

I will close this. I think the overridde warning is ok. Later aliases always override earlier ones. Sorry @whatyouhide!

lukaszsamson · 2023-03-08T09:58:08Z

But it feels like it is expected new aliases override old aliases

That's easy to understand in case of ordinary aliases. defmodule implicit ones are not so obvious (case in point - I've found another bug in elixir_sense alias handling)

whatyouhide · 2023-03-08T20:59:53Z

@josevalim yeah I think the behaviour is correct per se, but it's hard to understand that the defmodule overrides the alias. So, do we still want this? If yes, should I update the error message?

josevalim · 2023-03-08T21:05:44Z

Yeah, let's give it a try with an updated message. And we can run on Elixir main for a couple weeks before release and see if we catch anything in any of our existing projects.

lukaszsamson · 2023-03-09T06:41:06Z

I wanted to see how this alias overwriting work in case of external submodules and I've found something odd

defmodule Parent do
        alias OtherParent.Child
        def a1, do: IO.puts "Parent " <> inspect(__ENV__.aliases)
        defmodule Elixir.Child do
          def a2, do: IO.puts "Elixir.Child " <> inspect(__ENV__.aliases)
        end
        def a3, do: IO.puts "Parent " <> inspect(__ENV__.aliases)
end

iex(3)> Parent.a1
Parent [{Child, OtherParent.Child}]
:ok
iex(4)> Child.a2
Elixir.Child []
:ok
iex(5)> Parent.a3
Parent []
:ok

So it removes the alias. This does not happen for nested external submodule e.g. Elixir.Child.One

josevalim · 2023-03-09T06:47:19Z

I think that makes sense. Otherwise you wouldn't be able to refer to it as Child in the same module.

lukaszsamson · 2023-03-09T09:52:32Z

I think that makes sense. Otherwise you wouldn't be able to refer to it as Child in the same module.

OK but why there's no similar behavior in this case?

defmodule Parent do
        alias OtherParent.Child
        def a1, do: IO.puts "Parent " <> inspect(__ENV__.aliases)
        defmodule Elixir.Child.Some do
          def a2, do: IO.puts "Elixir.Child.Some " <> inspect(__ENV__.aliases)
        end
        def a3, do: IO.puts "Parent " <> inspect(__ENV__.aliases)
end

iex(2)> Parent.a1
Parent [{Child, OtherParent.Child}]
:ok
iex(3)> Child.Some.a2
Elixir.Child.Some [{Child, OtherParent.Child}]
:ok
iex(3)> Parent.a3
Parent [{Child, OtherParent.Child}]
:ok

A nested external submodule does not unalias Child. I'm not saying it's a bug, just a non obvious edge case.

josevalim · 2023-03-09T09:56:54Z

Ok, we should either do it for both or non. This is a bug.

submodule now properly sets alias in its own scope (introduced in 9de7dd8) submodule overwrites alias external submodule unaliases See discussions in elixir-lang/elixir#12451

whatyouhide · 2023-03-21T07:27:52Z

@josevalim so, do we still want the warning in this PR, or should we close this? 🙃

josevalim · 2023-03-22T12:47:41Z

I think we can close this and we have added a warning to Ecto. Sorry for the false alarm here.

Warn about name conflicts in nested modules

3b3597c

Closes #11130.

josevalim reviewed Mar 6, 2023

View reviewed changes

lib/elixir/test/elixir/kernel_test.exs Outdated Show resolved Hide resolved

Improve tests

4afbd8f

josevalim reviewed Mar 7, 2023

View reviewed changes

josevalim approved these changes Mar 7, 2023

View reviewed changes

whatyouhide added 2 commits March 7, 2023 22:23

Fix message

2e62e8a

FIX

3589107

josevalim closed this Mar 8, 2023

josevalim reopened this Mar 8, 2023

josevalim mentioned this pull request Mar 9, 2023

alias overrriding even with Elixir. prefix #12456

Closed

lukaszsamson mentioned this pull request Mar 9, 2023

Fix submodule implicit alias behavior elixir-lsp/elixir_sense#207

Merged

josevalim closed this Mar 22, 2023

whatyouhide deleted the al/alias-warning branch March 23, 2023 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn about name conflicts in nested modules #12451

Warn about name conflicts in nested modules #12451

whatyouhide commented Mar 6, 2023 •

edited

Loading

josevalim commented Mar 6, 2023

whatyouhide commented Mar 7, 2023

josevalim Mar 7, 2023

josevalim Mar 7, 2023 •

edited

Loading

whatyouhide Mar 8, 2023

josevalim left a comment

lukaszsamson commented Mar 8, 2023

josevalim commented Mar 8, 2023

josevalim commented Mar 8, 2023

lukaszsamson commented Mar 8, 2023

josevalim commented Mar 8, 2023 •

edited

Loading

josevalim commented Mar 8, 2023

josevalim commented Mar 8, 2023

lukaszsamson commented Mar 8, 2023

whatyouhide commented Mar 8, 2023

josevalim commented Mar 8, 2023

lukaszsamson commented Mar 9, 2023

josevalim commented Mar 9, 2023

lukaszsamson commented Mar 9, 2023

josevalim commented Mar 9, 2023

whatyouhide commented Mar 21, 2023

josevalim commented Mar 22, 2023

Warn about name conflicts in nested modules #12451

Warn about name conflicts in nested modules #12451

Conversation

whatyouhide commented Mar 6, 2023 • edited Loading

josevalim commented Mar 6, 2023

whatyouhide commented Mar 7, 2023

josevalim Mar 7, 2023

Choose a reason for hiding this comment

josevalim Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

whatyouhide Mar 8, 2023

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment

lukaszsamson commented Mar 8, 2023

josevalim commented Mar 8, 2023

josevalim commented Mar 8, 2023

lukaszsamson commented Mar 8, 2023

josevalim commented Mar 8, 2023 • edited Loading

josevalim commented Mar 8, 2023

josevalim commented Mar 8, 2023

lukaszsamson commented Mar 8, 2023

whatyouhide commented Mar 8, 2023

josevalim commented Mar 8, 2023

lukaszsamson commented Mar 9, 2023

josevalim commented Mar 9, 2023

lukaszsamson commented Mar 9, 2023

josevalim commented Mar 9, 2023

whatyouhide commented Mar 21, 2023

josevalim commented Mar 22, 2023

whatyouhide commented Mar 6, 2023 •

edited

Loading

josevalim Mar 7, 2023 •

edited

Loading

josevalim commented Mar 8, 2023 •

edited

Loading