Skip to content

Add more info for :with expressions in dbg/1 #13891

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Add more info for :with expressions in dbg/1 #13891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adb3f6a
Add more info for :with expressions in dbg/1
sabiwara Mar 18, 2023
e90c246
Add test for WithClauseError
sabiwara Oct 8, 2024
f55c6da
Reimplement without process dictionary
sabiwara Oct 9, 2024
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
97 changes: 97 additions & 0 deletions lib/elixir/lib/macro.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2674,6 +2674,84 @@ defmodule Macro do
end
end

defp dbg_ast_to_debuggable({:with, meta, args} = ast, _env) do
{opts, clauses} = List.pop_at(args, -1)

acc_var = unique_var(:acc, __MODULE__)

modified_clauses =
Enum.flat_map(clauses, fn
# We only detail assignments and pattern-matching clauses that
# can be helpful to understand how the result is constructed.
{:<-, _meta, [left, right]} ->
modified_left =
case left do
{:when, meta, [pattern, guard]} -> {:when, meta, [{pattern, quote(do: _)}, guard]}
pattern -> {pattern, quote(do: _)}
end

quote do
[
value = unquote(right),
unquote(acc_var) = [{unquote(escape(right)), value} | unquote(acc_var)],
unquote(modified_left) <- {value, unquote(acc_var)}
]
end

{:=, _meta, [left, right]} ->
quote do
[
value = unquote(right),
unquote(acc_var) = [{unquote(escape(right)), value} | unquote(acc_var)],
unquote(left) = value
]
end

# Other expressions like side effects are omitted.
expr ->
[expr]
end)

modified_opts =
Enum.map(opts, fn
{:do, do_block} ->
{:do, {do_block, acc_var}}

{:else, else_block} ->
clauses =
Enum.map(else_block, fn
{:->, meta, [[{:when, meta2, [pattern, guard]}], right]} ->
{:->, meta, [[{:when, meta2, [{pattern, acc_var}, guard]}], {right, acc_var}]}

{:->, meta, [[left], right]} ->
{:->, meta, [[{left, acc_var}], {right, acc_var}]}

invalid ->
invalid
end)

error_clause =
quote do
{other, _acc} -> raise WithClauseError, term: other
end

{:else, clauses ++ error_clause}

invalid ->
invalid
end)

modified_with_ast = {:with, meta, modified_clauses ++ [modified_opts]}

quote do
unquote(acc_var) = []

{value, acc} = unquote(modified_with_ast)

{:with, unquote(escape(ast)), Enum.reverse(acc), value}
end
end

# Any other AST.
defp dbg_ast_to_debuggable(ast, _env) do
quote do: {:value, unquote(escape(ast)), unquote(ast)}
Expand Down Expand Up @@ -2828,6 +2906,25 @@ defmodule Macro do
{formatted, result}
end

defp dbg_format_ast_to_debug({:with, ast, clauses, result}, options) do
formatted_clauses =
Enum.map(clauses, fn {clause_ast, clause_result} ->
dbg_format_ast_with_value(clause_ast, clause_result, options)
end)

formatted = [
dbg_maybe_underline("With clauses", options),
":\n",
formatted_clauses,
?\n,
dbg_maybe_underline("With expression", options),
":\n",
dbg_format_ast_with_value(ast, result, options)
]

{formatted, result}
end

defp dbg_format_ast_to_debug({:value, code_ast, value}, options) do
{dbg_format_ast_with_value(code_ast, value, options), value}
end
Expand Down
156 changes: 156 additions & 0 deletions lib/elixir/test/elixir/macro_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,162 @@ defmodule MacroTest do
"""
end

test "with with/1 (all clauses match)" do
opts = %{width: 10, height: 15}

{result, formatted} =
dbg_format(
with {:ok, width} <- Map.fetch(opts, :width),
double_width = width * 2,
IO.puts("just a side effect"),
{:ok, height} <- Map.fetch(opts, :height) do
{:ok, double_width * height}
end
)

assert result == {:ok, 300}

assert formatted =~ "macro_test.exs"

assert formatted =~ """
With clauses:
Map.fetch(opts, :width) #=> {:ok, 10}
Copy link
Member

@josevalim josevalim Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we include the format we were expecting as well? So the whole {:ok, width} <- Map.fetch...? And then the outcome in the next line:

Suggested change
Map.fetch(opts, :width) #=> {:ok, 10}
{:ok, width} <- Map.fetch(opts, :width)
#=> {:ok, 10}

Although I also worry that this will be too verbose for actual long values. Imagine long clauses with each of them returning a struct as a value (or an ecto schema).

So I do wonder if the best way to go is indeed to say which clause failed, allowing the user to further debug themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we include the format we were expecting as well? So the whole {:ok, width} <- Map.fetch...?

My thinking here is that we print the whole with just below, so just showing the expressions felt enough to debug the pipeline? As you said, I was fearing it could be too verbose otherwise.

allowing the user to further debug themselves.

I agree this might be a reasonable approach if really there is an issue with showing all steps, but given this is what we do for pipelines I don't think it should be an issue?

Most of the complexity comes from handling guards & raise, showing all steps is not the hard part I think.

Copy link
Member

Choose a reason for hiding this comment

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

My thinking here is that we print the whole with just below, so just showing the expressions felt enough to debug the pipeline?

I see, sounds good.

I agree this might be a reasonable approach if really there is an issue with showing all steps, but given this is what we do for pipelines I don't think it should be an issue?

But can we argue any other behaviour for pipelines? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But can we argue any other behaviour for pipelines? :)

Actually... :) One could argue that one might just add |> dbg() after every step they care about, and we just show the last one, in order to avoid noise. Allowing the user to further debug themselves.
I very rarely felt this way, but it did happen to me very anecdotally to fallback to |> IO.inspect after a pipeline for this reason.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. And it is actually easier to dbg individual parts in a pipeline then in a with.

I guess another way to explore this is: if we had an actual debugger, would we like to go clause by clause and else by else? The answer would be yes, so we should probably follow that principle. So this looks good to me.

width * 2 #=> 20
Map.fetch(opts, :height) #=> {:ok, 15}

With expression:
with {:ok, width} <- Map.fetch(opts, :width),
double_width = width * 2,
IO.puts("just a side effect"),
{:ok, height} <- Map.fetch(opts, :height) do
{:ok, double_width * height}
end #=> {:ok, 300}
"""
end

test "with with/1 (no else)" do
opts = %{width: 10}

{result, formatted} =
dbg_format(
with {:ok, width} <- Map.fetch(opts, :width),
{:ok, height} <- Map.fetch(opts, :height) do
{:ok, width * height}
end
)

assert result == :error

assert formatted =~ "macro_test.exs"

assert formatted =~ """
With clauses:
Map.fetch(opts, :width) #=> {:ok, 10}
Map.fetch(opts, :height) #=> :error

With expression:
with {:ok, width} <- Map.fetch(opts, :width),
{:ok, height} <- Map.fetch(opts, :height) do
{:ok, width * height}
end #=> :error
"""
end

test "with with/1 (else clause)" do
opts = %{width: 10}

{result, formatted} =
dbg_format(
with {:ok, width} <- Map.fetch(opts, :width),
{:ok, height} <- Map.fetch(opts, :height) do
width * height
else
:error -> 0
end
)

assert result == 0
assert formatted =~ "macro_test.exs"

assert formatted =~ """
With clauses:
Map.fetch(opts, :width) #=> {:ok, 10}
Map.fetch(opts, :height) #=> :error

With expression:
with {:ok, width} <- Map.fetch(opts, :width),
{:ok, height} <- Map.fetch(opts, :height) do
width * height
else
:error -> 0
end #=> 0
"""
end

test "with with/1 (guard)" do
opts = %{width: 10, height: 0.0}

{result, formatted} =
dbg_format(
with {:ok, width} when is_integer(width) <- Map.fetch(opts, :width),
{:ok, height} when is_integer(height) <- Map.fetch(opts, :height) do
width * height
else
_ -> nil
end
)

assert result == nil
assert formatted =~ "macro_test.exs"

assert formatted =~ """
With clauses:
Map.fetch(opts, :width) #=> {:ok, 10}
Map.fetch(opts, :height) #=> {:ok, 0.0}
Copy link
Contributor

@dkuku dkuku Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a good example that shows that we should add a bit more info why this failed - it looks ok at the beginning but it's the problematic line that did fail and you have to jump back and forth to compare.
Often you're debugging code written by someone else and there may be more lines inbetween.
It can even use elixir syntax:

match?({:ok, height} when is_integer(height), {:ok, 0.0}) #=> false
or 
{:ok, height} when is_integer(height) <- {:ok, 0.0}

Copy link
Contributor

Choose a reason for hiding this comment

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

Another example is pattern match on map key but the map is not including the key. This is one of the hardest to spot for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about showing it this way?

             {:ok, width} <- {:ok, 10}
             {:ok, height} <- :error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a trade-off, cf discussion above.
The risk is to get unwieldy and cluttered logs for bigger patterns, especially since the useful info is the value itself and the pattern is redundant since it's being logged just after:

Screenshot 2024-10-10 at 7 33 05

This approach is also consistent with what we do for case: we show the value being matched followed by the whole expression, without highlighting each pattern that matched or didn't:

Screenshot 2024-10-10 at 7 29 23

So, I think it's a reasonable trade-off, even if I understand for this small example that it might look nice.

Another possibility to address the "clutter" concern could be to inspect the pattern algebra with a limit, but I don't think that's possible today.

However, if I'm overthinking the verbosity concern and everybody is convinced of the benefit, I could easily change the code to replace with the whole arrow node.


With expression:
with {:ok, width} when is_integer(width) <- Map.fetch(opts, :width),
{:ok, height} when is_integer(height) <- Map.fetch(opts, :height) do
width * height
else
_ -> nil
end #=> nil
"""
end

test "with with/1 (guard in else)" do
opts = %{}

{result, _formatted} =
dbg_format(
with {:ok, width} <- Map.fetch(opts, :width) do
width
else
other when is_integer(other) -> :int
other when is_atom(other) -> :atom
end
)

assert result == :atom
end

test "with with/1 respects the WithClauseError" do
value = Enum.random([:unexpected])

error =
assert_raise WithClauseError, fn ->
dbg(
with :ok <- value do
true
else
:error -> false
end
)
end

assert error.term == :unexpected
end

test "with \"syntax_colors: []\" it doesn't print any color sequences" do
{_result, formatted} = dbg_format("hello")
refute formatted =~ "\e["
Expand Down
Loading