Consider making TryGetEnumeratedCount an interface method with default implementation

[agocke]

Right now TryGetEnumeratedCount is a LINQ extension method, meaning that the implementation is hard coded. It looks to see if the type implements ICollection<T> or ICollection. I could see a case where a user might not implement ICollection but want to provide an implementation of this method.

I would hope calling it is also at least as fast, maybe faster, than doing a dynamic type check.

@terrajobst @eiriktsarpalis

My proposed implementation:

public interface IEnumerable<T>
{
    bool TryGetEnumeratedCount(out int count)
    {
        count = 0;
        return false;
    }
}

and implementations matching the existing if statements for ICollection<T> and IListProvider<T>

There's a type check for ICollection in there, but ICollection and IEnumerable<T> are parallel heirarchies. If we want to include that I would move the method down to the non-generic IEnumerable (shouldn't make much of a difference otherwise).

I would consider keeping the extension method as well, since otherwise it won't appear in intellisense unless the implementation is public on the deriving type.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Right now TryGetEnumeratedCount is a LINQ extension method, meaning that the implementation is hard coded. Right now it looks to see if the type implements ICollection<T> or ICollection. I could see a case where a user might not implement ICollection but want to provide an implementation of this method.

I would hope calling it is also at least as fast, maybe faster, than doing a dynamic type check.

@terrajobst @eiriktsarpalis

Author:	agocke
Assignees:	-
Labels:	api-suggestion, area-System.Linq, untriaged
Milestone:	-

[agocke]

(Moving to 6.0 because this is likely now-or-never).

[jkoritzinsky]

If you want to get this to API review quickly, I think you need to mark this as ready-for-review and blocking

[NN---]

There is also IReadOnlyCollection with Count.

[GrabYourPitchforks]

Ref: #27183, #48239
/cc @eiriktsarpalis as he authored the in-box implementation

[eiriktsarpalis]

In order for such an addition to be useful we would need to provide overrides for common interfaces like IReadOnlyCollection<T> and ICollection<T>. However, this would break classes implementing both interfaces due to diamond ambiguity (and there are many collection classes that do this).

[GrabYourPitchforks]

@eiriktsarpalis Should this be unmarked as "ready-for-review" for the time being?

[eiriktsarpalis]

It's been scheduled for today's API review. I think the proposal needs work but as @agocke said it's now or never so might as well make a call during the meeting.

[bartonjs]

Video

We currently have apprehension about adding DIMs at this level. For this particular method we're concerned that the obvious solution would be to re-implement the methods in ICollection<T> and IReadOnlyCollection<T>, which now provides multiple non-unified implementations to concrete types; so that would manifest as a runtime breaking change for any type that already implements those two interfaces.

But we agree that we could use some better concrete guidelines here going forward.

[eiriktsarpalis]

FWIW I created a small repro that demonstrates both compile-time and runtime breaks when adding DIMs to existing interfaces:

https://github.com/eiriktsarpalis/dim-versioning-issues/

[agocke]

Awesome!

One thing to note: the current implementation of TryGetNonEnumeratedCount doesn't look like it works for IReadOnlyCollection<T>, but the above comments talk about a diamond around there. Is there something missing in the existing implementation? Or something I'm missing about the existing hierarchy?

[agocke]

Follow-up: why can't ICollection<T> implement IReadOnlyCollection<T>?

[eiriktsarpalis]

the current implementation of TryGetNonEnumeratedCount doesn't look like it works for IReadOnlyCollection

You mean the extension method? We decided not to include a type test for IReadOnlyCollection<T> in the code out of performance concerns (Enumerable.Count() doesn't test for it either) and because most of our collections implement both ICollection<T> and IReadOnlyCollection<T> (this includes immutable collections).

This constraint doesn't apply to the DIM variant, however the risk of introducing diamonds cancels out any potential for flexibility.

[eiriktsarpalis]

Follow-up: why can't ICollection implement IReadOnlyCollection?

Cf. #31001. TL;DR similar concerns about versioning apply.

[agocke]

This constraint doesn't apply to the DIM variant, however the risk of introducing diamonds cancels out any potential for flexibility.

Not sure how to interpret this. If we do the same thing here, and don't implement TryGetNonEnumeratedCount on IReadOnlyCollection does that solve the diamond problem?

[eiriktsarpalis]

It would eliminate that particular instance of a diamond, but what you're suggesting is we only implement it for ICollection<T> and nothing else. That doesn't provide many benefits over the extension method and also doesn't prevent third-party library authors from inadvertently introducing diamonds in downstream dependencies.

[bartonjs]

Thinking forward from Eirik's last comment:

partial interface IEnumerable<T>
{
    bool TryGetNonEnumeratedCount(out int count)
    {
        count = 0;
        return false;
    }
}

partial interface IReadOnlyCollection<T> : IEnumerable<T>
{
    bool IEnumerable<T>.TryGetNonEnumeratedCount(out int count)
    {
        count = Count;
        return true;
    }
}

// Nothing else in the BCL implements this method.  Very important.

This works in whatever version it's introduced in. No one can have beaten us to creating a method slot for bool IEnumerable<T>.TryGetNonEnumeratedCount(out int), so there can't be existing implementations. No diamonds. Huzzah.

However, it now becomes a potential runtime breaking change for anyone to add IReadOnlyCollection<T> to any existing, unsealed type anywhere in the .NET universe:

NugetPackageA, vAlreadyInMarket:

public interface ICanHazLength<T> : IEnumerable<T>
{
    int Length { get; }
}

NugetPackageB, vAlreadyInMarket:

public class SomethingAppropriate<T> : ICollection<T>, ICanHazLength<T>
{
    int ICollection<T>.Count => Length;
    public int Length => ...;

    ...
}

Now two things happen in either order, neither of which is a break at first:

NugetPackageA, vLater takes a bug fix for "hey, we should report we're countable":

partial interface ICanHazLength<T>
{
    bool IEnumerable<T>.TryGetNonEnumeratedCount(out int count)
    {
        count = Length;
        return true;
    }
}

NugetPackageB, vLater takes a bug fix for "SomethingAppropriate should also implement IReadOnlyCollection"

partial class SomethingAppropriate<T> : IReadOnlyCollection<T>
{
    int IReadOnlyCollection<T>.Count => Length;
}

Whichever one happened second results in a runtime error for any execution that has both libraries at the higher version (SomethingAppropriate<T> has multiple/conflicting implementations of IEnumerable<T>.TryGetNonEnumeratedCount). If NugetPackageB is targeting an old TFM (like netstandard2.0) they can't even see the DIM method to override, so their fix requires doing a split build.

The SomethingAppropriate class could also fail to load if we updated a collection type from a netstandard2.0 TFM assembly (that doesn't have a split build already) to add the IReadOnlyCollection conformance declaration and it derived from our type... so that adds new vectors where updates from dotnet/runtime cause runtime type loads for community types... and I think we're generally against that, despite our willingness to make more breaking changes than we had in .NET Framework 😄. (I don't actually know what happens if the collection came from a TFM-split package... does "my base class handled this" beat "I declared conformance to an interface that replaced it"? I assume it does at runtime, since the method slot is already populated. Don't know what the recompile behavior is, though, I could go either way...)

Maybe it wouldn't come up in this specific situation (what's a countable-enumerable that isn't an I(ReadOnly)Collection?), but it shows the room for where the breaks come in (basically, any time an interface provides an implementation for a base interface slot). Actually, the gedankenexperiment shows that the problem comes when a /second/ override appears (anywhere in the universe), but "this can be done precisely once" brings in the question of who gets to do it (same assembly only? introduced together in the same assembly?). The alternative, "no one should do this," is easier to reason about (which gave rise to the proto-guideline "DO NOT use Default Interface Methods to provide an implementation for a member from a base interface.").

That was pretty rambly, though, hopefully there were some useful thoughts in there.

[agocke]

Hmm I was thinking the same thing but came to a different conclusion: why should we assume the rest of the community have the same breaking change bar as the framework?

Yes, third party library usages could end up creating a diamond situation for their customers, but I'm not sure that should carry the same weight...

[eiriktsarpalis]

Given that this discussion has turned out to be inconclusive, it looks like TryGetNonEnumeratedCount will be shipped in .NET 6 as an extension method. So we could probably close this issue, or perhaps punt it to Future using a different name (TryGetNonEnumeratedCountV2 is still available amirite?)