mix xref graph --group

[marcandre]

Summary

mix xref graph --label compile-connected would benefit from a way to deal with an acceptable list of core dependencies that should be excluded, provided they don't have any outside dependencies.

I am proposing a --trim API to merge these core paths and remove the resulting group after verifying it has no dependency. The command --merge exposes the merging part without the deletion.

Details

Use case

We have a CI script that insures we don't introduce transitive compile-time dependencies, using --label compile-connected and --fail-above 0.

One complexity is that we have a core of compile-time dependencies that are in more than a single file. In particular, in the same way that we have use BobbyWeb, :something in most of the web layer, we have use Bobby used in the app layer, and web layer via use BobbyWeb, and these may import some basic macros (e.g. sigil_d for decimals).

To have --label compile-connected not be triggered because of these, what we currently do is we --exclude all these core paths.

We must then run mix xref graph again to insure that these core paths do not themselves refer to anything else outside this core (any type of dependency).

Let's say we have 3 core paths A, B and C, a first attempt looks like:

mix xref graph --label=compile-connected \
--exclude=A --exclude=B --exclude=C \
--fail-above 0 &&
mix xref graph \
--source=A --source=B --source=C \
--fail-above 0

That won't work if there are dependencies between A, B and C though, so if one wants a generic way to do this for the three paths, the solution looks like:

mix xref graph --label=compile-connected \
--exclude=A --exclude=C --exclude=B \
--fail-above 0 &&
mix xref graph \
--source=A \
--exclude=B --exclude=C \
--fail-above 0 &&
mix xref graph \
--source=B \
--exclude=A --exclude=C \
--fail-above 0 &&
mix xref graph \
--source=C \
--exclude=A --exclude=B \
--fail-above 0

This makes for an ugly and inefficient solution.

Merge

This PR adds a --merge command to merge many files into a group via vertex identification.

Any file that depended on any component of the group will depend on the resulting group; duplicates are removed and highest type of dependency is selected (compile > export > runtime)
The group will similarly depend on any file that any of its components depended on.
Dependencies between members of the group are removed.

Trim

This PR adds a --trim command which does exactly what --merge does, but then proceeds to fail if the resulting group has any dependency, or else it excludes it.

The usecase above can then be resolved with:

mix xref graph --label=compile-connected \
--trim=A --trim=B --trim=C \
--fail-above 0

Questions

It wasn't immediately clear what solution I wanted to come up with so I started coding the merge functionality first. It's only later that I realized that I really wanted the trim functionality. I am not even sure that the --trim is the best API (my Covid brain isn't helping 😅 )

I am unsure how useful the --merge command can be. If it is deemed preferable to remove it, I could probably come up with a simpler (but maybe uglier) implementation for --trim, because the generic operations are more complex than the merge-check-delete combination. In particular it takes care of selecting the highest type of dependency when deduping them, only to see them be removed later... I found it ironic that the erlang order of :compile, :export and nil (i.e. runtime) happened to be exactly what I wanted it to be, but I chose to be explicit about it in my implementation.

[josevalim]

Hi @marcandre! Thanks for the PR and the context.

Here is what I propose. Introduce a --group option with a list of modules separated by ,. As in merge, any dependency into the group is considered a single one and deduped. Any dependency out of the group is considered a single one and deduped. The group also always be evaluated against the current --label option and --fail-above semantics.

You can create as many groups as you want.

Would this work?

[marcandre]

So what you are proposing to rename --merge to --group and list all paths at once, comma separated (by "list of modules", you actually meant "list of files", right?)

What about --trim? I can do without it, but I can't do all the checks that I want in a single command (i.e. no compile-connected dependencies from the bulk of the files and no outgoing dependencies from the core deps), so I'd have to write:

mix xref graph --label=compile-connected \
--group=A,B,C --exclude=A \
--fail-above 0 &&
mix xref graph \
--group=A,B,C --source=A \
--fail-above 0 &&

[josevalim]

So what you are proposing to rename --merge to --group and list all paths at once, comma separated (by "list of modules", you actually meant "list of files", right?)

Whatever you believe is best. If list of files, it should be separated by | as , is valid on Unix and Windows (| is valid on Unix but invalid on Windows).

What about --trim? I can do without it, but I can't do all the checks that I want in a single command (i.e. no compile-connected dependencies from the bulk of the files and no outgoing dependencies from the core deps), so I'd have to write:

That's the part I don't fully grasp. With the --group approach, you would check that the whole group has no outgoing dependencies (i.e. because someone has a compile time dep in the group, they can't depend on anything else). I.e. the group should have the same semantics as any other node. So perhaps I am proposing to rename --trim into --group?

[marcandre]

If list of files, it should be separated by |

I'd go with files, as they are the basis of mix xref graph. Sorry I confused things by using capital letter for paths.

Allowing multiple paths in a single argument seems good. Should we allow it for --exclude, --source and --sink too, i.e. --exclude a.ex --exclude b.ex could also be written with --exclude a.ex|b.ex, or should we keep it as current?

That's the part I don't fully grasp. With the --group approach, ...

You are correct that --group could be sufficient, but one would have to group together not just the core dependencies like lib/bobby.ex, but all compile-time dependencies that use them, say lib/bobby/my_behaviour.ex:

defmodule Bobby.MyBehaviour do
  # Bring in standard aliases
  use Bobby

  # Define behavior
  @callback example(my_arg :: %MyAppSchema{}) :: :ok
end

I don't want to include this file in the group. The list of these behaviors or modules with macros is much more subject to change than the core dependencies does. I want to keep the group minimal and stable.

Adding --group is a big step forward as it allows me to achieve my goal in two operations (1 to check the core deps, 1 to check the rest of the app) instead of n+1 (n to check the n core dependencies, 1 to check the rest of the app).

Having also --trim (I guess a clearer but much longer name could be --exclude-if-independent) would allow to make it in a single operation.

[josevalim]

So let's go with --group and make it file based. The only reason I want to support multiple files on group is because we probably want to allow distinct groups to be created. The other flags do not have such requirement. :)

[marcandre]

I'll see if I can make a better case for --trim separately.

PR amended for only --group with pipe separated paths.

Should there be an error or warning if a given --group has no pipe (or if multiple such options are given, none of which have a pipe)?

[josevalim]

Gah, I just realized that | is the pipe operator on the shell, so we will need quoting... or we need to pick something else.

[josevalim]

I had another idea... what if we allow this to be specified in the mix.exs instead? We already have a xref key, we could do this:

xref: [
  groups: [
    "lib-use-modules": ~w(lib/foo.ex lib/foo/web.ex)
  ]
]

[marcandre]

I'd use commas as delimiter (even if technically one could have them in their paths)

[marcandre]

I had another idea... what if we allow this to be specified in the mix.exs instead?

So... this would apply to every run of mix xref graph??

[josevalim]

Yes. WDYT?

[josevalim]

Maybe in the future we could have --ungroup lib-use-modules but if we have to ungroup... then perhaps that's not the best idea.

[josevalim]

In any case, if we are sticking with --group, agreed on using , as delimiter.

[marcandre]

Yes. WDYT?

It doesn't seem intuitive nor really workable to me. In particular, if there is an unexpected dependency from the group, then one will want to inspect the graph with --lib/foo.ex but that won't be available anymore.

[marcandre]

Time for bed, maybe I'll wake up with a different opinion 😸

[marcandre]

Edited with commas

[josevalim]

💚 💙 💜 💛 ❤️