Skip to content

Bugfix: Macro.escape/1 properly escapes meta in :quote tuples #14773

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 7 commits into from
Sep 16, 2025
+34 −21
Merged

Bugfix: Macro.escape/1 properly escapes meta in :quote tuples #14773

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dc12d87
Bugfix: Macro.escape/1 properly escapes meta in :quote tuples
sabiwara Sep 16, 2025
58e1369
escape directly calls do_escape if unquote is false
sabiwara Sep 16, 2025
ff5c1d2
Change op types to :escape | :escape_and_prune
sabiwara Sep 16, 2025
f054dd2
Attempt of skipping the clauses - fails to bootstrap
sabiwara Sep 16, 2025
c9f2b3e
Replace none -> escape, add_context -> quote
sabiwara Sep 16, 2025
8ea7ad8
Revert to previous version - not called when escaping with unquote: f…
sabiwara Sep 16, 2025
a96d1ee
Add a comment about unquote: true pitfall
sabiwara Sep 16, 2025
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
22 changes: 10 additions & 12 deletions lib/elixir/src/elixir_quote.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,23 +340,13 @@ quote(Expr, Q) ->
do_quote({quote, Meta, [Arg]}, Q) when is_list(Meta) ->
Copy link
Member

@josevalim josevalim Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should not be entering this clause altogether. If I am escaping a value, the {:quote, _, _} triplet should not have any semantic value nor behave in any special way. So I think the fix is this:

Suggested change
do_quote({quote, Meta, [Arg]}, Q) when is_list(Meta) ->
do_quote({quote, Meta, [Arg]}, #elixir_quote{op=add_context} = Q) when is_list(Meta) ->

My suggestion is actually to rename op to something clearer. Perhaps it should be:

op=quote | escape | escape_and_prune

That should make it clearer that the above should not be executed while escaping.

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 was my original attempt (actually I tried adding escape=true), but it seemed to break the compiler, we might be relying on it.

Also, with this reasoning, I was wondering: is there any reason for these other do_quote clauses, why couldn't we just call escape() -> do_escape()?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can escape -> do_escape but we need to be careful with unquote, as some escape may unquote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh gotcha, perhaps we could skip it if unquote=false then? Will give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I think I managed to split do_quote and do_escape apart: 58e1369.

But it's not directly related to the fix, still need to apply the op=quote | escape | escape_and_prune change I think, and skip these {:quote, _, _} clauses.

Copy link
Contributor Author

@sabiwara sabiwara Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My suggestion is actually to rename op to something clearer. Perhaps it should be:

op=quote | escape | escape_and_prune

Done in ff5c1d2

But then skipping the clause seems to indeed kill bootstrap - I'll need to narrow it down 😅
f054dd2

TArg = do_quote(Arg, Q#elixir_quote{unquote=false}),

NewMeta = case Q of
#elixir_quote{op=add_context, context=Context} -> keystore(context, Meta, Context);
_ -> Meta
end,

{'{}', [], [quote, meta(NewMeta, Q), [TArg]]};
{'{}', [], [quote, quote_meta(Meta, Q), [TArg]]};

do_quote({quote, Meta, [Opts, Arg]}, Q) when is_list(Meta) ->
TOpts = do_quote(Opts, Q),
TArg = do_quote(Arg, Q#elixir_quote{unquote=false}),

NewMeta = case Q of
#elixir_quote{op=add_context, context=Context} -> keystore(context, Meta, Context);
_ -> Meta
end,

{'{}', [], [quote, meta(NewMeta, Q), [TOpts, TArg]]};
{'{}', [], [quote, quote_meta(Meta, Q), [TOpts, TArg]]};

%
do_quote({unquote, Meta, [Expr]}, #elixir_quote{unquote=true, shallow_validate=Validate}) when is_list(Meta) ->
Expand Down Expand Up @@ -598,6 +588,14 @@ argument_error(Message) ->
meta(Meta, Q) ->
generated(keep(keydelete(column, Meta), Q), Q).

quote_meta(Meta, Q) ->
Meta1 = do_quote(Meta, Q),
Meta2 = case Q of
#elixir_quote{op=add_context, context=Context} -> keystore(context, Meta1, Context);
_ -> Meta1
end,
meta(Meta2, Q).

generated(Meta, #elixir_quote{generated=true}) -> [{generated, true} | Meta];
generated(Meta, #elixir_quote{generated=false}) -> Meta.

Expand Down
5 changes: 5 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 @@ -146,6 +146,11 @@ defmodule MacroTest do
assert Macro.escape({:quote, [], [[do: :foo]]}) == {:{}, [], [:quote, [], [[do: :foo]]]}
end

test "escapes the content of :quote tuples" do
assert Macro.escape({:quote, [%{}], [{}]}) ==
{:{}, [], [:quote, [{:%{}, [], []}], [{:{}, [], []}]]}
end

test "escape container when a reference cannot be escaped" do
assert_raise ArgumentError, ~r"contains a reference", fn ->
Macro.escape(%{re_pattern: {:re_pattern, 0, 0, 0, make_ref()}})
Expand Down