Skip to content

[Repo Assist] fix: CircularBuffer.GetEnumerator infinite-recursion and destructive-enumeration #257

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

Draft
github-actions wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[Repo Assist] fix: CircularBuffer.GetEnumerator infinite-recursion and destructive-enumeration #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f31babd
fix: CircularBuffer.GetEnumerator infinite-recursion and destructive …
github-actions[bot] Apr 25, 2026
5c63c23
ci: trigger checks
github-actions[bot] Apr 25, 2026
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
20 changes: 6 additions & 14 deletions src/FSharpx.Collections/CircularBuffer.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,24 +97,16 @@ type CircularBuffer<'T>(bufferSize: int) =
member this.Enqueue(value) =
this.Enqueue([| value |], 0, 1)

member this.GetEnumerator() =
let rec loop() =
seq {
if length > 0 then
yield this.Dequeue(1).[0]

yield! loop()
}

loop().GetEnumerator()
member this.GetEnumerator() : IEnumerator<'T> =
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

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

GetEnumerator builds a seq that reads the mutable tail field on each iteration (buffer.[(tail + i) % bufferSize]). If the buffer is modified during enumeration, this can shift the effective start index mid-enumeration and produce duplicated/skipped elements. Consider snapshotting tail (and length) into local let bindings before constructing the sequence so the enumerator is stable for the duration of enumeration.

Suggested change
member this.GetEnumerator() : IEnumerator<'T> =
member this.GetEnumerator() : IEnumerator<'T> =
let tail = tail
let length = length

Copilot uses AI. Check for mistakes.
(seq { for i in 0 .. length - 1 -> buffer.[(tail + i) % bufferSize] }).GetEnumerator()

interface IEnumerable<'T> with
member this.GetEnumerator() =
(this :> IEnumerable<'T>).GetEnumerator()
member this.GetEnumerator() : IEnumerator<'T> =
this.GetEnumerator()

interface IEnumerable with
member this.GetEnumerator() =
(this :> IEnumerable).GetEnumerator()
member this.GetEnumerator() : IEnumerator =
(this.GetEnumerator() :> IEnumerator)

interface IReadOnlyCollection<'T> with
member this.Count = this.Count
52 changes: 51 additions & 1 deletion tests/FSharpx.Collections.Tests/CircularBufferTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,4 +320,54 @@ module CircularBufferTests =
//|> Async.Start
//// [/snippet]
//printfn "CircularStream.AsyncWrite(array) tests passed in %d ms" stopwatch.ElapsedMilliseconds
]

test "Enqueue(array, offset) 2-arg overload enqueues from offset to end" {
let circularBuffer = CircularBuffer<int> 5
circularBuffer.Enqueue([| 10; 20; 30; 40; 50 |], 2)
Expect.equal "count" 3 <| circularBuffer.Count
Expect.equal "dequeued" [| 30; 40; 50 |] <| circularBuffer.Dequeue 3
}

test "Seq.toList is non-destructive: buffer is unchanged after enumeration" {
let circularBuffer = CircularBuffer<int> 5
circularBuffer.Enqueue(1)
circularBuffer.Enqueue(2)
circularBuffer.Enqueue(3)
let asList = Seq.toList circularBuffer
Expect.equal "seq contents" [ 1; 2; 3 ] asList
Expect.equal "count unchanged" 3 <| circularBuffer.Count
}

test "for-in loop enumerates elements in FIFO order" {
let circularBuffer = CircularBuffer<int> 5
circularBuffer.Enqueue [| 10; 20; 30 |]
let mutable result = []

for x in circularBuffer do
result <- result @ [ x ]

Comment on lines +347 to +348
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

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

Building result via result <- result @ [ x ] inside the loop repeatedly appends to the end of a list (O(n^2) behavior). Prefer accumulating with x :: result and reversing once at the end, or using a ResizeArray/System.Collections.Generic.List and converting to a list at the end.

Suggested change
result <- result @ [ x ]
result <- x :: result
let result = List.rev result

Copilot uses AI. Check for mistakes.
Expect.equal "for-in" [ 10; 20; 30 ] result
}

test "Seq.toList on wrapped-around buffer preserves FIFO order" {
let circularBuffer = CircularBuffer<int> 4
circularBuffer.Enqueue [| 1; 2; 3; 4 |]
circularBuffer.Dequeue 2 |> ignore
circularBuffer.Enqueue [| 5; 6 |]
// Internal state: head has wrapped; FIFO order should be [3;4;5;6]
Expect.equal "wrapped seq" [ 3; 4; 5; 6 ] (Seq.toList circularBuffer)
}

test "Seq.toList on empty buffer returns empty list" {
let circularBuffer = CircularBuffer<int> 5
Expect.equal "empty seq" [] (Seq.toList circularBuffer)
}

test "IReadOnlyCollection Count matches after wrap" {
let circularBuffer = CircularBuffer<int> 3
circularBuffer.Enqueue [| 1; 2; 3 |]
circularBuffer.Dequeue 1 |> ignore
circularBuffer.Enqueue(4)
let irc = circularBuffer :> System.Collections.Generic.IReadOnlyCollection<int>
Expect.equal "IReadOnlyCollection.Count" 3 irc.Count
} ]
Loading