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

Improve errors raised, simplify/generalize some code #254

Merged
merged 1 commit into from
May 24, 2024
Merged

Improve errors raised, simplify/generalize some code #254

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
52 changes: 27 additions & 25 deletions src/FSharp.Control.TaskSeq/TaskSeqInternal.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,21 @@ module internal TaskSeqInternal =

let inline raiseEmptySeq () = invalidArg "source" "The input task sequence was empty."

let inline raiseCannotBeNegative name = invalidArg name "The value must be non-negative."
/// Moves the enumerator to its first element, assuming it has just been allocated.
/// Raises "The input sequence was empty" if there was no first element.
let inline moveFirstOrRaiseUnsafe (e: IAsyncEnumerator<_>) = task {
let! hasFirst = e.MoveNextAsync()

if not hasFirst then
invalidArg "source" "The input task sequence was empty."
}

/// Tests the given integer value and raises if it is -1 or lower.
let inline raiseCannotBeNegative name value =
if value >= 0 then
()
else
invalidArg name $"The value must be non-negative, but was {value}."
Copy link
Member

Choose a reason for hiding this comment

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

argumentoufofrange? I guess you're refactoring, which answers that...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll double-check, but the errors raised should be the same as for similar functions in Seq.xxx. I vaguely remember being surprised about this previously. It may be because the argument check is immediate, and the range check happens while iterating the sequence (which, in turn, may have a dynamic length).

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually started the refactoring to limit some allocations in an upcoming PR (and more generally, to improve the code).

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot about this discussion point. These should be invalidArg. We throw ArgumentOutOfRange if we find that, while iterating, we end up past the last index, for instance. Here, it is clearly an argument error, as there's never an index that's negative.

(this is my interpretation from MS's logic in the same/similar cases for Array/Seq/List)


let inline raiseOutOfBounds name =
invalidArg name "The value or index must be within the bounds of the task sequence."
Expand Down Expand Up @@ -183,10 +197,7 @@ module internal TaskSeqInternal =

task {
use e = source.GetAsyncEnumerator CancellationToken.None
let! nonEmpty = e.MoveNextAsync()

if not nonEmpty then
raiseEmptySeq ()
do! moveFirstOrRaiseUnsafe e

let mutable acc = e.Current

Expand All @@ -202,10 +213,7 @@ module internal TaskSeqInternal =

task {
use e = source.GetAsyncEnumerator CancellationToken.None
let! nonEmpty = e.MoveNextAsync()

if not nonEmpty then
raiseEmptySeq ()
do! moveFirstOrRaiseUnsafe e

let value = e.Current
let mutable accProjection = projection value
Expand All @@ -228,10 +236,7 @@ module internal TaskSeqInternal =

task {
use e = source.GetAsyncEnumerator CancellationToken.None
let! nonEmpty = e.MoveNextAsync()

if not nonEmpty then
raiseEmptySeq ()
do! moveFirstOrRaiseUnsafe e

let value = e.Current
let! projValue = projectionAsync value
Expand Down Expand Up @@ -276,7 +281,10 @@ module internal TaskSeqInternal =

let count =
match count with
| Some c -> if c >= 0 then c else raiseCannotBeNegative (nameof count)
| Some c ->
raiseCannotBeNegative (nameof count) c
c

| None -> Int32.MaxValue

match initializer with
Expand Down Expand Up @@ -741,9 +749,7 @@ module internal TaskSeqInternal =

let skipOrTake skipOrTake count (source: TaskSeq<_>) =
checkNonNull (nameof source) source

if count < 0 then
raiseCannotBeNegative (nameof count)
raiseCannotBeNegative (nameof count) count

match skipOrTake with
| Skip ->
Expand Down Expand Up @@ -907,8 +913,7 @@ module internal TaskSeqInternal =

/// InsertAt or InsertManyAt
let insertAt index valueOrValues (source: TaskSeq<_>) =
if index < 0 then
raiseCannotBeNegative (nameof index)
raiseCannotBeNegative (nameof index) index

taskSeq {
let mutable i = 0
Expand All @@ -933,8 +938,7 @@ module internal TaskSeqInternal =
}

let removeAt index (source: TaskSeq<'T>) =
if index < 0 then
raiseCannotBeNegative (nameof index)
raiseCannotBeNegative (nameof index) index

taskSeq {
let mutable i = 0
Expand All @@ -951,8 +955,7 @@ module internal TaskSeqInternal =
}

let removeManyAt index count (source: TaskSeq<'T>) =
if index < 0 then
raiseCannotBeNegative (nameof index)
raiseCannotBeNegative (nameof index) index

taskSeq {
let mutable i = 0
Expand All @@ -970,8 +973,7 @@ module internal TaskSeqInternal =
}

let updateAt index value (source: TaskSeq<'T>) =
if index < 0 then
raiseCannotBeNegative (nameof index)
raiseCannotBeNegative (nameof index) index

taskSeq {
let mutable i = 0
Expand Down
Loading