Skip to content

Conversation

Guest0x0
Copy link
Contributor

MoonBit currently allows calling impl of foreign type with dot syntax/TypeName::, but only inside current package. This behavior was added to allow locally extend dot syntax of foreign type, but is not refactor safe (if upstream add a method of the same name, behavior of downstream code will silently change), so we decided to deprecate and remove this feature in the future.

This PR removes usage of this behavior in core.

Copy link

Direct use of foreign type impl methods has been replaced with trait interface calls

Category
Maintainability
Code Snippet
Bytes::makei(sz, fn() { Byte::arbitrary(size, rs) })
-> Bytes::makei(sz, fn(
) { Arbitrary::arbitrary(size, rs) })
Recommendation
The change is correct. Consider adding a comment explaining why direct trait interface calls are preferred over dot syntax for foreign types
Reasoning
The change improves refactoring safety by preventing potential silent behavior changes if upstream adds methods with the same name. Using trait interface calls makes the code's intent clearer.

Type annotation added for clarity in String arbitrary implementation

Category
Maintainability
Code Snippet
let c : Char = Arbitrary::arbitrary(i, rs)
s = s + c.to_string()
Recommendation
Consider consistently applying type annotations in similar situations throughout the codebase
Reasoning
The added type annotation makes the code more explicit and easier to understand, especially when dealing with trait implementations and type inference

String concatenation in a loop might be inefficient

Category
Performance
Code Snippet
for i in 0..<len {
s = s + c.to_string()
}
Recommendation
Consider using a string builder or buffer if available in MoonBit:

let mut buffer = StringBuffer::new()
for i in 0..<len {
  let c : Char = Arbitrary::arbitrary(i, rs)
  buffer.append(c)
}
buffer.to_string()

Reasoning
Repeated string concatenation in a loop can lead to quadratic time complexity due to string copying. A string buffer would be more efficient.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6991

Details

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 92.505%

Changes Missing Coverage Covered Lines Changed/Added Lines %
quickcheck/arbitrary.mbt 2 3 66.67%
Totals Coverage Status
Change from base Build 6990: 0.001%
Covered Lines: 8738
Relevant Lines: 9446

💛 - Coveralls

@Guest0x0 Guest0x0 merged commit c6f3a15 into main May 30, 2025
34 of 36 checks passed
@Guest0x0 Guest0x0 deleted the Guest0x0/remove-local-dot branch May 30, 2025 06:50
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.

2 participants