Skip to content
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

Jump to bottom

Array.insertManyAt returns original array for empty insertion #18352

Open
albert-du opened this issue Mar 1, 2025 · 1 comment · May be fixed by #18353
Open

Array.insertManyAt returns original array for empty insertion #18352

albert-du opened this issue Mar 1, 2025 · 1 comment · May be fixed by #18353
Labels
Bug Needs-Triage
Milestone
Backlog

Comments

@albert-du
Copy link
Contributor

When calling Array.insertManyAt, if the values inserted are empty, the source array is always returned, rather than a copy.

Documented behavior is "Return a new array with new items inserted before the given index." however returning the original source can create unexpected side effects when the values being inserted are empty.

Consider:

    let original = [| 1; 2; 3 |]

    let newArr = Array.insertManyAt 3 [| 4; 5 |] original

    printfn $"%b{original = newArr}"
    //  false

    printfn $"%A{original}"
    printfn  $"%A{newArr}" 
    //  [|1; 2; 3|]
    //  [|1; 2; 3; 4; 5|]

    // do some weird things to newArr

    newArr[0] <- 987654321

    printfn $"%A{original}"
    printfn  $"%A{newArr}" 
    //   [|1; 2; 3|]
    //   [|987654321; 2; 3; 4; 5|]

    // original is unaffected

sharplab

and when the values inserted are empty

    let original = [| 1; 2; 3 |]
    
    // insert empty array
    let newArr = Array.insertManyAt 3 [||] original
    
    printfn $"%b{original = newArr}"
    //  true   (that's strange)
    
    printfn $"%A{original}"
    printfn  $"%A{newArr}" 
    //  [|1; 2; 3|]
    //  [|1; 2; 3|]
    
    // do some weird things to newArr
    
    newArr[0] <- 987654321
    
    printfn $"%A{original}"
    printfn  $"%A{newArr}" 
    //   [|987654321; 2; 3|]
    //   [|987654321; 2; 3|]
    
    // original has been affected by a mutation on the 'new' array

sharplab

Expected behavior

Always create a new array.

Actual behavior

When the inserted values are empty, the original array, rather than a copy, is returned.

Known workarounds

Preemptively copy the source array, or copy the returned value.

Related information

I believe the cause is

fsharp/src/FSharp.Core/array.fs

Lines 1936 to 1938 in 32c2764

if valuesArray.Length = 0 then
source
else

Fix would be adding a call to Array.copy in this branch.

This does not affect Seq.insertManyAt and List.insertManyAt as seq and list immutable.

Provide any related information (optional):

.NET / FSharp.Core 9.0
@github-actions github-actions bot added this to the Backlog milestone Mar 1, 2025
@muqiuhan
Copy link
Contributor

muqiuhan commented Mar 3, 2025

^_^ Maybe we can just use Clone, because checkNonNull is already done at the beginning of insertManyAt:

fsharp/src/FSharp.Core/array.fs

Lines 292 to 294 in 32c2764

let copy (array: 'T array) =
checkNonNull "array" array
(array.Clone() :?> 'T array) // this is marginally faster

muqiuhan added a commit to muqiuhan/fsharp that referenced this issue Mar 3, 2025
@muqiuhan
Bugfix for Array.insertManyAt returns original array for empty insert…
4b0213e 
…ion (dotnet#18352)
@muqiuhan muqiuhan linked a pull request Mar 3, 2025 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Needs-Triage
Projects
F# Compiler and Tooling
Status: New
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Bugfix for Array.insertManyAt returns original array for empty inserion (#18352) muqiuhan/fsharp
2 participants
@albert-du @muqiuhan