Skip to content

fix(matcher): include last start in unicode substring and leading-digit ignore-case - #104

Open
SebTardif wants to merge 1 commit into
helix-editor:masterfrom
SebTardif:fix/substring-suffix-and-leading-digits
Open

fix(matcher): include last start in unicode substring and leading-digit ignore-case#104
SebTardif wants to merge 1 commit into
helix-editor:masterfrom
SebTardif:fix/substring-suffix-and-leading-digits

Conversation

@SebTardif

Copy link
Copy Markdown

Summary

Include the last valid start when searching Unicode (and CRLF) haystacks, and search the full last-valid-start window for ignore-case ASCII needles whose first letter is not at index 0.

Problem

Two substring false negatives on current master.

Unicode and CRLF suffix miss

substring_match_non_ascii iterated haystack[start..haystack.len() - needle.len()], so the last valid start (len - needle.len()) was never tried. ASCII haystacks were fine. Any Unicode haystack, or a \r\n that forces the Unicode Utf32Str variant, missed a multi-character substring sitting at the end.

Matcher::substring_match("üfoo", "foo")  // None
Matcher::postfix_match("üfoo", "foo")    // Some(...)
Pattern::parse("'bar").match_list(["foo\r\nbar", "foo\nbar", "bar"])
// missed the CRLF row

This is leftover from #34. The exclusive end was introduced in 3d46b625 while closing that issue (panic / false positives). The bound is one short of the last valid start. The ASCII prefilter already uses haystack.len() - needle.len() + 1.

Leading-digit ignore-case window

When ignore_case is set and the first a-z in the needle is at index len >= 2, the code memmems the full needle in a window of haystack.len() - needle.len() + len (too short) and returns None with no fallback. Some(0) and Some(1) already use the correct window + 1.

This is not #100. The needles are already lowercase. Pattern / Atom fold mixed-case input before calling Matcher. The miss happens for '12foo vs both xx12FOOyy and xx12fooyy.

Matcher::substring_match("xx12FOOyy", "12foo")  // None (needle already lowercase)
Pattern::parse("'12foo").match_list(["xx12FOOyy", "xx12fooyy"])  // []

Change

In matcher/src/exact.rs:

  • Unicode exclusive end is now haystack.len() - needle.len() + 1 (last valid start is included).
  • Some(len) for len >= 1 uses the same path as Some(1): search the first non-letter byte on haystack[..haystack.len() - needle.len() + 1], then verify the tail with normalize.

Validation

Red (production from origin/master, new tests only):

  • tests::test_substring_unicode_suffix: "foo" did not match "üfoo" (None vs Some(56))
  • tests::test_substring_crlf_suffix: CRLF suffix should match: ["foo\nbar", "bar"]
  • tests::test_substring_leading_digits_ignore_case: "12foo" did not match "xx12FOOyy" (None vs Some(110))

Green after the two window fixes:

  • cargo test --workspace: 28 nucleo-matcher lib tests, 8 nucleo tests, and 9 doc-tests passed
  • cargo fmt --all --check passed

Clippy -D warnings is already red on master (#98); not used as a gate.

Refs

Ref #34

…it ignore-case

Unicode and CRLF substring used an exclusive end one short of the last
valid start, leftover from 3d46b62 while closing issue 34. Ignore-case
ASCII needles whose first letter sits at index >= 2 searched the full
needle in a too-short window and returned None.

Needles in the new tests are already lowercase. This is not PR 100.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
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.

1 participant