Runtime: Add overload to int? Sum(this IEnumerable<int?>) to allow null returning (and other Sum functions)

[ghost]

With the implementation of the overload of IEnumerable.Sum that takes a list of nullable int as parameter and returns a nullable int, a null value is actually never returned.

    public static int? Sum(this IEnumerable<int?> source)
    {
        if (source == null)
        {
            ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source);
        }

        int sum = 0;
        checked
        {
            foreach (int? v in source)
            {
                if (v != null)
                {
                    sum += v.GetValueOrDefault();
                }
            }
        }

        return sum;
    }

The same problem exists in other overloads taking nullable parameters.

[ghost]

I would expect that a null value is returned in the situation where the source collection contains only null values.

[stephentoub]

I'm not sure why it was designed to return int? rather than int, but at this point I don't see either the signature or the behavior changing.

[ghost]

As I see it, returning 0 when the whole input array is null values seem
wrong.

fre. 10. maj 2019 18.14 skrev Stephen Toub [e-mail]:

I'm not sure why it was designed to return int? rather than int, but at
this point I don't see either the signature or the behavior changing.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/corefx/issues/37580#issuecomment-491344812,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC5TAWASWCSMG5MCAKPZ55LPUWNM3ANCNFSM4HL75O6A
.

[stephentoub]

As I see it, returning 0 when the whole input array is null values seem wrong

That's a valid opinion.

But when the IEnumerable<int> overload has 0 elements, it also returns 0, so it's consistent with that: nothing to sum == 0.

On top of that, this has been the documented behavior:
"This method returns zero if source contains no elements."

I appreciate you don't like the design, but this is the design and has been since it shipped years ago. I don't think there's a compelling enough reason to change it.

[ghost]

My opinion is that if nothing to sum, then return the null value so the
caller may decide what should happen.

Med venlig hilsen/Best regards

Benny Tordrup
Software Architect
Mobile [phone]

Visma Software International, R&D Visma Business
Langebrogade 1, Bygning A, DK-1411 København K | http://www.visma.com

Stay ahead with leading cloud software
This communication is intended for the person(s) named above only. It
contains information that is confidential and legally privileged. If
received in error, please delete this e-mail and notify the sender.

------ Original Message ------
From: "Stephen Toub" [e-mail]
To: "dotnet/corefx" [e-mail]
Cc: "BennyTordrupVisma" [e-mail]; "Author"
[e-mail]
Sent: 10-05-2019 20:25:20
Subject: Re: [dotnet/corefx] int? Sum(this IEnumerable) never
returns null (#37580)

As I see it, returning 0 when the whole input array is null values
seem wrong

That's a valid opinion.

But when the IEnumerable overload has 0 elements, it also returns
0, so it's consistent with that: nothing to sum == 0.

On top of that, this has been the documented behavior:
"This method returns zero if source contains no elements."

I appreciate you don't like the design, but this is the design and has
been since it shipped years ago. I don't think there's a compelling
enough reason to change it.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/corefx/issues/37580#issuecomment-491386255,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC5TAWFGUQ4ZWQNLO3J2L3DPUW4ZBANCNFSM4HL75O6A.

[Gnbrkm41]

I think it's reasonable idea to return null if every element is null, I won't disagree; but it'd be a breaking change for someone that relied on the fact that it will always have a value. Imagine someone doing enumerable.Sum().Value (relying on the fact it never will be null), only to notice that it starts throwing InvalidOperationException at some point for what looks like no reason.

[ghost]

@Gnbrkm41, good point.

What about an overloaded version like this:

    public enum NullReturnings
    {
        Default = 0,
        NullIfEmptyCollection = 1,
        NullIfAllElementsAreNull = 2,
        NullIfSomeElementsAreNull = 3
    }

    public static int? Sum(this IEnumerable<int?> source, NullReturnings nullReturning = NullReturnings.Default)
    {
        if (source == null)
        {
            ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source);
        }
        bool anyElements = false;
        bool anyNonNull = false;
        int sum = 0;
        checked
        {
            foreach (int? v in source)
            {
                anyElements = true;
                if (v != null)
                {
                    anyNonNull = true;
                    sum += v.Value;
                }
            }
        }

        if (!anyElements && nullReturning == NullReturnings.NullIfEmptyCollection)
            return null;

        if (anyElements && !anyNonNull && nullReturning == NullReturnings.NullIfAllElementsAreNull)
            return null;

        if (anyElements && anyNonNull && nullReturning == NullReturnings.NullIfSomeElementsAreNull)
            return null;

        return sum;
    }

[ghost]

Or even making a direct call to the old overload when nullReturning parameter is Default

[Gnbrkm41]

Consider renaming the issue? something on the line of adding overloads to nullable Sum() method... maybe.

[ghost]

Any further thoughts on this one? Will it be a task worth implementing?

[Gnbrkm41]

Any API additions should go through API reviews before anything are done to implement it. Some suggest writing things in a formal way and putting it on the top post. Perhaps have a look at this as an example: https://github.com/dotnet/corefx/issues/37156.

[ghost]

@Gnbrkm41, do you think I can reuse this issue for the proposal, or should I create a new issue?

[Gnbrkm41]

I think updating the top post should be enough.

[maryamariyan]

Here's more info on API review process:
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

[eiriktsarpalis]

I don't think the value provided by the proposed overload justifies the added API complexity. I'm going to close this issue, but feel free to reopen if you would like this conversation to continue. Thank you for your contribution!