Normalize ordinary string literals at the frontend - #8603
Conversation
Signed-off-by: Christoph Knittel <ck@cca.io>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8603 +/- ##
==========================================
+ Coverage 76.44% 76.53% +0.08%
==========================================
Files 478 481 +3
Lines 63163 63438 +275
==========================================
+ Hits 48286 48553 +267
- Misses 14877 14885 +8
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
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. |
![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: 22bf547224
ℹ️ 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: ede3c1ede3
ℹ️ 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: de20e15933
ℹ️ 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: 9a7d63d24a
ℹ️ 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.
Nice fix for the core issue — I confirmed the headline behavior: | "a" => 1 | "\x61" => 2 now returns 1 for "a", where master returned 2. The decode_js_escapes decoder also looks solid; I went through \x, \u, \u{...}, surrogate pairs, line continuations and the overflow guard in the braced form and did not find an arithmetic problem.
I built the branch and checked the findings below against a PR-built bsc with a master-built one as control. Two of them I think are blocking.
Blocking
1. Unrecognized -> raise_errorf breaks bsc -reprint-source on any file containing a char pattern
transform_pat's new catch-all rejects every delimiter that isn't "js", but the printer path uses a delimiter of its own. In printer mode (p.mode <> ParseForTypeChecker), res_core.ml:1037 encodes char literals as:
Pconst_string (original, Some "INTERNAL_RES_CHAR_CONTENTS")and bs_builtin_ppx.ml:97 forwards every Ppat_constant (Pconst_string (s, Some delim)) to transform_pat unconditionally. Since Delim.parse_unprocessed returns Unrecognized for anything but "js", a plain char pattern now hard-errors:
switch x {
| 'a' => 1
| _ => 2
}master: bsc -reprint-source ch.res -> prints the source
PR: bsc -reprint-source ch.res -> "Tagged template literals are not supported in patterns" at 'a'
The guard needs to reject actual tag delimiters specifically rather than everything that isn't "js".
2. The fix is incomplete — constant-scrutinee string switches still fold by encoded form
const_compare is only one of the places that compares string constants. Js_stmt_make.string_switch (js_stmt_make.ml:150) selects a clause with a raw s = txt comparison, and Lam.stringswitch (lam.ml:326) uses Ext_list.assoc_by_string; neither decodes escapes. On this branch:
let h = () =>
switch "a" {
| "\x61" => 1
| "b" => 2
| "c" => 3
| _ => 4
}compiles to function h() { return 4; }. It should be 1, since "a" === "\x61" in JS. That is the same bug class the PR is fixing, so it would be good to either cover it here or note it explicitly as out of scope.
Performance
3. ~8x compile-time regression on large string matches
decode_js_escapes allocates a Buffer and rebuilds both operands on every comparison, and const_compare is called O(n^2) times by parmatch's redundancy analysis on top of the O(n log n) from sort_lambda_list. On a generated 2000-case string switch containing no escapes at all:
master: 0.11s
PR: 0.93s
control, 2000-case int switch: master 0.15s / PR 0.11s
A fast path in runtime_value should recover this — return s unchanged when not (String.contains s '\\'), before allocating anything.
Smaller points
-
Tagged template patterns become a hard error without a breaking-change note.
switch x { | json`abc` => 1 | _ => 2 }compiles on master (tox === "abc") and is a compile error here. If that is intended, it wants a#### :boom: Breaking ChangeCHANGELOG entry and atests/build_tests/super_errors/fixture — right now the new error is only exercised by an OUnit test callingtransform_patdirectly. Both newraise_errorfsites also need rows intests/ERROR_VARIANTS.md. -
Surrogate pairs are a new feature, not just a pattern fix. The
res_scanner.mlchange applies to all string literals, solet x = "\uD83D\uDE00"compiles now and was a scanner error before. Worth its own CHANGELOG line and a test outside pattern position. -
Duplicate diagnostics for one malformed surrogate escape. In the new
\ubranch,scan_digitshas already reported "unknown escape sequence" and returned-1before the caller testslow < 0xDC00and reports "escape sequence is invalid unicode code point" on top."\uD83D\uD83D\uDE00"yields two errors for one bad escape; gating the second report onlow >= 0would fix it. -
runtime_valuesilently aliases undecodable literals to their raw text. ReturningsonNonemeans an undecodable literal can compare equal to a different literal whose decoded value happens to match that raw text. This is unreachable today only becausetransform_patrejects undecodable*jpatterns — an implicit cross-module invariant. Worth documenting in the.mli, or havingcomparedistinguish decodable from undecodable operands. -
Minor: in
transform_patthe decode result is discarded (Some _ -> ()) and then recomputed byruntime_valueduring comparison. And intest_string_switch.mjsthe collapsed case emitsvalue === "\x61"rather thanvalue === "a"— runtime-identical, but the emitted spelling comes from a pattern that warning 11 reports as unused, so the output depends on dead code.
|
There's ongoing cleanup #8604 that also touches const strings. Does not seem to conflict right now, but just wanted to mention it. I have put it up for review so it will get out of the way in any case. |
|
Thanks! Will pull the relevant parts into this PR to avoid churn. |
|
One suggestion below that seems sensible at face value. Haven't verified it deeply. The trouble is that Pconst_string (s, delim) conflates two different things under one field. s is the source spelling — escape text, still undecoded — and delim is an unparsed marker meaning "this hasn't been resolved yet." Every downstream consumer that cares about the string's value therefore has to re-derive it. 8603 teaches exactly one consumer (parmatch.const_compare) to do that, which is why the fix is simultaneously incomplete, quadratic, and dependent on an unenforceable cross-module invariant. Those three findings aren't independent defects; they're the same defect observed from three angles. The right time to resolve is the moment the literal stops being syntax and becomes a value — the frontend boundary, in Ast_utf8_string.transform and its transform_exp/transform_pat callers. That's where the delimiter is consumed, and, importantly, that code already walks the string character by character to validate escapes and copy them into a buffer. Producing the decoded value there is free: the traversal is happening regardless. It's also the only place that still has a source location, so a bad escape becomes a proper error rather than a None that has to be defended against three modules later. |
Signed-off-by: Christoph Knittel <ck@cca.io>
9a7d63d to
ad78db8
Compare
|
Addressed in ad78db8 by pulling the ordinary-string normalization forward into this PR. The main change is that ordinary string literals are decoded once at the frontend boundary and stored as semantic strings from then on. This addresses the incomplete-fix, performance, fallback, duplicate-decoding, and dead-spelling points together:
I also added warning 11 to tests/ERROR_VARIANTS.md. The two Location.raise_errorf diagnostics are covered by fixtures/tests, but are not rows in that catalog because it catalogs named compiler error and warning variants rather than ad-hoc Location errors. Local verification: make test, make test-syntax, and make test-analysis all pass with the OCaml 5.5.0 switch. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad78db88ca
ℹ️ 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".
|
Follow-up: the first CI run exposed one more downstream encoded-string assumption in GenType. Semantic @as variant labels were emitted without TypeScript escaping, while record-key snapshots still reflected the old encoded representation. Fixed in bb12ab8; make test-gentype now passes locally, and the complete downstream branch stack has been rebased again. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1269f0f899
ℹ️ 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".
cknitt
changed the title
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>
1269f0f to
c2aad32
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2aad326c8
ℹ️ 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>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
c2aad32 to
d712635
Compare
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
First part of the String Theory series, which simplifies and normalizes string-literal handling across the compiler.
Summary
String literals currently retain their encoded source spelling after parsing. Consequently, equivalent values such as
"a"and"\x61"can be treated differently by pattern matching, constant folding, and other compiler consumers.This PR normalizes ordinary string literals to their decoded semantic value at the frontend boundary. As a result:
DStarJrepresentation is no longer needed;Tagged template literals are now rejected in patterns because a pattern cannot invoke its tag, and treating the raw payload as a string could produce incorrect matches.
The ast0 bridge converts ordinary literals to the normalized representation while preserving source spelling for templates and the compiler-reserved
%raw,%%raw,%ffi, and%reextensions. This keeps external PPX round trips compatible without changing escape-sensitive JavaScript payloads.GenType emission has also been updated to escape semantic
@asvalues correctly.This implements the normalization boundary proposed in #8602. Later parts of the series can build on it to introduce more explicit representations for templates and other non-semantic source payloads.
Tests
opam exec -- make testopam exec -- make test-syntaxopam exec -- make test-gentypeopam exec -- make test-analysis