Skip to content

[JULES] Refactor: Utilize standard helpers in random module #388

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
logbie merged 4 commits into from
Mar 3, 2026
Merged

[JULES] Refactor: Utilize standard helpers in random module #388

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3912a4d
Refactor: Utilize standard helpers in random module
google-labs-jules[bot] Mar 3, 2026
5bfe668
test: add tests for native_generate_uuid argument rejection and outpu…
google-labs-jules[bot] Mar 3, 2026
44d868c
fix: address formatting failure in random.rs
google-labs-jules[bot] Mar 3, 2026
3ace111
fix: validate bounds for random_between and avoid panics
google-labs-jules[bot] Mar 3, 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
188 changes: 73 additions & 115 deletions src/stdlib/random.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_list, expect_number};
use crate::interpreter::environment::Environment;
use crate::interpreter::error::RuntimeError;
use crate::interpreter::value::Value;
Expand All @@ -18,13 +19,7 @@ thread_local! {

/// Generate a cryptographically secure random number between 0 and 1
pub fn native_random(args: Vec<Value>) -> Result<Value, RuntimeError> {
if !args.is_empty() {
return Err(RuntimeError::new(
format!("random expects 0 arguments, got {}", args.len()),
0,
0,
));
}
check_arg_count("random", &args, 0)?;

RNG.with(|rng| {
let mut rng = rng.borrow_mut();
Expand All @@ -35,42 +30,22 @@ pub fn native_random(args: Vec<Value>) -> Result<Value, RuntimeError> {

/// Generate a random number between min and max (inclusive)
pub fn native_random_between(args: Vec<Value>) -> Result<Value, RuntimeError> {
if args.len() != 2 {
check_arg_count("random_between", &args, 2)?;

let min = expect_number(&args[0])?;
let max = expect_number(&args[1])?;

if !min.is_finite() || !max.is_finite() {
return Err(RuntimeError::new(
format!("random_between expects 2 arguments, got {}", args.len()),
format!(
"random_between: bounds must be finite numbers, got min: {}, max: {}",
min, max
),
0,
0,
));
}

let min = match &args[0] {
Value::Number(n) => *n,
_ => {
return Err(RuntimeError::new(
format!(
"random_between expects numbers, got {}",
args[0].type_name()
),
0,
0,
));
}
};

let max = match &args[1] {
Value::Number(n) => *n,
_ => {
return Err(RuntimeError::new(
format!(
"random_between expects numbers, got {}",
args[1].type_name()
),
0,
0,
));
}
};

if min > max {
return Err(RuntimeError::new(
format!(
Expand All @@ -91,35 +66,10 @@ pub fn native_random_between(args: Vec<Value>) -> Result<Value, RuntimeError> {

/// Generate a random integer between min and max (inclusive)
pub fn native_random_int(args: Vec<Value>) -> Result<Value, RuntimeError> {
if args.len() != 2 {
return Err(RuntimeError::new(
format!("random_int expects 2 arguments, got {}", args.len()),
0,
0,
));
}
check_arg_count("random_int", &args, 2)?;

let min = match &args[0] {
Value::Number(n) => *n as i64,
_ => {
return Err(RuntimeError::new(
format!("random_int expects numbers, got {}", args[0].type_name()),
0,
0,
));
}
};

let max = match &args[1] {
Value::Number(n) => *n as i64,
_ => {
return Err(RuntimeError::new(
format!("random_int expects numbers, got {}", args[1].type_name()),
0,
0,
));
}
};
let min = expect_number(&args[0])? as i64;
let max = expect_number(&args[1])? as i64;

if min > max {
return Err(RuntimeError::new(
Expand All @@ -141,13 +91,7 @@ pub fn native_random_int(args: Vec<Value>) -> Result<Value, RuntimeError> {

/// Generate a random boolean value
pub fn native_random_boolean(args: Vec<Value>) -> Result<Value, RuntimeError> {
if !args.is_empty() {
return Err(RuntimeError::new(
format!("random_boolean expects 0 arguments, got {}", args.len()),
0,
0,
));
}
check_arg_count("random_boolean", &args, 0)?;

RNG.with(|rng| {
let mut rng = rng.borrow_mut();
Expand All @@ -158,59 +102,31 @@ pub fn native_random_boolean(args: Vec<Value>) -> Result<Value, RuntimeError> {

/// Select a random element from a list
pub fn native_random_from(args: Vec<Value>) -> Result<Value, RuntimeError> {
if args.len() != 1 {
check_arg_count("random_from", &args, 1)?;

let list_ref = expect_list(&args[0])?;
let list = list_ref.borrow();

if list.is_empty() {
return Err(RuntimeError::new(
format!("random_from expects 1 argument, got {}", args.len()),
"random_from: cannot select from empty list".to_string(),
0,
0,
));
}

match &args[0] {
Value::List(list_ref) => {
let list = list_ref.borrow();
if list.is_empty() {
return Err(RuntimeError::new(
"random_from: cannot select from empty list".to_string(),
0,
0,
));
}

RNG.with(|rng| {
let mut rng = rng.borrow_mut();
let index = rng.random_range(0..list.len());
Ok(list[index].clone())
})
}
_ => Err(RuntimeError::new(
format!("random_from expects a list, got {}", args[0].type_name()),
0,
0,
)),
}
RNG.with(|rng| {
let mut rng = rng.borrow_mut();
let index = rng.random_range(0..list.len());
Ok(list[index].clone())
})
}
Comment on lines 104 to 123

Copilot AI Mar 3, 2026

Copy link

Choose a reason for hiding this comment

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

native_random_from was refactored but still has no local unit tests covering the key behaviors (returns an element from a non-empty list; errors on empty list; errors on non-list input). Since this module already has tests for other random functions, adding a couple focused tests here would help prevent regressions.

Copilot uses AI. Check for mistakes.

/// Set the random seed for reproducible results
pub fn native_random_seed(args: Vec<Value>) -> Result<Value, RuntimeError> {
if args.len() != 1 {
return Err(RuntimeError::new(
format!("random_seed expects 1 argument, got {}", args.len()),
0,
0,
));
}
check_arg_count("random_seed", &args, 1)?;

let seed = match &args[0] {
Value::Number(n) => *n as u64,
_ => {
return Err(RuntimeError::new(
format!("random_seed expects a number, got {}", args[0].type_name()),
0,
0,
));
}
};
let seed = expect_number(&args[0])? as u64;

RNG.with(|rng| {
// Replace the RNG with a seeded one
Expand All @@ -221,7 +137,9 @@ pub fn native_random_seed(args: Vec<Value>) -> Result<Value, RuntimeError> {

/// Generate a UUID v4 (random UUID)
/// Usage: generate_uuid() -> "550e8400-e29b-41d4-a716-446655440000"
pub fn native_generate_uuid(_args: Vec<Value>) -> Result<Value, RuntimeError> {
pub fn native_generate_uuid(args: Vec<Value>) -> Result<Value, RuntimeError> {
check_arg_count("generate_uuid", &args, 0)?;

Comment on lines +140 to +142

Copilot AI Mar 3, 2026

Copy link

Choose a reason for hiding this comment

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

native_generate_uuid now enforces 0 arguments via check_arg_count, whereas it previously accepted (and ignored) any arguments. If this stricter validation is intentional, consider calling it out in any user-facing changelog/docs; otherwise, keep backward compatibility by allowing extra args (or add a deprecation path).

Suggested change
pub fn native_generate_uuid(args: Vec<Value>) -> Result<Value, RuntimeError> {
check_arg_count("generate_uuid", &args, 0)?;
pub fn native_generate_uuid(_args: Vec<Value>) -> Result<Value, RuntimeError> {

Copilot uses AI. Check for mistakes.
use uuid::Uuid;

let uuid = Uuid::new_v4();
Expand Down Expand Up @@ -273,6 +191,32 @@ mod tests {
}
}

#[test]
fn test_generate_uuid_validates_args() {
let result = native_generate_uuid(vec![Value::Number(1.0)]);
assert!(result.is_err());
assert!(
result
.unwrap_err()
.message
.contains("generate_uuid expects 0 arguments")
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

#[test]
fn test_generate_uuid_format() {
let result = native_generate_uuid(vec![]);
assert!(result.is_ok());

if let Ok(Value::Text(uuid_str)) = result {
// Very basic UUID format check: 36 chars, 4 hyphens
assert_eq!(uuid_str.len(), 36);
assert_eq!(uuid_str.chars().filter(|&c| c == '-').count(), 4);
} else {
panic!("Expected text from generate_uuid");
}
}

#[test]
fn test_random_between_validates_range() {
let result = native_random_between(vec![Value::Number(5.0), Value::Number(10.0)]);
Expand All @@ -285,6 +229,20 @@ mod tests {
}
}

#[test]
fn test_random_between_rejects_non_finite_bounds() {
let nan = f64::NAN;
let inf = f64::INFINITY;

let result_nan = native_random_between(vec![Value::Number(nan), Value::Number(10.0)]);
assert!(result_nan.is_err());
assert!(result_nan.unwrap_err().message.contains("finite numbers"));

let result_inf = native_random_between(vec![Value::Number(0.0), Value::Number(inf)]);
assert!(result_inf.is_err());
assert!(result_inf.unwrap_err().message.contains("finite numbers"));
}

#[test]
fn test_random_int_produces_integers() {
let result = native_random_int(vec![Value::Number(1.0), Value::Number(10.0)]);
Expand Down