Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[API Proposal]: List<T>.Slice #66773

Closed
Tracked by #77391
333fred opened this issue Mar 17, 2022 · 26 comments · Fixed by #75383
Closed
Tracked by #77391

[API Proposal]: List<T>.Slice #66773

333fred opened this issue Mar 17, 2022 · 26 comments · Fixed by #75383
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Milestone

Comments

@333fred
Copy link
Member

333fred commented Mar 17, 2022

Background and motivation

List<T> is the most popular collection type in .NET, but it does not support slicing, which means that it doesn't work with the C# 7 or 11 language features that use slicing (listValue[1..] and listValue is [1, .. var rest]). List<T> is additionally based on arrays, which support slicing, but doesn't support slicing itself.

In addition, we should at some point go through all of System.Collections.Generic, and figure out what types should have a Slice method added to them, but I don't really have the time to take this inventory on.

API Proposal

namespace System.Collections.Generic
{
    public class List<T>
    {
+        public List<T> Slice(int start, int length);
    }
}

API Usage

var c = new List<int>() { 1, 2, 3, 4 };
var cSlice = c[1..3];

foreach (var v in cSlice)
    Console.WriteLine(v);

Alternative Designs

There are two options:

  • Maintain the status quo and don't add a Slice method.
  • Add a Slice method that takes a System.Range. We generally haven't been following this pattern in the BCL, but we could do so if we wanted.

Risks

Will users be confused that slicing allocates a new list, and doesn't refer to locations in the original list? Arrays slicing has the same behavior, and it is the standard behavior in the BCL (Slice always returns the containing type, not a different type), but it's something to consider.

@333fred 333fred added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections labels Mar 17, 2022
@ghost
Copy link

ghost commented Mar 17, 2022

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

List<T> is the most popular collection type in .NET, but it does not support slicing, which means that it doesn't work with the C# 7 or 11 language features that use slicing (listValue[1..] and listValue is [1, .. var rest]). List<T> is additionally based on arrays, which support slicing, but doesn't support slicing itself.

In addition, we should at some point go through all of System.Collections.Generic, and figure out what types should have a Slice method added to them, but I don't really have the time to take this inventory on.

API Proposal

namespace System.Collections.Generic
{
    public class List<T>
    {
+        public List<T> Slice(int start, int length);
    }
}

API Usage

// Fancy the value
var c = new List<int>() { 1, 2, 3, 4 };
var cSlice = c[1..3];

// Getting the values out
foreach (var v in cSlice)
    Console.WriteLine(v);

Alternative Designs

There are two options:

  • Maintain the status quo and don't add a Slice method.
  • Add a Slice method that takes a System.Range. We generally haven't been following this pattern in the BCL, but we could do so if we wanted.

Risks

Will users be confused that slicing allocates a new list, and doesn't refer to locations in the original list? Arrays slicing has the same behavior, and it is the standard behavior in the BCL (Slice always returns the containing type, not a different type), but it's something to consider.

Author: 333fred
Assignees: -
Labels:

api-suggestion, area-System.Collections

Milestone: -

@Clockwork-Muse
Copy link
Contributor

Will users be confused that slicing allocates a new list, and doesn't refer to locations in the original list? Arrays slicing has the same behavior, and it is the standard behavior in the BCL (Slice always returns the containing type, not a different type), but it's something to consider.

... personally, I would prefer slices refer back to sections in the original collection. This is how ArraySegment<T>.Slice()/Span<T>.Slice() works. It's a pity slicing an array doesn't return a Span...
This means that for most collections they'd need to return a read-only collection, IReadOnlyList<T> for List<T>, for example - because you couldn't grow the sliced collection.

@333fred
Copy link
Member Author

333fred commented Mar 17, 2022

... personally, I would prefer slices refer back to sections in the original collection.

I might agree, but unfortunately the ship on this has sailed, so I think it's better to be consistent with existing behavior. In arrays, for example, it's trivially easy to observe this behavior:

int[] a = new() { 1, 2, 3 };
int[] b = a[..1];
a[0] = 2;
Console.WriteLine(b[0]); // Prints 1

If you changed those int[]s to List<int>s and the value printed differed from the array, I'd be mightily confused.

@Clockwork-Muse
Copy link
Contributor

I might agree, but unfortunately the ship on this has sailed

... for arrays, not for anything else, especially given the existing methods that do return subsections of existing data.

@333fred
Copy link
Member Author

333fred commented Mar 18, 2022

for arrays, not for anything else

For most sliceable types in the BCL. As I mentioned, the BCL has a rule that slice methods always return the containing type. The new ImmutableArray.Slice, for example, does this. The ship really has sailed here.

@huoyaoyuan
Copy link
Member

Array is somehow different than List. There are many methods of list that modifies its size. If we add a check, it can also slow down normal lists.

@stephentoub
Copy link
Member

stephentoub commented Mar 18, 2022

The ship really has sailed here.

Yes. For better or worse, our policy is that Type.Slice returns Type. Span.Slice returns Span. Array.Slice returns Array. And so on. If we add a List<T>.Slice, it should return List<T>.

I for one wish we'd gone a different direction with Array.Slice; array's have a fixed length, and it's easy to accidentally allocate. But we debated it amongst many folks at length, thinking about Array and other types that might eventually get Slice methods, and we still landed on this policy. We mitigated this by adding analyzers, e.g.

ReadOnlySpan<char> slice = data[0..5];

will highlight if data is actually an array, since it's easy to miss that this is allocating a copy:
image
We could extend said analyzers to List<T> as well, although the primary reason it was so impactful with arrays is they're implicitly castable to spans, and that's not the case (nor is it likely to ever be the case) for List<T>.

There is some prior art here in the other direction. List<T>'s non-generic predecessor ArrayList exposed a GetRange method, which returned a new ArrayList that actually wrapped the old one. But that's not possible for List<T> in a pay-for-play manner due to its methods not being virtual as they are on ArrayList, and the fact that they're non-virtual is a good thing (List<T> also exposes a GetRange, which is exactly what's being requested in this issue but with a different name, but can't wrap the old one and instead copies). If we wanted to squint and say that the T.Slice returns a T rule could return an interface variation of the original type, e.g. that List<T>.Slice could return an IList<T>, then, maybe, in which case we could make the returned IList<T> be a wrapper over the original list. But even with that, there be dragons. Beyond the fact that you couldn't assign the slice back into the original variable, you now have to deal with the fact that the slice could become invalidated, e.g.

List<T> list = new List<T>() { 1, 2, 3, 4, 5 };
var slice = list.Slice(0, 3);
list.Clear();
T item = slice[2]; // UH OH

which to me is the biggest argument against having List<T>.Slice (if it exists) return something that wraps the original mutable list.

Given all that, if we decide to add a Slice method, I think it should be as Fred proposed. The implementation would effectively be:

public List<T> Slice(int start, int length) => GetRange(start, length);

Or we teach C# to recognize additional names beyond Slice.

@sab39
Copy link

sab39 commented Apr 22, 2022

It'd be pretty easy to solve the overhead problem through an extension method and helper class (either in the BCL or in individual codebases). A simple API might be:

public static IReadOnlyList<T> View<T>(this IReadOnlyList<T> list, Range range) => new ListViewHelper(list, range);

There are a bunch of other and more flexible approaches (which I've wasted a bunch of time already pondering and refining before I realized the details don't actually matter here, just the general principle). I think it might actually be helpful for the BCL to include something to this effect, but it's simple enough to implement in individual projects or in a NuGet even if not.

@eiriktsarpalis
Copy link
Member

What would prevent us from having the slice method accept a Range parameter here?

@eiriktsarpalis eiriktsarpalis added this to the Future milestone May 3, 2022
@333fred
Copy link
Member Author

333fred commented May 3, 2022

What would prevent us from having the slice method accept a Range parameter here?

Nothing. However, our existing pattern is to use an int parameter: see ImmutableArray.Slice.

@eiriktsarpalis
Copy link
Member

Sounds good. I guess we can debate during API review. @333fred would you like to champion the proposal?

@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 4, 2022
@jnyrup
Copy link
Contributor

jnyrup commented May 4, 2022

It seems Slice(int start, int length) has the exact same behavior as the existing

public List<T> GetRange(int index, int count)

Does that raise any concerns?

@hez2010
Copy link
Contributor

hez2010 commented May 29, 2022

IList<T> cannot be used with slice pattern is really frustrating. I would like to see a Range based indexer for IList<T> being implemented in .NET 7. Note that we already have default implementation for interfaces, so adding a new indexer on existing interface won't necessarily be a breaking change.

@Atulin
Copy link

Atulin commented Jul 12, 2022

Just gonna add, that ranges not working with anything but arrays really surprised me. I think the surprise of not being able to do

var newDictionary = oldDictionary[..^4];
var newList = oldList[6..^3];
var newHashSet = oldHashSet[^19..];

and so on is more surprising than the surprise of allocating a new list. Working with lists in any capacity pretty much implies new alocations already.

@333fred
Copy link
Member Author

333fred commented Jul 12, 2022

I wouldn't expect dictionaries or hashsets to be usable with slicing. They have no order, so what does "from the start until 4 from the end" even mean on a dictionary?

@Atulin
Copy link

Atulin commented Jul 12, 2022

You're right, yeah. OrderedDictionary and SortedSet then

@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 8.0.0 Aug 17, 2022
@terrajobst
Copy link
Contributor

After several years of having the slicing syntax in C# I have to say I find myself virtually never to use it. But maybe I'm in the minority here.

I'm OK with the proposal here, but I have to say that I don't see a ton of value in it, especially because there is a GetRange() method already.

@333fred
Copy link
Member Author

333fred commented Aug 19, 2022

GetRange does not work with slicing, including the new C# 11 feature.

@halter73
Copy link
Member

After several years of having the slicing syntax in C# I have to say I find myself virtually never to use it. But maybe I'm in the minority here.

I don't think you're necessarily a minority, but you're missing out. It's great when you have a need for it, but I can't speak to how many people have a need. We've used it a fair bit though.

I personally look forward to List support for slices.

@stephentoub
Copy link
Member

We've used it a fair bit though.
I personally look forward to List support for slices.

I'm excited to see the PR into HttpParser that uses an allocating List<T> slice operator 😛

@davidfowl
Copy link
Member

davidfowl commented Aug 19, 2022

Yea, I think @halter73 means the range syntax on spans, not on List. This feature I assume is the anti performance feature 😄 .

@halter73
Copy link
Member

I was not suggesting we would literally use List slices in the Kestrel's HttpParser. Only that in general, the new C# slicing syntax is nice to use if you need to do a lot of slicing. Not everything has the performance requirements Kestrel does. I use slices a lot in Python too.

@stephentoub
Copy link
Member

Understood; I was trying (and failing) to be funny. :)

@terrajobst
Copy link
Contributor

terrajobst commented Aug 19, 2022

@333fred

GetRange does not work with slicing, including the new C# 11 feature.

Understood. My point was that I don't use the syntax, so I don't see a huge difference in calling GetRange() vs slicing syntax. Hence, I don't see a tremendous value in adding another API to do the exact same thing.

@halter73

don't think you're necessarily a minority, but you're missing out.

I have tried to love it in parsing code, especially with strings and arrays/span but I find it way more difficult to read. My brain doesn't seem to warm up to the point where I can just look at range syntax and intuit what's going on. But as I said, I might be in the minority. If we believe there is customer demand for this, we can add it.

@terrajobst
Copy link
Contributor

terrajobst commented Aug 23, 2022

Video

  • Looks good as proposed. We should consider adding to immutable collections.
namespace System.Collections.Generic
{
    public class List<T>
    {
+        public List<T> Slice(int start, int length);
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 23, 2022
@davidfowl
Copy link
Member

Allocations 📈📈📈

333fred added a commit to 333fred/runtime that referenced this issue Sep 10, 2022
Closes dotnet#66773. I took the approach of just redirecting to GetRange, and using a Theory to make all direct GetRange tests run on both GetRange and Slice (and used list pattern syntax for the Slice version to make sure that's working end-to-end).
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 10, 2022
eiriktsarpalis added a commit that referenced this issue Sep 15, 2022
* Implement List<T>.Slice

Closes #66773. I took the approach of just redirecting to GetRange, and using a Theory to make all direct GetRange tests run on both GetRange and Slice (and used list pattern syntax for the Slice version to make sure that's working end-to-end).

* PR feedback.

* Correct parameter names

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 15, 2022
@eiriktsarpalis eiriktsarpalis closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2025
@333fred 333fred closed this as completed Feb 19, 2025
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Projects
None yet
Development

Successfully merging a pull request may close this issue.