Skip to content

Fix ZWJ swallowing the following character; clearer TypeErrors for bad input#230

Open
rantkiglorelgmx-lab wants to merge 3 commits into
jquast:masterfrom
rantkiglorelgmx-lab:fix/zwj-swallow-and-type-errors
Open

Fix ZWJ swallowing the following character; clearer TypeErrors for bad input#230
rantkiglorelgmx-lab wants to merge 3 commits into
jquast:masterfrom
rantkiglorelgmx-lab:fix/zwj-swallow-and-type-errors

Conversation

@rantkiglorelgmx-lab

Copy link
Copy Markdown

The bug (most significant of the 3 commits)

In wcswidth()/wcstwidth(), the ZWJ (U+200D) branch advances idx += 2, unconditionally skipping the character immediately after a ZWJ without measuring it. This silently swallows characters and produces wrong widths:

  • wcswidth('a‍\x07b') returns 2, but must return -1: the BEL (U+0007) after the ZWJ is a C0 control, and the docstring promises -1 for C0/C1 controls.
  • wcswidth('‍一') (leading ZWJ then CJK) returns 0, but must return 2: the wide ideograph is swallowed.
  • The same swallow affects wcstwidth, which shares the logic.

A ZWJ legitimately continues an emoji grapheme cluster (UAX #29 GB11), so the joined character should be consumed without adding width — but only when there is a measured base to join to and the following character isn't a control. The fix gates the skip on a new seen_base flag (set once any base character has been measured) and on the next character not being a C0/C1 control (its wcwidth is negative); otherwise it consumes only the ZWJ and lets the loop measure the next character normally.

This preserves every currently-correct output, including legitimate emoji ZWJ-sequence combining (e.g. '\U0001F468‍\U0001F469' == 2) — verified with a 60,000-input randomized sweep comparing base vs. fixed across ASCII, controls, CJK, combining marks, emoji, ZWJ, VS15/16, regional indicators, Fitzpatrick, and virama inputs for both wcswidth and wcstwidth. Every changed output was wrong→right; zero changes occurred on any input without a ZWJ. Non-vacuity confirmed: reverting only the source change makes the new regression tests fail for the documented reason (assert 2 == -1 / assert 0 == 2).

I recognize this touches a performance-sensitive hot path with a lot of careful prior work (virama, VS15/16, terminal overrides) — happy to adjust the gating condition or move the check if there's a cheaper way to express it that you'd prefer.

Also included (independent, smaller commits)

  • wcwidth(): clear TypeError for a multi-character string input (previously a cryptic ord() error), preserving all other input types (bytes, None, 0, single chars) unchanged.
  • wcswidth(): clear TypeError for non-string input (previously an AttributeError from .isascii()), add-only.

Verification

Full suite green: 1322 passed, 10 skipped, 1 deselected. All three commits independently red-before-green verified (each added test fails on the parent commit for the right reason) with 100% changed-line coverage.

Co-authored by Claude (Anthropic) as part of an automated hardening pass; independently re-verified (non-vacuity, backward-compatibility sweep, coverage) before submission.

hermes-overnight and others added 3 commits July 3, 2026 09:28
(cherry picked from commit a62107e0211fae99fbe7e129a6bf26a715a388fe)
(cherry picked from commit 31663a1912efd7ad2be40073602e63867d24c819)
The ZWJ (U+200D) branch advanced `idx += 2`, skipping the character
immediately after a ZWJ without measuring it.  This silently swallowed
characters and produced wrong widths:

  - wcswidth('a‍\x07b') returned 2 but must return -1: the BEL
    (U+0007) after the ZWJ is a C0 control, and the docstring promises
    -1 for C0/C1 controls.
  - wcswidth('‍一') (leading ZWJ then CJK) returned 0 but must
    return 2: the wide ideograph was swallowed.

The same swallow affected wcstwidth, which shares the logic.

A ZWJ legitimately continues an emoji grapheme cluster (GB11), so the
joined character is consumed without adding width -- but only when there
is a measured base to join to and the following character is not a
control.  Gate the skip on a new `seen_base` flag (set when any base
character is measured) and on the next character not being a C0/C1
control (its wcwidth is negative); otherwise consume only the ZWJ and
let the loop measure the next character normally.

This preserves every currently-correct output, including emoji
ZWJ-sequence combining (e.g. '\U0001F468‍\U0001F469' == 2) and the
documented "ZWJ after a base consumes the next character" behaviour, and
corrects only the swallowed-control and leading-ZWJ cases.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit 89869612407c024050d01443ab19d2f29ccd3e9b)
@jquast

jquast commented Jul 4, 2026

Copy link
Copy Markdown
Owner
  • Go ahead and burn tokens if you like to fix it to use ``\u200D` if you like, but I probably will not accept this PR unless you can provide a convincing use case. Maybe something like this could belong in a project like python-ftfy to remove erroneous ZWJ as one of the existing or a new "fixer" functions https://ftfy.readthedocs.io/en/latest/fixes.html

  • Regarding adding a len() check to wcwidth(), please consider this is at the base of all functions and any additional overhead multiplies to the cost of every single individual character, this function shall not have any such checks, use the function as it is documented and everything is fine. If a downstream developer is sending long strings to the wrong function, let them discover that error for themselves with their own test cases, we should not negatively impact performance of all users to shield against "future possible developer mistakes".

  • Zero-width unicode is used directly in your PR description and test files, making it impossible to see in the github PR, the test cases don't even make any sense, they are tricky to read in github's browser. We humans must copy and paste text containing ZWJ to an advanced REPL or editor to discover the hidden sequence, our eyes do not see ZWJ when used raw, and most editors do not even show them.

  • Can you provide or cite any text or terminal software output that contains text matching any of the given test cases, or, change the test cases to match any known phrases from popular publications -- ZWJ with a control character, CJK, ascii, etc.

  • Otherwise, I would consider any such strings "glitch unicode", and is intentionally not measured for that reason. There are "undefined behavior" areas, too many to go into in detail, but, to "consider" each edge case -- way out on the edge of random noise text, is harmful programming design to incorporate in a widely-used library. It creates more code, more tests, impacts performance, and has no identifiable benefit.

  • @pytest.mark.skipif(NARROW_ONLY, reason="Test cannot verify on python 'narrow' builds")
    def test_zwj_after_non_emoji():
    """ZWJ after non-emoji unconditionally consumes next character."""
    # This does *not* match most terminal behavior -- it is a negative test,
    # they fail because our library doesn't handle 'glitch' emoji as an
    # optimization. Non-emoji + ZWJ is undefined per Unicode UAX #29 GB11.
    assert wcwidth.wcswidth('xx\u200d\U0001F384') == 2
    assert wcwidth.wcswidth('a\u200d\U0001F600') == 1
    assert wcwidth.wcswidth('\u4e16\u200d\U0001F600') == 2

This does not match most terminal behavior -- it is a negative test,
they fail because our library doesn't handle 'glitch' emoji as an
optimization. Non-emoji + ZWJ is undefined per Unicode UAX #29 GB11.

I have experimented with, at least, validate now to follow ASCII/CJK with ZWJ and an Emoji, and I decide even against that.

This does however have a negative effect of measuring VS-16 and ZWJ-related "glitch" sequences, which is no problem, wcwidth makes no guarantees of their measured length, but its worth writing negative test case to verify these conditions in future changes.

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (5a71d7d) to head (4558922).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #230   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        27           
  Lines         1967      1980   +13     
  Branches       462       465    +3     
=========================================
+ Hits          1967      1980   +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jquast

jquast commented Jul 4, 2026

Copy link
Copy Markdown
Owner

I do not fight against "glitch emojis" in my libraries, the overhead of checking for constraints of non-likely strings serves very little purpose, except maybe to more early inform a developer that they might have used the wrong variable or argument.

@codspeed-hq

codspeed-hq Bot commented Jul 4, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 26.94%

❌ 1 regressed benchmark
✅ 107 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
test_wcswidth_emoji_sequence 63.1 µs 86.4 µs -26.94%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing rantkiglorelgmx-lab:fix/zwj-swallow-and-type-errors (4558922) with master (5a71d7d)

Open in CodSpeed

Comment thread wcwidth/_wcswidth.py
# pylint: disable=too-complex,too-many-branches,duplicate-code,too-many-nested-blocks

if not isinstance(pwcs, str):
raise TypeError(f'wcswidth() expects a string, got {type(pwcs).__name__}')

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library uses typing and the argument is typed for str. This extra isinstance check is not wanted or necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants