Repository navigation
fix(string): escape as one native pass that also accepts a fn cmap - #1074
Conversation
nnunley
left a comment
There was a problem hiding this comment.
The fn-cmap case from #1068 now matches Clojure. Three things before merge:
- The numbers are two interleaved runs on a loaded machine.
AGENTS.mdroutes perf claims throughdocs/perf/ratchet.md, so ratchet them or label them indicative. - A nil or non-fn cmap now throws even on an empty string, where the old defn returned "". Inline on
pkg/rt/native_prims.go. - The new tests cover BMP text only. Please add an astral fn-cmap case and the invalid-UTF-8 case. Inline on
test/quick_wins_test.lg.
I ran the quick_wins suite (62 tests, 0 failures), the pkg/rt, vm, api and compiler tests, check-generated, and the js/wasm, wasip1 and plan9 builds. Swapping in a copy on the clean path fails the alloc test at 40976 B/call.
| lookup = func(c vm.Char) (vm.Value, error) { return m.ValueAt(c), nil } | ||
| case vm.Fn: | ||
| lookup = func(c vm.Char) (vm.Value, error) { return m.Invoke([]vm.Value{c}) } | ||
| default: |
There was a problem hiding this comment.
This default: now rejects a nil or non-fn cmap before the loop runs, so (escape "" nil) throws. The old defn returned "" there because map over an empty string never calls cmap, and Clojure also returns "" without touching cmap. Consider moving the type check so it only fires when a character is actually looked up, or add a test that pins the empty-string case either way.
There was a problem hiding this comment.
The type check now fires on the first character looked up, so (escape "" nil) and (escape "" 5) return "", and (escape "x" nil) still throws. Tests pin both cases.
| (is (= "ab" (s/escape "ab" {\a false \b nil})))) | ||
| (testing "a replacement is converted with str" | ||
| (is (= "a1Z" (s/escape "abc" {\b 1 \c \Z})))) | ||
| (testing "multi-byte characters" |
There was a problem hiding this comment.
This block only covers BMP characters. Please add a case with an astral character as a fn cmap argument, since let-go passes one code point (128512) where Clojure passes two UTF-16 halves. Also worth pinning: an invalid byte with a cmap keyed on U+FFFD gets replaced, since the decoder yields U+FFFD for it.
There was a problem hiding this comment.
Added both. A fn cmap receives U+1F600 as one code point, 128512, with a comment noting that Clojure passes two UTF-16 halves. The invalid-byte case is a Go test, pkg/rt/string_escape_test.go, because a let-go string literal cannot hold an invalid byte: a cmap keyed on U+FFFD replaces the byte, and one without that key keeps it.
09c7da3 to
0fcf187
Compare
|
All three are in 0fcf187, on top of a rebase onto main. The body now labels the throughput numbers as indicative rather than ratchet results; the other two are answered inline. |
|
@nnunley Thanks for the review. Good catches- all addressed. |
0fcf187 to
2142e99
Compare
nooga
left a comment
There was a problem hiding this comment.
Approving. A single-pass native escape that reads map cmaps with ValueAt and calls fn cmaps (sets and keywords included). A falsey result keeps the char, non-string replacements are str'd, and a clean input string comes back without a copy. All of nnunley's points are addressed: (escape "" nil) still returns "" because the type check waits for the first lookup, the astral fn-cmap and invalid-UTF-8 cases now have tests, and the perf numbers are labelled indicative. The native is registered one way only, via //lg:native. Locally, make check-generated, go test ./pkg/rt -run Escape and make test LG_TEST=quick_wins pass.
clojure.string/escape was (apply str (map (fn [c] (or (get cmap c) c)) s)). `get` on a fn returns nil, so a function cmap was ignored, and the lazy seq plus apply str made it the slowest step of HTML escaping. escape is now a Go native in clojure.string, as upper-case is. A map cmap is read with ValueAt, a fn is invoked, and a falsey result keeps the character. A string with nothing to replace comes back as is, without a copy. The lg defn stays as the bootstrap fallback and calls (cmap c), so a fn works there too. On a hiccup-style page under ab -c 50, page throughput goes from 156 to 233 req/s on the VM and from 253 to 567 with the lowered core linked, with byte-identical output. Fixes #1068. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(escape "" nil) returns "" again, as the old defn and Clojure do: an empty string never calls cmap. Adds tests for an astral character passed to a fn cmap, for invalid UTF-8 (U+FFFD lookup, and the original byte kept when cmap has no entry), and for the empty-string case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2142e99 to
abf2fc7
Compare
Picks up nooga/let-go#1074, #1076, #1079, #1080, #1083 and #1084. No release tag covers them yet, so this is a pseudo-version; #41 stays open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #1068.
Why
clojure.string/escapewas(apply str (map (fn [c] (or (get cmap c) c)) s)), which has the two problems #1068 describes:geton a fn returns nil, so a functioncmapwas ignored. Clojure's docstring allows "a map or a function".apply strmade it the slowest step of HTML escaping, most of the render time in a hiccup-style page.What changes
escapeis now a Go native inclojure.string, registered the wayupper-caseis. It walks the string once:cmapis read withValueAtinstead of being invoked, so the usual HTML-escape call allocates no argument slice per character;cmapis invoked;nilorfalse) keeps the character, as in Clojure;str({\b 1}gives"a1c");Unmatched characters are copied as their original bytes, so invalid UTF-8 survives escaping the same way it survives the no-match path.
The lg
defnstays as the bootstrap fallback and now calls(cmap c), so a fn works there too.One behavior change: a
cmapthat is neither a map nor a fn now throws, naming its type, at the first character it would look up. Before,getreturned nil and the string came back unchanged. An empty string looks up nothing, so(escape "" nil)still returns"", as in Clojure.Numbers
These are indicative numbers, not ratchet results: page throughput on a small
http/serveapp shaped like the one in #1067 (hiccup-style page,ab -c 50, 10 s, two interleaved rounds), with byte-identical output in every run:lg compile+ lowered coreThat is 1.48× on the VM and 2.23× with the lowered core linked. The machine was heavily loaded, so the absolute numbers are low; the ratios held across both rounds.
Tests
The new cases are in
test/quick_wins_test.lg, including an astral character passed to a fncmap(one code point, where Clojure passes two UTF-16 halves) and the empty-string case. Invalid UTF-8 is inpkg/rt/string_escape_test.go, since a let-go string literal cannot hold an invalid byte: acmapkeyed on U+FFFD replaces the byte, and one without that key keeps it. The allocation test inpkg/rt/string_index_alloc_test.goholds a clean 33 KB string under 1 KB per call; with the no-copy return swapped for a copy, it fails at about 40 KB. On the rebased head,make check-generated, thetest/*.lgsuites,go testonpkg/rt,pkg/vm,pkg/apiandpkg/compiler, and builds for linux, js/wasm, wasip1/wasm, and plan9 pass.🤖 Generated with Claude Code