dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.91k stars 4.63k forks source link

List<T> should have Index & Range method overloads #321

Open monoclex opened 4 years ago

monoclex commented 4 years ago

Rationale and Usage

Removing the last item of a list is currently list.RemoveAt(list.Count - 1);. It currently isn't possible to use the hat operator to do such.

Proposed API

public class List<T>
{
    // for brevity, only the proposed APIs will be listed
    public List<T> this[Range range] { get; }
    public int BinarySearch(Range range, T item, IComparer<T>? comparer);
    public void CopyTo(Range sourceElements, T[] array, Index destinationIndex);
    int FindIndex(Range lookIn, Predicate<T> match);
    int FindIndex(Index startIndex, Predicate<T> match);
    int FindLastIndex(Range lookIn, Predicate<T> match);
    int FindLastIndex(Index startIndex, Predicate<T> match);
    List<T> GetRange(Range range);
    int IndexOf(T item, Range lookIn);
    int IndexOf(T item, Index startIndex);
    void Insert(Index index, T item);
    void InsertRange(Index index, IEnumerable<T> collection);
    int LastIndexOf(T item, Index startIndex);
    int LastIndexOf(T item, Range lookIn);
    void RemoveAt(Index index);
    void RemoveRange(Range range);
    void Reverse(Range range);
    void Sort(Range range, IComparer<T>? comparer);
}

Details

// hitting on points 1 and 3 in Details with this:
public interface IList<T>
{
    // existing things omitted for brevity
    void Insert(Index index, T item) => Insert(index.GetOffset(Count), item);
    void RemoveAt(Index index) => RemoveAt(index.GetOffset(Count));
}

Open Questions

Pull Request

N/A

Updates

N/A

danmoseley commented 4 years ago

@tarekgh I think maybe you were involved in the discussion of whether to add these back when you added Index and Range into the framework?

tarekgh commented 4 years ago

CC @terrajobst @jaredpar @GrabYourPitchforks @stephentoub

We need to look at what make sense to add and what is not. for example, in the current proposal, if we add the methods Slice to List, the range indexer

public List<T> this[Range range] { get; }

will be automatically added to the type without explicit exposure. Also

List<T> GetRange(Range range);

would be redundant to the indexer.

I would say, this need to filtered to decide which once make sense to include and try to avoid adding APIs to just use Index or Range.

https://github.com/jaredpar/csharplang/blob/8dd672612693932d86a4d065554bf4f97ac4a23e/proposals/index-range-changes.md

terrajobst commented 4 years ago

Agreed. @tarekgh, could you work with the @SirJosh3917 to get this API ready?

weitzhandler commented 4 years ago

And Collection<T>s. And merely anything that implements IList<T>. Maybe this can be achieved via default-implemented interfaces or extension methods.

We can only enjoy a fraction of the ranges power because it's limited to arrays.

Thaina commented 4 years ago

I want to add RemoveRange(int index) to remove from index to end

monoclex commented 4 years ago

Any update on this? Am I suppose to get in contact with @tarekgh someway or another?

tarekgh commented 4 years ago

@SirJosh3917 we are still considering investigating this later. Is this blocking you by any means in the current time?

monoclex commented 4 years ago

No, it's not critical to any of my development currently. I'd be happy to help get this going if there's anything I can do, but I understand if this has to be put on the back-burner.

MuleaneEve commented 4 years ago

I agree with the idea of adding this API using default interface methods.

weitzhandler commented 4 years ago

Never gonna happen with an interface. We're not looking for a shortcut, we're looking for a way to minimze the number of events raised.

MuleaneEve commented 4 years ago

My bad, I forgot about that requirement.

TonyValenti commented 3 years ago

I'd really like this feature too.

eiriktsarpalis commented 3 years ago

We should almost certainly add this feature given that we've already done it for IEnumerable<T> via https://github.com/dotnet/runtime/issues/28776. Though I'm not sure it will make it for .NET 6.

stephentoub commented 3 years ago

We should almost certainly add this feature

This is a lot of overloads on one of the most prominent collection types. This can have a non-trivial impact on app size, in particular for AOT. Let's be very thoughtful about exactly what we're adding and why (and where, e.g. could support be added instead for any IList<T> via extension methods). And if we find we're having to proliferate such Index/Range-based methods on all our collection types, we should work with the C# LDM to revisit decisions about enabling the language to automatically lower use of the index/range syntax for use with existing int index and int offset, int count-based overloads of arbitrary methods.

cc: @jkotas

eiriktsarpalis commented 3 years ago

Agreed that we probably don't need all the methods as listed in the OP, however at a bare minimum we should include an indexer overload accepting Index (which like you said could be addressed at the language level).

stephentoub commented 3 years ago

however at a bare minimum we should include an indexer overload accepting Index

We don't need to: the language already handles that for us (specifically for this[int]): https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA+ABATARgLABQGADAAQY4oDchhGAzOVqQMKkDehp35jAKqQCyAHj4A+ABQAZAJYBnAC6ixpADbyFAGlIBJAHYATGAlIyAlKQC8K9YoDaMgLo0CAXyA==

JaThePlayer commented 2 years ago

With List pattern matching being previewed in c#11, having this[Range range] would be really useful to have for consistency with arrays, consider this code which works for arrays but doesn't for lists: image

palladin commented 1 year ago

Recently, I encountered the same issue with pattern matching on List, and the error message was a bit cryptic.

    public int M(List<int> xs) {
        return xs switch
        {
            [32, ..var ys, 12] => ys.Count,
        };
    }

error CS1503: Argument 1: cannot convert from 'System.Range' to 'int'

and because ys was inferred as int error CS1061: 'int' does not contain a definition for 'Count' and no accessible extension method 'Count' accepting a first argument of type 'int' could be found (are you missing a using directive or an assembly reference?)

eiriktsarpalis commented 1 year ago

@stephentoub @CyrusNajmabadi it seems like an omission that pattern matching doesn't work with List. Should we consider adding a Range overload to this (and other list types) to unblock this scenario?

eiriktsarpalis commented 1 year ago

@palladin have you tried running your example using the latest C# preview? This works as expected for me:

Console.WriteLine(M(new() { 32, 1, 1, 1, 12 })); // 3

static int M(List<int> xs)
{
    return xs switch
    {
        [32, .. var ys, 12] => ys.Count,
        _ => -1
    };
palladin commented 1 year ago

@eiriktsarpalis Great! I was using SharpLab's latest main (26 Jul 2023).