Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 0 additions & 2 deletions lib/elixir/lib/inspect.ex
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,6 @@ defprotocol Inspect do
end

defimpl Inspect, for: Atom do
require Macro

def inspect(atom, opts) do
color_doc(Macro.inspect_atom(:literal, atom), color_key(atom), opts)
end
Expand Down
26 changes: 26 additions & 0 deletions lib/elixir/lib/kernel/lexical_tracker.ex
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ defmodule Kernel.LexicalTracker do
:gen_server.cast(pid, {:add_export, module})
end

@doc false
def add_require(pid, module, meta) when is_atom(module) do
:gen_server.cast(pid, {:add_require, module, meta})
end

@doc false
def add_import(pid, module, fas, meta, warn) when is_atom(module) do
:gen_server.cast(pid, {:add_import, module, fas, meta, warn})
Expand Down Expand Up @@ -119,12 +124,18 @@ defmodule Kernel.LexicalTracker do
:gen_server.call(pid, :unused_aliases, @timeout)
end

@doc false
def collect_unused_requires(pid) do
:gen_server.call(pid, :unused_requires, @timeout)
end

# Callbacks

def init(:ok) do
state = %{
aliases: %{},
imports: %{},
requires: %{},
references: %{},
exports: %{},
cache: %{},
Expand All @@ -150,6 +161,17 @@ defmodule Kernel.LexicalTracker do
{:reply, Enum.sort(imports), state}
end

def handle_call(:unused_requires, _from, state) do
unused_requires =
for {module, meta} <- state.requires,
Map.get(state.references, module) != :compile,
Keyword.get(meta[:opts], :warn, true) != false do
{module, meta}
end

{:reply, Enum.sort(unused_requires), state}
end

def handle_call(:references, _from, state) do
{compile, runtime} = partition(Map.to_list(state.references), [], [])
{:reply, {compile, Map.keys(state.exports), runtime, state.compile_env}, state}
Expand Down Expand Up @@ -245,6 +267,10 @@ defmodule Kernel.LexicalTracker do
{:noreply, put_in(state.imports[module][@warn_key], true)}
end

def handle_cast({:add_require, module, meta}, state) do
{:noreply, put_in(state.requires[module], meta)}
end

@doc false
def handle_info(_msg, state) do
{:noreply, state}
Expand Down
2 changes: 1 addition & 1 deletion lib/elixir/lib/protocol.ex
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,7 @@ defmodule Protocol do
# expression (and therefore most likely a compile-time one).
behaviour =
if is_atom(protocol) do
quote(do: require(unquote(protocol)))
quote(do: require(unquote(protocol), warn: false))
else
quote(do: protocol)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/elixir/src/elixir_import.erl
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import(Meta, Ref, Opts, E, Warn, Trace, InfoCallback) ->
{Functions, Macros, Added} ->
Trace andalso elixir_env:trace({import, Meta, Ref, Opts}, E),
EI = E#{functions := Functions, macros := Macros},
{ok, Added, elixir_aliases:require(Meta, Ref, Opts, EI, Trace)};
{ok, Added, elixir_aliases:require([{from_import, true} | Meta], Ref, Opts, EI, Trace)};
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of from_import we could do [{warn, false} | Opts] instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)


{error, Reason} ->
{error, Reason}
Expand Down
20 changes: 17 additions & 3 deletions lib/elixir/src/elixir_lexical.erl
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ run(#{tracers := Tracers} = E, ExecutionCallback, AfterExecutionCallback) ->
Res ->
warn_unused_aliases(Pid, LexEnv),
warn_unused_imports(Pid, LexEnv),
warn_unused_requires(Pid, LexEnv),
AfterExecutionCallback(LexEnv),
Res
after
Expand All @@ -33,10 +34,16 @@ run(#{tracers := Tracers} = E, ExecutionCallback, AfterExecutionCallback) ->
trace({alias_expansion, _Meta, Lookup, _Result}, #{lexical_tracker := Pid}) ->
?tracker:alias_dispatch(Pid, Lookup),
ok;
trace({require, Meta, Module, _Opts}, #{lexical_tracker := Pid}) ->
trace({require, Meta, Module, Opts}, #{lexical_tracker := Pid, module := CurrentModule}) ->
case lists:keyfind(from_macro, 1, Meta) of
{from_macro, true} -> ?tracker:remote_dispatch(Pid, Module, compile);
_ -> ?tracker:add_export(Pid, Module)
_ ->
?tracker:add_export(Pid, Module),
% Only track explicit requires, not implicit ones from imports
case lists:keyfind(from_import, 1, Meta) of
{from_import, true} -> ok;
_ -> ?tracker:add_require(Pid, Module, [{module, CurrentModule}, {opts, Opts} | Meta])
end
end,
ok;
trace({struct_expansion, _Meta, Module, _Keys}, #{lexical_tracker := Pid}) ->
Expand Down Expand Up @@ -95,6 +102,11 @@ warn_unused_imports(Pid, E) ->
{ModOrMFA, Meta} <- unused_imports_for_module(Module, Imports)],
ok.

warn_unused_requires(Pid, E) ->
[elixir_errors:file_warn(Meta, ?key(E, file), ?MODULE, {unused_require, Module})
|| {Module, Meta} <- ?tracker:collect_unused_requires(Pid)],
ok.

unused_imports_for_module(Module, Imports) ->
case Imports of
#{Module := Meta} -> [{Module, Meta}];
Expand All @@ -111,4 +123,6 @@ format_error({unused_alias, Module}) ->
format_error({unused_import, {Module, Function, Arity}}) ->
io_lib:format("unused import ~ts.~ts/~w", [elixir_aliases:inspect(Module), Function, Arity]);
format_error({unused_import, Module}) ->
io_lib:format("unused import ~ts", [elixir_aliases:inspect(Module)]).
io_lib:format("unused import ~ts", [elixir_aliases:inspect(Module)]);
format_error({unused_require, Module}) ->
io_lib:format("unused require ~ts", [elixir_aliases:inspect(Module)]).
2 changes: 1 addition & 1 deletion lib/elixir/test/elixir/code_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ defmodule CodeTest do
end

test "with defguard" do
require Integer
require Integer, warn: false
env = Code.env_for_eval(__ENV__)
quoted = quote do: Integer.is_even(1)
{false, binding, env} = Code.eval_quoted_with_env(quoted, [], env, prune_binding: true)
Expand Down
47 changes: 47 additions & 0 deletions lib/elixir/test/elixir/kernel/lexical_tracker_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,53 @@ defmodule Kernel.LexicalTrackerTest do
assert D.collect_unused_imports(config[:pid]) == [{String, %{}}]
end

test "unused requires", config do
D.add_require(config[:pid], String, module: TestModule, opts: [])
assert D.collect_unused_requires(config[:pid]) == [{String, [module: TestModule, opts: []]}]
end

test "used requires are not unused", config do
D.add_require(config[:pid], String, module: TestModule, opts: [])
D.remote_dispatch(config[:pid], String, :compile)
assert D.collect_unused_requires(config[:pid]) == []
end

test "requires with warn: false are not unused", config do
D.add_require(config[:pid], String, module: TestModule, opts: [warn: false])
assert D.collect_unused_requires(config[:pid]) == []
end

test "requires with warn: true are unused when not referenced", config do
D.add_require(config[:pid], String, module: TestModule, opts: [warn: true])

assert D.collect_unused_requires(config[:pid]) == [
{String, [module: TestModule, opts: [warn: true]]}
]
end

test "multiple unused requires", config do
D.add_require(config[:pid], String, module: TestModule, opts: [])
D.add_require(config[:pid], List, module: TestModule, opts: [])

assert D.collect_unused_requires(config[:pid]) == [
{List, [module: TestModule, opts: []]},
{String, [module: TestModule, opts: []]}
]
end

test "mixed used and unused requires", config do
D.add_require(config[:pid], String, module: TestModule, opts: [])
D.add_require(config[:pid], List, module: TestModule, opts: [])
D.remote_dispatch(config[:pid], String, :compile)
assert D.collect_unused_requires(config[:pid]) == [{List, [module: TestModule, opts: []]}]
end

test "function calls do not count as macro usage", config do
D.add_require(config[:pid], String, module: TestModule, opts: [])
D.remote_dispatch(config[:pid], String, :runtime)
assert D.collect_unused_requires(config[:pid]) == [{String, [module: TestModule, opts: []]}]
end

test "imports with no warn are not unused", config do
D.add_import(config[:pid], String, [], 1, false)
assert D.collect_unused_imports(config[:pid]) == []
Expand Down
174 changes: 174 additions & 0 deletions lib/elixir/test/elixir/kernel/warning_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -2269,6 +2269,180 @@ defmodule Kernel.WarningTest do
Enum.each(list, &purge/1)
end

# Test modules with macros for require testing
defmodule RequireTestMacroModule do
defmacro test_macro(value) do
quote do: unquote(value) * 2
end

defmacro another_macro(value) do
quote do: unquote(value) + 1
end

def test_function(value) do
value * 2
end
end

defmodule RequireTestAnotherMacroModule do
defmacro some_macro(value) do
quote do: unquote(value) - 1
end
end

test "unused require" do
assert_warn_compile(
["nofile:2:3", "unused require Kernel.WarningTest.RequireTestMacroModule"],
"""
defmodule Sample do
require Kernel.WarningTest.RequireTestMacroModule
def a, do: nil
end
"""
)

assert_warn_compile(
["nofile:1:1", "unused require Kernel.WarningTest.RequireTestMacroModule"],
"""
require Kernel.WarningTest.RequireTestMacroModule
"""
)
after
purge(Sample)
end

test "unused require with alias" do
assert_warn_compile(
["nofile:2:3", "unused require Kernel.WarningTest.RequireTestMacroModule"],
"""
defmodule Sample do
require Kernel.WarningTest.RequireTestMacroModule, as: TestMacro
def a, do: nil
end
"""
)
after
purge(Sample)
end

test "used require does not warn" do
assert capture_err(fn ->
defmodule KernelTest.UsedRequire do
require Kernel.WarningTest.RequireTestMacroModule
def fun, do: Kernel.WarningTest.RequireTestMacroModule.test_macro(5)
end
end) == ""
after
purge(KernelTest.UsedRequire)
end

test "function calls do not prevent require warnings" do
assert_warn_compile(
["nofile:2:3", "unused require Kernel.WarningTest.RequireTestMacroModule"],
"""
defmodule Sample do
require Kernel.WarningTest.RequireTestMacroModule
def fun, do: Kernel.WarningTest.RequireTestMacroModule.test_function(5)
end
"""
)
after
purge(Sample)
end

test "used require with alias does not warn" do
assert capture_err(fn ->
defmodule KernelTest.UsedRequireWithAlias do
require Kernel.WarningTest.RequireTestMacroModule, as: TestMacro
def fun, do: TestMacro.test_macro(5)
end
end) == ""
after
purge(KernelTest.UsedRequireWithAlias)
end

test "unused require with warn: false" do
assert capture_err(fn ->
defmodule KernelTest.UnusedRequireNoWarn do
require Kernel.WarningTest.RequireTestMacroModule, warn: false
def fun, do: nil
end
end) == ""
after
purge(KernelTest.UnusedRequireNoWarn)
end

test "unused require with warn: true (explicit)" do
assert_warn_compile(
["nofile:2:3", "unused require Kernel.WarningTest.RequireTestMacroModule"],
"""
defmodule Sample do
require Kernel.WarningTest.RequireTestMacroModule, warn: true
def a, do: nil
end
"""
)
after
purge(Sample)
end

test "unused require with warn: false and alias" do
assert capture_err(fn ->
defmodule KernelTest.UnusedRequireNoWarnAlias do
require Kernel.WarningTest.RequireTestMacroModule, as: TestMacro, warn: false
def fun, do: nil
end
end) == ""
after
purge(KernelTest.UnusedRequireNoWarnAlias)
end

test "multiple unused requires" do
assert_warn_compile(
[
"nofile:2:3",
"unused require Kernel.WarningTest.RequireTestMacroModule",
"nofile:3:3",
"unused require Kernel.WarningTest.RequireTestAnotherMacroModule"
],
"""
defmodule Sample do
require Kernel.WarningTest.RequireTestMacroModule
require Kernel.WarningTest.RequireTestAnotherMacroModule
def a, do: nil
end
"""
)
after
purge(Sample)
end

test "mixed warn options" do
assert_warn_compile(
["nofile:3:3", "unused require Kernel.WarningTest.RequireTestAnotherMacroModule"],
"""
defmodule Sample do
require Kernel.WarningTest.RequireTestMacroModule, warn: false
require Kernel.WarningTest.RequireTestAnotherMacroModule, warn: true
def a, do: nil
end
"""
)
after
purge(Sample)
end

test "warn: false preserves functionality" do
assert capture_err(fn ->
defmodule KernelTest.WarnFalsePreservesFunctionality do
require Kernel.WarningTest.RequireTestMacroModule, warn: false
def fun, do: Kernel.WarningTest.RequireTestMacroModule.test_macro(5)
end
end) == ""
after
purge(KernelTest.WarnFalsePreservesFunctionality)
end
Copy link
Member

Choose a reason for hiding this comment

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

We already test these different conditions in the unit tests in lexical tracker. In here, we need only a single test that verifies the format of the warning itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)


defp purge(module) when is_atom(module) do
:code.purge(module)
:code.delete(module)
Expand Down