Merge pull request #84 from G-Research/83-find_matches_as_indexes-ret…

…urns-bogus-values

Handle empty patterns more reasonably

CHANGELOG.md

@@ -1,8 +1,14 @@
 # Changelog
 
+## 0.19.0
+
+* If an empty string is passed in as a pattern, `AhoCorasick()` will now raise a `ValueError`.
+  Previously using empty patterns could result in garbage results or exceptions when matching.
+* Upgraded to `aho-corasick` v1.1.1.
+
 ## 0.18.0
 
-* Upgrade to `aho-corasick` v1.1.0, which can run faster on ARM machines like newer Macs.
+* Upgraded to `aho-corasick` v1.1.0, which can run faster on ARM machines like newer Macs.
 
 ## 0.17.0
 

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ahocorasick_rs"
-version = "0.18.0"
+version = "0.19.0"
 edition = "2021"
 authors = ["G-Research <[email protected]>", "Itamar Turner-Trauring <[email protected]>"]
 description = "Search a string for multiple substrings at once"

justfile

@@ -19,9 +19,12 @@ install-dev-dependencies:
 
 setup: venv install-dev-dependencies
 
-test:
+lint:
 	flake8 tests/
 	black --check tests/
+	mypy --strict tests
+
+test:
 	pytest tests/
 
 # Prepare for benchmarking; will only work on Linux:

rust-toolchain.toml

@@ -1,3 +1,3 @@
 [toolchain]
-channel = "1.72"
+channel = "1.72.1"
 components = ["rustfmt", "clippy"]

src/lib.rs

@@ -1,4 +1,6 @@
-use std::sync::{Arc, Mutex};
+use std::{sync::{
+    Arc, Mutex,
+}, cell::Cell};
 
 use aho_corasick::{
     AhoCorasick, AhoCorasickBuilder, AhoCorasickKind, Match, MatchError, MatchKind,
@@ -14,7 +16,9 @@ use pyo3::{
 ///
 /// Takes three arguments:
 ///
-/// * ``patterns``: A list of strings, the patterns to match against.
+/// * ``patterns``: A list of strings, the patterns to match against. Empty
+///   patterns are not supported and will result in a ``ValueError`` exception
+///   being raised.
 /// * ``matchkind``: Defaults to ``"MATCHKING_STANDARD"``.
 /// * ``store_patterns``: If ``True``, keep a reference to the patterns, which
 ///   will speed up ``find_matches_as_strings()`` but will use more memory. If
@@ -177,27 +181,39 @@ impl PyAhoCorasick {
             None
         };
 
-        let result = Ok(Self {
-            ac_impl: AhoCorasickBuilder::new()
-                .kind(implementation.map(|i| i.into()))
-                .match_kind(matchkind.into())
-                .build(
-                    first_few_patterns
-                        .into_iter()
-                        .chain(patterns_iter)
-                        .chunks(10 * 1024)
-                        .into_iter()
-                        .flat_map(|chunk| {
-                            let result = chunk.filter_map(|s| s.extract::<String>().ok());
-                            // Release the GIL in case some other thread wants to do work:
-                            py.allow_threads(|| ());
-                            result
-                        }),
-                )
-                // TODO make sure this error is menaingful to Python users
-                .map_err(|e| PyValueError::new_err(e.to_string()))?,
-            patterns,
-        });
+        let has_empty_patterns = Cell::new(false);
+        let ac_impl = AhoCorasickBuilder::new()
+            .kind(implementation.map(|i| i.into()))
+            .match_kind(matchkind.into())
+            .build(
+                first_few_patterns
+                    .into_iter()
+                    .chain(patterns_iter)
+                    .chunks(10 * 1024)
+                    .into_iter()
+                    .flat_map(|chunk| {
+                        let result =
+                            chunk
+                                .filter_map(|s| s.extract::<String>().ok())
+                                .inspect(|s| {
+                                    if s.is_empty() {
+                                        has_empty_patterns.set(true);
+                                    }
+                                });
+                        // Release the GIL in case some other thread wants to do work:
+                        py.allow_threads(|| ());
+                        result
+                    }),
+            ) // TODO make sure this error is menaingful to Python users
+            .map_err(|e| PyValueError::new_err(e.to_string()))?;
+
+        if has_empty_patterns.get() {
+            return Err(PyValueError::new_err(
+                "You passed in an empty string as a pattern",
+            ));
+        }
+
+        let result = Ok(Self { ac_impl, patterns });
         if let Ok(mut guard) = patterns_error.lock() {
             if let Some(err) = guard.take() {
                 return Err(err);

tests/test_ac.py

@@ -155,6 +155,45 @@ def test_unicode_extensive(
     assert ac.find_matches_as_strings(haystack) == [pattern]
 
 
+@pytest.mark.parametrize("bad_patterns", [[""], ["", "xx"], ["xx", ""]])
+@pytest.mark.parametrize("store_patterns", [True, False])
+def test_empty_patterns_are_not_legal(
+    bad_patterns: list[str], store_patterns: bool
+) -> None:
+    """
+    Passing in an empty pattern suggests a bug in user code, and the outputs
+    are bad when you do have that, so raise an error.
+    """
+    with pytest.raises(ValueError) as e:
+        AhoCorasick(bad_patterns, store_patterns=store_patterns)
+    assert "You passed in an empty string as a pattern" in str(e.value)
+
+
+@given(st.text(min_size=1), st.text(), st.sampled_from([True, False, None]))
+def test_unicode_totally_random(
+    pattern: str, haystack: str, store_patterns: Optional[bool]
+) -> None:
+    """
+    Catch more edge cases of patterns and haystacks.
+    """
+    if store_patterns is None:
+        ac = AhoCorasick([pattern])
+    else:
+        ac = AhoCorasick([pattern], store_patterns=store_patterns)
+
+    index_matches = ac.find_matches_as_indexes(haystack)
+    string_matches = ac.find_matches_as_strings(haystack)
+
+    expected_index = haystack.find(pattern)
+    if expected_index == -1:
+        assert index_matches == []
+        assert string_matches == []
+    else:
+        assert index_matches[0][1] == expected_index
+        assert [haystack[s:e] for (_, s, e) in index_matches][0] == pattern
+        assert string_matches[0] == pattern
+
+
 def test_matchkind() -> None:
     """
     Different matchkinds give different results.