Validate property names #7284

robertbastian · 2025-12-05T13:48:35Z

No description provided.

robertbastian · 2025-12-05T15:46:11Z

provider/source/src/properties/enum_codepointtrie.rs

 expand!(
    (
+        icu::properties::props::CanonicalCombiningClass,
        PropertyEnumCanonicalCombiningClassV1,


I tried doing something like icu::properties::props::CanonicalCombiningClass::DataMarker here, but unfortunately that implementation is not allowed as the associated type is from a different crate

sffc · 2025-12-05T18:22:25Z

/gemini review

It seems fine so I'm happy to stamp it, but I didn't take the time to learn the macro :). I think @Manishearth knows this code better than me

gemini-code-assist

Code Review

This pull request is a large refactoring to validate Unicode property names against the data files. It introduces validation logic in the data providers and updates property name definitions to match the standard. The changes improve code quality, robustness, and type safety. I have one suggestion for improving robustness in the data generation code.

provider/source/src/properties/bidi.rs

Manishearth

Overall looks good.

I found this PR a bit tedious to review: I didn't know where the actual important changes were vs changes downstream of that. It would be nice to have some context in PR bodies or a comment to help review.

Manishearth · 2025-12-05T18:46:35Z

components/properties/src/props.rs

 make_binary_property! {
-    name: "Alnum";
-    short_name: "Alnum";
+    name: "alnum";


observation: these don't follow the property naming convention, but that might be because they're compat properties defined by the regex spec.

https://www.unicode.org/reports/tr18/#Compatibility_Properties

nit: please document the source to use for these names on the macro. There are multiple.

this is what's in icuexportdata

I don't trust those names, icuexportdata is a hodgepodge. We should follow and check against something concrete.

To be clear: I'm not saying we should download the spec or UCD and check against it, I'm saying our code source of truth should be something that is not "this makes the tests pass", and if the tests fail we can see if things need to be fixed in icuexportdata.

Otherwise I don't really see the point of this PR.

I don't trust those names, icuexportdata is a hodgepodge

afaict icuexportdata uses the names that ICU4C uses. If not, that's a bug, but from the changes I made here it seems that they're all correct.

Well neither of those defines short names. It would take significant extra work to chase down all the standards and compatible libraries of these names. You have already approved this PR, so I assume this is not blocking?

Yes, this isn't blocking, but I also don't consider this PR to really be an improvement without that. Otherwise we're just shuffling things around, since Unicode is not very consistent about these.

From what I've seen Unicode is very consistent, I haven't seen alternative spellings anywhere expect for here.

This actually fixes new_for_ecma262, because that actually documents it uses the names from https://tc39.es/ecma262/#table-binary-unicode-properties, but some of them were not correct.

I'd like icuexportdata to be something we consider trustworthy.

provider/source/src/properties/script.rs

Manishearth · 2025-12-05T18:54:00Z

provider/source/src/properties/enum_codepointtrie.rs

-

-        $prop_name:literal)),+,) => {
+macro_rules! expand {


issue: please document this macro (specifically, the convert stuff). It's way harder to review changes to a complex macro when you don't know what it's supposed to do.

(Yes, I know it's previously undocumented)

trust? there's no data diff

I guess that's fine. I would prefer if macros gained documentation over time especially when they have weird input syntax.

Manishearth · 2025-12-05T18:58:32Z

provider/source/src/properties/bidi.rs

-        let bpt_trie = CodePointTrie::try_from(bpt_data).map_err(|e| {
-            DataError::custom("Could not parse CodePointTrie TOML").with_display_context(&e)
-        })?;
+        let bidi_m_cpinvlist = self


observation: this diff mostly looks like a cleanup, but it also switches get_binary_prop_for_code_point_set/etc over to accepting multiple names

Manishearth · 2025-12-05T18:59:29Z

provider/source/src/properties/bin_cp_set.rs

-            .ok_or_else(|| DataErrorKind::MarkerNotFound.into_error())
+            .ok_or_else(|| DataErrorKind::MarkerNotFound.into_error())?;
+
+        if name != data.long_name


observation: this is where the actual check happens

Manishearth · 2025-12-05T18:59:44Z

provider/source/src/properties/bin_cp_set.rs

+}
+
+impl super::uprops_serde::binary::BinaryProperty {
+    pub(crate) fn build_inversion_list(&self) -> CodePointInversionList<'static> {


praise: good refactor

Manishearth · 2025-12-05T19:01:58Z

provider/source/src/properties/bin_cp_set.rs

-    (PropertyBinaryXdigitV1, "xdigit"),
-    (PropertyBinaryXidContinueV1, "XIDC"),
-    (PropertyBinaryXidStartV1, "XIDS")
+    (


suggestion: can we rustfmt::skip this to avoid wrapping?

alternatively perhaps the syntax of expand can be turned into something like PropType => PropMarker. Or we can have it imply icu::properties::props::. Or something.

robertbastian · 2025-12-06T17:24:52Z

I found this PR a bit tedious to review: I didn't know where the actual important changes were vs changes downstream of that. It would be nice to have some context in PR bodies or a comment to help review.

I should have called this out I guess. If you agree with the changes in icu_properties, you can basically ignore the changes in datagen. They are correct (still generate the same data), and they triggered the changes in icu_properties.

robertbastian · 2025-12-08T14:15:31Z

Merging because I want to pick this up for the patch release

robertbastian force-pushed the names branch from 21f82da to 4cf5f6c Compare December 5, 2025 13:50

validate property names

65219c0

robertbastian force-pushed the names branch from 4cf5f6c to 65219c0 Compare December 5, 2025 15:44

robertbastian marked this pull request as ready for review December 5, 2025 15:44

robertbastian requested review from a team, Manishearth and sffc as code owners December 5, 2025 15:44

robertbastian commented Dec 5, 2025

View reviewed changes

gemini-code-assist bot reviewed Dec 5, 2025

View reviewed changes

provider/source/src/properties/bidi.rs Show resolved Hide resolved

Manishearth approved these changes Dec 5, 2025

View reviewed changes

robertbastian merged commit 9d734c2 into unicode-org:main Dec 8, 2025
32 checks passed

robertbastian deleted the names branch December 8, 2025 14:15

robertbastian added a commit to robertbastian/icu4x that referenced this pull request Dec 8, 2025

Validate property names (unicode-org#7284)

0656e98

robertbastian added a commit that referenced this pull request Dec 8, 2025

Validate property names (#7284)

e2db38e

Validate property names #7284

Validate property names #7284

Uh oh!

Conversation

robertbastian commented Dec 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc commented Dec 5, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian commented Dec 6, 2025

Uh oh!

robertbastian commented Dec 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robertbastian Dec 7, 2025 •

edited

Loading