Jens-G commented on PR #3786:
URL: https://github.com/apache/thrift/pull/3786#issuecomment-5567271802
### Code review
Reviewed at `97efe4d3d`. Built compilers from the PR head and from the merge
base (`59a949c80`) and compared the generated constant across 24 generators.
The compiler test suite is 17/17 green on the base and 18/18 on the PR head.
Two things first, since the ticket leaves them open.
**The existing escapes are not affected.** `\r \n \t \" \' \\` all still
convert, and every input that parsed before parses to the same bytes: for a
constant exercising all six, the output of all 24 generators is byte-identical
between the two compilers. The change reaches only input that used to be a hard
error.
**`\\` already does this today, with the same result.** On the merge base,
`const string ISO8601U = "Y-m-d\\TH:i:s.uP"` compiles, and
`gen-php/Example/Constant.php` comes out byte-identical to what this PR
produces for the ticket's unescaped spelling — `return "Y-m-d\\TH:i:s.uP";`,
which PHP evaluates to `Y-m-d\TH:i:s.uP`, exactly the format string the
reporter wanted. The same holds for the other 23 generators. So the capability
is not missing; the PR removes the need to double the backslash. That is worth
having, but it is an ergonomics change rather than a bug fix, and it has costs
`\\` does not:
- **It burns the escape namespace.** After this, `"\x41"`, `"\0"` and
`"\u00e9"` have a defined meaning — the literal characters. Adding `\xNN`, `\0`
or `\uXXXX` later becomes a silent behaviour change for anyone who wrote them
expecting pass-through.
- **It makes the escape set silently context-dependent.**
`"C:\dir\file.txt"` now yields `C:\dir\file.txt`, but `"C:\new\table.txt"`
yields `C:<LF>ew<TAB>able.txt` — same shape, opposite meaning, no diagnostic
either way. Before, the first was rejected, and that rejection was the signal
that the string needed escaping.
Found 3 issues:
1. Four generators cannot carry a literal backslash, so the value the parser
now accepts does not survive code generation. Against the PR compiler, with the
ticket's own constant:
- **rust** — `pub const ISO8601U: &str = "Y-m-d\TH:i:s.uP";` → `error:
unknown character escape: 'T'`. The generated crate does not compile.
- **c_glib** — `#define ISO8601U g_strdup ("Y-m-d\TH:i:s.uP")` → gcc
`warning: unknown escape sequence: '\T'`, and the value silently becomes
`Y-m-dTH:i:s.uP`. The backslash is gone.
- **kotlin** — `const val ISO8601U: kotlin.String = "Y-m-d\TH:i:s.uP"`,
the same shape as rust. No Kotlin toolchain here, so unlike the two above this
one is by inspection, not compiled.
- **st** — emits `"Y-m-d\\TH:i:s.uP"` into a Smalltalk literal, which has
no backslash escapes, so the value gains a second backslash.
All four are reachable on master today by writing `\\`, so these are
pre-existing generator bugs rather than regressions. They matter here because
the point of the change is to make that input natural to write: after it, the
ticket's literal example silently produces a Rust crate that does not compile
and a C constant with the wrong value.
https://github.com/apache/thrift/blob/97efe4d3db67637e867bbe3c573d02d06ee5ce2b/compiler/cpp/src/thrift/thriftl.ll#L324-L329
2. This changes IDL string-literal semantics with no spec change.
[`doc/specs/idl.md`](https://github.com/apache/thrift/blob/97efe4d3db67637e867bbe3c573d02d06ee5ce2b/doc/specs/idl.md#L186-L190)
defines `Literal ::= ('"' [^"]* '"') | ("'" [^']* "'")` and documents no
escape mechanism at all — so neither the six escapes the lexer already honours
nor the new "everything else is literal" rule is written down anywhere. Since
the reporter's argument in THRIFT-4244 is precisely a reading of that grammar,
the grammar is the thing that needs to say what the answer is.
3. The regression test asserts the new behaviour but not the behaviour most
at risk from editing this switch. There is no assertion that `\n`, `\t`, `\r`,
`\"`, `\'` and `\\` still convert — a one-line constant plus one `REQUIRE`
would pin all six, and it is the only guard against a future edit to the same
`default:` arm swallowing them.
https://github.com/apache/thrift/blob/97efe4d3db67637e867bbe3c573d02d06ee5ce2b/compiler/cpp/tests/cpp/t_cpp_parser_string_constant_tests.cc#L43-L49
Three suggestions, below the bar for the list above but verified:
- The test writes its fixture into the process CWD, unlike every sibling in
that directory, which loads a committed `.thrift` via `source_dir()` +
`join_path()` (`test_enum_class.thrift`, `test_forward_setter.thrift`, …). It
also leaks the file whenever an assertion fails, since `std::remove()` sits
after the `REQUIRE`s and Catch unwinds past it.
https://github.com/apache/thrift/blob/97efe4d3db67637e867bbe3c573d02d06ee5ce2b/compiler/cpp/tests/cpp/t_cpp_parser_string_constant_tests.cc#L29-L42
- `namespace php Example` in the fixture makes the suite print `No generator
named 'php' could be found!` on every run — the standalone test binary
registers cpp/netstd/ocaml/rb only — and nothing in the test depends on it.
- `Client: cpp,php` understates the blast radius: this is a lexer change, so
it affects every binding, and the trailer feeds the CHANGES generator. Also,
`CONTRIBUTING.md` asks for a single squashed commit and there are currently two.
The `case EOF` / `case '\n'` additions inside the escape are a straight
improvement — `"abc\` at end of file used to fall into `default:` and report
`Bad escape character`.
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react
with 👎.</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
