Skip to content

refactor: migrate rand and arbitrary #2327

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.

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

peter-jerry-ye
Copy link
Collaborator

@peter-jerry-ye peter-jerry-ye commented Jun 24, 2025

Goal

Make Arbitrary accept general generators

Blocking issue

Currently the Arbitrary uses @splitmix.RandomState

  1. All its methods need to be provided by the successor (&Source or Rand).
  2. The import of package @random must be added.
  3. There's the split method that is used and can not be replaced, resulting in
  4. The behavior would be different, breaking all the expectation tests.

Copy link

Trait constraint change in arbitrary type signature may affect existing code

Category
Correctness
Code Snippet
arbitrary(Int, @splitmix.RandomState) -> Self
// changed to:
arbitrary(Int, &@random.Source) -> Self
Recommendation
Ensure all existing implementations of Arbitrary are updated to handle the new Source trait interface instead of RandomState directly
Reasoning
The signature change from concrete RandomState to abstract Source type is a breaking change that could cause compilation errors in existing code that implements Arbitrary trait

Arbitrary implementations for collection types should be documented

Category
Maintainability
Code Snippet
pub impl[X : Arbitrary] Arbitrary for Array[X] with arbitrary(size, rs) {
let len = if size == 0 { 0 } else { rs.next_positive_int() % size }
Array::makei(len, fn(x) { X::arbitrary(x, rs) })
}
Recommendation
Add documentation comments explaining the generation strategy for collection types, including any invariants maintained during generation
Reasoning
Collection type generation behavior affects testing outcomes and should be well documented for users implementing property-based tests

Unnecessary allocation in View arbitrary implementation

Category
Performance
Code Snippet
pub impl[A : Arbitrary] Arbitrary for @array.View[A] with arbitrary(size, rs) {
(Arbitrary::arbitrary(size, rs) : Array[A])[:]
}
Recommendation
Consider implementing View generation directly without creating an intermediate Array
Reasoning
Creating a full Array just to get a View creates unnecessary allocation overhead. A direct implementation could be more efficient.

@peter-jerry-ye peter-jerry-ye requested a review from bobzhang June 24, 2025 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant