From d3d9ef8340477a990b2e62bc06b318c9b83c1e69 Mon Sep 17 00:00:00 2001 From: rajashidattapy Date: Tue, 11 Aug 2026 18:01:13 +0530 Subject: [PATCH] fix(normalize_id): ensure idempotency and valid character output by adjusting casefolding order --- graphify/ids.py | 30 ++++-- tests/test_id_normalization_contract.py | 117 ++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 6 deletions(-) diff --git a/graphify/ids.py b/graphify/ids.py index 0740d0a8..424f3ee9 100644 --- a/graphify/ids.py +++ b/graphify/ids.py @@ -16,10 +16,19 @@ module exists so the recipe lives in one place and the two callers can no longer diverge. The recipe: NFKC-normalize (so composed/decomposed Unicode forms collapse), -replace runs of non-word characters with a single underscore (``re.UNICODE`` so -CJK/Cyrillic/Arabic/accented-Latin letters survive instead of collapsing to a -per-file node), collapse repeated underscores, strip leading/trailing -underscores, and casefold. +casefold, NFKC-normalize again (casefold can *expand* a character into a base +letter plus a combining mark — ``İ`` -> ``i`` + U+0307 — and the second pass +recomposes what can be recomposed), replace runs of non-word characters with a +single underscore (``re.UNICODE`` so CJK/Cyrillic/Arabic/accented-Latin letters +survive instead of collapsing to a per-file node), collapse repeated +underscores, then strip leading/trailing underscores. + +Casefolding runs BEFORE the non-word filter, not after. With it last, the +combining marks casefold introduces were never filtered: ``İslemYap`` produced +``i̇slemyap`` — an id containing U+0307, which is not a ``\\w`` character — and a +second pass collapsed it to ``i_slemyap``, so the function was not idempotent +and the builder's re-normalization disagreed with the extractor's ``make_id`` +for any Turkish identifier. """ from __future__ import annotations @@ -32,12 +41,21 @@ __all__ = ["normalize_id", "make_id"] def normalize_id(s: str) -> str: r"""Normalize a single ID string to its canonical form. - Idempotent: ``normalize_id(normalize_id(s)) == normalize_id(s)``. + Guarantees, both enforced by tests: + + - Idempotent: ``normalize_id(normalize_id(s)) == normalize_id(s)``. + - The result contains only ``\w`` characters and ``_``. + + Casefolding before the ``[^\w]+`` filter is what makes both hold — see the + module docstring for why the reverse order silently broke them. """ s = unicodedata.normalize("NFKC", s) + # casefold can expand one character into a letter + combining mark, so it + # must run while the non-word filter can still see the result. + s = unicodedata.normalize("NFKC", s.casefold()) s = re.sub(r"[^\w]+", "_", s, flags=re.UNICODE) s = re.sub(r"_+", "_", s) - return s.strip("_").casefold() + return s.strip("_") def make_id(*parts: str) -> str: diff --git a/tests/test_id_normalization_contract.py b/tests/test_id_normalization_contract.py index 9da1da99..221305e9 100644 --- a/tests/test_id_normalization_contract.py +++ b/tests/test_id_normalization_contract.py @@ -39,6 +39,30 @@ CONTRACT_CASES = [ "x_c1", # must NOT be treated as a chunk suffix here "__dunder__", # leading/trailing underscores stripped "tab\tnewline\nspace ", # whitespace runs -> single underscore + # Casefolding these EXPANDS them into a base letter plus a combining + # mark. With casefold last, the mark landed in the id after the [^\w] filter + # had already run, so the id carried a non-word character and a second pass + # changed it. Turkish identifiers are the common real-world case. + "İ", # İ -> i + U+0307 + "İslemYap", # İslemYap + "fileİname", # İ mid-identifier + "İı_Mixed", # İ with dotless ı + "Große", # ß -> ss (length-changing casefold) + "ẞ", # ẞ capital sharp s -> ss +] + +# Characters whose casefold expands or recomposes — the exact class that broke +# the contract. Kept separate from CONTRACT_CASES because a few of them +# (e.g. U+01F0) legitimately normalize to a precomposed character that is not +# equal to its own casefold, which the lowercase assertion below would reject. +CASE_EXPANDING_CHARS = [ + "İ", # İ LATIN CAPITAL LETTER I WITH DOT ABOVE -> i + U+0307 + "ǰ", # ǰ casefold expands, NFKC then recomposes it + "ͅ", # ͅ COMBINING GREEK YPOGEGRAMMENI -> ι + "ͺ", # ͺ GREEK YPOGEGRAMMENI -> space + ι + "ẞ", # ẞ -> ss + "ῗ", # ῗ iota with dialytika and perispomeni + "ὒ", # ὒ upsilon with psili and varia ] @@ -84,6 +108,58 @@ def test_normalized_ids_are_safe_node_ids(): assert not out.startswith("_") and not out.endswith("_") +@pytest.mark.parametrize("ch", CASE_EXPANDING_CHARS) +def test_case_expanding_chars_yield_word_only_ids(ch): + """The postcondition the old recipe silently broke. + + ``normalize_id`` must emit only ``\\w`` characters and ``_``. Casefolding + last let the combining mark that ``İ``.casefold() produces slip past the + ``[^\\w]+`` filter, so ids carried U+0307 — invisible in most terminals, and + a second normalization pass then rewrote it to ``_``. + """ + out = normalize_id(f"a{ch}b") + assert not re.search(r"[^\w]", out.replace("_", "")), ( + f"normalize_id({ch!r}) -> {out!r} contains a non-word character " + f"({[hex(ord(c)) for c in out]})" + ) + + +@pytest.mark.parametrize("ch", CASE_EXPANDING_CHARS) +def test_case_expanding_chars_are_idempotent(ch): + once = normalize_id(f"a{ch}b") + assert normalize_id(once) == once, ( + f"normalize_id not idempotent for {ch!r}: {once!r} -> {normalize_id(once)!r}" + ) + + +@pytest.mark.parametrize("ch", CASE_EXPANDING_CHARS) +def test_case_expanding_chars_normalize_case_insensitively(ch): + """The point of casefolding: upper and lower spellings must land on one id. + + Asserted instead of ``out == out.casefold()`` because casefold and NFKC do + not commute — ``ǰ`` normalizes to the precomposed U+01F0, which is lowercase + but is not equal to its own casefold. Case-insensitivity is the property the + graph actually depends on. + """ + assert normalize_id(f"a{ch.upper()}b") == normalize_id(f"a{ch.lower()}b") + + +def test_turkish_identifier_ids_match_between_extractor_and_builder(): + """End to end: the drift that split a Turkish symbol into ghost nodes. + + ``make_id`` minted ``islem_i̇slemyap`` (with U+0307) while the builder's + re-normalization produced ``islem_i_slemyap``, so ``_semantic_id_remap``'s + ``startswith(new_stem)`` check missed and the re-key silently no-opped. + """ + stem, symbol = "islem", "İslemYap" + minted = make_id(stem, symbol) + assert _normalize_id(minted) == minted, "builder re-normalization drifts from make_id" + assert minted.startswith(make_id(stem) + "_"), ( + "symbol id lost its file stem prefix, so the re-key cannot relate them" + ) + assert minted == "islem_i_slemyap" + + def test_both_callers_share_one_implementation(): """Guard against re-forking: the two public callers must resolve to the same underlying function object as graphify.ids.normalize_id.""" @@ -117,3 +193,44 @@ def test_property_make_id_equals_normalize_id(s): def test_property_normalize_id_idempotent(s): once = normalize_id(s) assert normalize_id(once) == once + + +# The plain st.text() property above already existed when this bug shipped, and did +# not catch it: the bug needs one specific codepoint (U+0130) out of ~1.1M, which +# a uniform draw essentially never produces. These strategies bias the search +# toward the characters that actually stress the recipe — case-expanding letters +# and combining marks — so the class stays covered rather than relying on luck. +_stress_alphabet = st.one_of( + st.sampled_from(CASE_EXPANDING_CHARS), + st.sampled_from("Iıİi_.-/aZ0"), # Turkish dotted/dotless pairs + separators + st.characters(categories=["Lu", "Ll", "Lt", "Mn", "Nd", "Pc"]), +) +_stress_text = st.text(alphabet=_stress_alphabet, max_size=12) + + +@given(_stress_text) +def test_property_normalize_id_idempotent_under_case_stress(s): + once = normalize_id(s) + assert normalize_id(once) == once, f"not idempotent for {s!r} -> {once!r}" + + +@given(_stress_text) +def test_property_normalize_id_emits_only_word_chars(s): + """The postcondition the old recipe violated: only \\w and _ may survive.""" + out = normalize_id(s) + assert not re.search(r"[^\w]", out.replace("_", "")), ( + f"normalize_id({s!r}) -> {out!r} leaked a non-word character" + ) + + +@given(_stress_text) +def test_property_normalize_id_agrees_with_its_own_caseless_form(s): + """Feeding an already-caseless string must not change the answer. + + Deliberately NOT ``normalize_id(s.upper()) == normalize_id(s.lower())``: + ``str.upper()`` is locale-independent and lossy, so Turkish dotless ``ı`` + uppercases to ``I`` and then casefolds to ``i`` — the two spellings are + genuinely different characters, not a normalization failure. Caseless + equivalence via ``casefold`` is the invariant the graph relies on. + """ + assert normalize_id(s) == normalize_id(s.casefold())