Complete rustpython-unicode isolation: case mapping, casing predicates, sre parity - #8237
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThis PR moves Unicode casing and whitespace handling to ChangesUnicode casing migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/unicode/src/case.rs (1)
139-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract shared surrogate-passthrough logic to reduce duplication.
The surrogate handling in
capitalize_wtf8(lines 139-144) duplicates the three-line pattern inmap_wtf8(lines 192-196), differing only by thefirst = falseside-effect. Extracting a helper would eliminate the copy.Proposed refactor
+fn push_surrogate(out: &mut Vec<u8>, c: CodePoint) { + let mut buf = Wtf8Buf::new(); + buf.push(c); + out.extend_from_slice(buf.as_bytes()); +} + fn capitalize_wtf8(text: &Wtf8) -> Wtf8Buf { // ... Wtf8Chunk::Surrogate(c) => { first = false; - let mut buf = Wtf8Buf::new(); - buf.push(c); - out.extend_from_slice(buf.as_bytes()); + push_surrogate(&mut out, c); } + +fn map_wtf8(text: &Wtf8, f: impl Fn(&str, &mut FmtWriter<'_>)) -> Wtf8Buf { + // ... + Wtf8Chunk::Surrogate(c) => { - let mut buf = Wtf8Buf::new(); - buf.push(c); - out.extend_from_slice(buf.as_bytes()); + push_surrogate(&mut out, c); }Also applies to: 192-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/unicode/src/case.rs` around lines 139 - 144, The surrogate passthrough logic is duplicated in capitalize_wtf8 and map_wtf8; extract the shared Wtf8Chunk::Surrogate handling into a helper so both paths reuse it, while preserving the first = false side-effect only in capitalize_wtf8. Update the branches in those functions to call the shared helper and keep the existing behavior identical otherwise.crates/unicode/tests/differential.rs (1)
312-346: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression check to
regen_mapping_version_skew.
regen_version_skew(line 202-208) refuses to recordcpython=true/crate=falsedivergences as version skew, treating them as regressions (with aKNOWN_RECATEGORIZATIONSexception).regen_mapping_version_skewhas no equivalent guard — a mapping regression (CPython maps a code point, crate doesn't) would be silently written to the skew file without warning.The
simple_mappings_match_cpython_except_documented_version_skewtest is the ultimate safety net, but adding a parallel assertion here would catch regressions at regen time, consistent with the predicate path.Proposed fix
let reference = parse_mappings(MAPPINGS); let divergences = all_mapping_divergences(&reference); + // A `cpython maps, crate doesn't` divergence means a code point lost its + // mapping in a later Unicode release — a real regression, not version skew. + let regressions: Vec<_> = divergences + .iter() + .filter(|(name, cp)| { + let expected = reference.get(name).and_then(|t| t.get(cp)).copied().unwrap_or(*cp); + expected != *cp && crate_mapping(name, *cp) == *cp + }) + .collect(); + assert!( + regressions.is_empty(), + "refusing to record {} mapping regression(s) — these are regressions, \ + not version skew: {:?}", + regressions.len(), + ®ressions[..regressions.len().min(20)] + ); + let mut by_mapping: BTreeMap<String, BTreeSet<u32>> = BTreeMap::new();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/unicode/tests/differential.rs` around lines 312 - 346, Add the same regression guard used by regen_version_skew to regen_mapping_version_skew so mapping regressions are not silently written into the skew file. In regen_mapping_version_skew, before building the output body, inspect the divergences from all_mapping_divergences and assert that any cpython=true/crate=false cases are either absent or explicitly covered by the existing KNOWN_RECATEGORIZATIONS-style exception, then fail fast with a clear message if a true regression is detected. Use the existing regen_version_skew logic and simple_mappings_match_cpython_except_documented_version_skew predicate as the reference for the expected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/unicode/src/case.rs`:
- Line 30: Remove the decorative section separator comments in case.rs and
replace them with simple short comments or remove them entirely where they only
act as headings. Update the affected section markers around the code-point
mapping areas in the Unicode case mapping code so they comply with the coding
guidelines, using the existing context in case.rs rather than trailing hyphen
dividers.
- Around line 216-234: Update the doc comment on titlecase_string to match the
actual segmenting logic used by previous_is_cased = is_cased(ch): segments are
split by non-cased characters, not by case-ignorable characters or whitespace
specifically. Keep the examples, but rewrite the explanatory sentence so it
accurately reflects the behavior implemented in titlecase_segment and
lowercase_or_sigma.
---
Nitpick comments:
In `@crates/unicode/src/case.rs`:
- Around line 139-144: The surrogate passthrough logic is duplicated in
capitalize_wtf8 and map_wtf8; extract the shared Wtf8Chunk::Surrogate handling
into a helper so both paths reuse it, while preserving the first = false
side-effect only in capitalize_wtf8. Update the branches in those functions to
call the shared helper and keep the existing behavior identical otherwise.
In `@crates/unicode/tests/differential.rs`:
- Around line 312-346: Add the same regression guard used by regen_version_skew
to regen_mapping_version_skew so mapping regressions are not silently written
into the skew file. In regen_mapping_version_skew, before building the output
body, inspect the divergences from all_mapping_divergences and assert that any
cpython=true/crate=false cases are either absent or explicitly covered by the
existing KNOWN_RECATEGORIZATIONS-style exception, then fail fast with a clear
message if a true regression is detected. Use the existing regen_version_skew
logic and simple_mappings_match_cpython_except_documented_version_skew predicate
as the reference for the expected behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 182721fd-f264-4a4f-978c-d5695f5011a8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/sre_engine/src/string.rscrates/unicode/Cargo.tomlcrates/unicode/src/case.rscrates/unicode/tests/data/cpython3.14_mappings.txtcrates/unicode/tests/data/cpython3.14_predicates.txtcrates/unicode/tests/data/version_skew_cpython3.14.txtcrates/unicode/tests/data/version_skew_mappings_cpython3.14.txtcrates/unicode/tests/differential.rscrates/unicode/tests/generate_reference.pycrates/vm/Cargo.tomlcrates/vm/src/anystr.rscrates/vm/src/builtins/str.rscrates/vm/src/stdlib/_sre.rscrates/vm/src/utils.rs
💤 Files with no reviewable changes (2)
- crates/vm/Cargo.toml
- crates/vm/src/utils.rs
Add code-point simple mappings (simple_lowercase/uppercase/titlecase/fold), casing predicates (is_lowercase/is_uppercase/is_titlecase/is_cased/ is_case_ignorable), and the string-level capitalize/title/swapcase/casefold helpers to crates/unicode/src/case.rs, moving the titlecase segmentation and final-sigma logic out of vm/builtins/str.rs verbatim. str.rs and anystr.rs now call through the crate; the is_cased kernel takes plain fn(char)->bool predicates instead of icu BinaryProperty generics. Remove icu_casemap/icu_locale/icu_properties/writeable from crates/vm and the now-unused VecFmtWriter helper. crates/unicode is the only workspace member with a direct icu dependency. str.lower/upper keep using Wtf8::to_lowercase/ to_uppercase. Assisted-by: Claude
SRE_UNI_IS_SPACE is Py_UNICODE_ISSPACE. Replace the hand-rolled BMP code-point list with classify::is_space, which is differential-tested against CPython. A full-range sweep confirms the two agree on every code point. Assisted-by: Claude
lower_unicode/upper_unicode took the first char of the full case mapping, so
code points with a full mapping but no simple one were miscased (e.g.
upper_unicode('ß') returned 'S'). Route them through case::simple_lowercase/
simple_uppercase, matching Py_UNICODE_TOLOWER/TOUPPER.
_sre.unicode_iscased derived casedness from those mappings, which only held
while the full mapping was used; with simple mappings a cased code point that
maps to itself (e.g. the ſt/st ligatures, U+FB05/06) read as uncased and lost its
_casefix equivalence. Query the Cased property directly via case::is_cased.
Assisted-by: Claude
Extend the differential harness to cover is_lowercase/is_uppercase/is_titlecase/ is_cased over the full scalar range, sourced from str.islower/isupper, the Lt category, and _sre.unicode_iscased. Add a parallel sweep of the simple lowercase mapping (Py_UNICODE_TOLOWER via _sre.unicode_tolower) with its own version-skew allow-list; CPython exposes no simple-uppercase oracle, so toupper stays on the SRE unit tests. Record the U+0295 Ll->Lo recategorization (Unicode 16.0.0 -> 17.0.0) as a known reverse-direction divergence so the skew regenerator still rejects genuine regressions. Assisted-by: Claude
Follow-up to #7560, completing the
rustpython-unicodeisolation begun in #8211. Implements #8236.What this does
Moves the remaining case mapping and casing predicates out of
rustpython-vmintorustpython-unicode, so the crate is the single authoritative Unicode path andcrates/unicodebecomes the only workspace member with a directicu4xdependency. Also fixes two long-standingTODO: check with cpythonquirks in the SRE engine.Commits
Move str casing into
rustpython-unicode::caseand drop icu from vm — adds code-point simple mappings (simple_lowercase/uppercase/titlecase/fold), casing predicates (is_lowercase/is_uppercase/is_titlecase/is_cased/is_case_ignorable), and string-levelcapitalize/title/swapcase/casefoldhelpers.str.rs/anystr.rsroute through the crate;icu_casemap/icu_locale/icu_properties/writeableleavecrates/vm.str.lower/upperkeep usingWtf8::to_lowercase/to_uppercase(no icu, and std already applies the final-sigma rule).Route sre
is_uni_spacethroughclassify::is_space— replaces the hand-rolled BMP list. A full-range sweep confirms it agrees with the old list on every code point, andclassify::is_spaceis differential-tested against CPython.Use simple case mappings and the
Casedproperty for sre IGNORECASE —lower_unicode/upper_unicodetook the first char of the full mapping, miscasing code points with a full but no simple mapping (e.g.upper_unicode('ß')returned'S'). Now usesimple_lowercase/simple_uppercase(Py_UNICODE_TOLOWER/TOUPPER)._sre.unicode_iscasedderived casedness from those mappings, which broke the_casefixligature equivalences (ſt/st) once simple mappings were used; it now queries theCasedproperty directly.Sweep casing predicates and simple lowercase mapping against CPython — extends the differential harness (full
0..0x110000range) to the new casing predicates and the simple lowercase mapping, each with its own version-skew allow-list. Records the U+0295Ll→Lorecategorization (Unicode 16.0.0 → 17.0.0) as a known reverse-direction divergence.Verification
crates/unicodeunit + differential tests pass; no_std target (thumbv7em-none-eabi) still builds.test_unicodedata,test_str,test_re,test_pkgutil(run=381).re.IGNORECASEnow matches CPython onß/SS, the ſt/st ligatures, and Kelvin K.crates/unicodeis the only workspace member with a direct icu dependency.🤖 Generated with Claude Code
Summary by CodeRabbit