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.

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

Add mix xref --callers M.f/a #4683

Merged
merged 13 commits into from May 25, 2016

Conversation

ericentin
Copy link
Contributor

Related: #4656.

mix xref --callers accepts a Module, Module.function, or Module.function/arity.

Output looks like:

file: lib/a.ex
  A.a/0:2,3

I chose this format because:

  • it looks good if you are simply manually using the task
  • file paths can have all kinds of weird characters in them (like : and " and ,) and this should hopefully simplify parsing for libraries that will rely on the output of this task without having to define any kind of escaping

The task also returns data like:

[{"lib/a.ex", [{A, :a, 0, [3, 2]}]}]

from run, so if you use it from within Elixir itself you don't have to parse anything.

Do we need any additional switches, like maybe --silent (for use from Elixir apps that will simply use the return value of run)?

I also added some missing record_remotes to get all of our compile dispatches in, and also fixed a bug where xref was not checking captured remotes.

/cc @josevalim

@josevalim
Copy link
Member

Thank you @antipax! We should get feedback from @tonini and @msaraiva but I would stick with the following for formatting:

lib/a.ex:2: A.a/0
lib/a.ex:3: A.a/0

The idea is that this is a common format for Elixir developers, since we see it frequently in stacktraces and there is a chance editors are already parsing those. :)

@ericentin
Copy link
Contributor Author

I have no problem changing it, but what about paths like "lib/my:crazy:path/a.ex"?

@@ -144,6 +144,7 @@ do_expand_import(Meta, {Name, Arity} = Tuple, Args, Module, E, Result) ->
check_deprecation(Meta, Receiver, Name, Arity, E),
elixir_lexical:record_import({Receiver, Name, Arity}, ?m(E, lexical_tracker)),
elixir_locals:record_import(Tuple, Receiver, Module, ?m(E, function)),
elixir_lexical:record_remote(Receiver, Name, Arity, nil, ?line(Meta), ?m(E, lexical_tracker)),
Copy link
Member

Choose a reason for hiding this comment

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

Preferably we would add record_import/6 and call the lexical tracker just once and it would write to both places.

@josevalim
Copy link
Member

It is still easy to match on ":\d+: " if you need a reference point. :)

On Wednesday, May 25, 2016, Eric Entin notifications@github.com wrote:

I have no problem changing it, but what about paths like
"lib/my:crazy:path/a.ex"?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4683 (comment)

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

## Print callers

defp print_calls(file, calls) do
IO.write(["file: ", file, ?\n])
Copy link
Member

@lexmag lexmag May 25, 2016

Choose a reason for hiding this comment

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

We can use IO.puts/1 here, and on later line as well.

defp source_calls_for_filter(source, filter) do
runtime_dispatches = source(source, :runtime_dispatches)
compile_dispatches = source(source, :compile_dispatches)
dispatches = merge_dispatches(runtime_dispatches, compile_dispatches)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be improved by merging dispatches after filtering them, I suppose. Let me know if I should make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just gonna go for it actually.

@ericentin
Copy link
Contributor Author

Tests are passing but do not merge, there is an issue with the last commit.

@@ -134,13 +138,16 @@ defmodule Kernel.LexicalTracker do
end

def handle_cast({:import_dispatch, {module, function, arity}}, state) do
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this format? Can we write all of them in the format below? :)

@josevalim
Copy link
Member

@antipax thank you! I have added my last batch of comments. ❤️

@ericentin
Copy link
Contributor Author

Ok, unless anyone has any further comments I think this is ready to be merged.

{{func, arity}, lines} <- fals,
line <- lines,
do: {line, module, func, arity})
|> Enum.sort()
Copy link
Member

Choose a reason for hiding this comment

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

I assume there is no need for sorting.

@josevalim josevalim merged commit b15d641 into elixir-lang:master May 25, 2016
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@ericentin ericentin deleted the add-mix-xref-callers branch June 6, 2016 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants