[JULES] Refactor src/stdlib/pattern.rs with standard type helpers - #448
[JULES] Refactor src/stdlib/pattern.rs with standard type helpers#448logbie wants to merge 1 commit into
Conversation
…helpers Created an `expect_pattern` helper using the `generate_expect!` macro and refactored all native functions in `src/stdlib/pattern.rs` to use it along with `expect_text`. This eliminates repetitive manual matching of arguments. As a side effect, it optimizes string allocations in `native_pattern_split` and `native_pattern_replace` by reusing `Arc<str>` references instead of allocating new buffers. Updated integration tests to match standard error strings. 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 docstrings
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let text = expect_text(&args[0])?; | ||
| let _pattern = expect_pattern(&args[1])?; | ||
| let _replacement = expect_text(&args[2])?; |
There was a problem hiding this comment.
🟡 Type-error messages in native_pattern_replace and native_pattern_split lose source location (line/column)
The functions native_pattern_replace and native_pattern_split accept line and column parameters (passed from the AST by the interpreter at src/interpreter/mod.rs:6736 and src/interpreter/mod.rs:6749) specifically to provide accurate source positions in error messages. The old inline match arms forwarded these values into RuntimeError::new(…, line, column). The new expect_text / expect_pattern helpers always hardcode (0, 0) (src/stdlib/helpers.rs:201-204), so type-mismatch errors now report position (0, 0) instead of the real source location. This is a regression for these two functions only — the other three pattern functions (pattern_matches_native, pattern_find_native, pattern_find_all_native) already used (0, 0) before this change.
Prompt for agents
The functions native_pattern_replace (line 131) and native_pattern_split (line 153) in src/stdlib/pattern.rs accept line and column parameters that represent the source code location from the AST. Before this refactoring, type-validation errors used those values in RuntimeError::new(msg, line, column). Now the expect_text / expect_pattern helpers (generated by the generate_expect! macro in src/stdlib/helpers.rs:188-208) always hardcode 0, 0.
To fix this properly, either:
1. Keep the inline match arms for these two functions (since they need line/column), or
2. Extend the generate_expect! macro to produce a second variant that accepts line/column parameters (e.g. expect_text_at(value, line, column)), or
3. Map the error after calling the helper, e.g. expect_text(&args[0]).map_err(|e| RuntimeError::new(e.message().to_string(), line, column))?
Option 3 is the least invasive.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stdlib/pattern.rs`:
- Around line 144-147: The helper extractors (expect_text/expect_pattern)
currently produce RuntimeError with 0,0 which strips source coordinates; update
native_pattern_replace and native_pattern_split to call the helpers but, on Err,
replace the error's line and column with the original Arg's source coordinates
(use args[0]/args[1]/args[2] as appropriate) before returning the error so
type-mismatch diagnostics keep the correct line/column context for text,
pattern, and replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5373d6d-50de-4094-966d-f16c215142e5
📒 Files selected for processing (3)
src/stdlib/helpers.rssrc/stdlib/pattern.rssrc/stdlib/pattern_test.rs
| let text = expect_text(&args[0])?; | ||
| let _pattern = expect_pattern(&args[1])?; | ||
| let _replacement = expect_text(&args[2])?; | ||
|
|
There was a problem hiding this comment.
Preserve source coordinates for type errors in interpreter entrypoints.
Line 144 and Line 166 now call shared extractors that emit RuntimeError with 0,0, so native_pattern_replace/native_pattern_split lose their line/column context on type mismatch. That regresses runtime diagnostics.
Proposed fix (keep helper usage, restore line/column)
- let text = expect_text(&args[0])?;
- let _pattern = expect_pattern(&args[1])?;
- let _replacement = expect_text(&args[2])?;
+ let text = expect_text(&args[0]).map_err(|_| {
+ RuntimeError::new(format!("Expected text, got {}", args[0].type_name()), line, column)
+ })?;
+ let _pattern = expect_pattern(&args[1]).map_err(|_| {
+ RuntimeError::new(
+ format!("Expected a Pattern, got {}", args[1].type_name()),
+ line,
+ column,
+ )
+ })?;
+ let _replacement = expect_text(&args[2]).map_err(|_| {
+ RuntimeError::new(format!("Expected text, got {}", args[2].type_name()), line, column)
+ })?;- let text = expect_text(&args[0])?;
- let pattern = expect_pattern(&args[1])?;
+ let text = expect_text(&args[0]).map_err(|_| {
+ RuntimeError::new(format!("Expected text, got {}", args[0].type_name()), line, column)
+ })?;
+ let pattern = expect_pattern(&args[1]).map_err(|_| {
+ RuntimeError::new(
+ format!("Expected a Pattern, got {}", args[1].type_name()),
+ line,
+ column,
+ )
+ })?;Also applies to: 166-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stdlib/pattern.rs` around lines 144 - 147, The helper extractors
(expect_text/expect_pattern) currently produce RuntimeError with 0,0 which
strips source coordinates; update native_pattern_replace and
native_pattern_split to call the helpers but, on Err, replace the error's line
and column with the original Arg's source coordinates (use
args[0]/args[1]/args[2] as appropriate) before returning the error so
type-mismatch diagnostics keep the correct line/column context for text,
pattern, and replacement.
There was a problem hiding this comment.
Pull request overview
Refactors the pattern stdlib natives to use standardized type-extraction helpers (generated via generate_expect!) and reduces unnecessary Arc<str> allocations in some paths, aiming to improve maintainability and minor runtime performance.
Changes:
- Replaced repeated manual
match-based argument type extraction insrc/stdlib/pattern.rswithexpect_text/expect_pattern. - Added
expect_patternhelper generated viagenerate_expect!insrc/stdlib/helpers.rs. - Updated pattern unit test expectations to match the standardized error wording.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/stdlib/pattern.rs | Uses helper extractors for args; adjusts text/pattern usage and avoids some redundant string allocations. |
| src/stdlib/pattern_test.rs | Updates assertions to match new standardized error message text. |
| src/stdlib/helpers.rs | Adds expect_pattern helper via generate_expect! for consistent type extraction/errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let text = expect_text(&args[0])?; | ||
| let _pattern = expect_pattern(&args[1])?; | ||
| let _replacement = expect_text(&args[2])?; |
There was a problem hiding this comment.
native_pattern_replace receives line/column from the interpreter, but expect_text / expect_pattern return RuntimeError with (0,0) locations (via generate_expect!). This regresses error spans for type errors in pattern_replace. Consider mapping these extractor errors to the provided line/column (preserving the message), or adding location-aware helpers (e.g., expect_text_at, expect_pattern_at) for interpreter-called natives.
| let text = expect_text(&args[0])?; | |
| let _pattern = expect_pattern(&args[1])?; | |
| let _replacement = expect_text(&args[2])?; | |
| let text = expect_text(&args[0]) | |
| .map_err(|err| RuntimeError::new(err.to_string(), line, column))?; | |
| let _pattern = expect_pattern(&args[1]) | |
| .map_err(|err| RuntimeError::new(err.to_string(), line, column))?; | |
| let _replacement = expect_text(&args[2]) | |
| .map_err(|err| RuntimeError::new(err.to_string(), line, column))?; |
| let text = expect_text(&args[0])?; | ||
| let pattern = expect_pattern(&args[1])?; |
There was a problem hiding this comment.
Same as native_pattern_replace: native_pattern_split uses expect_text / expect_pattern, which produce errors with (0,0) even though this function is called with source line/column. Please propagate the provided location into type errors so runtime diagnostics point at the correct source span.
| generate_expect!( | ||
| /// Extracts a CompiledPattern value from a WFL Value, returning it as a reference-counted compiled pattern. | ||
| /// | ||
| /// Returns an `Rc<CompiledPattern>` to enable efficient memory sharing. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `value` - The WFL Value to extract from | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// Returns an `Rc<CompiledPattern>` clone (incrementing the reference count) if the value | ||
| /// is a Pattern variant. Multiple references to the same pattern share the underlying data. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns `RuntimeError` if the value is not a Pattern, with an error message | ||
| /// indicating the expected type and the actual type received. | ||
| expect_pattern, | ||
| Pattern, | ||
| Rc<crate::pattern::CompiledPattern>, | ||
| "a Pattern", | ||
| |p: &Rc<crate::pattern::CompiledPattern>| Rc::clone(p) | ||
| ); |
There was a problem hiding this comment.
expect_pattern is generated by generate_expect!, which hard-codes RuntimeError positions to (0,0). That’s fine for most stdlib calls, but it becomes problematic in interpreter entry points that do have source locations (e.g., native_pattern_split/native_pattern_replace). Consider extending generate_expect! (or providing parallel *_at(value, line, column) helpers) so callers can preserve accurate error locations when available.
| generate_expect!( | |
| /// Extracts a CompiledPattern value from a WFL Value, returning it as a reference-counted compiled pattern. | |
| /// | |
| /// Returns an `Rc<CompiledPattern>` to enable efficient memory sharing. | |
| /// | |
| /// # Arguments | |
| /// | |
| /// * `value` - The WFL Value to extract from | |
| /// | |
| /// # Returns | |
| /// | |
| /// Returns an `Rc<CompiledPattern>` clone (incrementing the reference count) if the value | |
| /// is a Pattern variant. Multiple references to the same pattern share the underlying data. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns `RuntimeError` if the value is not a Pattern, with an error message | |
| /// indicating the expected type and the actual type received. | |
| expect_pattern, | |
| Pattern, | |
| Rc<crate::pattern::CompiledPattern>, | |
| "a Pattern", | |
| |p: &Rc<crate::pattern::CompiledPattern>| Rc::clone(p) | |
| ); | |
| /// Extracts a CompiledPattern value from a WFL Value, returning it as a reference-counted compiled pattern. | |
| /// | |
| /// Returns an `Rc<CompiledPattern>` to enable efficient memory sharing. | |
| /// | |
| /// # Arguments | |
| /// | |
| /// * `value` - The WFL Value to extract from | |
| /// | |
| /// # Returns | |
| /// | |
| /// Returns an `Rc<CompiledPattern>` clone (incrementing the reference count) if the value | |
| /// is a Pattern variant. Multiple references to the same pattern share the underlying data. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns `RuntimeError` if the value is not a Pattern, with an error message | |
| /// indicating the expected type and the actual type received. | |
| pub fn expect_pattern( | |
| value: &Value, | |
| ) -> Result<Rc<crate::pattern::CompiledPattern>, RuntimeError> { | |
| expect_pattern_at(value, 0, 0) | |
| } | |
| /// Extracts a CompiledPattern value from a WFL Value, preserving source location | |
| /// information for any resulting type error. | |
| /// | |
| /// # Arguments | |
| /// | |
| /// * `value` - The WFL Value to extract from | |
| /// * `line` - The source line to attach to any `RuntimeError` | |
| /// * `column` - The source column to attach to any `RuntimeError` | |
| /// | |
| /// # Returns | |
| /// | |
| /// Returns an `Rc<CompiledPattern>` clone if the value is a Pattern variant. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns `RuntimeError` if the value is not a Pattern, using the provided | |
| /// source location instead of the default `(0, 0)`. | |
| pub fn expect_pattern_at( | |
| value: &Value, | |
| line: usize, | |
| column: usize, | |
| ) -> Result<Rc<crate::pattern::CompiledPattern>, RuntimeError> { | |
| match value { | |
| Value::Pattern(p) => Ok(Rc::clone(p)), | |
| _ => Err(RuntimeError::new( | |
| format!("Expected {}, got {}", "a Pattern", value.type_name()), | |
| line, | |
| column, | |
| )), | |
| } | |
| } |
Summary of Changes
src/stdlib/pattern.rsfile contained significant repetition in extracting values from arguments via manualmatchblocks. Functions likepattern_matches_native,pattern_find_native,pattern_find_all_native,native_pattern_replace, andnative_pattern_splitall repeated the exact same 10-line boilerplate to assert argument types. In addition, the split and replace functions suffered from performance debt by unnecessarily allocating new reference-counted string buffers usingArc::fromon&strinstead of cloning the underlyingArc<str>.expect_patterninsrc/stdlib/helpers.rsand replaced all the manual matching blocks with standard helper calls. Reused theArc<str>reference directly where applicable in pattern replacement and string splitting to avoid new string allocations. Updated tests to mirror the standardized error messages.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 2133405730978822157 started by @logbie
Summary by CodeRabbit
Bug Fixes
Tests