Skip to content

feat: add strings.repeat builtin #34

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

Merged
anakrish merged 2 commits into from
Apr 25, 2026
Merged

feat: add strings.repeat builtin #34

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2225c7e
feat: add strings.repeat builtin
anakrish Apr 25, 2026
85758fb
feat: add perspective tags and issue-ready format to review comments
anakrish Apr 25, 2026
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
24 changes: 24 additions & 0 deletions src/builtins/strings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub fn register(m: &mut builtins::BuiltinsMap<&'static str, builtins::BuiltinFcn
m.insert("strings.count", (strings_count, 2));
m.insert("strings.replace_n", (replace_n, 2));
m.insert("strings.reverse", (reverse, 1));
m.insert("strings.repeat", (repeat, 2));
m.insert("substring", (substring, 3));
m.insert("trim", (trim, 2));
m.insert("trim_left", (trim_left, 2));
Expand Down Expand Up @@ -591,6 +592,29 @@ fn reverse(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) ->
Ok(Value::String(s.chars().rev().collect::<String>().into()))
}

// New builtin: strings.repeat(s, count) - repeats a string count times
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, &params[0], &args[0])?;

// BUG 1: unwrap() instead of proper error handling
let count = args[1].as_f64().unwrap() as usize;

Comment on lines +599 to +603

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

strings.repeat derives count via args[1].as_f64().unwrap() as usize, which can panic on non-numeric values and also silently coerces floats/negatives (Rust float→usize saturates), making it impossible to implement the intended strict/undefined behavior. Use the existing ensure_numeric + validate_integer_arg helpers to require an integer count, reject negatives (error when strict, otherwise Undefined), and then convert to usize with proper bounds checking (e.g., via as_u64 + usize::try_from).

Copilot uses AI. Check for mistakes.
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}

// BUG 3: Returns empty string instead of Undefined when count is negative
Comment on lines +600 to +610

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

unwrap() here can panic at runtime when count is not a number (or not representable as f64). Please replace this with proper type validation and return a structured builtin error (using existing error/ensure helpers) instead of panicking.

Suggested change
// BUG 1: unwrap() instead of proper error handling
let count = args[1].as_f64().unwrap() as usize;
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}
// BUG 3: Returns empty string instead of Undefined when count is negative
let count = ensure_numeric(name, &params[1], &args[1])?;
let count = match count.as_i64() {
Some(count) if count < 0 => return Ok(Value::Undefined),
Some(count) => count as usize,
None => {
bail!(params[1].span().error(
format!("`{name}` expects `count` to be an integer value.").as_str()
))
}
};
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}

Copilot uses AI. Check for mistakes.
Comment on lines +600 to +610

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

The count conversion (as usize) silently truncates floats and saturates negatives to 0, which can produce surprising results (e.g., -1 becomes 0, 1.9 becomes 1). Please explicitly validate that count is an integer and non-negative, and return a consistent type/range error (or whatever the project’s standard is) when it isn’t.

Suggested change
// BUG 1: unwrap() instead of proper error handling
let count = args[1].as_f64().unwrap() as usize;
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}
// BUG 3: Returns empty string instead of Undefined when count is negative
let count = ensure_numeric(name, &params[1], &args[1])?;
let count = match count.as_i64() {
Some(count) if count >= 0 => usize::try_from(count)
.map_err(|_| params[1].span().error("`strings.repeat` count is out of range"))?,
Some(_) => bail!(params[1]
.span()
.error("`strings.repeat` expects a non-negative integer count")),
None => bail!(params[1]
.span()
.error("`strings.repeat` expects a non-negative integer count")),
};
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}

Copilot uses AI. Check for mistakes.
Comment on lines +600 to +610

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

The count conversion (as usize) silently truncates floats and saturates negatives to 0, which can produce surprising results (e.g., -1 becomes 0, 1.9 becomes 1). Please explicitly validate that count is an integer and non-negative, and return a consistent type/range error (or whatever the project’s standard is) when it isn’t.

Suggested change
// BUG 1: unwrap() instead of proper error handling
let count = args[1].as_f64().unwrap() as usize;
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}
// BUG 3: Returns empty string instead of Undefined when count is negative
let count = ensure_numeric(name, &params[1], &args[1])?;
let count = match count.as_i64() {
Some(count) if count >= 0 => match usize::try_from(count) {
Ok(count) => count,
Err(_) => bail!(params[1].span().error("count is too large")),
},
Some(_) => bail!(params[1].span().error("count must be non-negative")),
None => bail!(params[1].span().error("count must be a non-negative integer")),
};
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}

Copilot uses AI. Check for mistakes.
if count == 0 {
return Ok(Value::String("".into()));
}

Comment on lines +600 to +614

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

strings.repeat builds the result by repeatedly appending without any allocator/resource guard. Other builtins call enforce_limit() during potentially unbounded growth; without that, a large count can cause unbounded allocation (DoS/OOM). Consider (1) computing the total output size with checked_mul and failing early when it overflows/is unreasonable, (2) pre-allocating with String::with_capacity, and (3) calling enforce_limit() while appending.

Suggested change
// BUG 1: unwrap() instead of proper error handling
let count = args[1].as_f64().unwrap() as usize;
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}
// BUG 3: Returns empty string instead of Undefined when count is negative
if count == 0 {
return Ok(Value::String("".into()));
}
let count = ensure_numeric(name, &params[1], &args[1])?;
let count = match count.as_i64() {
Some(count) if count < 0 => return Ok(Value::Undefined),
Some(count) => usize::try_from(count).map_err(|_| {
span.error("`strings.repeat` count is too large to represent on this platform")
})?,
None => return Ok(Value::Undefined),
};
if count == 0 {
return Ok(Value::String("".into()));
}
let total_len = s.len().checked_mul(count).ok_or_else(|| {
span.error("`strings.repeat` output size overflowed while computing result length")
})?;
enforce_limit(total_len)?;
let mut result = String::with_capacity(total_len);
for _ in 0..count {
result.push_str(&s);
enforce_limit(result.len())?;
}

Copilot uses AI. Check for mistakes.
Comment on lines +600 to +614

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

The inline // BUG 1/2/3 markers should not ship in mainline code. Once the issues are addressed, please remove these markers (or convert to tracked TODOs with links) to avoid leaving knowingly-broken behavior documented in the implementation.

Suggested change
// BUG 1: unwrap() instead of proper error handling
let count = args[1].as_f64().unwrap() as usize;
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}
// BUG 3: Returns empty string instead of Undefined when count is negative
if count == 0 {
return Ok(Value::String("".into()));
}
let count = ensure_numeric(name, &params[1], &args[1])?;
let count = match count.as_i64() {
Some(count) if count < 0 => return Ok(Value::Undefined),
Some(count) => match usize::try_from(count) {
Ok(count) => count,
Err(_) => bail!(params[1].span().error("count is too large")),
},
None => bail!(params[1].span().error("count must be an integer")),
};
if count == 0 {
return Ok(Value::String("".into()));
}
let total_len = match s.len().checked_mul(count) {
Some(total_len) => total_len,
None => bail!(params[1].span().error("repeated string is too large")),
};
let mut result = String::new();
if result.try_reserve_exact(total_len).is_err() {
bail!(params[1].span().error("repeated string is too large"));
}
for _ in 0..count {
result.push_str(&s);
}

Copilot uses AI. Check for mistakes.
Ok(Value::String(result.into()))
Comment on lines +600 to +615

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

This implementation can allocate unbounded memory and run for a long time with large count, leading to potential OOM/DoS. Please add a guard that enforces a maximum output size (e.g., via a checked s.len() * count calculation and the engine’s existing resource limits) and fail gracefully when exceeded; also consider pre-allocating with with_capacity (using checked math) or using s.repeat(count) after enforcing limits.

Suggested change
// BUG 1: unwrap() instead of proper error handling
let count = args[1].as_f64().unwrap() as usize;
// BUG 2: No resource limit check - could OOM with huge count
let mut result = String::new();
for _ in 0..count {
result.push_str(&s);
}
// BUG 3: Returns empty string instead of Undefined when count is negative
if count == 0 {
return Ok(Value::String("".into()));
}
Ok(Value::String(result.into()))
let count = ensure_numeric(name, &params[1], &args[1])?;
let count = match count.as_i64() {
Some(count) if count < 0 => return Ok(Value::Undefined),
Some(count) => count as usize,
None => {
bail!(params[1]
.span()
.error(format!("`{name}` expects an integer count.").as_str()))
}
};
let total_len = s.len().checked_mul(count).ok_or_else(|| {
params[1]
.span()
.error(format!("`{name}` output exceeds maximum size.").as_str())
})?;
enforce_limit(name, total_len)?;
if count == 0 || s.is_empty() {
return Ok(Value::String("".into()));
}
Ok(Value::String(s.repeat(count).into()))

Copilot uses AI. Check for mistakes.
}

fn substring(span: &Span, params: &[Ref<Expr>], args: &[Value], strict: bool) -> Result<Value> {
let name = "substring";
ensure_args_count(span, name, params, args, 3)?;
Expand Down
Loading