fix(expr)!: match Python semantics in character classification functions - #341
Conversation
fa16d8f to
f8350e2
Compare
leongdl
left a comment
There was a problem hiding this comment.
Reviewed at tip 0258acf with the full suite green locally (3615 passed) and a mutation harness over the new behaviours — every claimed fix has a named falsifying test (7/7 mutants caught, including the table binary search being pinned by its own exhaustive binary-vs-linear test, which is a genuinely independent oracle). Expected values in the new integration tests reproduce 0-mismatch against a local CPython. Nice work — the generator-verifies-against-CPython-before-writing design is exactly right.
One real (low-severity, non-blocking) finding below on guard ordering, verified three ways rather than by inspection.
e8d9e1d to
45d3a9d
Compare
|
Reviewer orientation — TLDR and call stack (posted to help others review; verified locally at TLDRThe expression functions
Call stackAll error paths return What makes the tables trustworthy (the crux of the review)
Known-open items are in the inline threads above: the |
45d3a9d to
477af40
Compare
…nctions isdigit/isalpha/isalnum/isspace/isupper/islower used Rust char predicates, which draw from different Unicode properties than Python's str methods: - isdigit used is_ascii_digit(), missing 878 code points (non-ASCII decimal digits like Arabic-Indic, and Numeric_Type=Digit characters like superscripts). This made isdigit inconsistent with isalnum for those characters, the inconsistency reported in the issue. - isalpha used the Alphabetic property, a superset of Python's L* categories (extra: Nl, Other_Alphabetic marks Mn/Mc/So). - isspace used White_Space, missing Python's U+001C..U+001F separators. - isupper/islower filtered by alphabetic rather than Python's cased characters, diverging on uncased letters (e.g. CJK) and titlecase. The predicates now use lookup tables generated directly from CPython by scripts/generate_unicode_tables.py (currently CPython 3.14.3 / Unicode 16.0.0), with an exhaustive all-code-points round-trip verification at generation time. isupper/islower implement the cased-character rule. Fixes OpenJobDescription#309 BREAKING CHANGE: string classification results change for non-ASCII input to match Python exactly — e.g. isdigit('٣') is now true, isalpha('Ⅻ') is now false, isspace(FS/GS/RS/US) is now true, and islower/isupper ignore uncased characters. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Review follow-up to the classification predicate fix: the two
case-transforming functions in the same file also used Rust char
semantics and diverged from CPython.
- title() used is_alphanumeric() for word boundaries; Python's do_title
advances words on uncased characters, so digits restart words
(title('1st') == '1St').
- Both functions uppercased where Python titlecases: the dz-digraph
U+01C6 must map to titlecase U+01C5, and word-start full mappings
apply (title('ssß') == 'Ssß' start expansion, capitalize('ßx') ==
'Ssx').
- Lowercasing the rest of a word ignored the Final_Sigma context rule
(title('OΣ K') == 'Oς K'), which Rust's context-free
char::to_lowercase cannot express.
The table generator now also emits TITLE_MAP (full ToTitleFull
mappings probed from CPython single-character str.title()) and a
CASE_IGNORABLE table (probed via CPython's Final_Sigma handling, since
unicodedata does not expose the property). title()/capitalize()
implement CPython's do_title/do_capitalize over those tables.
BREAKING CHANGE: title() and capitalize() results change to match
Python exactly — digits and other uncased characters now start new
words in title(), titlecase digraphs map to Lt forms instead of
uppercase, and U+03A3 lowers to final sigma where Python does.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
CPython's int() and float() replace Numeric_Type=Decimal characters
(general category Nd) with their ASCII values before parsing, so
int('\u0663') == 3. The expression functions previously parsed ASCII only,
which broke the guard pattern `int(Param.X) if isdigit(Param.X) else 0`
for Nd digits once isdigit() gained Python-parity Unicode semantics.
- Add a DECIMAL (Nd) table and decimal_digit_value() to the generated
unicode tables; the generator verifies every range is a whole number
of zero-aligned 0-9 runs so the value is (cp - range_start) % 10.
- Normalize Nd digits to ASCII in int_from_string and float_from_string.
Numeric_Type=Digit characters like '\u00b2' remain errors, exactly as in
CPython. Implicit string coercion (ExprValue::from_str_coerce) is
deliberately unchanged.
- Document the semantics and remaining intentional divergences in
specs/expr/function-library.md.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
CPython's str.strip/lstrip/rstrip and no-separator str.split/rsplit use
the same Py_UNICODE_ISSPACE predicate as str.isspace, which is Unicode
White_Space plus the information separators U+001C..U+001F. The
implementations used Rust's str::trim/split_whitespace (exactly
White_Space), so after isspace() gained the Python-parity SPACE table
the three disagreed: isspace('\u001c') was true but strip and split
ignored it. Route all of them through the SPACE table.
int()/float() trimming deliberately stays on Rust's str::trim: CPython's
int()/float() accept White_Space around the number but reject
U+001C..U+001F (int('\u001c5') raises even though isspace('\u001c') is
true), so White_Space is already the CPython-exact set there. Pinned by
test and documented in specs/expr/function-library.md.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
binary_search_matches_linear_scan did a full linear scan of the ranges for each of the 1.1M code points (~290M iterator steps in the debug profile), and title_map_is_well_formed's per-code-point find() over the 1,479-entry TITLE_MAP was worse (~1.6B steps). Replace both inner scans with a forward-advancing cursor over the sorted entries — the same trick verify() in the generator uses — keeping the exhaustive coverage of every range boundary while making the sweeps linear overall. Measured on the debug profile: the unicode_tables test module drops from 2.65s to 0.27s. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
54bc72f to
b6584df
Compare
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
b6584df to
ee81bc4
Compare
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Fixes: #309
What was the problem/requirement? (What/Why)
Background: OpenJD job template expressions include string classification
functions —
isdigit,isalpha,isalnum, and friends — that are namedafter, and meant to behave like, Python's
strmethods. "Is this character adigit?" sounds like it has one answer, but it doesn't: every language draws
its own lines through the Unicode character set. Rust's standard library
draws different lines than Python does, and this implementation was using
Rust's.
Issue #309 reported the visible symptom: for
'٣'(the Arabic-Indic digitthree),
isdigitsaidfalsewhileisalnumsaidtrue— the predicatescontradicted each other, because
isdigitwas checking only ASCII0-9while
isalnumaccepted anything Unicode calls alphabetic or numeric.Comparing every one of Unicode's ~1.1 million code points against CPython
showed the problem was wider than the report:
isdigitwas wrong for 878 characters (all non-ASCII digit systems, plussuperscripts like
²)isalpha/isalnumdisagreed with Python on thousands of characters(Roman numerals like
Ⅻ, combining marks, circled letters)isspacemissed four control characters Python counts as whitespaceisupper/islowerignored the wrong characters: Python skips uncasedcharacters (so
'a五'is lowercase), this implementation skippednon-alphabetic ones
What was the solution? (How)
Since the expression language dialect is defined by Python's behavior, the
predicates now use lookup tables generated directly from CPython by a new
script,
scripts/generate_unicode_tables.py. The script asks CPython itself("is this character a digit to you?") for every code point, writes the
answers into
crates/openjd-expr/src/functions/unicode_tables.rsas compactrange tables (checked in, currently CPython 3.14.3 / Unicode 16.0.0), and
verifies the generated tables round-trip exactly before writing. The six
predicates are now binary searches over those tables, with
isupper/islowerimplementing Python's cased-character rule.
To adopt a newer Unicode version later, rerun the script with a newer CPython.
What is the impact of this change?
The classification functions now return exactly what Python returns for any
input. Results change only for non-ASCII input (and the four
isspacecontrol characters).
How was this change tested?
analysis, with CPython-verified expected values.
an exhaustive binary-search-vs-linear-scan check over all code points).
code points at generation time.
openjd-exprsuite: 3,313 tests pass;cargo test --workspacegreen.Unicode classification conformance test (submitted separately to
openjd-specifications) that fails against the previous implementation and
passes against this one.
-D warnings, rustfmt, and copyright header checks all clean.Was this change documented?
Yes —
specs/expr/function-library.mddocuments the Python-parity semantics,the cased-character rule, and the table regeneration procedure. A companion
openjd-specifications PR clarifies RFC 0006 and the Expression Language spec,
which previously didn't say which character-class convention applies.
Is this a breaking change?
Yes — the commit is marked
fix(expr)!with aBREAKING CHANGEfooter.Classification results change for non-ASCII input: e.g.
isdigit('٣')is nowtrue,isalpha('Ⅻ')is nowfalse, andislower('a五')is nowtrue.No code changes are required of users; templates that relied on the old
answers for non-ASCII input will see the Python-correct results.
Does this change impact security?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.