Bugfix: Macro.escape/1 properly escapes meta in :quote tuples #14773
Conversation
|
|
||
| %% quote/unquote | ||
|
|
||
| do_quote({quote, Meta, [Arg]}, Q) when is_list(Meta) -> |
There was a problem hiding this comment.
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:
| 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.
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
Yes, we can escape -> do_escape but we need to be careful with unquote, as some escape may unquote.
There was a problem hiding this comment.
Oh gotcha, perhaps we could skip it if unquote=false then? Will give it a try.
There was a problem hiding this comment.
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.
lib/elixir/src/elixir_quote.erl
Outdated
| file=nil, | ||
| context=nil, | ||
| op=none, % none | prune_metadata | add_context | ||
| op=none, % none | escape | escape_and_prune | add_context |
There was a problem hiding this comment.
It should be:
| op=none, % none | escape | escape_and_prune | add_context | |
| op=escape, % escape | escape_and_prune | quote |
There was a problem hiding this comment.
Oh I thought elixir_quote:quote would be adding :none too, but turns out it's add_context. Got it!
Close #14771 Also internally renames the `op` field inside `elixir_quote`: `none -> escape`, `prune_metadata` -> `escape_and_prune`, `add_context -> quote`.
|
Added the doc comment a96d1ee and backported ✅ |
2 participants
Close #14771