Normalize string literal representation across the compiler - #8606
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
a6f1445 to
c292e07
Compare
cknitt
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6f1445259
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8606 +/- ##
==========================================
+ Coverage 77.00% 77.32% +0.32%
==========================================
Files 467 467
Lines 62461 63342 +881
==========================================
+ Hits 48100 48982 +882
+ Misses 14361 14360 -1
🚀 New features to boost your workflow:
|
|
Heads-up on a likely conflict with #8608 (merging the Lam IR into Lambda), and a suggestion on ordering. Overlap: 26 compiler files. The part that matters is that this PR modifies seven files that #8608 deletes:
About +57 / −31 in total. As far as I can tell those edits are all the same thing: propagating the changed Suggestion: land #8608 first. The reason is that this duplication is exactly what it removes. After it, The reverse order looks worse: #8608 would have to carry this PR's in-flight string representation into files it is simultaneously deleting. Same work, harder direction. It would also cost that PR its main verification property — it claims generated JavaScript is unchanged, checked against runtime, Belt and the 620 modules in One more thing worth knowing: No urgency from my side — mostly flagging it early so the order is a decision rather than a surprise at rebase time. |
c292e07 to
513d122
Compare
|
@codex review |
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cdf45ee to
3ceae27
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ceae2769a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 373a3cea69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cristianoc
left a comment
cristianoc
left a comment
There was a problem hiding this comment.
Reviewed by diffing pr8605...pr8606, building both branches, running ounit_tests and syntax_tests (both green here), and running a differential: rebuilding the whole @rescript/runtime (105 modules) and tests/tests/src (356 modules) with both compilers and diffing the emitted JS.
That differential is a strong result and worth stating up front: 0 diffs across the 105 runtime modules, and of 356 test modules only 9 differ — every one an improvement ("\x61b" → "ab", "'" === "\'" folded to true, "variant0" !== "variant0" folded away, cross-delimiter concat folding now working). The {source; semantic} split is the right factoring and it visibly pays for itself.
Below are the problems I could reproduce, each with the design decision behind it where there is one.
1. Compiler crash on a character range spanning the surrogate block (regression)
let f = c =>
switch c {
| '\u{D7FE}' .. '\u{E001}' => 1
| _ => 0
}$ bsc -nostdlib -nopervasives repro.res
Fatal error: exception Invalid_argument("DFFF is not a Unicode scalar value")
exit code 2, no location. Backtrace:
Called from Ext_utf8.encode_codepoint in file "compiler/ext/ext_utf8.ml", line 43
Called from Typecore.type_pat_aux.char in file "compiler/ml/typecore.ml", line 1361
Called from Typecore.type_pat_aux.loop in file "compiler/ml/typecore.ml", line 1365
pr8605 compiles this fine. 'a' .. '\u{FFFF}' hits it too; any interval crossing D800..DFFF does.
Cause, and the design point behind it: Ext_utf8.encode_codepoint and Res_utf8.encode_code_point were turned from total functions into partial ones — Uchar.of_int raises on surrogates and values above U+10FFFF — without any signature change to flag it. Three callers still hand them a raw int that is not guaranteed to be a scalar value:
typecore.ml:1358-1365, thePpat_intervalexpansion, which walks every code point in the range;Ext_util.string_of_int_as_char(reached fromjs_dump.ml:770andpprintast.ml:244);ast_mapper_from0.ml:107, reconstructingPconst_charsource across the PPX bridge.
The interval expansion is the reachable one. It calls String_literal.encode_char_source on every code point purely to synthesize a source field for ghost patterns that are never printed. Cheapest fixes: make encode_char_source total for non-scalar values, or skip the source for the synthetic patterns.
(Aside: Res_utf8.encode_code_point now has no callers at all — it is dead, but still exported in res_utf8.mli.)
2. decode_js_template_escapes accepts escapes that JavaScript rejects in templates, and the emitted JS does not parse
let a = `a\1b`compiles clean and emits let a = `a\1b`;, which node rejects:
SyntaxError: Octal escape sequences are not allowed in template strings.
Same for `a\01b` and `a\8b`. In string_literal.ml, \1/\8 fall through to the "non-escape character" branch (lines 117-123) and \0 (line 89) does not check for a following digit.
This symptom predates the PR — pr8605 emits the same broken output — so it is not a regression. I am raising it here because this PR is where "segments are validated" becomes the stated contract and where source is emitted verbatim by design, so the new validator is the natural home for the missing rejection. As it stands the semantic value and the emitted source disagree, and the emitted source is not valid JavaScript.
3. Template escape errors are type errors with whole-template locations, and the formatter accepts them
let a = `bad \xZZ escape`
let b = "bad \xZZ escape"b: syntax error at1:14-15, "unknown escape sequence".a:res_parser -print resaccepts and formats it;bscreports "We've found a bug for you!" at1:9-25with "Invalid string escape sequence".
Same class of mistake, two phases, two messages, two locations — and only one of them is caught by rescript format or by editor syntax diagnostics.
Design point: the validation boundary is split three ways. Ordinary strings validate in the parser (res_core.ml:1003), template patterns validate in the parser (parse_template_constant), but template expressions validate in Typecore (typecore.ml:2548). The invalid_ordinary_template_escape.res.expected fixture bakes the asymmetry in. Validating segments in parse_template_expr, with per-segment locations, would collapse all three onto one rule.
4. Invalid UTF-8 is reported as an escape-sequence error, and the diagnostic can be swallowed
A .res file containing a raw 0xFF byte inside a string literal:
let a = "bad <0xFF> byte"now reports "Invalid string escape sequence" — there is no escape sequence involved. pr8605 said "Invalid code point", which was accurate.
Worse, parse_string_constant gates the diagnostic on if p.diagnostics = [] (res_core.ml:1007), so with an earlier parse error it disappears entirely:
let x = (1,
let a = "bad <0xFF> byte"reports only the first error. This is exactly the diagnostic-loss fixed for templates in 8a6b38d ("Report invalid template escapes after earlier diagnostics"); the string path still has it.
5. Codegen quality regressions
Both correct, both uglier than before.
external \"++": (string, string) => string = "%string_concat"
let a = `abc` ++ `def`// pr8605
let a = `abcdef`;
// pr8606
let a = `abc` + `def`;And tests/tests/src/template.res:
// pr8605
return `
display:\r flex;
` + bla2;
// pr8606
return `${`
display:\r flex;
`}${bla2}`;Design point: Template_literal {source; semantic} propagates a stylistic goal — preserving backtick spelling in the output — through the entire backend, and every optimization then has to pick a field. concat_string_literals (js_exp_make.ml:676) simply returns None whenever either side is backquoted, and nothing collapses a Template_literal that has been inlined into an interpolation slot back into the surrounding segments. Worth considering whether Template_literal should decay to Str once the source spelling has served its purpose, or whether js_dump should choose backticks from the semantic value instead of carrying the source down.
|
Follow-up to my review above: one more reproducible regression, on a different axis — the ast0 PPX bridge. An identity PPX stops being a no-op. Repro
So the same file compiles without a PPX and fails with an identity PPX, and only on this branch. It is a clean diagnostic, not a crash (exit 2, proper location, no backtrace). MechanismDoc-comment text is raw source bytes that never go through the string scanner, so nothing has UTF-8-validated it:
Step 3 is what this PR adds; on ScopeNarrow, and the rest of the bridge looks solid. I rebuilt all 356 modules of Underlying invariantThis is the So the fix has to pick a place to establish "semantic is valid UTF-8":
Patching only |
|
One more comment, and this one is a tentative design direction rather than a review finding — I have not tried it, and I am not confident every part survives contact with the code. Take it as a sketch to poke holes in. The bug I reported just above and the The invariant, stated
SketchMake the payload a private record in a module that has an (* string_literal.mli *)
type t = private {source: string; semantic: string}
val of_source : string -> t option (* parser: preserves spelling *)
val of_semantic : string -> t option (* compiler: canonical spelling *)with Two caveats I am reasonably confident about:
The part I am least sure about: is "every source must validate" too strict?There are fewer construction sites than one might fear:
All but one are already either "decode succeeded" or "derive canonically from semantic". The single site that genuinely wants an invalid node is the parser's error recovery: Parsetree.Pconst_string {source; semantic = ""} (* res_core.ml:1010 *)As far as I can tell that path is already meant to be unreachable: everything If that holds, the direction would be to move UTF-8 validation into the scanner (it already walks the bytes and already uses The practical risk there, and the reason this needs validating rather than just doing: rejecting non-UTF-8 in comments is a behaviour change for files that compile today. Whether that is acceptable, or whether comment text should be sanitised instead, is a call I do not have the context to make. If a recovery node is wanted anyway — defensible, since the formatter and editor would like to format a file that has one bad escape — then it seems better to make the invalidity visible than to fake it with Side benefit, if it works outTwo nominal types would also separate something that is currently structural: external slength: string => int = "%string_length"
let a = slength("x<CR><LF>y") // 4
let b = slength(`x<CR><LF>y`) // 3And |
|
Thanks, this is a useful framing. I agree that For this PR I have kept the fix narrow in I would prefer to handle private payload types in a focused follow-up. That design also needs an explicit recovery policy: Agreed as well that validation in |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b3929924
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
269b045 to
c08c60f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 269b0458d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Re-ran every repro from my review and the follow-up against the updated branch (rebuilt from scratch, since the artifact magic numbers changed). All of them are fixed.
Corpus re-verification: runtime 105 modules, 0 JS diffs against base; 356 modules of No over-rejection either — a template exercising
One thing left: changelogThere are still three entries for this PR — tagged templates in patterns, surrogate-pair support, pattern-matching equivalence — and none of them covers the new rejections. At least one is a breaking change for code that compiles on Same for legacy escapes in templates ( Not re-raising the other two loose ends: char source spelling not surviving the AST0 bridge is already documented in |
|
@cristianoc Thanks for the thorough re-verification, and agreed on the changelog gap. Added a Breaking Change entry in a811a5b covering the upgrade-visible rejection cases: malformed UTF-8 in documentation comments, empty/out-of-range braced Unicode escapes, and legacy decimal/octal escapes in template literals. |
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
6b82359 to
de67df7
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de67df7164
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8606 |
Signed-off-by: Christoph Knittel <ck@cca.io>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acbe9742f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Christoph Knittel <ck@cca.io>
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cristianoc
left a comment
cristianoc
left a comment
There was a problem hiding this comment.
I'm Claude Opus 5. Rebuilt 8eeaedae5 and verified all four bugs Grok 4.6 reported are fixed, including #4 end to end; my earlier findings and the runtime oracles all still hold.
Re-approving — this supersedes my note that the previous approval did not cover the tip.
Signed-off-by: Christoph Knittel <ck@cca.io>
2 participants
This is the second PR in the two-PR String Theory stack, built on #8605. It replaces the compiler’s historical delimiter protocol with explicit representations for semantic strings, templates, JSON literals, and raw JavaScript.
Consistent representations
Each layer now distinguishes runtime string values from source text that must be preserved:
This structure continues through the typed tree, Lambda, and JavaScript IR. The frozen AST v0 bridge converts to and from its legacy encoding at that single compatibility boundary.
A shared
String_literalmodule now owns JavaScript escape decoding, canonical encoding, surrogate handling, UTF-16 string operations, and template line-ending semantics.String and template payloads use an abstract, kind-indexed representation (
string_kindandtemplate_kind). Valid payloads can only be created through decoding, semantic encoding, explicit conversion, or template concatenation. Template concatenation preserves combined source spelling only when it still decodes to the combined semantic value; otherwise it falls back to canonical encoding. Invalid parser input has a separate recovery representation that retains its original spelling.This removes:
DNone,DStarJ,DBackQuotes, andDNoQuotesdelimiter protocol"js","*j", and"bq"marker strings++trees and attributes for interpolated templatesAst_utf8_stringandAst_utf8_string_interpimplementationsUnicode handling
Compiler-owned UTF-8 handling now uses OCaml’s standard
String,Uchar, andBufferAPIs instead of maintaining separate byte classifiers, decoders, and encoders.Output behavior
Template literals now remain explicit throughout compilation and carry over into the generated JavaScript:
++applications.Behavior fixes
The normalized representation also resolves several correctness issues:
"a"and"\x61"now compare by runtime value, preserve source order, and produce the expected redundant-pattern warning.jsoninterpolation and unsupported uses of JSON literals are rejected.@asvalues and Unicode line separators in generated paths.Testing
Coverage spans parsing and printing, the AST v0 bridge, type checking, Lambda and JavaScript IR, constant folding, generated JavaScript, GenType, analysis, and end-to-end behavior. Unicode regressions include overlong UTF-8, encoded surrogates, values above U+10FFFF, JavaScript escaping of malformed bytes, and UTF-16 position accounting.
Verified with:
make testmake test-syntaxmake test-syntax-roundtripmake test-gentypemake test-analysisCloses #8602.