Do not allow loops in dot graph formats

Author: José Valim

lib/mix/lib/mix/tasks/app.tree.ex

@@ -49,19 +49,18 @@ defmodule Mix.Tasks.App.Tree do
     excluded = Keyword.get_values(opts, :exclude) |> Enum.map(&String.to_atom/1)
     excluded = @default_excluded ++ excluded
 
-    app_tree_callback =
-      fn {type, app} ->
-        load(app)
-        {"#{app}#{type(type)}", children_for(app, excluded)}
-      end
+    callback = fn {type, app} ->
+      load(app)
+      {{app, type(type)}, children_for(app, excluded)}
+    end
 
     if opts[:format] == "dot" do
-      app_tree = Mix.Utils.build_dot_graph("application tree", [{:normal, app}], app_tree_callback, opts)
-      filename = "app_tree.dot"
-      File.write!(filename, app_tree <> "\n")
-      Mix.shell.info("Generated `#{filename}` in current directory")
+      Mix.Utils.write_dot_graph!("app_tree.dot", "application tree",
+                                 {:normal, app}, callback, opts)
+      Mix.shell.info "Generated \"app_tree.dot\" in current directory.\n" <>
+                     "You can use http://www.graphviz.org/ to open it."
     else
-      Mix.Utils.print_tree([{:normal, app}], app_tree_callback, opts)
+      Mix.Utils.print_tree({:normal, app}, callback, opts)
     end
   end
 
@@ -79,6 +78,6 @@ defmodule Mix.Tasks.App.Tree do
     Enum.map(apps, &{:normal, &1}) ++ Enum.map(included_apps, &{:included, &1})
   end
 
-  defp type(:normal),   do: ""
-  defp type(:included), do: " (included)"
+  defp type(:normal),   do: nil
+  defp type(:included), do: "(included)"
 end

lib/mix/lib/mix/tasks/deps.tree.ex

@@ -39,8 +39,6 @@ defmodule Mix.Tasks.Deps.Tree do
 
     deps_opts = if only = opts[:only], do: [env: :"#{only}"], else: []
     deps      = Mix.Dep.loaded(deps_opts)
-    excluded  = Keyword.get_values(opts, :exclude) |> Enum.map(&String.to_atom/1)
-    top_level = Enum.filter(deps, & &1.top_level)
 
     root =
       case args do
@@ -51,9 +49,23 @@ defmodule Mix.Tasks.Deps.Tree do
           find_dep(deps, app) || Mix.raise("could not find dependency #{app}")
       end
 
-    deps_tree_callback =
-      fn
-        %Mix.Dep{app: app} = dep ->
+    if opts[:format] == "dot" do
+      callback = callback(&format_dot/1, deps, opts)
+      Mix.Utils.write_dot_graph!("deps_tree.dot", "dependency tree", root, callback, opts)
+      Mix.shell.info "Generated \"deps_tree.dot\" in current directory.\n" <>
+                     "You can use http://www.graphviz.org/ to open it."
+    else
+      callback = callback(&format_tree/1, deps, opts)
+      Mix.Utils.print_tree(root, callback, opts)
+    end
+  end
+
+  defp callback(formatter, deps, opts) do
+    excluded  = Keyword.get_values(opts, :exclude) |> Enum.map(&String.to_atom/1)
+    top_level = Enum.filter(deps, & &1.top_level)
+
+    fn
+      %Mix.Dep{app: app} = dep ->
           deps =
             # Do not show dependencies if they were
             # already shown at the top level
@@ -62,42 +74,42 @@ defmodule Mix.Tasks.Deps.Tree do
             else
               find_dep(deps, app).deps
             end
-          {format_dep(dep, opts), exclude(deps, excluded)}
+          {formatter.(dep), exclude(deps, excluded)}
         app ->
-          {Atom.to_string(app), exclude(top_level, excluded)}
-      end
-
-    if opts[:format] == "dot" do
-      deps_tree = Mix.Utils.build_dot_graph("dependency tree", [root], deps_tree_callback, opts)
-      filename = "deps_tree.dot"
-      File.write!(filename, deps_tree <> "\n")
-      Mix.shell.info("Generated `#{filename}` in current directory")
-    else
-      Mix.Utils.print_tree([root], deps_tree_callback, opts)
+          {{Atom.to_string(app), nil}, exclude(top_level, excluded)}
     end
   end
 
   defp exclude(deps, excluded) do
     Enum.reject deps, & &1.app in excluded
   end
 
-  defp format_dep(%{app: app, scm: scm, requirement: requirement, opts: deps_opts}, opts) do
+  defp format_dot(%{app: app, requirement: requirement, opts: opts}) do
     override =
-      if deps_opts[:override] do
-        "#{IO.ANSI.bright} *override*#{IO.ANSI.normal}"
+      if opts[:override] do
+        " *override*"
       else
         ""
       end
-    if opts[:format] == "dot" do
-      {app, "#{requirement(requirement)}#{override}"}
-    else
-      "#{app}#{requirement(requirement)} (#{scm.format(deps_opts)})#{override}"
-    end
+
+    requirement = requirement && requirement(requirement)
+    {app, "#{requirement}#{override}"}
+  end
+
+  defp format_tree(%{app: app, scm: scm, requirement: requirement, opts: opts}) do
+    override =
+      if opts[:override] do
+        IO.ANSI.format([:bright, " *override*"])
+      else
+        ""
+      end
+
+    requirement = requirement && "#{requirement(requirement)} "
+    {app, "#{requirement}(#{scm.format(opts)})#{override}"}
   end
 
-  defp requirement(nil), do: ""
-  defp requirement(%Regex{} = regex), do: " #{inspect regex}"
-  defp requirement(binary) when is_binary(binary), do: " #{binary}"
+  defp requirement(%Regex{} = regex), do: "#{inspect regex}"
+  defp requirement(binary) when is_binary(binary), do: binary
 
   defp find_dep(deps, app) do
     Enum.find(deps, & &1.app == app)

lib/mix/lib/mix/utils.ex

@@ -149,21 +149,22 @@ defmodule Mix.Utils do
   must either return `{printed, children}` tuple or
   `false` if the given node must not be printed.
   """
-  @spec print_tree([term], (term -> {String.t, [term]}), Keyword.t) :: :ok
-  def print_tree(nodes, callback, opts \\ []) do
+  @spec print_tree(term, (term -> {String.t, [term]}), Keyword.t) :: :ok
+  def print_tree(root, callback, opts \\ []) do
     pretty =
       case Keyword.get(opts, :format) do
         "pretty" -> true
         "plain" -> false
         _ -> elem(:os.type, 0) != :win32
       end
-    print_tree(nodes, [], pretty, callback)
+    print_tree([root], [], pretty, callback)
   end
 
   defp print_tree([], _depth, _pretty, _callback), do: :ok
   defp print_tree([node | nodes], depth, pretty, callback) do
-    {print, children} =  callback.(node)
-    Mix.shell.info("#{depth(pretty, depth)}#{prefix(pretty, depth, nodes)}#{print}")
+    {{name, info}, children} =  callback.(node)
+    space = if info, do: " ", else: ""
+    Mix.shell.info("#{depth(pretty, depth)}#{prefix(pretty, depth, nodes)}#{name}#{space}#{info}")
     print_tree(children, [(nodes != []) | depth], pretty, callback)
     print_tree(nodes, depth, pretty, callback)
   end
@@ -190,35 +191,34 @@ defmodule Mix.Utils do
   must either return `{printed, children}` tuple or
   `false` if the given node must not be printed.
   """
-  @spec build_dot_graph(String.t, [term], (term -> {String.t, [term]}), Keyword.t) :: :ok
-  def build_dot_graph(title, nodes, callback, _opts \\ []) do
-    {parent_name, _} = callback.(hd(nodes))
-    "digraph \"#{title}\" {\n" <>
-    do_build_dot_graph(parent_name, nodes, callback) <>
-    "}"
-  end
-
-  defp do_build_dot_graph(_parent, [], _callback), do: ""
-  defp do_build_dot_graph(parent, [node | nodes], callback) do
-    {name, children} = callback.(node)
-
-    parent_name = case parent do
-      {parent_name, _} -> parent_name
-      parent_name -> parent_name
+  @spec write_dot_graph!(Path.t, String.t, term, (term -> {String.t, [term]}), Keyword.t) :: :ok
+  def write_dot_graph!(path, title, root, callback, _opts \\ []) do
+    {{parent, _}, children} = callback.(root)
+    {dot, _} = build_dot_graph(parent, children, %{}, callback)
+    File.write! path, "digraph \"#{title}\" {\n#{dot}}\n"
+  end
+
+  defp build_dot_graph(_parent, [], seen, _callback), do: {"", seen}
+  defp build_dot_graph(parent, [node | nodes], seen, callback) do
+    {{name, edge_info}, children} = callback.(node)
+    {current, seen}  = build_dot_current(parent, name, edge_info, seen)
+    {children, seen} = build_dot_graph(name, children, seen, callback)
+    {siblings, seen} = build_dot_graph(parent, nodes, seen, callback)
+    {current <> children <> siblings, seen}
+  end
+
+  defp build_dot_current(parent, name, edge_info, seen) do
+    key = {parent, name}
+    case seen do
+      %{^key => _} ->
+        {"", seen}
+      %{} when is_nil(edge_info) ->
+        {~s(  "#{parent}" -> "#{name}"\n),
+         Map.put(seen, key, true),}
+      %{} ->
+        {~s(  "#{parent}" -> "#{name}" [label=\"#{edge_info}\"]\n),
+         Map.put(seen, key, true)}
     end
-
-    if parent != name do
-      case name do
-        {node_name, edge_info} ->
-          ~s(  "#{parent_name}" -> "#{node_name}" [label=\"#{edge_info}\"]\n)
-        node_name ->
-          ~s(  "#{parent_name}" -> "#{node_name}"\n)
-      end
-    else
-      ""
-    end <>
-    do_build_dot_graph(name, children, callback) <>
-    do_build_dot_graph(parent, nodes, callback)
   end
 
   @doc false

lib/mix/test/mix/tasks/app.tree_test.exs

@@ -92,7 +92,7 @@ defmodule Mix.Tasks.App.TreeTest do
           "logger" -> "elixir"
           "test" -> "app_deps_sample"
           "app_deps_sample" -> "app_deps2_sample"
-          "app_deps2_sample" -> "app_deps4_sample (included)"
+          "app_deps2_sample" -> "app_deps4_sample" [label="(included)"]
           "app_deps_sample" -> "app_deps3_sample"
         }
         """

lib/mix/test/mix/tasks/deps.tree_test.exs

@@ -117,8 +117,8 @@ defmodule Mix.Tasks.Deps.TreeTest do
 
       assert File.read!("deps_tree.dot") == """
         digraph "dependency tree" {
-          "sample" -> "git_repo" [label=" >= 0.1.0"]
-          "sample" -> "deps_on_git_repo" [label=" 0.2.0"]
+          "sample" -> "git_repo" [label=">= 0.1.0"]
+          "sample" -> "deps_on_git_repo" [label="0.2.0"]
         }
         """
 
@@ -127,8 +127,8 @@ defmodule Mix.Tasks.Deps.TreeTest do
 
       assert File.read!("deps_tree.dot") == """
         digraph "dependency tree" {
-          "sample" -> "git_repo" [label=" >= 0.1.0"]
-          "sample" -> "deps_on_git_repo" [label=" 0.2.0"]
+          "sample" -> "git_repo" [label=">= 0.1.0"]
+          "sample" -> "deps_on_git_repo" [label="0.2.0"]
           "deps_on_git_repo" -> "git_repo" [label=""]
         }
         """