String to implement IReadOnlyList<char> instead of IEnumerable<char>

[stas-sultanov]

I would like to suggest to make class String to implement IReadOnlyList{char} instead of IEnumerable{char}.

Possibly interface should be implemented explicitly because of the Length property, which is extern and hardcoded in JIT. On the other hand Length could be deprecated and Count should be used instead.

[pharring]

@stas-sultanov Thanks for the suggestion. Can you give an example of where this would be useful?

[stas-sultanov]

Sure. Here is an example from one of my projects:
A ConvertEx class which provides a set of methods to convert data from base-n string to byte array and vice-versa. This class has a lot of methods like:

public static TryResult<String> TryToBase16String(byte[] data, int offset, int length)
public static TryResult<Byte[]> TryFromBase16String(string data, int offset, int length)

for this methods i'd like to use single method that checks arguments

    private static void CheckArguments<T>(IReadOnlyCollection<T> data, int offset, int length)
    {
        if (data == null)
            throw new ArgumentNullException(@"data");
        if (length < 0)
            throw new ArgumentOutOfRangeException(@"length", length, @"Is less than 0.");
        if (offset < 0)
            throw new ArgumentOutOfRangeException(@"offset", offset, @"Is less than 0.");
        if (offset > (data.Count - length))
            throw new ArgumentOutOfRangeException(@"data", data.Count, @"Offset plus length is greater than the length of data.");
    }

But unfortunately it is not possible because byte[] and string has only one common interface IEnumerable<T>. Use IEnumerable<T> and get length via Count() method is not an option due to performance consideration.

[pharring]

A non-generic overload of CheckArguments would solve this, of course.

[stas-sultanov]

of course it would, and that it is the way it is made now. :) but it dose not looks like a good design.

maybe example is not good enough, however i strongly believe that this small change would made coreclr more versatile.

[shmuelie]

Just a quick point but unless I'm wrong array's don't implement IReadOnlyList<T> so you'd still have to use the Enumerable.Count(). Do access the length/count of both an array and string in an efficient manner we'd have to Enumerable.Count() smarter. It already looks for types that implement ICollection or ICollection<T> and uses the Count property instead of looping. If we add a shortcut for IReadOnlyList<T> to that too then you could do what you want.

[stas-sultanov]

Arrays do implement IReadOnlyList<T>, please take a look at comments of Array class.
Or try to compile followin code:

IReadOnlyList<int> test = new [] {1, 2, 3};

the Enumerable class that implements Count method is not part of the coreclr.

[JohanLarsson]

Not much perf diff between .Count() and .Length for an array
IReadOnlyList is a nice interface though.

[shmuelie]

Ah yes, I had looked before posting but missed that comment. My bad. So then yes I'm all for making string implement IReadOnlyList<T>. Sorry about that

[stas-sultanov]

Not much perf diff between .Count() and .Length for an array

indeed, but once again:

the Enumerable class that implements Count method is not part of the coreclr.

[VladimirGnenniy]

I believe that this would be nice improvement of the architecture of the .net classes.

[terrajobst]

@KrzysztofCwalina How do you think about this? It seems that String would eventually support Span<char>, would this additional interface make sense in general?

[Lukazoid]

+1 for this, would be really useful to be able to pass strings directly as IReadOnlyList/IReadOnlyCollection parameters instead of going via .ToCharArray() or using a wrapper class.

[KrzysztofCwalina]

To me string is a primitive type. I think it's already a mistake that it implements IEnumerable.

[sharwell]

@KrzysztofCwalina I totally get that view. It leads to special cases in certain code that isn't required in, say, Java. But for better or worse, that ship has sailed. Given that string already implements IEnumerable<char> (and that isn't going to change), the question is whether or not it should also implement IReadOnlyList<char>.

💭 I think it makes sense specifically for some cases where code currently needs to call ToCharArray(). I couldn't find any clear-cut cases where the new implementation would simplify my own existing code, but especially given it already implements IEnumerable<char> I think it makes sense to implement IReadOnlyList<char> as well.

[KrzysztofCwalina]

If we implemented it explicitly and made sure the Linq extensions don't show on String, then I guess it would be fine.

[sharwell]

@KrzysztofCwalina It already implements IEnumerable<char>, so the LINQ extensions already appear.

[KrzysztofCwalina]

Hmmm, I was under the impression that we special cased it in C#.

[KrzysztofCwalina]

@jaredpar, @MadsTorgersen, I thought C# was special cased to not show Linq methods on String?

[sharwell]

In the PCL, string only implements IEnumerable (not IEnumerable<char>), so only OfType and Cast appear. When I target .NET 4.5+ desktop, I see the LINQ methods appearing in VS 2015 RC.

[MadsTorgersen]

@KrzysztofCwalina I think you just spent too much time with PCL. :-) In the full framework, LINQ always worked over string - for better or worse.

[KrzysztofCwalina]

Yeah, it's all slowly coming to me. I think we initially wanted to special case it, but then we just decided to implement IEnumerable. Then in 8.1 we added IEnumerable (why?), and we are back to the original problem that string intellisense shows all these linq methods that probably should not be used on strings.

[danmoseley]

API request -- moving to CoreFX

[danmoseley]

Issue moved to dotnet/corefx dotnet/coreclr#14336 via ZenHub