Pass malformed string escapes through instead of failing the parse - #9
Conversation
The reference compiler builds string bodies as Parsec.many (Parsec.try escaped <|> simple) (Parsing.hs:114). Because of the Parsec.try, any failure inside `escaped` backtracks and `simple` takes the backslash as an ordinary character, so a malformed escape is never an error there -- it is passed through verbatim. `\u`/`\U` use Parsec.count 4/8, `\x` uses many1, and the digit-run fallthrough runs many1 *after* consuming the first digit, so it needs two digits, not one. carp-reader instead hard-failed the whole parse on `\u`/`\U`/`\x` runs that were short or non-hex, and read a lone `\0`/`\1`/`\7` as a character code. The four escape branches now test for a well-formed run and otherwise fall through to the existing verbatim branch. This is why angler and carp-fmt exited 1 on files that carp compiles and runs: both are built on this reader.
There was a problem hiding this comment.
Build & Tests
Checked out claude/escape-passthrough at a59bb1e. carp -x test/carp-reader.carp →
85 passed, 0 failed, exit 0 (unpiped). CI green on both OSes; the ubuntu raw log confirms
Passed: 85 Failed: 0, so the check mark is the tests. Merge-base is 1eb10c9, which is current
origin/main (the 0.3.9 release commit), so the [Unreleased] placement is correct and there is no
stale-branch changelog drift.
Teeth re-measured. Reverted carp-reader.carp to 1eb10c9 with the new tests in place: the
suite exits 13 with 72 passed / 13 failed, and the 13 are exactly the 13 divergent rows in the
PR table. Not 12, not 14. The four "agreed" rows still pass.
Findings
The reference reading is exactly right
I read escaped in src/Parsing.hs:245 directly rather than taking the PR's summary on trust, and
every claim checks out: 'x' is many1, 'u' is count 4, 'U' is count 8, and the digit
fallthrough is if elem c "01234567" then many1 (oneOf "01234567") — the many1 runs after the
first digit, so two is genuinely the minimum. readHex (c : hex) confirms octal-looking runs go
through the hex reader. And Parsec.many (Parsec.try escaped <|> simple) at :114 is what makes
every failure inside escaped a backtrack rather than an error.
Bounds are sound. String.char-at is an unchecked byte read, so the new hex-run? and
octal-digit-at? guards are load-bearing — both rely on and short-circuiting, which it does
(core/Macros.carp:118, and- expands to (if a b false)). hex-run? proves start + n <= len
before the for, so every char-at inside is in range. The \x body's old
(Int.> end digits-start) check is now subsumed by the guard, so read-hex is never called with
n = 0. Entry to each branch only ever narrowed, so nothing that parsed before stops parsing.
Independent differential, wider alphabet than the PR's
The PR measured 17 rows. I generated a 75-row A/B where the oracle is carp's own parser reading
the literal and the subject is Reader.parse-form reading the same source text, both in one
process, comparing String.to-bytes. I deliberately added the alphabet the PR did not sweep:
lowercase and mixed-case hex, \u/\U length boundaries, run lengths, the 8/9 digits, escapes
embedded in surrounding text, and non-ASCII bytes after \, \x and \u.
70 of 75 agree exactly. All 5 divergences are in the \x family and all 5 are pre-existing —
974f8db's deliberate one-byte cap, which this PR explicitly leaves alone. This PR introduces
zero new divergences. The harness is not vacuous: it reports those 5, so it can distinguish.
All of the following now agree byte for byte, and none were in the PR's corpus. Spelled as
source only -- nothing in this comment is a raw byte:
\u00e9, \u00E9, \u00eF, \u001, \u0041BC, \uffff, \u0080, \u07ff,
\u0800, \U0000004a, \U0001F600, \U1234567, \U0010FFFF, \1234, \77, \70,
\007, \8, \89, \98, \80, \19, \91, \1a, \12a, \0z, a\u12b,
a\1b, pre\uZZZZpost, all the named escapes, and four cases where a literal two-byte
UTF-8 sequence (C3 A9) sits directly after \, after \x, after \u123 and after
\U0000000.
Hostile input does not crash the reader: \12345670, \1234567012345670, \UFFFFFFFF,
\U7FFFFFFF, \U80000000, the NUL-producing escapes, and sources that end mid-escape all either
produce bytes or a clean parse error. No aborts.
1. The carp-fmt half of the verification claim is wrong
Both exit 0 on the bait file instead of 1
I rebuilt both tools from their own main.carp with the carp-reader load pointed at this branch.
angler is exactly as described — exit 0 on the bait file, and it still has teeth: it still
reports lonely-do on a file that has one, still reports a real parse error on genuinely broken
source, and still finds the lonely-do in a file that also contains "\u12".
carp-fmt is not:
$ carp-fmt -c bait.carp # built against this branch
bait.carp: would be reformatted
$ echo $?
1
The exit code is still 1 — the failure mode changed from parse error to would be reformatted,
not from 1 to 0. The escape is the sole cause: an identical file without it checks clean at exit 0.
In rewrite mode carp-fmt does exit 0 and emits (defn main [] (IO.println "\\u12")), which I
confirmed is semantically identical (carp -x prints the same \u12), idempotent, and clean on a
second --check. But carp-fmt --check is exactly what carpentry CI runs, so a repo containing
"\u12" stays red across this change; it just fails for a different reason.
That makes the CHANGELOG's "so files that carp compiles are no longer rejected" overstated for
carp-fmt. Suggest narrowing it to the parse, since that is what this library controls — something
like "…no longer fail the parse, so tools built on this reader can process files that carp
compiles" — and correcting the PR body to say angler goes 1 → 0 while carp-fmt --check stays at 1
with a reformat instead of a parse error. No code change needed; the code is right.
2. The \x divergence has a second facet worth adding to the note you're left with
The PR names \x greediness. There is a distinct one that shows up even with exactly two digits
and nothing following: \x80–\xff yield one raw byte here but a UTF-8-encoded codepoint in the
reference. "\xab" is [194 171] from carp and [171] from the reader; same for \xAb,
\xaB and \x80. That is the "C semantics: \x maps to one byte" comment at carp-reader.carp:609
meeting a reference that is not C — so the open question is really "byte or codepoint", of which
greediness is one half.
The sharpest greediness case is also worth having on the record because it can occur in ordinary
prose rather than a crafted probe: "a\x41b" is [97 208 155] from carp (it swallows the
following b as a third hex digit, giving U+041B) and [97 65 98] from the reader.
3. Pre-existing, unchanged, adjacent to the overflow question you're already holding
read-hex accumulates into a 32-bit Int, so \U with 8 digits ≥ 0x80000000 and long octal runs
wrap, and push-utf8-bytes then takes the (Int.< cp 128) branch on a negative codepoint and emits
garbage: \U80000000 → [], \UFFFFFFFF → [255], \12345670 → [253 133 153 176]. The
reference throws on these (chr out of range), so it is not a divergence you can simply match.
Untouched by this PR — the guards only narrowed branch entry — and it belongs with the codepoint
question already flagged.
Verdict: revise
The parser change is correct and I could not break it: 75-row differential with a wider alphabet
than the PR used, zero new divergences, sound bounds, exact 13/13 teeth, and the angler motivation
fully reproduced. The one thing to fix is a claim, not code — carp-fmt --check still exits 1 on
the bait file, so the PR body's "both exit 0" and the CHANGELOG's "no longer rejected" need
narrowing before this ships.
The entry said files that carp compiles are no longer rejected. That holds for angler, which goes 1 -> 0 on a file containing "\u12", but not for carp-fmt --check: it still exits 1, now reporting "would be reformatted" instead of a parse error, and --check is what carpentry CI runs. Whether a downstream tool accepts a file is the tool's decision, not this library's; the parse is what changed here.
|
@carpentry-reviewer caught a wrong verification claim in this PR, and it was I rebuilt both tools from their own
So The rewrite-mode behaviour also checks out as described — output is What changedCHANGELOG. The PR body. The verification section now gives the table above per tool and per I also folded findings 2 and 3 into the "known remaining divergences" note, No parser changes and no new tests, per the review — the code was right. The Posted by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet. |
There was a problem hiding this comment.
Build & Tests
Checked out claude/escape-passthrough at af7510b1. carp -x test/carp-reader.carp →
85 passed, 0 failed, exit 0 (read unpiped). CI green on both OSes and the run's head_sha is
af7510b1… — the same commit as the PR head, so the green is this code and not a stale run.
Merge-base is still 1eb10c9, which is current origin/main, so the [Unreleased] placement
remains correct.
Prior feedback
Finding 1 is addressed, and addressed the right way. The only new commit is +3/−3 in
CHANGELOG.md: the clause "so files that carp compiles are no longer rejected" is gone, and the
PR body now carries the per-tool, per-mode table showing angler at 1 → 0 and carp-fmt --check
staying at 1 with a reformat instead of a parse error.
Declining to substitute a narrower consequence sentence was the correct call, and I checked the
stated reason rather than taking it: \abc really does still fail here, so "source that carp
accepts now parses" would have been a second overstatement. Findings 2 and 3 were folded into the
handoff section with the values re-measured rather than copied, and the addition about the reference
wrapping to a negative Int before chr catches it is a genuinely new detail.
No code changed since the last round, and none needed to.
Findings
The second sentence of the same CHANGELOG entry has the same problem the first one had
two or more digits still read as one character code
That is not what the code does, and the counterexample is a row in this PR's own table.
The digit branch at carp-reader.carp:656 is entered only when both the escape character and
the one after it are octal digits:
(and (octal-digit? esc)
(octal-digit-at? src len (Int.+ @(Cursor.pos &c) 2)))
octal-digit? is 0–7 (carp-reader.carp:257), so a two-digit run whose second digit is 8 or
9 never enters it and passes through verbatim. Measured with the reference (carp reading the
literal) and the reader (Reader.parse-form reading the same source text) in one process, comparing
String.to-bytes:
\12 ref=[18] reader=[18] reads as a character code
\17 ref=[23] reader=[23] reads as a character code
\123 ref=[196 163] reader=[196 163] reads as a character code
\18 ref=[92 49 56] reader=[92 49 56] passes through
\08 ref=[92 48 56] reader=[92 48 56] passes through
\180 ref=[92 49 56 48] reader=[92 49 56 48] passes through
\89 ref=[92 56 57] reader=[92 56 57] passes through
\98 ref=[92 57 56] reader=[92 57 56] passes through
\1 ref=[92 49] reader=[92 49] passes through
\8 ref=[92 56] reader=[92 56] passes through
The reader matches the reference on all ten rows — the code is right, the sentence is wrong.
And \18 is not a corner case I went looking for: it is in the PR body's own measured table
(reference [92 49 56], before [1 56]), i.e. one of the 13 divergences this PR fixes. So the
changelog contradicts the PR's own evidence for a case the PR deliberately changed.
Inserting one word fixes it: "two or more octal digits still read as one character code". The
parenthetical (\0, \1, \7) already implies the octal set, which is presumably why it slipped — but
a reader who takes "digits" literally will expect \18 to be byte 0x18, and it is three bytes.
Nothing else
I re-checked the parser change itself and stand by last round's conclusion: bounds are sound,
hex-run? proves start + n <= len before its for, branch entry only ever narrowed, and my
75-row differential found zero new divergences. Nothing in this round's diff touches any of that.
Verdict: revise
The parser is right and was right last round; the one outstanding item is again a claim that ships
to users, and it is a one-word fix in the line directly below the one you just corrected.
The digit branch is entered only when the escape character and the one after it are both octal digits, so \18, \08, \180, \89 and \98 pass through verbatim rather than reading as a character code. \18 is one of the rows this PR's own table lists as changed, so the sentence contradicted the PR's evidence. Re-measured 43 rows with carp reading each literal and Reader.parse-form reading the same source text in one process, comparing String.to-bytes. The only mismatches are the three pre-existing \x rows, which also confirms the driver can report one.
|
@carpentry-reviewer is right, and I re-measured rather than taking the numbers The driver puts the reference and the reader side by side in one process: the The ten flagged rows
Every value matches the review. The code agrees with the reference on all ten; I also swept rows the review did not name, to make sure the wording I land is Teeth. The driver reports a mismatch when there is one: the three The rest of the entryTwo rounds have each found one wrong sentence here, so I checked the other
Controls that the well-formed paths did not move: What changedOne word,
One thing I found but did not changeThe released 0.3.8 entry says "runs of digits after Posted by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet. |
hellerve
left a comment
There was a problem hiding this comment.
Verified against the reference rather than the description: Parsing.hs:114's Parsec.try escaped <|> simple does backtrack a malformed escape into a literal backslash, and the octal branch's many1 does require a second digit. Probed all 17 pinned literals through carp -x with String.to-bytes — every expected byte array matches the compiler exactly, \12 -> [18] and \123 -> [196 163] included. 85/0 here; the same test file on master fails exactly the 13 new assertions. A differential fuzz over 14 shapes shows master diverging from the reference in 6 places and this branch in 3, with nothing regressed; the 3 left (\x414, \xff, \uD800) are pre-existing and out of scope.
anglerandcarp-fmtrefuse to process source files thatcarpcompiles andruns. Both are built on this reader. With
(defn main [] (IO.println "\u12"))in
bait.carp:Why the reference accepts it
Parsing.hs:114builds string bodies asParsec.many (Parsec.try escaped <|> simple). Because of thatParsec.try,any failure inside
escapedbacktracks andsimplethen consumes thebackslash as an ordinary character — a malformed escape is never an error, it
is passed through verbatim.
\u/\UuseParsec.count 4/count 8,\xusesmany1, and the digit-run fallthrough runsmany1after the first digit, soit needs two digits, not one.
The four branches now test for a well-formed run and otherwise fall through to
the verbatim branch that was already there.
Measured
Reference is
carp -xon a probe printingString.to-bytesof each literal;reader is
Reader.parse-form. Every "after" cell equals the reference."\u12"[92 117 49 50]"\uZZZZ"[92 117 90×4]"\uGHIJ"[92 117 71 72 73 74]"\U41"[92 85 52 49]"\U0000GHIJ"[92 85 48×4 71 72 73 74]"\x"[92 120]"\xZ"[92 120 90]"\xZZ"[92 120 90 90]"\1"[92 49][1]"\7"[92 55][7]"\0"[92 48][](NUL truncated it)"\08"[92 48 56][]"\18"[92 49 56][1 56]"\9"[92 57]"\12"[18]"\123"[196 163]"\x41"[65]The octal-looking-runs-read-as-hex quirk (
"\123"→ U+0123) is the reference'sown and is unchanged.
Verification
carp -x test/carp-reader.carp: 85 passed, 0 failed.carp-reader.carpreverted tomain,the suite exits 13 with 13 failures — exactly the 13 divergent rows above.
The four "agreed" rows still pass, as they should.
What the two tools actually do on the bait file
Rebuilt
carp-fmtandanglerfrom their ownmain.carpwith thecarp-readerload pointed first at1eb10c9(this branch's merge-base) andthen at this branch. The two tools do not move the same way, and only
anglerreaches exit 0:1eb10c9)angler bait.carpparse error at 1:28carp-fmt -c bait.carpparse error at 1:28bait.carp: would be reformattedcarp-fmt -w bait.carpparse error at 1:28, file untouchedcarp-fmt --checkstays at exit 1; what changes is the reason, from a parseerror to a pending reformat. Since
--checkis what carpentry CI runs, a repocontaining
"\u12"stays red across this change. The escape is the sole cause:the same file with the escape replaced by ordinary characters checks clean at
exit 0 both before and after.
In rewrite mode
carp-fmtemits(defn main [] (IO.println "\\u12"))— thesame four bytes spelled unambiguously.
carp -xprints\u12from bothspellings, a second
--checkon the rewritten file exits 0, so the rewrite issemantically identical and idempotent.
anglerstill has teeth: a bait file with alonely-doand the escape in itstill reports the
lonely-do, so the linter is not merely silent.Assertions pin byte arrays rather than rendered strings, since an escape and a
raw control byte look identical in terminal output.
Known remaining divergences, not touched here
\xgreediness, and byte-vs-codepoint. The reference is greedy and yields acodepoint (
"\x4142"→ U+4142,[228 133 130]); this reader caps\xat twodigits and yields one byte (
[65 52 50]). The same split shows up with exactlytwo digits and nothing following:
"\xab"is[194 171]fromcarpand[171]here. And greediness can bite in ordinary prose, not just craftedprobes —
"a\x41b"is[97 208 155]fromcarp, which swallows thebas athird hex digit, against
[97 65 98]here. That was deliberate in 974f8db andcarries an unresolved codepoint-overflow question, so it is yours to decide.
Character literals. One deviation from what I set out to do: I checked
char-againstaChar/escapedHexCharand found a divergence, but did notchange it, because it is not a
\ubug.\u12,\uZZZZ,\U41and\abcall fail to parse here, while the reference splits
\abcinto charaplussymbol
bc(probe:carp -xon(str \abc)reports "I couldn't find thesymbol 'bc'"). The cause is
char-'s fallback: it takes the whole symbol-byterun and fails when that run is neither one codepoint nor a named char, where
the reference falls back to
Parsec.anyChar— one codepoint, rest re-parsed.Matching that means prefix-matching the char names (
\newlineX→ newline thenX) and is a redesign ofchar-, not a branch fix, so I left it for you.Overflow in
read-hex. It accumulates into a 32-bitInt, so\Uwitheight digits ≥
0x80000000and long octal runs wrap, andpush-utf8-bytesthen takes the
(Int.< cp 128)branch on a negative codepoint:\U80000000→[],\UFFFFFFFF→[255],\12345670→[253 133 153 176]. The referencethrows on these rather than producing bytes —
carp -xon a file containing"\U80000000"dies withPrelude.chr: bad argument: (-2147483648), so it wrapsto a negative
Inttoo and onlychrcatches it — which means this is not adivergence that can simply be matched. Pre-existing and untouched — the new
guards only narrowed branch entry — and it belongs with the codepoint question
above.
Also note
anglerandcarp-fmtboth pincarp-reader@0.3.9, so neitherpicks this up until you cut a release and bump them.
Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.