Keep --ignore-regex from shifting match offsets#3980
Open
chuenchen309 wants to merge 1 commit into
Open
Conversation
_ignore_word_sub() replaced each ignored region with a single space, which changes the length of the text. Matches are found in that substituted text but several later checks index back into the original line, so every offset after an ignored region was wrong: - the escape-sequence check reads line[match.start() - 1] - --ignore-sic matches the line from match.end() - the interactive context display slices the line The result was false positives from an --ignore-regex that does not overlap the reported word at all. With `var = 'IGNOREME \nwe are here'`, codespell is quiet by default (the \n escape suppresses "nwe"), but --ignore-regex=IGNOREME made it report `nwe ==> new`. Pad each ignored region to its own length instead. This also matches what the option is documented to do -- ignore by treating the region as whitespace -- and the extra spaces do not affect word splitting, so detection is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_ignore_word_sub()replaces each ignored region with a single space, which changes the length of the text. Matches are then found in that substituted text, but several checks index back into the original line:line[match.start() - 1](_codespell.py:1055)--ignore-sicmatches the line frommatch.end()(:1067):1079→:825)So any ignored region longer than one character shifts every offset after it, and those checks read the wrong place. The visible result is a false positive caused by an
--ignore-regexthat doesn't overlap the reported word at all.Reproducing
IGNOREMEis 8 characters and becomes 1, sonweis found at offset 10 of the substituted text while it really lives at 17. The escape check readsline[9]('N') instead ofline[16]('\').--ignore-sicbreaks the same way when the ignored region precedes the word:The change
Pad each ignored region to its own length. This also lines up with what the option is documented to do — ignore by treating the region as whitespace — and the extra spaces don't affect
[\w\-'’]+word splitting, so detection is unchanged.--ignore-regex=donnonabandonned donnstill reports 0,\bdonn\bstill reports 1.On performance
@DimitriPapadopoulos — you raised this on #3978 and it's a fair thing to ask here too, since this one is on the per-line path rather than the
-wpath. So I measured rather than guessed:--ignore-regex: literally unchanged. Theif ignore_word_regex:guard means the substitution never runs, so there is no new cost for anyone not using the option.--ignore-regex, end-to-end: 62,280 lines / 2.1 MB, best-of-5, patternSKIP:[a-z]*|IGNOREME→ 0.296s → 0.308s, about +4%.sub()call alone: a line containing two matches goes 0.96 µs → 1.18 µs; a line with no matches is identical, because the callable is never invoked. Most lines have no matches.I used
m.end() - m.start()rather thanlen(m.group())— same result, measurably cheaper (1.18 µs vs 1.38 µs) since it skips materializing the matched string.If +4% on that path is still too much, the alternative is to drop the masking entirely and filter matches by ignored span instead, but that's a much larger change and I didn't want to reach for it uninvited.
Testing
test_ignore_regex_does_not_shift_offsetscovers both the escape-sequence and[sic]paths. It fails onmain(assert 1 == 0) and passes with the fix. Existingtest_ignore_regex_optionis untouched and still passes.test_command,test_stdin,test_args_from_file) are pre-existingFileNotFoundErrorfor thecodespellexecutable in my sandbox — I confirmed the failing set is byte-identical with and without my change by stashing it.ruff check,ruff format --check, andcodespell --toml pyproject-codespell.precommit-tomlare all clean.Note there's an ancient open PR, #2049, that touches this same line for an unrelated reason (consecutive matches). It's independent of this fix, but flagging it since it would conflict textually if it were ever revived.
Disclosure: I used an AI coding assistant (Claude Code) to find this and help write the fix. The repros and benchmarks above are ones I ran myself; happy to answer anything about the change.