Macro.escape/1 does not properly escape {:quote, _, _} tuples

Question

Macro.escape/1 does not properly escape {:quote, _, _} tuples

Closed this issue 2 months ago · 13 comments

Schultzer commented 2 months ago

Elixir and Erlang/OTP versions

Erlang/OTP 28 [erts-16.0.2] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]

Elixir 1.18.4 (compiled with Erlang/OTP 28)

Operating system

macOS

Current behavior

Currently Macro.escape/1 raises a compilation error when the list of tokens ends on:
{:quote, [span: {0, 108, 0, 217}, offset: {0, 1}, type: :literal, file: "iex"], ~c"f"}

~SQL[SELECT A FROM TABLE_E061_05_01_01 WHERE A LIKE 'foo' ESCAPE 'f']

error: invalid quoted expression: {0, 108, 0, 217}

Please make sure your quoted expressions are made of valid AST nodes. If you would like to introduce a value into the AST, such as a four-element tuple or a map, make sure to call Macro.escape/1 before
└─ iex

** (CompileError) cannot compile code (errors have been logged)

[
  {:select, [span: {0, 1, 0, 6}, offset: {0, 0}, type: :reserved, file: "iex"],
   [
     {:ident,
      [
        span: {0, 8, 0, 8},
        offset: {0, 1},
        type: :non_reserved,
        tag: :a,
        file: "iex"
      ], ~c"A"}
   ]},
  {:from, [span: {0, 10, 0, 13}, offset: {0, 1}, type: :reserved, file: "iex"],
   [
     {:ident,
      [span: {0, 15, 0, 33}, offset: {0, 1}, type: :literal, file: "iex"],
      ~c"TABLE_E061_05_01_01"}
   ]},
  {:where, [span: {0, 35, 0, 39}, offset: {0, 1}, type: :reserved, file: "iex"],
   [
     {:ident,
      [
        span: {0, 41, 0, 41},
        offset: {0, 1},
        type: :non_reserved,
        tag: :a,
        file: "iex"
      ], ~c"A"},
     {:like,
      [span: {0, 43, 0, 46}, offset: {0, 1}, type: :reserved, file: "iex"], []},
     {:quote,
      [span: {0, 48, 0, 99}, offset: {0, 1}, type: :literal, file: "iex"],
      ~c"foo"},
     {:escape,
      [span: {0, 101, 0, 106}, offset: {0, 1}, type: :reserved, file: "iex"],
      []},
     {:quote,
      [span: {0, 108, 0, 217}, offset: {0, 1}, type: :literal, file: "iex"],
      ~c"f"}
   ]}
]

Removing the last qoute or using double_qoute works fine or changing the span key to not be a tuple:

~SQL[SELECT A FROM TABLE_E061_05_01_01 WHERE A LIKE 'foo' ESCAPE]

~SQL[SELECT A FROM TABLE_E061_05_01_01 WHERE A LIKE 'foo' ESCAPE "f"]

Expected behavior

Should not raise.

Answer 1 · 2025-09-15T22:56:29.000Z

Thank you @Schultzer for reporting! 💜

The problem doesn't seem to be due to tuples, but to fail to escape list in {:quote, list, [_]}.
The compilation error happens later because it results in invalid AST.

iex>  Macro.escape({:quote, [%{}, {}], [:x]})
{:{}, [], [:quote, [%{}, {}], [:x]]}
iex>  Macro.escape({:foo, [%{}, {}], [:x]})
{:{}, [], [:foo, [{:%{}, [], []}, {:{}, [], []}], [:x]]}

Looking at the code and tests, it looks like intended behavior, but it feels weird to give special treatment to some AST tuples, I feel we should always generate the AST corresponding to the given term (even if it is the AST of the AST)?
e.g. escape should just be calling do_escape, not do_quote?

Answer 2 · 2025-09-15T23:51:25.000Z

Thank you @Schultzer for reporting! 💜

The problem doesn't seem to be due to tuples, but to fail to escape list in {:quote, list, [_]}. The compilation error happens later because it results in invalid AST.

iex> Macro.escape({:quote, [%{}, {}], [:x]})
{:{}, [], [:quote, [%{}, {}], [:x]]}
iex> Macro.escape({:foo, [%{}, {}], [:x]})
{:{}, [], [:foo, [{:%{}, [], []}, {:{}, [], []}], [:x]]}
Looking at the code and tests, it looks like intended behavior, but it feels weird to give special treatment to some AST tuples, I feel we should always generate the AST corresponding to the given term (even if it is the AST of the AST)? e.g. escape should just be calling do_escape, not no_quote?

That’s weird because this issue only surfaced when I added the four element tuple, if I change the tuple to a list then there is no issue.

Is it possible to get a stack trace to see where it fails, I took a look at the underlying module but as you said, it happens latter in the compilation stage.

Answer 3 · 2025-09-15T23:58:41.000Z

if I change the tuple to a list then there is no issue.

Yes because lists are their own AST (and therefore valid AST).

Is it possible to get a stack trace to see where it fails, I took a look at the underlying module but as you said, it happens latter in the compilation stage.

It's tricky, we added shallow validation to detect obvious mistakes like forgetting to call Macro.escape before injecting inside unquote, but doing a deep check that you're building valid AST would be expensive.

Answer 4 · 2025-09-16T02:44:22.000Z

@sabiwara I was thinking the same but there is indeed a bug here, no? Escape should escape everything, including metadata, and it seems we are assuming metadata to always be valid, but that should not be true for escape?

Answer 5 · 2025-09-16T05:17:03.000Z

@josevalim indeed we're escaping the arg but not the meta, seems like doing this should be enough to fix the bug for now. Sent a PR doing this.

But on a more philosophical level, I'm wondering why we need special treatment for these AST tuples, given they are just terms that could be escaped as such, disregarding what their AST represents?

Answer 6 · 2025-09-16T07:05:29.000Z

Because escape must return a valid AST that when evaluated becomes the given term (which may be invalid AST). Given a four element tuple is invalid AST, we need to escape it.

Answer 7 · 2025-09-16T07:29:07.000Z

Because escape must return a valid AST that when evaluated becomes the given term (which may be invalid AST). Given a four element tuple is invalid AST, we need to escape it.

My point is that we should always escape tuples with the same logic: {x, y} -> {escape(x), escape(y)}, anything else {:{}, [], escape(Tuple.to_list(tuple)}. The fact the tuple before escaping was AST is relevant, we can return valid AST for it with this simple algo, no?

Answer 8 · 2025-09-16T07:34:42.000Z

Yes. The issue is that we were skipping the simple algorithm because the node name accidentally matched :quote.

Answer 9 · 2025-09-16T19:53:35.000Z

Hmm, I'm not sure if this is entirely fixed, I just pulled down main with the last commit 29e8883 I use mise so maybe I'm doing this wrong.

➜  sql git:(main) ✗ mise use elixir@path:/Users/benjaminschultzer/src/elixir
mise ~/src/sql/mise.toml tools: elixir@path:/Users/benjaminschultzer/src/elixir

➜  sql git:(main) ✗ elixir -v
Erlang/OTP 28 [erts-16.0.3] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]

Elixir 1.20.0-dev (29e8883) (compiled with Erlang/OTP 28)

➜  sql git:(main) ✗ mix test
error: invalid quoted expression: {0, 108, 0, 217}

Please make sure your quoted expressions are made of valid AST nodes. If you would like to introduce a value into the AST, such as a four-element tuple or a map, make sure to call Macro.escape/1 before
└─ test/conformance/e_test.exs


== Compilation error in file test/conformance/e_test.exs ==
** (CompileError) test/conformance/e_test.exs: cannot compile file (errors have been logged)
    (sql 0.4.0) expanding macro: SQL.sigil_SQL/2

Erlang/OTP 28 [erts-16.0.3] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]

Interactive Elixir (1.20.0-dev) - press Ctrl+C to exit (type h() ENTER for help)
The database for SQL.Repo has already been created
iex(1)> ~SQL[SELECT A FROM TABLE_E061_05_01_01 WHERE A LIKE 'foo' ESCAPE 'f']
error: invalid quoted expression: {0, 108, 0, 217}

Please make sure your quoted expressions are made of valid AST nodes. If you would like to introduce a value into the AST, such as a four-element tuple or a map, make sure to call Macro.escape/1 before
└─ iex

** (CompileError) cannot compile code (errors have been logged)

iex(1)>

Could this have something to do with I'm calling Macro.escape/1 on the struct https://github.com/elixir-dbvisor/sql/blob/581b51445382aa24c0cf4f8fbf82cbfa9b65e6c5/lib/sql.ex#L113 ?

Answer 10 · 2025-09-16T20:09:49.000Z

@Schultzer do you have a branch with a failing test that I can try?

Answer 11 · 2025-09-16T20:13:52.000Z

Here is a WIP branch https://github.com/elixir-dbvisor/sql/tree/elixir, not all of my tests are green but that shouldn't get in your way.

@sabiwara I've added a commit with the specific failing test: elixir-dbvisor/sql@d837243

Answer 12 · 2025-09-16T20:25:29.000Z

@Schultzer awesome thanks! I could reproduce indeed.

Answer 13 · 2025-09-16T20:53:28.000Z

I managed to isolate it: the last element of a list is not properly escaped:

iex> Macro.escape [{:quote, [span: {}], [true]}, {:quote, [span: {}], [false]}]
[
  {:{}, [], [:quote, [span: {:{}, [], []}], [true]]},
  {:{}, [], [:quote, [span: {}], [false]]}
]

We need to rewrite do_escape([H | T],, it still calls some do_quote* functions. Will try to push a fix tomorrow.