feat: make copy faster

[peter-jerry-ye]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 1273

Details

• 3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.01%) to 89.601%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
builtin/arraycore_nonjs.mbt	3	4	75.0%

Totals
Change from base Build 1271:	-0.01%
Covered Lines:	9297
Relevant Lines:	10376

---

💛 - Coveralls

[Yu-zh]

This implementation is better than before. But there's still an unnecessary pass of initializing the array. I think it should be implemented as intrinsics from the moonc side.

So I'm not sure whether we should move forward with this PR.

[peter-jerry-ye-code-review]

Inconsistent array initialization in non-JS copy implementation

Category
Correctness
Code Snippet
let arr = Array::make_uninit(len)
Array::unsafe_blit(arr, 0, self, 0, len)
Recommendation
Use Array::make(len, self[0]) instead of Array::make_uninit(len) to ensure proper initialization before blitting, or add proper initialization handling for uninitialized arrays
Reasoning
The new implementation uses make_uninit which creates an uninitialized array, but then uses unsafe_blit without ensuring the destination is properly initialized. This could lead to undefined behavior or memory safety issues.

Missing JSArray helper functions in builtin module

Category
Maintainability
Code Snippet
JSArray::ofAnyArray(self).copy().toAnyArray()
Recommendation
Add the missing JSArray::ofAnyArray and JSArray::toAnyArray functions to the builtin module, or update the JS implementation to use available functions
Reasoning
The JS implementation in builtin/arraycore_js.mbt references JSArray::ofAnyArray and toAnyArray functions that were removed from array/array_js.mbt but don't appear to be defined in the builtin module, which could cause compilation errors.

Potential performance regression with make_uninit usage

Category
Performance
Code Snippet
let arr = Array::make_uninit(len)
Array::unsafe_blit(arr, 0, self, 0, len)
Recommendation
Consider using a more efficient initialization pattern or ensure make_uninit is properly handled to avoid unnecessary overhead
Reasoning
While make_uninit might be intended for performance optimization, if not handled correctly it could introduce overhead or safety issues. The original implementation using Array::make(len, self[0]) was safer and the performance difference should be measured.

[Copilot]

Pull Request Overview

Moves Array.copy into builtin array core for both non-JS and JS targets and optimizes the non-JS implementation for performance by avoiding redundant initialization.

• Add Array::copy to builtin API and implement it in arraycore_nonjs.mbt with make_uninit + unsafe_blit
• Implement Array::copy for JS in arraycore_js.mbt via JS slice(0)
• Remove prior implementations and declarations from the array package to de-duplicate

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
builtin/pkg.generated.mbti	Declares Array::copy in the builtin API surface
builtin/arraycore_nonjs.mbt	Adds optimized non-JS implementation of Array.copy
builtin/arraycore_js.mbt	Adds JS implementation of Array.copy using arr.slice(0)
array/pkg.generated.mbti	Removes Array.copy declaration from array package (now provided by builtin)
array/array_nonjs.mbt	Removes previous non-JS implementation (superseded by builtin)
array/array_js.mbt	Removes previous JS implementation and redundant identity helpers (now in builtin)

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The method is declared without the Array:: qualifier, which is inconsistent with other methods in this file (for example, Array::fill) and with the new signature exposed in builtin/pkg.generated.mbti. Define it as a method on Array to match the declared API: change the signature to:

484 |pub fn[T] Array::copy(self : Array[T]) -> Array[T] {

Suggested change

	pub fn[T] copy(self : Array[T]) -> Array[T] {
	pub fn[T] Array::copy(self : Array[T]) -> Array[T] {

[Copilot]

Same issue as the non-JS implementation: the method should be namespaced as Array::copy to match the API surface and the style used elsewhere in this file. Update the signature to:

384 |pub fn[T] Array::copy(self : Array[T]) -> Array[T] {

Suggested change

	pub fn[T] copy(self : Array[T]) -> Array[T] {
	pub fn[T] Array::copy(self : Array[T]) -> Array[T] {

[Copilot]

The documentation refers to a parameter named array, but the function uses a receiver self and has no such parameter. Please update the docs to reflect the receiver-based API (for example, remove the Parameters section or replace it with “Copies and returns a new Array from self.”).

Suggested change

	/// Creates and returns a new array with a copy of all elements from the input
	/// array.
	///
	/// Parameters:
	///
	/// * `array` : The array to be copied.
	///
	/// Copies and returns a new array containing all elements from `self`.
	///

[Copilot]

Documentation mentions a parameter array, but the function is a method on Array taking self; there is no such parameter. Please align the docs with the receiver-based API (e.g., remove the Parameters section or refer to self).

Suggested change

	/// Creates and returns a new array with a copy of all elements from the input
	/// array.
	///
	/// Parameters:
	///
	/// * `array` : The array to be copied.
	///
	/// Creates and returns a new array with a copy of all elements from this array.
	///

[Copilot]

[nitpick] Since make_uninit is used, a brief comment would help future readers understand the safety invariants for unsafe_blit (that len == self.length(), no overlap, and arr becomes fully initialized). Suggest adding a short comment above this call clarifying these guarantees.

Suggested change

	let arr = Array::make_uninit(len)
	let arr = Array::make_uninit(len)
	// Safety invariants for unsafe_blit:
	// - len == self.length()
	// - arr is freshly allocated, so no overlap with self
	// - arr will be fully initialized by this blit