From 1581bf8093f27f19d6eb8f16749b985ecf53d269 Mon Sep 17 00:00:00 2001 From: JohnnyT Date: Sat, 5 Sep 2026 07:05:45 -0600 Subject: [PATCH] Escapes a backslash inside a quoted object key format_object_key/1 escaped the quote character of the style it was writing but never the escape character itself - the same defect px-v3b had just fixed one screen above it, in the same visitor. An object key carrying a backslash rendered it unescaped, so the source parsed back to a different key with the backslash silently gone, and a key ending in one rendered an unterminated literal that did not parse at all. A quoted key is read by the same lexer string rule as a literal, so it always owed the same escaping. Both quoted clauses now route through escape_within/2, the helper px-v3b introduced, rather than growing a second implementation: two escape paths in one writer is how this defect outlived the first fix. The identifier style is untouched - the parser only produces it for a bare identifier, which carries neither a quote nor a backslash. The corpus the escaping is tested over moves to Predicator.EscapeCorpus in test_helper.exs, matching this repo's test-support pattern, so the literal suite and the new object-key suite enumerate the same awkward cases from one place instead of drifting apart. Refs: px-0tz --- changelog.d/px-0tz.md | 8 ++ lib/predicator/visitors/string_visitor.ex | 17 ++- .../visitors/string_visitor_escape_test.exs | 26 +--- .../string_visitor_object_key_escape_test.exs | 114 ++++++++++++++++++ test/test_helper.exs | 48 ++++++++ 5 files changed, 189 insertions(+), 24 deletions(-) create mode 100644 changelog.d/px-0tz.md create mode 100644 test/predicator/visitors/string_visitor_object_key_escape_test.exs diff --git a/changelog.d/px-0tz.md b/changelog.d/px-0tz.md new file mode 100644 index 00000000..1775aae5 --- /dev/null +++ b/changelog.d/px-0tz.md @@ -0,0 +1,8 @@ +### Fixed + +- The string writer escapes a backslash inside a quoted object key, in both + quote styles, so `{'a\b': 1}` survives parse-then-decompile instead of + rendering source that parses back to a different key - or, for a key ending + in a backslash, source that does not parse at all. Quoted keys now go + through the same escaping helper as string literals rather than a second + copy of it. diff --git a/lib/predicator/visitors/string_visitor.ex b/lib/predicator/visitors/string_visitor.ex index 2b0d8c02..bd544af4 100644 --- a/lib/predicator/visitors/string_visitor.ex +++ b/lib/predicator/visitors/string_visitor.ex @@ -479,14 +479,27 @@ defmodule Predicator.Visitors.StringVisitor do |> String.replace(quote_char, "\\" <> quote_char) end + # px-0tz: a quoted object key is read by the same string rule as a string + # literal (`parse_object_key/1` in `parser.ex` consumes a STRING token), so + # it needs exactly the escaping `quoted_string/2` above applies and for the + # same reasons - a key carrying a backslash used to render it unescaped and + # parse back as a different key, silently. Both quoted clauses therefore go + # through `escape_within/2` rather than escaping the quote character alone: + # two escape implementations in one writer is how this defect outlived the + # px-v3b fix that introduced the helper. + # + # The identifier style has no escaping to do. The parser only ever produces + # it for a bare identifier, which by construction contains neither a quote + # nor a backslash, and quoting one on the writer's own initiative would + # switch a style the writer is required to render as asked for. @spec format_object_key(Parser.object_key()) :: binary() defp format_object_key({:object_key, value, :identifier, _position}), do: value defp format_object_key({:object_key, value, :double, _position}), - do: ~s("#{String.replace(value, "\"", "\\\"")}") + do: ~s("#{escape_within(value, "\"")}") defp format_object_key({:object_key, value, :single, _position}), - do: ~s('#{String.replace(value, "'", "\\'")}') + do: ~s('#{escape_within(value, "'")}') # ADR-0013: `else if` is parser sugar with no AST node of its own - the # else slot is a synthetic block holding exactly one `if` diff --git a/test/predicator/visitors/string_visitor_escape_test.exs b/test/predicator/visitors/string_visitor_escape_test.exs index 10477e37..3ecb67b9 100644 --- a/test/predicator/visitors/string_visitor_escape_test.exs +++ b/test/predicator/visitors/string_visitor_escape_test.exs @@ -25,28 +25,10 @@ defmodule Predicator.Visitors.StringVisitorEscapeTest do alias Predicator.Visitors.StringVisitor # Every character the lexer's string rule gives meaning to, plus ordinary - # text to sit them next to. `take_string/6` in `lexer.ex` decodes `\\`, `\"`, - # `\'`, `\n`, `\t` and `\r`, and keeps a raw newline, tab or return verbatim. - @special ["\\", "'", "\"", "\n", "\t", "\r"] - @filler ["", "a", "ab"] - - defp values do - singles = @special ++ @filler - - pairs = - for left <- @special, right <- @special, do: left <> right - - surrounded = - for prefix <- @filler, - special <- @special, - suffix <- @filler, - do: prefix <> special <> suffix - - triples = - for left <- @special, right <- @special, do: left <> right <> "z" - - Enum.uniq(singles ++ pairs ++ surrounded ++ triples) - end + # text to sit them next to. The enumeration lives in + # `Predicator.EscapeCorpus` (test_helper.exs) because the object-key suite + # (px-0tz) is read back by the same lexer rule and needs the same cases. + defp values, do: Predicator.EscapeCorpus.values() describe "a string literal renders to source that parses back to an equal node" do test "for every value in the corpus, in both quote styles" do diff --git a/test/predicator/visitors/string_visitor_object_key_escape_test.exs b/test/predicator/visitors/string_visitor_object_key_escape_test.exs new file mode 100644 index 00000000..2c37521f --- /dev/null +++ b/test/predicator/visitors/string_visitor_object_key_escape_test.exs @@ -0,0 +1,114 @@ +defmodule Predicator.Visitors.StringVisitorObjectKeyEscapeTest do + @moduledoc """ + px-0tz: `format_object_key/1` in `StringVisitor` escaped the quote character + of the style it was writing but never the escape character itself, so a + quoted object key containing a backslash rendered to source the parser reads + differently - the backslash silently gone, or, for a key ending in one, an + unterminated literal that does not parse at all. + + A quoted object key is lexed by the same string rule as a string literal + (`parse_object_key/1` in `parser.ex` reads a STRING token), so the law under + test is the same round trip px-v3b pinned for literals: for every key and + every quote style, the source the writer produces parses back to an object + carrying an equal key, in the style it was asked for. That is why the fix + routes both quoted clauses through the writer's shared escaping helper + rather than growing a second implementation - two escape paths in one writer + is how this defect survived the first fix. + + The corpus is enumerated from the characters the lexer treats specially + rather than transcribed from examples, so it ranges over the combinations - + a backslash immediately before the quote is the case a naive escape order + gets wrong - instead of over the ones someone happened to think of. + """ + + use ExUnit.Case, async: true + + alias Predicator.Visitors.StringVisitor + + # The same enumeration px-v3b built for literals, shared from + # `Predicator.EscapeCorpus` (test_helper.exs): a quoted key is read by the + # same lexer rule, so the awkward cases are the same ones and the two suites + # should not drift apart. + defp keys, do: Predicator.EscapeCorpus.values() + + defp render(key, style) do + StringVisitor.visit( + {:object, [{{:object_key, key, style, nil}, {:literal, 1, nil}}], nil}, + [] + ) + end + + describe "a quoted object key renders to source that parses back to an equal key" do + test "for every key in the corpus, in both quote styles" do + corpus = keys() + assert length(corpus) > 50 + + for key <- corpus, style <- [:single, :double] do + source = render(key, style) + + assert {:ok, {:object, [{parsed_key, _value}], _position}} = Predicator.parse(source), + "rendered #{inspect(source)} for key #{inspect(key)} in #{style} quotes, " <> + "which does not parse" + + assert {:object_key, key, style, elem(parsed_key, 3)} == parsed_key, + "key #{inspect(key)} in #{style} quotes rendered as #{inspect(source)}, " <> + "which parses to #{inspect(parsed_key)}" + end + end + + test "parse -> decompile -> parse is a fixed point for every key" do + for key <- keys(), style <- [:single, :double] do + expression = render(key, style) + + assert {:ok, ast} = Predicator.parse(expression) + written = Predicator.decompile(ast) + + assert {:ok, reparsed} = Predicator.parse(written), + "#{inspect(expression)} decompiled to #{inspect(written)}, which does not parse" + + assert reparsed == ast, + "#{inspect(expression)} decompiled to #{inspect(written)}, which parses to a " <> + "different object" + end + end + end + + describe "the named cases the bead calls out" do + test "a key that is a lone backslash is a terminated literal in both styles" do + assert render("\\", :single) == ~S({'\\': 1}) + assert render("\\", :double) == ~S({"\\": 1}) + end + + test "a backslash immediately before the quote escapes as two, not as one" do + # Escaping the quote first would produce `{'\\'': 1}`, whose trailing + # quote closes the key one character early. + assert render("\\'", :single) == ~S({'\\\'': 1}) + assert render("\\\"", :double) == ~S({"\\\"": 1}) + end + + test "the quote character of the style is escaped, not switched away from" do + assert render("'", :single) == ~S({'\'': 1}) + assert render("\"", :double) == ~S({"\"": 1}) + end + + test "the other style's quote character needs no escape" do + assert render("\"", :single) == ~S({'"': 1}) + assert render("'", :double) == ~S({"'": 1}) + end + + test "an ordinary key gains no escapes it does not need" do + assert render("first name", :single) == "{'first name': 1}" + assert render("first name", :double) == ~s({"first name": 1}) + end + end + + describe "the identifier style is untouched" do + test "an unquoted key renders bare and round-trips" do + source = render("cardholder_name", :identifier) + assert source == "{cardholder_name: 1}" + + assert {:ok, {:object, [{{:object_key, "cardholder_name", :identifier, _p}, _v}], _pos}} = + Predicator.parse(source) + end + end +end diff --git a/test/test_helper.exs b/test/test_helper.exs index bbd7b9f0..4031536a 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -313,6 +313,54 @@ defmodule Predicator.Conformance.SchemaValidator do end end +defmodule Predicator.EscapeCorpus do + @moduledoc """ + The corpus of awkward values the string writer's escaping is tested over. + + Enumerated from the characters the lexer's string rule gives meaning to + rather than transcribed from examples, so it ranges over the combinations - + a backslash immediately before a quote is the case a naive escape order gets + wrong - instead of over the ones someone happened to think of. + + Shared by `string_visitor_escape_test.exs` (px-v3b, string literals) and + `string_visitor_object_key_escape_test.exs` (px-0tz, quoted object keys) + because both surfaces are read back by the same lexer rule and so have the + same awkward cases. Lives in `test_helper.exs`, matching this repo's + existing test-support pattern (`Predicator.SpanSlicing`, + `Predicator.ASTShape` above) rather than a `test/support/` directory, which + would need an `elixirc_paths` change to `mix.exs`. + """ + + # `take_string/6` in `lexer.ex` decodes `\\`, `\"`, `\'`, `\n`, `\t` and + # `\r`, and keeps a raw newline, tab or return verbatim. + @special ["\\", "'", "\"", "\n", "\t", "\r"] + @filler ["", "a", "ab"] + + @doc "The characters the lexer's string rule treats specially." + @spec special() :: [binary()] + def special, do: @special + + @doc "Every corpus value: the specials alone, in pairs, and surrounded by ordinary text." + @spec values() :: [binary()] + def values do + singles = @special ++ @filler + + pairs = + for left <- @special, right <- @special, do: left <> right + + surrounded = + for prefix <- @filler, + special <- @special, + suffix <- @filler, + do: prefix <> special <> suffix + + triples = + for left <- @special, right <- @special, do: left <> right <> "z" + + Enum.uniq(singles ++ pairs ++ surrounded ++ triples) + end +end + # Ensure the Predicator application is started before tests run # This ensures system functions are registered and available Application.ensure_all_started(:predicator)