Skip to content

[JULES] Scheduled Maintenance: ⚡ Bolt: refactor pattern args logic and remove allocations #465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
logbie wants to merge 1 commit into from
Closed

[JULES] Scheduled Maintenance: ⚡ Bolt: refactor pattern args logic and remove allocations #465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/stdlib/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,3 +639,12 @@ where
let list = expect_list(&args[0])?;
op(list, val)
}

generate_expect!(
/// Extracts a compiled pattern from a WFL Value, returning it as a reference-counted CompiledPattern.
expect_pattern,
Pattern,
Rc<crate::pattern::CompiledPattern>,
"a pattern",
|p: &Rc<crate::pattern::CompiledPattern>| Rc::clone(p)
);
186 changes: 34 additions & 152 deletions src/stdlib/pattern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::helpers::{check_arg_count, expect_pattern, expect_text};
use crate::interpreter::environment::Environment;
use crate::interpreter::error::RuntimeError;
use crate::interpreter::value::Value;
Expand All @@ -19,74 +20,22 @@ pub fn register(env: &mut Environment) {
/// Native function: pattern_matches(text, pattern) -> boolean
/// Tests if text matches the given compiled pattern
pub fn pattern_matches_native(args: Vec<Value>) -> Result<Value, RuntimeError> {
if args.len() != 2 {
return Err(RuntimeError::new(
"pattern_matches requires exactly 2 arguments (text, pattern)".to_string(),
0,
0,
));
}

let text_str = match &args[0] {
Value::Text(s) => s.as_ref(),
_ => {
return Err(RuntimeError::new(
"First argument to pattern_matches must be text".to_string(),
0,
0,
));
}
};

let compiled_pattern = match &args[1] {
Value::Pattern(p) => p,
_ => {
return Err(RuntimeError::new(
"Second argument to pattern_matches must be a compiled pattern".to_string(),
0,
0,
));
}
};
check_arg_count("pattern_matches", &args, 2)?;
let text_str = expect_text(&args[0])?;
let compiled_pattern = expect_pattern(&args[1])?;

let matches = compiled_pattern.matches(text_str);
let matches = compiled_pattern.matches(text_str.as_ref());
Ok(Value::Bool(matches))
}

/// Native function: pattern_find(text, pattern) -> object or null
/// Finds the first match of pattern in text
pub fn pattern_find_native(args: Vec<Value>) -> Result<Value, RuntimeError> {
if args.len() != 2 {
return Err(RuntimeError::new(
"pattern_find requires exactly 2 arguments (text, pattern)".to_string(),
0,
0,
));
}

let text_str = match &args[0] {
Value::Text(s) => s.as_ref(),
_ => {
return Err(RuntimeError::new(
"First argument to pattern_find must be text".to_string(),
0,
0,
));
}
};

let compiled_pattern = match &args[1] {
Value::Pattern(p) => p,
_ => {
return Err(RuntimeError::new(
"Second argument to pattern_find must be a compiled pattern".to_string(),
0,
0,
));
}
};
check_arg_count("pattern_find", &args, 2)?;
let text_str = expect_text(&args[0])?;
let compiled_pattern = expect_pattern(&args[1])?;

match compiled_pattern.find(text_str) {
match compiled_pattern.find(text_str.as_ref()) {
Some(match_result) => {
let mut result_map = HashMap::new();
result_map.insert(
Expand Down Expand Up @@ -120,37 +69,11 @@ pub fn pattern_find_native(args: Vec<Value>) -> Result<Value, RuntimeError> {
/// Native function: pattern_find_all(text, pattern) -> list
/// Finds all matches of pattern in text
pub fn pattern_find_all_native(args: Vec<Value>) -> Result<Value, RuntimeError> {
if args.len() != 2 {
return Err(RuntimeError::new(
"pattern_find_all requires exactly 2 arguments (text, pattern)".to_string(),
0,
0,
));
}

let text_str = match &args[0] {
Value::Text(s) => s.as_ref(),
_ => {
return Err(RuntimeError::new(
"First argument to pattern_find_all must be text".to_string(),
0,
0,
));
}
};

let compiled_pattern = match &args[1] {
Value::Pattern(p) => p,
_ => {
return Err(RuntimeError::new(
"Second argument to pattern_find_all must be a compiled pattern".to_string(),
0,
0,
));
}
};
check_arg_count("pattern_find_all", &args, 2)?;
let text_str = expect_text(&args[0])?;
let compiled_pattern = expect_pattern(&args[1])?;

let matches = compiled_pattern.find_all(text_str);
let matches = compiled_pattern.find_all(text_str.as_ref());
let mut result_list = Vec::new();

for match_result in matches {
Expand Down Expand Up @@ -197,41 +120,14 @@ pub fn native_pattern_replace(
));
}

let text = match &args[0] {
Value::Text(t) => t.as_ref(),
_ => {
return Err(RuntimeError::new(
"First argument must be text".to_string(),
line,
column,
));
}
};

let _pattern = match &args[1] {
Value::Pattern(p) => p.as_ref(),
_ => {
return Err(RuntimeError::new(
"Second argument must be a pattern".to_string(),
line,
column,
));
}
};

let _replacement = match &args[2] {
Value::Text(t) => t.as_ref(),
_ => {
return Err(RuntimeError::new(
"Third argument must be text".to_string(),
line,
column,
));
}
};
let text = expect_text(&args[0]).map_err(|e| RuntimeError::new(e.message, line, column))?;
let _pattern =
expect_pattern(&args[1]).map_err(|e| RuntimeError::new(e.message, line, column))?;
let _replacement =
expect_text(&args[2]).map_err(|e| RuntimeError::new(e.message, line, column))?;
Comment on lines +123 to +127

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

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

map_err(|e| RuntimeError::new(e.message, line, column)) recreates the error. Since RuntimeError fields are public, you can set line/column on the existing error in the closure and return it (preserves other fields like kind and avoids reconstructing).

Suggested change
let text = expect_text(&args[0]).map_err(|e| RuntimeError::new(e.message, line, column))?;
let _pattern =
expect_pattern(&args[1]).map_err(|e| RuntimeError::new(e.message, line, column))?;
let _replacement =
expect_text(&args[2]).map_err(|e| RuntimeError::new(e.message, line, column))?;
let text = expect_text(&args[0]).map_err(|mut e| {
e.line = line;
e.column = column;
e
})?;
let _pattern = expect_pattern(&args[1]).map_err(|mut e| {
e.line = line;
e.column = column;
e
})?;
let _replacement = expect_text(&args[2]).map_err(|mut e| {
e.line = line;
e.column = column;
e
})?;

Copilot uses AI. Check for mistakes.

// TODO: Update to use new pattern system for replacement
Ok(Value::Text(Arc::from(text)))
Ok(Value::Text(Arc::clone(&text)))

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

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

text is already an Arc<str> returned from expect_text. Since it's only used for the return value here, you can move it into Value::Text(text) instead of Arc::clone(&text) to avoid an extra refcount increment.

Suggested change
Ok(Value::Text(Arc::clone(&text)))
Ok(Value::Text(text))

Copilot uses AI. Check for mistakes.
}

/// Native function for pattern splitting (called by interpreter)
Expand All @@ -248,41 +144,27 @@ pub fn native_pattern_split(
));
}

let text = match &args[0] {
Value::Text(t) => t.as_ref(),
_ => {
return Err(RuntimeError::new(
"First argument must be text".to_string(),
line,
column,
));
}
};

let pattern = match &args[1] {
Value::Pattern(p) => p,
_ => {
return Err(RuntimeError::new(
"Second argument must be a pattern".to_string(),
line,
column,
));
}
};
let text = expect_text(&args[0]).map_err(|e| RuntimeError::new(e.message, line, column))?;
let pattern =
expect_pattern(&args[1]).map_err(|e| RuntimeError::new(e.message, line, column))?;
Comment on lines +147 to +149

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

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

Same pattern here: consider mutating and returning the existing RuntimeError from expect_* in the map_err closure (set line/column) instead of reconstructing it, to keep error metadata intact.

Suggested change
let text = expect_text(&args[0]).map_err(|e| RuntimeError::new(e.message, line, column))?;
let pattern =
expect_pattern(&args[1]).map_err(|e| RuntimeError::new(e.message, line, column))?;
let text = expect_text(&args[0]).map_err(|mut e| {
e.line = line;
e.column = column;
e
})?;
let pattern = expect_pattern(&args[1]).map_err(|mut e| {
e.line = line;
e.column = column;
e
})?;

Copilot uses AI. Check for mistakes.

// Find all matches of the pattern in the text
let matches = pattern.find_all(text);
let matches = pattern.find_all(text.as_ref());

// If no matches, return the entire text as a single element
if matches.is_empty() {
let parts = vec![Value::Text(Arc::from(text))];
let parts = vec![Value::Text(Arc::clone(&text))];

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

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

In the no-matches early return, text can be moved directly into Value::Text (since this branch returns immediately) rather than Arc::clone(&text), avoiding an unnecessary atomic refcount bump.

Suggested change
let parts = vec![Value::Text(Arc::clone(&text))];
let parts = vec![Value::Text(text)];

Copilot uses AI. Check for mistakes.
return Ok(Value::List(Rc::new(RefCell::new(parts))));
}

// Build character-to-byte index mapping
let char_to_byte: Vec<usize> = text.char_indices().map(|(byte_idx, _)| byte_idx).collect();
let char_to_byte: Vec<usize> = text
.as_ref()
.char_indices()
.map(|(byte_idx, _)| byte_idx)
.collect();
let mut char_to_byte = char_to_byte;
char_to_byte.push(text.len()); // Add final byte position
char_to_byte.push(text.as_ref().len()); // Add final byte position

// Split the text at match positions
let mut parts = Vec::new();
Expand All @@ -293,19 +175,19 @@ pub fn native_pattern_split(
let start_byte = if match_result.start < char_to_byte.len() {
char_to_byte[match_result.start]
} else {
text.len()
text.as_ref().len()
};
let last_end_byte = if last_end_char < char_to_byte.len() {
char_to_byte[last_end_char]
} else {
text.len()
text.as_ref().len()
};

// Add the text before this match
if match_result.start > last_end_char
|| (match_result.start == last_end_char && last_end_char == 0)
{
let part = &text[last_end_byte..start_byte];
let part = &text.as_ref()[last_end_byte..start_byte];
parts.push(Value::Text(Arc::from(part)));
} else if match_result.start == last_end_char && last_end_char > 0 {
// Add empty string for consecutive matches
Expand All @@ -317,7 +199,7 @@ pub fn native_pattern_split(
// Add any remaining text after the last match
if last_end_char < char_to_byte.len() {
let last_end_byte = char_to_byte[last_end_char];
let part = &text[last_end_byte..];
let part = &text.as_ref()[last_end_byte..];
parts.push(Value::Text(Arc::from(part)));
}

Expand Down
13 changes: 9 additions & 4 deletions src/stdlib/pattern_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ mod tests {
result
.unwrap_err()
.to_string()
.contains("exactly 2 arguments")
.contains("expects 2 arguments")
);
}

Expand All @@ -33,7 +33,7 @@ mod tests {
result
.unwrap_err()
.to_string()
.contains("exactly 2 arguments")
.contains("expects 2 arguments")
);
}

Expand All @@ -45,7 +45,7 @@ mod tests {
result
.unwrap_err()
.to_string()
.contains("exactly 2 arguments")
.contains("expects 2 arguments")
);
}

Expand All @@ -57,6 +57,11 @@ mod tests {
];
let result = pattern_matches_native(args);
assert!(result.is_err());
assert!(result.unwrap_err().to_string().contains("First argument"));
assert!(
result
.unwrap_err()
.to_string()
.contains("Expected text, got Number")
);
}
}
Loading