Add beamish

[jadnohra]

No description provided.

[jadnohra]

• Add beamish-2 #895
• Add beamish #864 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add the label merge-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

The syntax in this if let statement is incorrect. Rust doesn't support combining if let with && in this manner. This should be rewritten as:

if let Some(pattern_op) = op {
    if pattern_op != expr_op {
        return false;
    }
}

This issue appears in multiple places in the pattern matching code.

Suggested change

	if let Some(pattern_op) = op
	&& pattern_op != expr_op {
	return false;
	}
	if let Some(pattern_op) = op {
	if pattern_op != expr_op {
	return false;
	}
	}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The current implementation returns the original expression structure rather than applying the optimization described in the design document. For true optimization, this should transform the expression into the optimized form: (b ^ 0xFFFFFFFFFFFFFFFF) & c ^ (a ^ result), which would compile to a single AND constraint instead of three.

Suggested change

	|bindings| {
	// This would be optimized to a single AND constraint
	// For now, return a marked expression
	Expr::Binary {
	op: BinOp::Xor,
	left: Box::new(bindings["a"].clone()),
	right: Box::new(Expr::Binary {
	op: BinOp::And,
	left: Box::new(Expr::Unary {
	op: UnOp::Not,
	expr: Box::new(bindings["b"].clone()),
	}),
	right: Box::new(bindings["c"].clone()),
	}),
	}
	|bindings| {
	// Apply optimization to reduce to a single AND constraint
	// Transform a ^ ((~b) & c) into (b ^ 0xFFFFFFFFFFFFFFFF) & c ^ (a ^ result)
	let not_b = Expr::Binary {
	op: BinOp::Xor,
	left: Box::new(bindings["b"].clone()),
	right: Box::new(Expr::Literal(Literal::Integer(u64::MAX))),
	};
	Expr::Binary {
	op: BinOp::Xor,
	left: Box::new(Expr::Binary {
	op: BinOp::And,
	left: Box::new(not_b),
	right: Box::new(bindings["c"].clone()),
	}),
	right: Box::new(bindings["a"].clone()),
	}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The apply_rule_recursive method has inconsistent behavior between expression types. For unary expressions, it always returns a new expression when a rule applies to the inner expression, but for binary expressions, it only returns a new expression when the inner expressions actually change. This inconsistency could lead to infinite recursion or unnecessary object creation.

Consider updating the unary case to match the binary pattern:

Expr::Unary { op, expr: inner } => {
    if let Some(new_inner) = self.apply_rule_recursive(rule, inner) {
        Some(Expr::Unary {
            op: *op,
            expr: Box::new(new_inner),
        })
    } else {
        None
    }
}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The xor_many function will panic if called with an empty slice since it unconditionally accesses exprs[0] on line 20. Consider handling the empty case explicitly, either by:

pub fn xor_many<T: BitType>(exprs: &[Expr<T>]) -> Expr<T> {
    if exprs.is_empty() {
        // Return zero or panic with descriptive message
        return constant(0); // or panic!("Cannot XOR empty slice")
    }
    
    exprs.iter()
        .skip(1)
        .fold(exprs[0].clone(), |acc, e| xor(&acc, e))
}

This prevents unexpected runtime failures when the function is called with an empty collection.

Suggested change

	pub fn xor_many<T: BitType>(exprs: &[Expr<T>]) -> Expr<T> {
	exprs.iter()
	.skip(1)
	.fold(exprs[0].clone(), |acc, e| xor(&acc, e))
	}
	pub fn xor_many<T: BitType>(exprs: &[Expr<T>]) -> Expr<T> {
	if exprs.is_empty() {
	return constant(0);
	}
	exprs.iter()
	.skip(1)
	.fold(exprs[0].clone(), |acc, e| xor(&acc, e))
	}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The syntax in this pattern matching logic is incorrect. The current code uses if let Some(pattern_op) = op && pattern_op != expr_op which combines a pattern match with a boolean condition in an invalid way. This should be rewritten as either:

if let Some(pattern_op) = op {
    if pattern_op != expr_op {
        return false;
    }
}

Or more concisely:

if let Some(pattern_op) = op && *pattern_op != *expr_op {
    return false;
}

This issue appears in multiple places in the pattern matching code and would cause compilation errors.

Suggested change

	if let Some(pattern_op) = op
	&& pattern_op != expr_op {
	return false;
	}
	if let Some(pattern_op) = op {
	if pattern_op != expr_op {
	return false;
	}
	}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The import statement in the SHA256 example uses beamish::*, but the crate name in Cargo.toml is defined as binius-beamish. This will cause compilation errors. The import should be changed to binius_beamish::* to match the actual crate name. This pattern appears in several example files and should be corrected throughout.

Suggested change

	use beamish::types::U32;
	use binius_beamish::types::U32;

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The fmt_str function calls format!("{}", node) which could lead to infinite recursion since Display for ExprNode calls fmt_node, which in turn may call fmt_str again. Consider using a direct approach to avoid this potential issue:

fn fmt_str(node: &ExprNode) -> String {
    let mut s = String::new();
    let mut f = fmt::Formatter::new(&mut s);
    fmt_node(node, &mut f).unwrap();
    s
}

Or alternatively, implement a separate formatting path specifically for this helper function.

Suggested change

	fn fmt_str(node: &ExprNode) -> String {
	format!("{}", node)
	}
	fn fmt_str(node: &ExprNode) -> String {
	let mut s = String::new();
	let mut f = fmt::Formatter::new(&mut s);
	fmt_node(node, &mut f).unwrap();
	s
	}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Dependencies 'log' and 'env_logger' should refer to workspace-wide dependencies declared in the top-level Cargo.toml using the workspace syntax (e.g., log.workspace = true and env_logger.workspace = true) instead of specifying version numbers directly. This ensures consistent dependency versions across the workspace.

Spotted by Diamond (based on custom rule: Irreducible Rust and Cargo)



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The test file does not use StdRng::seed_from_u64 for reproducible tests as required by the Pseudo-Random Testing rule. Tests should use seeded random number generation to ensure reproducibility. Additionally, the tests should consider using helper functions from binius_math::test_utils module where appropriate for better test utilities usage.

Spotted by Diamond (based on custom rule: Monbijou Testing Patterns)



Is this helpful? React 👍 or 👎 to let us know.