unicode crate - #8211

crates/stdlib/src/unicodedata.rs (1)

15-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate the parsing logic already provided by NormalizeForm::FromStr.

rustpython_unicode::NormalizeForm already implements FromStr with the exact same "NFC"/"NFKC"/"NFD"/"NFKD" mapping. Re-implementing it here means the two copies can drift if a new form is ever added upstream.

♻️ Proposed refactor to reuse `NormalizeForm::FromStr`

 impl<'a> TryFromBorrowedObject<'a> for NormalizeFormArg {
     fn try_from_borrowed_object(vm: &VirtualMachine, obj: &'a PyObject) -> PyResult<Self> {
         obj.try_value_with(
-            |form: &PyStr| match form.as_bytes() {
-                b"NFC" => Ok(Self(NormalizeForm::Nfc)),
-                b"NFKC" => Ok(Self(NormalizeForm::Nfkc)),
-                b"NFD" => Ok(Self(NormalizeForm::Nfd)),
-                b"NFKD" => Ok(Self(NormalizeForm::Nfkd)),
-                _ => Err(vm.new_value_error("invalid normalization form")),
-            },
+            |form: &PyStr| {
+                form.to_str()
+                    .and_then(|s| s.parse().ok())
+                    .map(Self)
+                    .ok_or_else(|| vm.new_value_error("invalid normalization form"))
+            },
             vm,
         )
     }
 }

🤖 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/stdlib/src/unicodedata.rs` around lines 15 - 29, The normalization
form parsing in NormalizeFormArg duplicates the existing NormalizeForm::FromStr
mapping, so refactor the TryFromBorrowedObject implementation to delegate to
that parser instead of matching on the byte strings directly. Update the logic
in unicodedata::NormalizeFormArg::try_from_borrowed_object to convert the PyStr
into a Rust string and parse it through NormalizeForm::from_str, keeping the
same invalid-input error handling.

crates/unicode/src/classify.rs (1)

99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant round-trip in test iteration.

('\0'..='\u{2FFFF}') already yields char values (Rust's char range excludes surrogates), so .filter_map(|c| char::from_u32(c as u32)) always returns Some(c) unchanged — it never filters anything.

♻️ Simplify the iteration

-        for c in ('\0'..='\u{2FFFF}').filter_map(|c| char::from_u32(c as u32)) {
+        for c in '\0'..='\u{2FFFF}' {

🤖 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/classify.rs` around lines 99 - 109, The iteration in
numeric_type_chain_holds is doing a redundant char::from_u32 round-trip because
the '\0'..='\u{2FFFF}' range already produces valid char values; simplify the
loop to iterate directly over the char range and remove the filter_map
conversion. Keep the rest of the assertions in numeric_type_chain_holds
unchanged.

crates/unicode/tests/differential.rs (1)

28-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

is_continue (XID_Continue) is never differentially validated.

crate_predicate/classify_is_identifier_char only exercise identifier::is_start via the single-char isidentifier reference data. There's no equivalent sweep validating identifier::is_continue (used for all characters after the first in str.isidentifier) against CPython, so a divergence in XID_Continue handling would go undetected by this suite.

Consider adding a second reference predicate (e.g. "isident_continue", generated in generate_reference.py via ('_' + chr(cp)).isidentifier() sliced appropriately, or directly via unicodedata-adjacent XID_Continue data) and mapping it here to identifier::is_continue.

🤖 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 28 - 52, The differential
test only validates identifier start behavior through
`classify_is_identifier_char` and `identifier::is_start`, so XID_Continue is
never compared against CPython. Add a second predicate path in `crate_predicate`
for identifier continuation (for example an `"isident_continue"` case) and map
it to `rustpython_unicode::identifier::is_continue`. Update the reference
generation to produce a matching single-character continue oracle, so the test
suite exercises both start and continue behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

extra_tests/snippets/stdlib_unicode_shared.py (1)

83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Ruff RUF001 false positive — intentional Unicode digit test.

The flagged ambiguous-character warning on line 84 is expected here since the test intentionally exercises non-ASCII decimal digits. Consider adding a # noqa: RUF001 if you want clean lint output, otherwise safe to ignore.

🤖 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 `@extra_tests/snippets/stdlib_unicode_shared.py` around lines 83 - 86, The
Unicode digit assertions in the test are intentional, so the Ruff RUF001 warning
should be silenced where the non-ASCII decimal digit cases are exercised. Update
the relevant assert in stdlib_unicode_shared.py, near the unicode digit
fullmatch checks in the test snippet, to explicitly suppress RUF001 so the
intentional ambiguity does not fail linting.

Source: Linters/SAST tools

crates/unicode/src/normalize.rs (1)

81-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add NFKC/NFKD coverage to is_normalized tests.

Tests exercise Nfc/Nfd round-tripping and the surrogate-skip behavior, but no case exercises NormalizeForm::Nfkc/Nfkd branches of is_normalized. Since these are separate match arms dispatching to distinct ICU normalizer types, a regression there wouldn't be caught by the current suite.

✅ Suggested addition

     #[test]
     fn is_normalized_skips_lone_surrogates() {
         // A lone surrogate splits the text into UTF-8 runs; each run is checked
         // independently, so a surrogate next to normalized text stays normalized.
         let mut buf = Wtf8Buf::from("é");
         buf.push(CodePoint::from_u32(0xD800).unwrap());
         assert!(is_normalized(NormalizeForm::Nfc, &buf));
         assert!(!is_normalized(NormalizeForm::Nfd, &buf));
     }
+
+    #[test]
+    fn is_normalized_compatibility_forms() {
+        let composed = Wtf8Buf::from("fi"); // U+FB01 LATIN SMALL LIGATURE FI
+        assert!(is_normalized(NormalizeForm::Nfc, &composed));
+        assert!(!is_normalized(NormalizeForm::Nfkc, &composed));
+        assert!(!is_normalized(NormalizeForm::Nfkd, &composed));
+    }
 }

🤖 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/normalize.rs` around lines 81 - 111, The current tests in
normalize.rs cover NFC/NFD only, so add `is_normalized` cases that explicitly
exercise the `NormalizeForm::Nfkc` and `NormalizeForm::Nfkd` match arms. Extend
the `tests` module with assertions using a string that differs under
compatibility normalization (for example, a compatibility character plus its
normalized equivalent) and verify both `is_normalized` and `normalize` behavior
for `Nfkc` and `Nfkd`, alongside the existing `normalization_round_trips`
coverage.