Skip to content

fix: clamp n to len(pwcs) in wcswidth/wcstwidth to avoid IndexError#227

Closed
gaoflow wants to merge 2 commits into
jquast:masterfrom
gaoflow:fix-wcswidth-n-exceeds-length
Closed

fix: clamp n to len(pwcs) in wcswidth/wcstwidth to avoid IndexError#227
gaoflow wants to merge 2 commits into
jquast:masterfrom
gaoflow:fix-wcswidth-n-exceeds-length

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Bug

`wcswidth()` and `wcstwidth()` raise `IndexError` when the `n` parameter is larger than the length of the string.

```python

from wcwidth import wcswidth
wcswidth('hello', 10)
IndexError: string index out of range
```

Root cause: The `end` variable is set directly to `n`:

```python
end = len(pwcs) if n is None else n
```

When `n > len(pwcs)`, the `while idx < end` loop tries to access `pwcs[idx]` past the end of the string.

Fix

Clamp `end` to `len(pwcs)`:

```python
end = len(pwcs) if n is None else min(n, len(pwcs))
```

This is the natural Python semantics for sized strings: `n` larger than the string length is equivalent to measuring the whole string. The same fix applies to `wcstwidth()`.

Test

Added `test_wcswidth_n_exceeds_length` covering ASCII, wide characters, and ZWJ clusters.

```
1316 passed, 10 skipped
```

Passing n > len(pwcs) to wcswidth() or wcstwidth() causes an
IndexError when the loop index reaches the end of the string, because
`end` was set to n directly.

Fix: `end = min(n, len(pwcs))` so that n larger than the string length
is treated the same as measuring the whole string, which is the natural
Python semantics for sized strings.

Adds a regression test covering ASCII, wide characters, and ZWJ clusters.
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.79%. Comparing base (9df7261) to head (49c5e74).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##            master     #227      +/-   ##
===========================================
- Coverage   100.00%   99.79%   -0.21%     
===========================================
  Files           27       27              
  Lines         1924     1967      +43     
  Branches       456      462       +6     
===========================================
+ Hits          1924     1963      +39     
- Misses           0        2       +2     
- Partials         0        2       +2     

☔ 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.

@codspeed-hq

codspeed-hq Bot commented Jun 25, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 108 untouched benchmarks


Comparing gaoflow:fix-wcswidth-n-exceeds-length (49c5e74) with master (d1c99fe)

Open in CodSpeed

@gaoflow

gaoflow commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Pushed ab680a8 to address the red checks:

  • applied the docformatter change requested for the new docstring
  • added wcstwidth(..., n > len(...)) coverage for the same overflow case
  • added narrow_wider terminal override coverage so the _wcswidth.py / _width.py missing lines are exercised

Local verification:

  • PYTHONPATH=$PWD uv run --python python3.11 --with-requirements requirements-tests39.txt python -m pytest tests/test_core.py::test_wcswidth_n_exceeds_length tests/test_term_overrides.py::test_narrow_wider_width -q
  • uvx --python python3.11 --from docformatter==1.7.7 docformatter --check --diff --recursive --pre-summary-newline --wrap-summaries=100 --wrap-descriptions=100 wcwidth/ bin tests/
  • full local Python 3.11 test suite with coverage: 1318 passed, 10 skipped; _wcswidth.py and _width.py both report 100% in that run.

@jquast

jquast commented Jun 29, 2026

Copy link
Copy Markdown
Owner

Thanks, this will be in next release #228

Not sure if you caught this bug from integration, or from linting, but in general this argument is not recommended to be used, #208 (comment) but I have since removed this recommendation

@jquast jquast closed this Jun 29, 2026
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