Skip to content

Commit

Permalink
Fix warnings on conflicting callbacks (#13006)
Browse files Browse the repository at this point in the history
* Improve imprecise message

* Remove false positive warnings on conflicting behaviours
  • Loading branch information
sabiwara authored Oct 15, 2023
1 parent 87b5ee0 commit 88ade1d
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 14 deletions.
37 changes: 24 additions & 13 deletions lib/elixir/lib/module/types/behaviour.ex
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ defmodule Module.Types.Behaviour do

context =
case context.callbacks do
%{^callback => {_kind, conflict, _optional?}} ->
%{^callback => [{_kind, conflict, _optional?} | _]} ->
warn(
context,
{:duplicate_behaviour, context.module, behaviour, conflict, kind, callback}
Expand All @@ -79,11 +79,15 @@ defmodule Module.Types.Behaviour do
context
end

put_in(context.callbacks[callback], {kind, behaviour, original in optional_callbacks})
new_callback = {kind, behaviour, original in optional_callbacks}
callbacks = context.callbacks[callback] || []

put_in(context.callbacks[callback], [new_callback | callbacks])
end

defp check_callbacks(context, all_definitions) do
for {callback, {kind, behaviour, optional?}} <- context.callbacks,
for {callback, callbacks} <- context.callbacks,
{kind, behaviour, optional?} <- callbacks,
reduce: context do
context ->
case :lists.keyfind(callback, 1, all_definitions) do
Expand Down Expand Up @@ -186,10 +190,10 @@ defmodule Module.Types.Behaviour do
end

defp behaviour_callbacks_for_impls([fa | tail], behaviour, callbacks) do
case callbacks[fa] do
{_, ^behaviour, _} ->
[{fa, behaviour} | behaviour_callbacks_for_impls(tail, behaviour, callbacks)]

with list when is_list(list) <- callbacks[fa],
true <- Enum.any?(list, &match?({_, ^behaviour, _}, &1)) do
[{fa, behaviour} | behaviour_callbacks_for_impls(tail, behaviour, callbacks)]
else
_ ->
behaviour_callbacks_for_impls(tail, behaviour, callbacks)
end
Expand All @@ -201,8 +205,12 @@ defmodule Module.Types.Behaviour do

defp callbacks_for_impls([fa | tail], callbacks) do
case callbacks[fa] do
{_, behaviour, _} -> [{fa, behaviour} | callbacks_for_impls(tail, callbacks)]
nil -> callbacks_for_impls(tail, callbacks)
list when is_list(list) ->
Enum.map(list, fn {_, behaviour, _} -> {fa, behaviour} end) ++
callbacks_for_impls(tail, callbacks)

nil ->
callbacks_for_impls(tail, callbacks)
end
end

Expand All @@ -214,8 +222,11 @@ defmodule Module.Types.Behaviour do
defp warn_missing_impls(context, impl_contexts, defs) do
for {pair, kind, meta, _clauses} <- defs, kind in [:def, :defmacro], reduce: context do
context ->
with {:ok, {_, behaviour, _}} <- Map.fetch(context.callbacks, pair),
true <- missing_impl_in_context?(meta, behaviour, impl_contexts) do
with {:ok, callbacks} <- Map.fetch(context.callbacks, pair),
{_, behaviour, _} <-
Enum.find(callbacks, fn {_, behaviour, _} ->
missing_impl_in_context?(meta, behaviour, impl_contexts)
end) do
warn(context, {:missing_impl, pair, kind, behaviour},
line: :elixir_utils.get_line(meta)
)
Expand Down Expand Up @@ -302,9 +313,9 @@ defmodule Module.Types.Behaviour do

def format_warning({:duplicate_behaviour, module, behaviour, conflict, kind, callback}) do
[
"conflicting behaviours found. ",
"conflicting behaviours found. Callback ",
format_definition(kind, callback),
" is required by ",
" is defined by both ",
inspect(conflict),
" and ",
inspect(behaviour),
Expand Down
35 changes: 34 additions & 1 deletion lib/elixir/test/elixir/kernel/warning_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1379,7 +1379,7 @@ defmodule Kernel.WarningTest do
assert_warn_eval(
[
"nofile:9: ",
"conflicting behaviours found. function foo/0 is required by Sample1 and Sample2 (in module Sample3)"
"conflicting behaviours found. Callback function foo/0 is defined by both Sample1 and Sample2 (in module Sample3)"
],
"""
defmodule Sample1 do
Expand All @@ -1400,6 +1400,39 @@ defmodule Kernel.WarningTest do
purge([Sample1, Sample2, Sample3])
end

test "conflicting behaviour (but one optional callback)" do
message =
capture_compile("""
defmodule Sample1 do
@callback foo :: term
end
defmodule Sample2 do
@callback foo :: term
@callback bar :: term
@optional_callbacks foo: 0
end
defmodule Sample3 do
@behaviour Sample1
@behaviour Sample2
@impl Sample1
def foo, do: 1
@impl Sample2
def bar, do: 2
end
""")

assert message =~
"conflicting behaviours found. Callback function foo/0 is defined by both Sample1 and Sample2 (in module Sample3)"

refute message =~ "module attribute @impl was not set"
refute message =~ "this behaviour does not specify such callback"
after
purge([Sample1, Sample2, Sample3])
end

test "duplicate behaviour" do
assert_warn_eval(
[
Expand Down

0 comments on commit 88ade1d

Please sign in to comment.