Skip to content

Conversation

Guest0x0
Copy link
Contributor

This PR fixes waning 0045. In the future, a trait where all methods have default implementation can no longer be implemented explicitly. A impl Trait for Type declaration must be declared. In the case of @quickcheck, all methods of trait Shrink has default implementation.

While fixing, a subtle bug is found. Due to the coherence issue in moonbitlang/moonbit-docs#819, many implementations of Shrink are not pub, but this is never discovered. In some cases (directly apply Shrink::shrink on a known type) this can lead to unexpected behavior.

Some builtin types, such as Double and String, are missing a Shrink implementation. This can lead to warnings for downstream users, so I added empty placeholder impl declaration for them.

Copy link

peter-jerry-ye-code-review bot commented Jun 25, 2025

Ensure all public Shrink implementations handle edge cases

Category
Correctness
Code Snippet
pub impl Shrink for Double
pub impl Shrink for Float
pub impl Shrink for String
Recommendation
Add proper shrinking implementations that handle edge cases like infinity, NaN for floating point types and empty/large strings
Reasoning
Empty implementations may lead to poor test case reduction. Proper shrinking implementations help find minimal failing cases by intelligently reducing test inputs

Update quick_check function signature documentation

Category
Maintainability
Code Snippet
fn[P : Testable] quick_check(P, max_shrinks? : Int, max_success? : Int, max_size? : Int, discard_ratio? : Int, expect~ : Expected = .., abort~ : Bool = ..) -> Unit raise Failure
Recommendation
Add documentation comments explaining the purpose of each optional parameter and their default values
Reasoning
The function signature was updated to include more optional parameters but lacks documentation about their meaning and typical usage patterns

Explicit error handling for TestError type

Category
Correctness
Code Snippet
fn State::run_single_test(Self, Property) -> Result[TestSuccess, Self] raise TestError
Recommendation
Document the conditions under which TestError is raised and how it should be handled
Reasoning
The error handling behavior was changed from using ! to raise but the conditions that trigger TestError are not clear from the interface

@Guest0x0 Guest0x0 merged commit e4dbec7 into main Jun 25, 2025
10 of 14 checks passed
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