[JULES] Refactor pattern stdlib to eliminate redundancy and allocations - #395
[JULES] Refactor pattern stdlib to eliminate redundancy and allocations#395logbie wants to merge 1 commit into
Conversation
- Introduced `expect_pattern` helper in `src/stdlib/helpers.rs` - Refactored `pattern_matches_native`, `pattern_find_native`, `pattern_find_all_native`, `native_pattern_replace`, and `native_pattern_split` to use `check_arg_count`, `expect_text`, and `expect_pattern` helpers, removing redundant match blocks and `.to_string()` allocations in error messages. - Reduced static key allocations by replacing `.to_string()` with `String::from()` in hash map insertions within pattern match functions. - Optimized `native_pattern_split` to eliminate full `O(N)` memory allocation of character-to-byte mappings by using an incremental `char_indices` iterator. - Updated unit tests in `src/stdlib/pattern_test.rs` to reflect standardized error messages from the helper functions. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR refactors pattern module functions to use shared helper utilities for argument validation and type checking. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/stdlib/pattern.rs (1)
224-226: Dead code can be removed.This branch is unreachable: if
text.is_empty(), thenmatches.is_empty()would have triggered the early return at lines 160-162. Even if a pattern could match empty text, the conditionlast_end_byte == text.len() && text.is_empty()simplifies to0 == 0 && true, which is already handled by the precedingif.🧹 Suggested cleanup
// Add any remaining text after the last match if last_end_byte < text.len() { let part = &text[last_end_byte..]; parts.push(Value::Text(Arc::from(part))); - } else if last_end_byte == text.len() && text.is_empty() { - // Should not happen, covered by is_empty check above, but for completeness }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/pattern.rs` around lines 224 - 226, Remove the unreachable branch that checks `else if last_end_byte == text.len() && text.is_empty()` in src/stdlib/pattern.rs: the early return when `matches.is_empty()` already handles empty `text`, so delete this conditional and its empty body (cleanup the surrounding if/else to preserve flow in the function that contains `last_end_byte`, `text`, and `matches`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/stdlib/pattern.rs`:
- Around line 224-226: Remove the unreachable branch that checks `else if
last_end_byte == text.len() && text.is_empty()` in src/stdlib/pattern.rs: the
early return when `matches.is_empty()` already handles empty `text`, so delete
this conditional and its empty body (cleanup the surrounding if/else to preserve
flow in the function that contains `last_end_byte`, `text`, and `matches`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbfdd2e5-843a-4630-a720-9da6c23b2a63
📒 Files selected for processing (3)
src/stdlib/helpers.rssrc/stdlib/pattern.rssrc/stdlib/pattern_test.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48bf566868
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if last_end_byte < text.len() { | ||
| let part = &text[last_end_byte..]; | ||
| parts.push(Value::Text(Arc::from(part))); |
There was a problem hiding this comment.
This new tail-handling condition drops the final empty element whenever the last pattern match reaches the end of the string (for example, splitting "a,b," on "," now returns ["a","b"] instead of ["a","b",""]). The previous implementation included that trailing empty segment, which is important for round-tripping delimiter-separated data and for parity with existing split behavior in the codebase, so this is a user-visible regression in pattern_split results for inputs with trailing delimiters.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refactors the pattern standard library module (src/stdlib/pattern.rs) to eliminate boilerplate code for argument validation and type extraction by leveraging shared helper functions from src/stdlib/helpers.rs. It also replaces the O(N) Vec<usize> allocation in native_pattern_split with an incremental char_indices() iterator approach.
Changes:
- Added a new
expect_patternhelper function insrc/stdlib/helpers.rsthat extracts a compiled pattern from aValue, following the same pattern as the existingexpect_texthelper. - Refactored all pattern native functions (
pattern_matches_native,pattern_find_native,pattern_find_all_native,native_pattern_replace,native_pattern_split) to usecheck_arg_count,expect_text, andexpect_patterninstead of inline validation, and replaced.to_string()on string keys withString::from(). - Optimized
native_pattern_splitto use an incrementalchar_indices()iterator for character-to-byte index conversion, eliminating the previous fullVec<usize>collection, and updated test expectations to match the new standardized error messages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/stdlib/helpers.rs |
Added expect_pattern helper with Rc<CompiledPattern> extraction and documentation |
src/stdlib/pattern.rs |
Replaced inline validation with shared helpers; optimized native_pattern_split char-to-byte conversion |
src/stdlib/pattern_test.rs |
Updated expected error message strings to match new standardized helper output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if last_end_byte == text.len() && text.is_empty() { | ||
| // Should not happen, covered by is_empty check above, but for completeness |
There was a problem hiding this comment.
This else if branch has an empty body and is dead code. The condition last_end_byte == text.len() && text.is_empty() can never be true here because: (1) if text is empty, find_all would either return no matches (handled by the early return at line 160) or return zero-width matches where last_end_byte would be 0 == text.len() — but the branch body does nothing anyway. The comment "covered by is_empty check above" is also misleading since the check above is matches.is_empty(), not text.is_empty(). Consider removing this dead else if branch entirely for clarity.
| } else if last_end_byte == text.len() && text.is_empty() { | |
| // Should not happen, covered by is_empty check above, but for completeness |
| // Extract matched text from match result directly using matched_text | ||
| // Or slice using byte offsets. However, MatchResult currently only has `start` and `end` character indices. | ||
| // In Rust, using `.char_indices()` incrementally is more efficient than building a full vector. |
There was a problem hiding this comment.
This multi-line comment is somewhat rambling and confusing. Line 165 mentions "Extract matched text from match result directly using matched_text" which suggests using match_result.matched_text directly, but the code below doesn't use matched_text at all — it slices using byte offsets. The comment on line 166 says "Or slice using byte offsets" as if presenting an alternative, but that's what the code actually does. Consider replacing these three lines with a single clear comment like: "Convert character indices from MatchResult to byte offsets using an incremental char_indices() iterator, avoiding a full Vec allocation."
| // Extract matched text from match result directly using matched_text | |
| // Or slice using byte offsets. However, MatchResult currently only has `start` and `end` character indices. | |
| // In Rust, using `.char_indices()` incrementally is more efficient than building a full vector. | |
| // Convert character indices from MatchResult to byte offsets using an incremental char_indices() iterator, avoiding a full Vec allocation. |
|
Closing: bot-generated PR, cleaning up duplicates. |
Summary of Changes
src/stdlib/pattern.rsmodule contained significant boilerplate code for argument validation and type extraction (Value::TextandValue::Pattern), which resulted in duplicated code and unnecessaryto_string()allocations for error messages across all its native functions. Furthermore,native_pattern_splitperformed a highly inefficientO(N)memory allocation by collecting all character indices into aVec<usize>(text.char_indices().map(...).collect()) just to perform character-to-byte offset conversions during splitting.native_pattern_splitfunction eliminates an unnecessaryO(N)allocation, which reduces memory consumption and improves algorithmic efficiency when working with large strings.expect_patternhelper insrc/stdlib/helpers.rsto complement the existingexpect_texthelper.pattern_matches_native,pattern_find_native, etc.) to usecheck_arg_count,expect_text, andexpect_pattern..to_string()on static string keys in map constructions withString::from()to avoid format machinery overhead.native_pattern_splitto use an incremental, state-preservingtext.char_indices()iterator, avoiding the intermediateVec<usize>allocation entirely.src/stdlib/pattern_test.rsto match the standardized output.Verification Checklist
cargo fmtexecuted and passed.cargo clippyreturned no warnings or errors.cargo testsuites passed (100% success rate).PR created automatically by Jules for task 12956706219001740784 started by @logbie
Summary by CodeRabbit