Skip to content

fix(builtins): prevent panic in strings.count on empty needle - #77

Open
anakrish wants to merge 3 commits into
mainfrom
fix/strings-count-empty-needle-panic
Open

fix(builtins): prevent panic in strings.count on empty needle#77
anakrish wants to merge 3 commits into
mainfrom
fix/strings-count-empty-needle-panic

Conversation

@anakrish

@anakrish anakrish commented Aug 27, 2026

Copy link
Copy Markdown
Owner

Summary

strings.count panicked at runtime when called with an empty needle string because .windows(0) is not valid in Rust. This PR fixes three tightly coupled issues in strings_count:

Bugs fixed

# Bug Impact
1 Wrong param index (params[0] used for substring error span instead of params[1]) Misleading error messages
2 Panic: .windows(needle.len()) panics when needle is empty (len == 0) Process crash
3 Semantics: byte-level .windows() counted overlapping matches; OPA uses non-overlapping semantics (Go strings.Count) Wrong results, e.g. strings.count("aaaa", "aa") returned 3 instead of 2

Fix

// Before (bugs):
let substring = ensure_string(name, &params[0], &args[1])?;  // wrong index
Ok(Value::from(
    search.as_bytes().windows(substring.len())  // panics on empty
        .filter(|&w| w == substring.as_bytes()).count()  // overlapping
))

// After:
let substring = ensure_string(name, &params[1], &args[1])?;
if substring.is_empty() {
    // Empty needle: matches between every character and at both ends
    // consistent with Go strings.Count and OPA semantics
    return Ok(Value::from(search.chars().count().saturating_add(1)));
}
Ok(Value::from(search.matches(substring.as_ref()).count()))

Tests

A YAML regression test is added at tests/interpreter/cases/builtins/strings/strings_count.yaml covering:

  • Basic match, no-match, multi-match
  • Empty needle on non-empty string (was panic, now returns char_count + 1)
  • Empty needle on empty string (returns 1)
  • Non-overlapping semantics (strings.count("aaaa", "aa") = 2, not 3)
  • Multi-byte UTF-8 string with empty needle

microsoft#794)

Bumps the per-dependency group in /bindings/ruby with 1 update: [rb_sys](https://github.com/oxidize-rb/rb-sys).


Updates `rb_sys` from 0.9.128 to 0.9.130
- [Release notes](https://github.com/oxidize-rb/rb-sys/releases)
- [Commits](oxidize-rb/rb-sys@v0.9.128...v0.9.130)

---
updated-dependencies:
- dependency-name: rb_sys
  dependency-version: 0.9.130
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: per-dependency
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change removes a confirmed panic, corrects semantics to match OPA/Go behavior, and adds targeted regression tests for the affected edge cases.

Pull request overview

Fixes strings.count builtin to avoid a panic on empty needles, corrects error-span indexing, and aligns match-counting semantics with OPA/Go (strings.Count) by using non-overlapping matches and the correct empty-needle behavior.

Changes:

  • Fix argument span selection for the substring parameter (params[1] instead of params[0]).
  • Handle empty needle explicitly (return char_count + 1) and switch to str::matches() for non-overlapping counting.
  • Add YAML regression coverage for empty-needle and non-overlapping semantics (including UTF-8).
File summaries
File Description
src/builtins/strings.rs Removes empty-needle panic path, fixes span indexing, and updates counting logic to OPA/Go-compatible behavior.
tests/interpreter/cases/builtins/strings/strings_count.yaml Adds regression tests covering empty-needle semantics, non-overlapping counts, and UTF-8 cases.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

jaylorch and others added 2 commits August 28, 2026 10:10
feat(verus): Verification of `number`

Signed-off-by: Jay Lorch <jaylorch@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
strings.count panicked when called with an empty needle because
.windows(0) is invalid. Fix three issues in strings_count:

1. Wrong param index: params[0] was used for the substring error span
   instead of params[1].
2. Panic: .windows(needle.len()) panics when needle is empty (len == 0).
3. Semantics: the byte-level .windows() approach counted overlapping
   matches; OPA uses non-overlapping match semantics (Go strings.Count).

The fix:
- Correct the param index to params[1] for the substring argument.
- Handle empty needle explicitly: return chars().count() + 1, matching
  Go's strings.Count behaviour (empty needle matches between every
  character and at both ends).
- Use str::matches() for non-overlapping count on non-empty needles.

A YAML regression test is added covering basic match, no-match,
multi-match, empty-needle (must not panic), empty-needle on empty
string, non-overlapping semantics, and multi-byte UTF-8 strings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@anakrish
anakrish force-pushed the fix/strings-count-empty-needle-panic branch from 67aadd3 to e0e0e31 Compare August 28, 2026 15:34
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.

3 participants