feat: add strings.repeat builtin - #35
Conversation
Adds strings.repeat(s, count) which repeats string s count times.
Example: strings.repeat("ab", 3) returns "ababab"
There was a problem hiding this comment.
Pull request overview
Adds a new strings.repeat(s, count) builtin to the Rego string builtin set, aligning with other strings.* helpers in src/builtins/strings.rs.
Changes:
- Register a new builtin name
strings.repeatwith arity 2. - Implement
repeatbuiltin logic to concatenate a stringcounttimes.
|
|
||
| let mut result = String::new(); | ||
| for _ in 0..count { |
There was a problem hiding this comment.
The repeat loop grows result potentially many times without any call to enforce_limit(). Other builtins that accumulate in loops call enforce_limit() to respect the engine’s memory limits; without it, strings.repeat can more easily trigger OOM/DoS for large count. Add limit enforcement during repetition (and/or before growth) so large repeats fail gracefully under configured limits.
| let mut result = String::new(); | |
| for _ in 0..count { | |
| let part_len = s.len(); | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| let next_len = match result.len().checked_add(part_len) { | |
| Some(next_len) => next_len, | |
| None => bail!(params[1].span().error("repeat result exceeds supported size")), | |
| }; | |
| enforce_limit(next_len)?; |
|
|
||
| let mut result = String::new(); |
There was a problem hiding this comment.
result is built with String::new() and repeated push_str, which can lead to repeated reallocations for larger counts. Consider reserving capacity up front using a checked length computation (e.g., checked_mul of s.len() and count) or otherwise minimizing reallocations, while ensuring the approach can’t panic on overflow.
| let mut result = String::new(); | |
| let capacity = s | |
| .len() | |
| .checked_mul(count) | |
| .ok_or_else(|| params[1].span().error("repeat count produces a string that is too large"))?; | |
| let mut result = String::new(); | |
| result | |
| .try_reserve(capacity) | |
| .map_err(|_| params[1].span().error("repeat count produces a string that is too large"))?; |
| fn repeat(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> { | ||
| let name = "strings.repeat"; | ||
| ensure_args_count(span, name, params, args, 2)?; | ||
|
|
||
| let s = ensure_string(name, ¶ms[0], &args[0])?; | ||
| let count = args[1].as_f64().unwrap() as usize; | ||
|
|
||
| if count == 0 { | ||
| return Ok(Value::String("".into())); | ||
| } | ||
|
|
||
| let mut result = String::new(); | ||
| for _ in 0..count { | ||
| result.push_str(&s); | ||
| } | ||
|
|
||
| Ok(Value::String(result.into())) | ||
| } |
There was a problem hiding this comment.
A new builtin (strings.repeat) is being added but there are no corresponding interpreter YAML test cases under tests/interpreter/cases/builtins/strings/ (this directory already contains coverage for other string builtins like concat, contains, etc.). Add tests for basic behavior (e.g., count 0/1/N), Unicode handling, and invalid count cases (non-integer / negative) to lock down semantics across execution paths.
| m.insert("strings.any_suffix_match", (any_suffix_match, 2)); | ||
| m.insert("strings.count", (strings_count, 2)); | ||
| m.insert("strings.replace_n", (replace_n, 2)); | ||
| m.insert("strings.repeat", (repeat, 2)); |
There was a problem hiding this comment.
strings.repeat is now registered but the builtin list in docs/builtins.md has a dedicated Strings table that enumerates supported builtins (e.g., strings.replace_n, strings.reverse). Add strings.repeat there as well so the documented builtin surface matches what the engine exposes.
| fn repeat(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> { | ||
| let name = "strings.repeat"; | ||
| ensure_args_count(span, name, params, args, 2)?; | ||
|
|
||
| let s = ensure_string(name, ¶ms[0], &args[0])?; | ||
| let count = args[1].as_f64().unwrap() as usize; |
There was a problem hiding this comment.
args[1].as_f64().unwrap() as usize can panic (unwrap), silently truncates non-integers, and will produce a huge usize for negative counts due to casting. This violates the project’s no-panics requirement and yields incorrect semantics for invalid count values. Parse count via ensure_numeric and validate it as a non-negative integer (e.g., using validate_integer_arg), then convert using an explicit checked/option-based conversion (returning Undefined or a strict-mode error as appropriate).
| fn repeat(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> { | |
| let name = "strings.repeat"; | |
| ensure_args_count(span, name, params, args, 2)?; | |
| let s = ensure_string(name, ¶ms[0], &args[0])?; | |
| let count = args[1].as_f64().unwrap() as usize; | |
| fn repeat(span: &Span, params: &[Ref<Expr>], args: &[Value], strict: bool) -> Result<Value> { | |
| let name = "strings.repeat"; | |
| ensure_args_count(span, name, params, args, 2)?; | |
| let s = ensure_string(name, ¶ms[0], &args[0])?; | |
| let count = ensure_numeric(name, ¶ms[1], &args[1])?; | |
| let count = match count.as_i64().and_then(|value| usize::try_from(value).ok()) { | |
| Some(count) => count, | |
| None if strict => { | |
| bail!(params[1] | |
| .span() | |
| .error("`strings.repeat` expects a non-negative integer count")) | |
| } | |
| None => return Ok(Value::Undefined), | |
| }; |
Adds a new
strings.repeat(s, count)builtin that repeats a stringcounttimes.Example:
This follows the pattern of other string builtins like
strings.reverseandstrings.count.