Warn for remote calls that cannot be resolved at compile time #4639

Merged
merged 13 commits into from May 22, 2016

Projects

None yet

3 participants

@antipax
Contributor
antipax commented May 17, 2016 edited
@antipax
Contributor
antipax commented May 18, 2016

Added tests, and cleaned up the code somewhat.

@antipax
Contributor
antipax commented May 18, 2016

For this file:

defmodule MissingModuleReferencer do
  def reference do
    MissingModule
  end
end

defmodule MissingModuleReferencer2 do
  def reference do
    MissingModule
    MissingModule2.call()
    MissingModuleReferencer
  end
end

defprotocol MissingModuleProtocol do
  def func(arg)
end

defimpl MissingModuleProtocol, for: MissingModuleReferencer do
  def func(_) do
    MissingModule2
  end
end

output looks like:
Imgur

@antipax
Contributor
antipax commented May 18, 2016

Protocols and protocol implementations are not checked. Protocols do not need to be checked, because they cannot reference a module at runtime (right?), but implementations maybe should be?

For implementations the problem seemed to be that they refer to "missing" implementations, which will need to be excluded in some way.

Probably would need to make the data in Protocol.builtin/0 available to other code, or we could try chopping off the last component of the module name and seeing if the rest is a protocol or not.

@antipax
Contributor
antipax commented May 18, 2016 edited

Ok, just thought why not go for one of the options and modified the privacy of Protocol.builtin/0 so that I can detect the unimportant references. It's in a separate commit so can back that out easily if needed.

Now displays warnings for referenced unloaded modules in protocol implementations:

Files:

lib/missing_module_protocol.ex

defprotocol MissingModuleProtocol do
  def func(arg)
end

defimpl MissingModuleProtocol, for: MissingModuleReferencer do
  def func(_) do
    MissingModule
    MissingModuleReferencer
  end
end

lib/missing_module_referencer.ex

defmodule MissingModuleReferencer do
  def reference do
    MissingModule
  end
end

defmodule MissingModuleReferencer2 do
  def reference do
    MissingModule
    MissingModule2.call()
    MissingModuleReferencer
  end
end

looks like:

Imgur

@antipax
Contributor
antipax commented May 18, 2016 edited

Some interesting stuff from a project of mine compiled with this branch:

==> combine
Compiling 6 files (.ex)
warning: Module String.T was referenced, but is unloaded.

From compiled module:
  Combine.Parsers.Base (lib/combine/parsers/base.ex)

Generated combine app

Found a typespec typo in combine! :P

==> db_connection
Compiling 24 files (.ex)
warning: Module :sbroker was referenced, but is unloaded.

From compiled modules:
  DBConnection.Sojourn (lib/db_connection/sojourn.ex)
  DBConnection.Connection (lib/db_connection/connection.ex)

Generated db_connection app

:sbroker is optional, so we should consider providing some way to suppress this warning probably?

==> plug
Compiling 44 files (.ex)
warning: Module Plug.Keys was referenced, but is unloaded.

From compiled modules:
  Plug.Supervisor (lib/plug/supervisor.ex)
  Plug.Session.COOKIE (lib/plug/session/cookie.ex)

Generated plug app

Plug.Keys is an ETS table name, which is a pretty common pattern in Elixir apps (my own apps almost always use module aliases as table names). Same for supervisor names. Not sure how to address this, again, probably best to provide a way to suppress the warning.

==> phoenix
...
00:22:56.573 [error] Loading of /Users/ericentin/Documents/Code/elixir/bin/../lib/elixir/ebin/Elixir.beam failed: :badfile


00:22:56.573 [error] beam/beam_load.c(1278): Error loading module 'Elixir':
  module name in object code is elixir
...
warning: Module Elixir was referenced, but is unloaded.

From compiled module:
  Mix.Phoenix (lib/mix/phoenix.ex)
...

Not sure how best to fix this one. Maybe Elixir just needs to be excluded?

@antipax
Contributor
antipax commented May 18, 2016

Also, now that the code uses Code.ensure_loaded? to check whether the module is (or, now, can be) loaded, maybe the error message isn't quite as appropriate anymore. Maybe something like:

warning: Module Plug.Keys was referenced, but could not be loaded.

would work better now.

@josevalim
Member

That's fantastic work @antipax!

Plug.Keys is an ETS table name, which is a pretty common pattern in Elixir apps (my own apps almost always use module aliases as table names). Same for supervisor names. Not sure how to address this, again, probably best to provide a way to suppress the warning.

I think the issue in this case is in the information we collect. We collect any reference to an external module, regardless if we call it or not, because the compiler needs to be conservative. It is best if we don't lose a reference and the cost is just compiling a module fro time to time when we shouldn't.

The warning system is different, we cannot afford false positives. But because we store any reference we see, we get false positives. I am afraid this is also why we get false positive for protocols and implementations. That said, I believe it is time to take the next step. :)

All this information is stored in Kernel.LexicalTracker here: https://github.com/elixir-lang/elixir/blob/master/lib/elixir/lib/kernel/lexical_tracker.ex#L17-L28

I have just updated this file, so make sure you rebase and are on latest. The idea is, every time we dispatch to a module (due to a remote or an import dispatch), we add a reference (see calls to add_reference). I believe what we should do is to introduce a new remote_dispatch, that also receives the function, arity and line, as well as update the import_dispatch to receive the line and store this information on the table. This way, we know exactly which functions are being called and at which line, allowing us to not only track if a module exists or not but also if the function that we are calling effectively exists.

Let me know if you have any questions. :)

@antipax antipax referenced this pull request in bitwalker/combine May 18, 2016
Merged

Fix typespec typo in Combine.Parsers.Base #6

@antipax antipax changed the title from Compile warning on referenced unloaded modules to Warn for remote calls that cannot be resolved at compile time. May 19, 2016
@antipax antipax and 1 other commented on an outdated diff May 19, 2016
lib/elixir/lib/kernel/lexical_tracker.ex
end
- def handle_cast({:import_dispatch, {module, function, arity}}, {d, dest}) do
+ def handle_cast({:remote_dispatch, module, fas, line, mode}, {d, dest, parent}) do
+ if parent, do: remote_dispatch(parent, module, fas, line, mode)
+
+ add_reference(d, module, mode)
+ add_reference(d, module, fas, line, mode)
+ {:noreply, {d, dest, parent}}
+ end
+
+ def handle_cast({:import_dispatch, {module, function, arity}}, {d, dest, parent}) do
@antipax
antipax May 19, 2016 Contributor

I tried adding the line to :import_dispatch as you suggested, @josevalim, but I couldn't really figure out how it should be used.

It seems dispatches to imported funcs end up making it through :remote_dispatch, so they show up in the call graph, and it didn't seem semantically correct to add them to the "compile_remote_calls" from here (which is all I could figure might make sense), because they aren't really called at compile time. The current functionality involving import warnings didn't seem to need the line data either, so I just decided to leave it out.

If I should add it back in, can you describe where the data would be used and how?

@antipax
antipax May 19, 2016 Contributor

Maybe could add it as another element of remotes/1 that simply lists the functions that were imported and where they were dispatched from? Still not sure what it would be used for, though.

@josevalim
josevalim May 19, 2016 Member

It seems dispatches to imported funcs end up making it through :remote_dispatch, so they show up in the call graph,

If they are already in the graph, ignore me. :)

@antipax antipax and 1 other commented on an outdated diff May 19, 2016
lib/elixir/lib/kernel/lexical_tracker.ex
@@ -46,8 +60,8 @@ defmodule Kernel.LexicalTracker do
# Starts the tracker and returns its pid.
@doc false
- def start_link(dest) do
- :gen_server.start_link(__MODULE__, dest, [])
+ def start_link(dest, parent) do
@antipax
antipax May 19, 2016 edited Contributor

Kernel.LexicalTracker.start_link now takes a parameter parent, which is another tracker pid that will get all lexical data forwarded from the new tracker.

Before now, all modules in a file would actually end up using the same tracker, which resulted in all modules after the first containing all of the references of the ones defined above them. Because the data was only used to determine what the dependencies for compilation were, this was not an issue.

Now, there is a tree of trackers, and compiling a file with multiple modules results in a file-scope tracker which receives all data about the modules, as well as individual module-scoped trackers. This means that the file-scope tracker can determine whether there should be warnings for imports in the file scope, and that module-scope trackers only contain calls within their module's definition.

@josevalim
josevalim May 19, 2016 Member

@antipax I may be missing something but I don't understand why this is needed.

Before now, all modules in a file would actually end up using the same tracker, which resulted in all modules after the first containing all of the references of the ones defined above them.

The warnings you are calculating are per file. In fact, this change probably means we would never emit a warning for unused functions at the end of a file (outside of a module) like this:

defmodule Foo do
  # ...  
end
Enum.map([], fn _ -> IO.i_dont_exist end)

If there is an issue in this code, I would rather say the issue is in the Mix compiler which is tracking all this data per module while it should be doing so per file. We will need to change how we store data in the manifest. Please ping me later when you have some time. :)

@antipax
antipax May 19, 2016 Contributor

Derp. You're totally right, I was just working within the confines of each_module and missed this case. I'll back all the "parent" stuff out.

@josevalim
josevalim May 19, 2016 Member

Cool. I will ping you soon on IRC. Do not worry about squashing commits for
now, it makes possible reverts like this easier. :)

On Thursday, May 19, 2016, Eric Entin notifications@github.com wrote:

In lib/elixir/lib/kernel/lexical_tracker.ex
#4639 (comment):

@@ -46,8 +60,8 @@ defmodule Kernel.LexicalTracker do

Starts the tracker and returns its pid.

@doc false

  • def start_link(dest) do
  • :gen_server.start_link(MODULE, dest, [])
  • def start_link(dest, parent) do

Derp. You're totally right, I was just working within the confines of
each_module and missed this case. I'll back all the "parent" stuff out.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/elixir-lang/elixir/pull/4639/files/26701d64859636f381e5078ac8b0b131a2783afb#r63877760

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

@antipax antipax and 1 other commented on an outdated diff May 19, 2016
lib/elixir/src/elixir_exp.erl
if
is_atom(Receiver) ->
- elixir_lexical:record_remote(Receiver, ?m(E, function), ?m(E, lexical_tracker));
+ elixir_lexical:record_remote(Receiver, ?m(E, function), Right, length(EArgs), ?line(Meta), ?m(E, lexical_tracker));
@antipax
antipax May 19, 2016 Contributor

It seemed right to use EArgs here. Presumably if the args contain a call to unquote_splicing, the number of args might increase after expansion? Just wanted to confirm.

@josevalim
josevalim May 19, 2016 Member

Both are correct. EArgs is not going to be expanded here because you already have the AST. Splicing works when building the AST.

@antipax antipax commented on an outdated diff May 19, 2016
lib/elixir/src/elixir_module.erl
@@ -47,14 +47,11 @@ compile(Module, Block, Vars, #{line := Line} = Env) when is_atom(Module) ->
_ -> Env#{lexical_tracker := nil, function := nil, module := Module}
end,
- case ?m(LexEnv, lexical_tracker) of
- nil ->
- elixir_lexical:run(?m(LexEnv, file), nil, fun(Pid) ->
- do_compile(Line, Module, Block, Vars, LexEnv#{lexical_tracker := Pid})
- end);
- _ ->
- do_compile(Line, Module, Block, Vars, LexEnv)
- end;
+ Lexical = ?m(LexEnv, lexical_tracker),
+
+ elixir_lexical:run(?m(LexEnv, file), elixir_lexical:dest(Lexical), Lexical, fun(Pid) ->
+ do_compile(Line, Module, Block, Vars, LexEnv#{lexical_tracker := Pid})
+ end);
@antipax
antipax May 19, 2016 Contributor

This code now always creates a new lexical tracker for every module that gets compiled, with the current tracker as its parent.

@antipax antipax and 1 other commented on an outdated diff May 19, 2016
lib/elixir/lib/kernel/lexical_tracker.ex
@@ -172,6 +208,11 @@ defmodule Kernel.LexicalTracker do
defp add_reference(d, module, :compile) when is_atom(module),
do: :ets.insert(d, {{:mode, module}, :compile})
+ defp add_reference(d, module, fas, line, :runtime) when is_atom(module),
+ do: :ets.insert_new(d, {{:mode, module, fas, line}, :runtime})
@antipax
antipax May 19, 2016 Contributor

One thing I wanted to note here is that with the current table type of "set" we can't track multiple references to the same MFA on the same line. I figured this seemed like an acceptable compromise against additional complexity, but we could also create a second table that is purely for these references so that they can accumulate over a single line.

@josevalim
josevalim May 19, 2016 Member

Why would we want to track multiple references to the same MFA on the same line? :)

@antipax
antipax May 19, 2016 Contributor

Yeah, it seems useless.

@antipax antipax and 1 other commented on an outdated diff May 19, 2016
lib/mix/lib/mix/compilers/elixir.ex
@@ -153,10 +154,16 @@ defmodule Mix.Compilers.Elixir do
|> List.delete(module)
|> Enum.reject(&match?("elixir_" <> _, Atom.to_string(&1)))
+ runtime_remote_calls =
+ runtime_remote_calls
+ |> Enum.reject(&match?("elixir_" <> _, Atom.to_string(elem(&1, 0))))
@antipax
antipax May 19, 2016 edited Contributor

Maybe we shouldn't filter these here, actually? I guess a user could refer to an :elixir_* module in their own code improperly. (But they shouldn't!)

@josevalim
josevalim May 19, 2016 Member

If they are doing that, they don't deserve a warning. :)

@antipax antipax commented on an outdated diff May 19, 2016
lib/mix/lib/mix/compilers/elixir.ex
+ []
+ )
+ end
+ end)
+ end
+
+ defp exports_for_module(module) do
+ if match?("Elixir." <> _, to_string(module)) do
+ module.__info__(:functions) ++ module.__info__(:macros)
+ else
+ exports = module.module_info(:exports)
+
+ if module == :erlang do
+ # We have to add these two because they're not really in :erlang.
+ # See: lib/elixir/src/elixir_translator.erl, Erlang op section
+ exports ++ [andalso: 2, orelse: 2]
@antipax
antipax May 19, 2016 Contributor

Not sure if this is 100% correct, but I think this is the only two of these that need to be included here.

@antipax antipax commented on an outdated diff May 19, 2016
lib/mix/lib/mix/compilers/elixir.ex
@@ -193,6 +200,53 @@ defmodule Mix.Compilers.Elixir do
Mix.shell.info "Compiling #{source} (it's taking more than #{threshold}s)"
end
+ defp warn_for_missing_remote_functions(runtime_remote_calls, source, :module),
+ do: do_warn_for_missing_remote_functions(runtime_remote_calls, source)
+
+ defp warn_for_missing_remote_functions(runtime_remote_calls, source, {:impl, _}),
+ do: do_warn_for_missing_remote_functions(runtime_remote_calls, source)
+
+ defp warn_for_missing_remote_functions(_, _, _),
+ do: :ok
+
+ defp do_warn_for_missing_remote_functions(runtime_remote_calls, source) do
+ Enum.sort_by(runtime_remote_calls, fn {_, _, line} -> line end)
+ |> Enum.each(fn {module, {func, arity} = fas, line} ->
+ if Code.ensure_loaded?(module) do
+ unless fas in exports_for_module(module) do
@antipax
antipax May 19, 2016 edited Contributor

This could be optimized to only ensure a module is loaded/get its exports once per compilation, but this PR didn't seem to be affecting performance noticeably for me, so I went for simplicity instead.

@antipax
antipax May 19, 2016 Contributor

We could also potentially add "did you mean?" using jaro distance on the MFA, I think! That might be cool.

@antipax
antipax May 19, 2016 Contributor

Now maybe I am just getting too excited here, but we could also highlight the diff in the "did you mean?" using the same technique that ExUnit now does to diff assertions. That would make it pretty easy to see even when you only have a single similar-looking-character-type typo.

@antipax
Contributor
antipax commented May 19, 2016

Now,

for file:

defprotocol MissingModuleProtocol do
  def func(arg)
end

defmodule MissingModuleReferencer do
  import Record
  require Integer

  def reference do
    _ = extract(1, 2)
    _ = is_record({:record})
    _ = Integer.is_even(2)

    NotAModule
    MissingModuleReferencer.no_func()
  end
end

defmodule MissingModuleReferencer2 do
  def reference(_) do
    NotAModule2
    MissingModule2.call()
    MissingModuleReferencer.reference()
    MissingModuleReferencer.reference(1)
    MissingModuleReferencer.no_func2()
  end
end

defimpl MissingModuleProtocol, for: MissingModuleReferencer do
  def func(_) do
    MissingModuleReferencer.reference()
    MissingModuleReferencer.no_func3()
  end
end

output looks like:

Imgur

Thanks for the guidance, @josevalim, hope this is close to what you were imagining! :bowtie:

@josevalim josevalim commented on an outdated diff May 19, 2016
lib/elixir/src/elixir_lexical.erl
@@ -46,6 +47,9 @@ record_import({Module, Function, Arity}, Ref) ->
record_remote(Module, Function, Ref) ->
if_tracker(Ref, fun(Pid) -> ?tracker:remote_dispatch(Pid, Module, mode(Function)), ok end).
+record_remote(Module, Function, ReferencedFunction, Arity, Line, Ref) ->
@josevalim
josevalim May 19, 2016 Member

I know this is a super minor nitpick but can we order the arguments are: record_remote(Module, Fun, Arity, EnvFunction, Line, Ref)? I think Fun and Arity belong conceptually closer to Module.

@josevalim josevalim commented on an outdated diff May 19, 2016
lib/mix/lib/mix/compilers/elixir.ex
+ []
+ )
+ end
+ else
+ IO.warn(
+ "Module #{inspect module} cannot be found\n" <>
+ " In remote call to #{inspect module}.#{func}/#{arity} at:\n" <>
+ " #{source}:#{line}",
+ []
+ )
+ end
+ end)
+ end
+
+ defp exports_for_module(module) do
+ if match?("Elixir." <> _, to_string(module)) do
@josevalim
josevalim May 19, 2016 Member

Don't match on the module name, check for function_exported?(module, :__info__, 1).

@josevalim josevalim commented on an outdated diff May 19, 2016
lib/elixir/lib/kernel/lexical_tracker.ex
@@ -70,6 +84,10 @@ defmodule Kernel.LexicalTracker do
:gen_server.cast(pid, {:remote_dispatch, module, mode})
end
+ def remote_dispatch(pid, module, fas, line, mode) do
+ :gen_server.cast(pid, {:remote_dispatch, module, fas, line, mode})
@josevalim
josevalim May 19, 2016 edited Member

This should be called fa and not fas as it is a single function-arity and not a list of them. We should fix it here and down below.

@josevalim
Member
josevalim commented May 19, 2016 edited

Hi @antipax, that's great progress. I think though your changes have revealed some limitations in how we store data in the manifest. I think we should break it apart into {module, Source} and {file, Compile, Runtime, ...} and so on. This may be a complex change which I think we will have to do in another PR before merging this one. If you prefer, I can work on that, and then we can continue the changes on this PR.

@josevalim
Member
josevalim commented May 19, 2016 edited

Just to recap our IRC discussion. We are going to first improve Mix.Compilers.Elixir before we move on with those changes. @antipax, please give it a try and let me know if there is anything I can do to help. The each_file/1 callback in the parallel compiler will be your new best friend when implementing this. :)

@antipax antipax commented on an outdated diff May 20, 2016
lib/mix/lib/mix/compilers/elixir.ex
+ end
+
+ defp warn_for_missing_remote_functions(source_record) do
+ compile_dispatches = source(source_record, :compile_dispatches)
+ runtime_dispatches = source(source_record, :runtime_dispatches)
+ source = source(source_record, :source)
+
+ do_warn_for_missing_remote_functions(compile_dispatches ++ runtime_dispatches, source)
+ end
+
+ defp do_warn_for_missing_remote_functions(runtime_dispatches, source) do
+ Enum.sort_by(runtime_dispatches, fn {_, _, line} -> line end)
+ |> Enum.each(fn {module, {func, arity}, line} ->
+ if Code.ensure_loaded?(module) do
+ unless function_exported?(module, func, arity) or
+ is_macro?(module, func, arity) or
@antipax
antipax May 20, 2016 Contributor

I think there's 2 more bugs here, first we should probably only check if it's a macro if it's in the compile dispatches, and also right now macro dispatches are appearing in runtime rather than compile as I think they should be.

@antipax
Contributor
antipax commented May 20, 2016

I'd like to improve the display of the warnings more (using jaro_distance and the diff stuff from ExUnit) but I think that would be better in a separate PR. This all seems to be working nicely and accurately now.

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/mix/lib/mix/compilers/elixir.ex
@@ -219,9 +234,61 @@ defmodule Mix.Compilers.Elixir do
do: relative
end
- defp each_file(source) do
- Mix.shell.info "Compiled #{source}"
+ defp each_file(pid, verbose, source) do
+ if verbose, do: Mix.shell.info "Compiled #{source}"
+
+ Agent.get(pid, fn {_, sources} -> Enum.find(sources, & source(&1, :source) == source) end)
@josevalim
josevalim May 20, 2016 Member

Use List.keyfind. :)

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/mix/lib/mix/compilers/elixir.ex
@@ -6,7 +6,14 @@ defmodule Mix.Compilers.Elixir do
import Record
defrecordp :module, [:module, :kind, :source, :beam, :binary]
- defrecordp :source, [:source, compile: [], runtime: [], external: []]
+ defrecordp :source, [
+ source: nil,
+ compile: [],
+ runtime: [],
+ compile_dispatches: [],
+ runtime_dispatches: [],
@josevalim
josevalim May 20, 2016 Member

Do we care if the dispatch was compile time or not? Maybe we should just put them all into the same bucket called compile_dispatches? In fact, the only thing I can think of is that we don't need to check compile_dispatches because if it happened at compile time, it certainly worked. :) Let's talk on IRC later when you are back about the next steps.

@josevalim
josevalim May 20, 2016 Member

Ok, after some more thought, I am thinking we should ignore compile_dispatches and simply store runtime_dispatches. We could change this up in the lexical tracker itself and call "dispatches" simply everything that will happen at runtime. The rationale is the following: all the compile time dispatches will be invoked at compile time anyway. The only exception is when the code is not meant to be invoked at all which means we should not validate it as well.

For example, imagine this code at the module level, outside of a function:

if function_exported?(List, :flatten, 1) do
  List.flatten([1, 2, 3])
else
  List.old_flatten([1, 2, 3])
end

It is common code that checks for a particular version and acts accordingly. We could even add a test that ensures the code above won't emit any warnings.

At the function body, there is nothing we can do, but we can at least define internal functions that act differently:

if function_exported?(List, :flatten, 1)
  def flatten(arg), do: List.flatten(arg)
else
  def flatten(arg), do: List.old_flatten(arg)
end

And because only one function will be defined, the runtime will see only one of the two versions and not emit warnings.

@josevalim josevalim changed the title from Warn for remote calls that cannot be resolved at compile time. to Warn for remote calls that cannot be resolved at compile time May 20, 2016
@josevalim
Member

There is one last change we need to do. When a module depends on another module at runtime, there is no guarantee that the other module is compiled. This means we should not run this callback after each file is compiled but only after all files are compiled. My suggestion would be to create a new compiler called compile.xref that will look if the manifest generated by "compile.elixir" exists and, if so, traverse the manifest sources looking for the dispatches and cross-reference them. This is also a good opportunity to move the tests around to a separate file, as it is effectively a new module.

We are definitely gearing towards the last steps here. :D

@antipax
Contributor
antipax commented May 20, 2016

The mix test suite increased to 60 seconds on my machine from 40. This is probably due to the increased amount of data written in the manifest, as well as the second read of the manifest in Mix.Tasks.Xref and the new xref manifest that's written so that it can tell if xref has been run for the given elixir manifest yet or not. Not sure if it's possible to improve that or not without being able to pass data from one compiler to another.

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/mix/lib/mix/tasks/compile.xref.ex
+ end
+ end
+ else
+ :noop
+ end
+ end
+
+ @doc """
+ Returns xref manifests.
+ """
+ def manifests, do: [manifest]
+ defp manifest, do: Path.join(Mix.Project.manifest_path, @manifest)
+
+ def write_manifest do
+ data = {@manifest_vsn, System.version}
+ File.mkdir_p!(Mix.Project.manifest_path())
@josevalim
josevalim May 20, 2016 Member

You don't need to write anything for this manifest. An empty file is fine. :)

@josevalim josevalim and 1 other commented on an outdated diff May 20, 2016
lib/mix/lib/mix/tasks/compile.xref.ex
+ * `--force` - forces checking regardless of modification time
+ * `--warnings-as-errors` - treat warnings as errors and return a non-zero exit code
+
+ """
+
+ @doc """
+ Runs this task.
+ """
+ @spec run(OptionParser.argv) :: :ok | :noop
+ def run(args) do
+ {opts, _, _} =
+ OptionParser.parse(args, switches: [force: :boolean, warnings_as_errors: :boolean])
+
+ if Mix.Utils.stale?(E.manifests(), manifests()) or opts[:force] do
+ if Mix.Task.run("xref", args) == :ok do
+ write_manifest()
@josevalim
josevalim May 20, 2016 Member

You should always write this manifest if you ran the task and if it did not fail.

@antipax
antipax May 20, 2016 Contributor

So it should be written when this returns :noop as well?

@josevalim
josevalim May 20, 2016 Member

This compiler should always return :noop because it does not generate any artifact.

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/mix/lib/mix/tasks/xref.ex
+ @shortdoc "Performs remote dispatch checking"
+ @recursive true
+
+ @moduledoc """
+ Performs remote dispatch checking.
+
+ When this task runs, it looks for all runtime remote dispatches in the
+ application. It will then check that all of the modules/functions referred to
+ by the dispatches are available. If any are not, a warning will be printed.
+ """
+
+ @doc """
+ Runs this task.
+ """
+ @spec run(OptionParser.argv) :: :ok | :error
+ def run(_args) do
@josevalim
josevalim May 20, 2016 Member

Let's add Mix.Task.run "compile" for the cases this task is invoked directly.

@antipax
Contributor
antipax commented May 20, 2016 edited

I was wrong about the difference in mix test suite time. I was just comparing the time on my other machine to the one I'm working on at the moment. Master is 60 seconds on this machine, which is the same as this branch.

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/mix/lib/mix/compilers/elixir.ex
@@ -313,7 +332,8 @@ defmodule Mix.Compilers.Elixir do
end
# Similar to read manifest but supports data migration.
- defp parse_manifest(manifest) do
+ @doc false
@josevalim
josevalim May 20, 2016 Member

The whole module is @moduledoc false, so we can move this up and add @doc to it.

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/mix/lib/mix/compilers/elixir.ex
|> List.delete(module)
|> Enum.reject(&match?("elixir_" <> _, Atom.to_string(&1)))
- runtime =
- runtime
+ runtime_references =
+ runtime_references
|> List.delete(module)
|> Enum.reject(&match?("elixir_" <> _, Atom.to_string(&1)))
@josevalim
josevalim May 20, 2016 Member

We can remove this line because I think those calls won't happen at runtime.

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/mix/lib/mix/compilers/elixir.ex
|> List.delete(module)
|> Enum.reject(&match?("elixir_" <> _, Atom.to_string(&1)))
+ {compile_dispatches, runtime_dispatches} = Kernel.LexicalTracker.remote_dispatches(module)
+
+ compile_dispatches =
+ compile_dispatches
+ |> Enum.reject(&match?("elixir_" <> _, Atom.to_string(elem(&1, 0))))
+
+ runtime_dispatches =
+ runtime_dispatches
+ |> Enum.reject(&match?("elixir_" <> _, Atom.to_string(elem(&1, 0))))
@josevalim
josevalim May 20, 2016 Member

We can remove this one too.

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/mix/lib/mix/tasks/compile.xref.ex
+
+ """
+
+ @doc """
+ Runs this task.
+ """
+ @spec run(OptionParser.argv) :: :ok | :noop
+ def run(args) do
+ {opts, _, _} =
+ OptionParser.parse(args, switches: [force: :boolean, warnings_as_errors: :boolean])
+
+ if Mix.Utils.stale?(E.manifests(), manifests()) or opts[:force] do
+ if Mix.Task.run("xref", args) == :ok do
+ write_manifest()
+ else
+ if opts[:warnings_as_errors] do
@josevalim
josevalim May 20, 2016 Member

You should also check Mix.Project.config()[:elixirc_options][:warnings_as_errors] I believe.

@josevalim josevalim commented on an outdated diff May 20, 2016
lib/elixir/test/elixir/kernel/lexical_tracker_test.exs
@@ -111,4 +129,56 @@ defmodule Kernel.LexicalTrackerTest do
assert Foo.Bar in runtime
refute Foo.Bar in compile
end
+
+ test "call graph" do
+ remotes =
@josevalim
josevalim May 20, 2016 Member

This whole test needs to be indented once more. :)

@lexmag lexmag and 1 other commented on an outdated diff May 20, 2016
lib/elixir/lib/kernel/lexical_tracker.ex
end
+
+ defp map_put_new(map, key, value) do
+ case :maps.find(key, map) do
+ {:ok, _} -> map
+ :error -> map_put(map, key, value)
+ end
+ end
+
+ defp map_put(map, key, value),
@lexmag
lexmag May 20, 2016 Member

Seems this function doesn't bring anything to us.

@antipax
antipax May 20, 2016 Contributor

Happy to change it, but I think it makes it a bit easier to read?

@lexmag
lexmag May 20, 2016 Member

Honestly, I don't see any difference, it just brings a question why we have this function defined (and it simply more code for no gain). :bowtie:

@antipax
antipax May 21, 2016 Contributor

Ok, I'll get rid of it.

@lexmag lexmag and 1 other commented on an outdated diff May 20, 2016
lib/mix/lib/mix/tasks/xref.ex
+ warn_for_missing_remote_functions(runtime_dispatches, source, result)
+ end
+ end
+ end
+
+ defp warn_for_missing_remote_functions(runtime_dispatches, source, result) do
+ Enum.sort_by(runtime_dispatches, fn {_, _, line} -> line end)
+ |> Enum.reduce(result, &reduce_dispatch(source, &1, &2))
+ end
+
+ defp reduce_dispatch(source, {module, {func, arity}, line}, result) do
+ if Code.ensure_loaded?(module) do
+ if function_exported?(module, func, arity) or is_erlang_op?(module, func, arity) do
+ result
+ else
+ IO.warn(
@lexmag
lexmag May 20, 2016 Member

I think it should be :elixir_errors.warn(line , source, message) thus we get proper formatting.

@antipax
antipax May 20, 2016 Contributor

Aha, cool. I switched to :elixir_errors.warn/1 but will use /3 instead.

@lexmag lexmag commented on an outdated diff May 20, 2016
lib/mix/lib/mix/tasks/xref.ex
+ if function_exported?(module, func, arity) or is_erlang_op?(module, func, arity) do
+ result
+ else
+ IO.warn(
+ "Remote function #{inspect module}.#{func}/#{arity} cannot be found\n" <>
+ " #{source}:#{line}",
+ []
+ )
+
+ :error
+ end
+ else
+ if builtin_protocol_impl?(module, func, arity) do
+ result
+ else
+ IO.warn(
@lexmag
lexmag May 20, 2016 edited Member

Also it'd be better to build message outside of the call. :bowtie:

@lexmag lexmag commented on an outdated diff May 20, 2016
lib/mix/lib/mix/tasks/xref.ex
+ if builtin_protocol_impl?(module, func, arity) do
+ result
+ else
+ IO.warn(
+ "Module #{inspect module} cannot be found\n" <>
+ " In remote call to #{inspect module}.#{func}/#{arity} at:\n" <>
+ " #{source}:#{line}",
+ []
+ )
+
+ :error
+ end
+ end
+ end
+
+ defp is_erlang_op?(:erlang, func, 2) when func in [:andalso, :orelse],
@lexmag
lexmag May 20, 2016 Member

By convention we should drop is_ prefix.

@lexmag lexmag and 1 other commented on an outdated diff May 20, 2016
lib/mix/lib/mix/tasks/compile.xref.ex
+ else
+ write_manifest()
+ end
+ end
+ else
+ :noop
+ end
+ end
+
+ @doc """
+ Returns xref manifests.
+ """
+ def manifests, do: [manifest]
+ defp manifest, do: Path.join(Mix.Project.manifest_path, @manifest)
+
+ def write_manifest do
@lexmag
lexmag May 20, 2016 Member

Looks like it needs to be defp.

@antipax
antipax May 20, 2016 Contributor

Oops, thanks

@antipax
Contributor
antipax commented May 20, 2016

Ok, going to focus on cleaning up tests now, unless anyone has any more feedback they'd like me to address.

@josevalim josevalim merged commit 0395bac into elixir-lang:master May 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@josevalim
Member

❤️ 💚 💙 💛 💜

@antipax antipax deleted the antipax:warn-on-referenced-unloaded-modules branch Jun 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment