Skip to content

feat: make copy faster #2721

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.

Sign up for GitHub

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

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

feat: make copy faster #2721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 0 additions & 28 deletions array/array_js.mbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@
///|
priv type JSArray

///|
fn[T] JSArray::ofAnyArray(array : Array[T]) -> JSArray = "%identity"

///|
fn[T] JSArray::toAnyArray(self : JSArray) -> Array[T] = "%identity"

///|
fn[T] JSArray::ofAnyFixedArray(array : FixedArray[T]) -> JSArray = "%identity"

Expand All @@ -30,25 +24,3 @@ fn[T] JSArray::toAnyFixedArray(self : JSArray) -> FixedArray[T] = "%identity"
///|
extern "js" fn JSArray::copy(self : JSArray) -> JSArray =
#|(arr) => arr.slice(0)

///|
/// Creates and returns a new array with a copy of all elements from the input
/// array.
///
/// Parameters:
///
/// * `array` : The array to be copied.
///
/// Returns a new array containing all elements from the original array.
///
/// Example:
///
/// ```moonbit
/// let original = [1, 2, 3]
/// let copied = original.copy()
/// inspect(copied, content="[1, 2, 3]")
/// inspect(physical_equal(original, copied), content="false")
/// ```
pub fn[T] copy(self : Array[T]) -> Array[T] {
JSArray::ofAnyArray(self).copy().toAnyArray()
}
29 changes: 0 additions & 29 deletions array/array_nonjs.mbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,32 +11,3 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

///|
/// Creates and returns a new array with a copy of all elements from the input
/// array.
///
/// Parameters:
///
/// * `array` : The array to be copied.
///
/// Returns a new array containing all elements from the original array.
///
/// Example:
///
/// ```moonbit
/// let original = [1, 2, 3]
/// let copied = original.copy()
/// inspect(copied, content="[1, 2, 3]")
/// inspect(physical_equal(original, copied), content="false")
/// ```
pub fn[T] copy(self : Array[T]) -> Array[T] {
let len = self.length()
if len == 0 {
[]
} else {
let arr = Array::make(len, self[0])
Array::unsafe_blit(arr, 0, self, 0, len)
arr
}
}
1 change: 0 additions & 1 deletion array/pkg.generated.mbti
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ impl[T : Eq] Eq for FixedArray[T]
impl[T : Hash] Hash for FixedArray[T]
impl[X : @quickcheck.Arbitrary] @quickcheck.Arbitrary for FixedArray[X]

fn[T] Array::copy(Self[T]) -> Self[T]
fn[A, B] Array::filter_map(Self[A], (A) -> B? raise?) -> Self[B] raise?
fn[T] Array::from_iter(Iter[T]) -> Self[T]
fn[A : @string.ToStringView] Array::join(Self[A], StringView) -> String
Expand Down
26 changes: 26 additions & 0 deletions builtin/arraycore_js.mbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,3 +358,29 @@ pub fn[A] Array::fill(
}
JSArray::ofAnyArray(self).fill(JSValue::ofAny(value), start, end)
}

///|
extern "js" fn JSArray::copy(self : JSArray) -> JSArray =
#|(arr) => arr.slice(0)

///|
/// Creates and returns a new array with a copy of all elements from the input
/// array.
///
/// Parameters:
///
/// * `array` : The array to be copied.
///
Comment on lines +367 to +373
Copy link

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
/// Returns a new array containing all elements from the original array.
///
/// Example:
///
/// ```moonbit
/// let original = [1, 2, 3]
/// let copied = original.copy()
/// inspect(copied, content="[1, 2, 3]")
/// inspect(physical_equal(original, copied), content="false")
/// ```
pub fn[T] copy(self : Array[T]) -> Array[T] {
Copy link

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
JSArray::ofAnyArray(self).copy().toAnyArray()
}
29 changes: 29 additions & 0 deletions builtin/arraycore_nonjs.mbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,3 +462,32 @@ pub fn[A] Array::fill(
}
self.buf.unchecked_fill(start, value, length - start)
}

///|
/// Creates and returns a new array with a copy of all elements from the input
/// array.
///
/// Parameters:
///
/// * `array` : The array to be copied.
///
Comment on lines +467 to +473
Copy link

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
/// Returns a new array containing all elements from the original array.
///
/// Example:
///
/// ```moonbit
/// let original = [1, 2, 3]
/// let copied = original.copy()
/// inspect(copied, content="[1, 2, 3]")
/// inspect(physical_equal(original, copied), content="false")
/// ```
pub fn[T] copy(self : Array[T]) -> Array[T] {
Copy link

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
let len = self.length()
if len == 0 {
[]
} else {
let arr = Array::make_uninit(len)
Copy link

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copilot uses AI. Check for mistakes.
Array::unsafe_blit(arr, 0, self, 0, len)
arr
}
}
1 change: 1 addition & 0 deletions builtin/pkg.generated.mbti
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ fn[T] Array::chunk_by(Self[T], (T, T) -> Bool raise?) -> Self[Self[T]] raise?
fn[T] Array::chunks(Self[T], Int) -> Self[Self[T]]
fn[T] Array::clear(Self[T]) -> Unit
fn[T : Eq] Array::contains(Self[T], T) -> Bool
fn[T] Array::copy(Self[T]) -> Self[T]
fn[T : Eq] Array::dedup(Self[T]) -> Unit
fn[T] Array::drain(Self[T], Int, Int) -> Self[T]
fn[T] Array::each(Self[T], (T) -> Unit raise?) -> Unit raise?
Expand Down
Loading