Escapes a backslash inside a quoted object key

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.

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`

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

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

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)