csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

Champion "Permit IEnumerable<T> as the type of a params parameter"

Open gafter opened this issue 8 years ago • 49 comments

  • [ ] Proposal added
  • [ ] Discussed in LDM
  • [ ] Decision in LDM
  • [ ] Finalized (done, rejected, inactive)
  • [ ] Spec'ed

See also

  • github.com/dotnet/roslyn/issues/36
  • https://github.com/dotnet/csharplang/issues/178

Design meetings

https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-08-31.md#params-ienumerable https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-09-26.md#params-improvements

gafter avatar Feb 24 '17 20:02 gafter

I would assume that params was originally introduced so that the programmer didn't have the duality of having to write two methods just so that a piece of functionality could work with either a T or a collection of T, such as below:

public void MyMethod(string item) 
{
    MyMethod(new string[] {item});
}

public void MyMethod(string[] items)
{
    // ... Do something with items
}

While this was great, unfortunately as soon as IEnumerable<T> was introduced and it became the de facto standard base collection type we're back to the duality again:

public void MyMethod(params string[] item) => MyMethod(items);

public void MyMethod(IEnumerable<string> items)
{
    // ... Do something with items
}

Actually, since we're on the subject, I'd like to propose that not only is params IEnumerable<T> allowed, but so should params IReadOnlyList<T>, params IList<T> etc. These types are all allowed because they are all implemented by T[]. So, more formally:

params M<U> identifier is allowed if U[] : M<U> and M<U> : IEnumerable<U>.

This works because at the callsite of method

public void MyMethod(params T<U> items)

the call:

MyMethod(item1, item2, item3)

gets converted to:

MyMethod(new[] {item1, item2, item3})

So the params keyword should be allowed on any type that is assignable from an array.

This is desirable because, as an API designer, it can sometimes feel dangerous to accept IEnumerable<T> as a parameter when you expect an in-memory collection because you don't know that the given argument is actually a collection at runtime. It could be lazily-loaded like a database query, or it might even be infinite, in which case you cannot safely call Count() or even foreach.

Richiban avatar Mar 22 '17 18:03 Richiban

@Richiban I'm not sure I agree. There are many extension methods on IEnumerable<T> that will fail if it is an infinite sequence but work fine if it is implemented by an arbitrary but ultimately terminal enumerator. In practice I do not think that this has been a problem.

Also, this introduces a lot of complexity in terms of the number of possible signatures and potentially unexpected type requirements and allocations. What if I want params IImmutableSet<T>? While it would be weird if a method implicitly constructed a one from a varargs call, it is also vaguely justifiable since the callee determines the semantics.

I think params IEnumerable<T> is really all that is needed and provides enough value without support for additional types.

aluanhaddad avatar Apr 01 '17 22:04 aluanhaddad

I have so many methods that take IReadOnlyCollection<> or IReadOnlyList<>. Just support for those two would be phenomenal for me.

jnm2 avatar Apr 01 '17 22:04 jnm2

It would require teh compiler having some baked in knowledge, but i would also love "params ImmutableArray<T> array" support.

Perhaps you could have "params SomeType array" (where SomeType is a concrete type) and the compiler would look for "SomeType.Create(...)" as what it would call in that case.

CyrusNajmabadi avatar Apr 02 '17 00:04 CyrusNajmabadi

I don't see any issue with params IList<> or anything else that Array already implements.

IReadOnlyList<> would require the compiler knowing about AsReadOnly() etc.

<AConcreteType>.Create(...) would work for concrete types, but not for additional interfaces - unless you told the compiler about a specific concrete type, e.g. void Foo([ParameterList(typeof(SomeConcreteType))] params ISomeInterface bar) - but that seems ripe for abuse.

yaakov-h avatar Apr 02 '17 00:04 yaakov-h

@CyrusNajmabadi that certainly would be useful but if infrastructure is going to be put in place for using such patterns to instantiate params collections, would it make sense to make this part of a larger feature supporting collection literals?

There have been many requests for a collection literals as well as for JSON literals (collection literals) so if such a pattern is codified it would be great it were generalized to enable these scenarios as well.

Personally I think collection literals would be great, but only if you could customize the target type to be, as you say, ImmutableArray.

I don't think JSON literals make any sense at all but people ask for them.

aluanhaddad avatar Apr 02 '17 01:04 aluanhaddad

I think that talk of the compiler automatically inserting calls to extension methods is taking it a bit far...

What is very easy to understand and (I'm assuming) easy to implement would be that anywhere you could previously write this:

public void MyMethod<U>(M<U> items)
{
	// Do stuff...
}

public void Callee()
{
	MyMethod(new [] { 1, 2, 3 });
}

You can now write this:

public void MyMethod<U>(params M<U> items)
{
	// Do stuff...
}

public void Callee()
{
	MyMethod(1, 2, 3);
}

and there's no further magic than that.

Richiban avatar Apr 03 '17 16:04 Richiban

@Richiban

So the params keyword should be allowed on any type that is assignable from an array.

I think accepting any type that is assignable from T[] is not possible because we can't always determine T (so the interface must have a generic parameter).

alrz avatar Dec 29 '17 20:12 alrz

~~if that's desirable, it's pretty easy to implement, I should see if I can handle stackalloc optimization for Span (https://github.com/dotnet/csharplang/issues/535).~~ scratch that, Span doesn't implement any interface because it's a ref struct.

alrz avatar Dec 29 '17 20:12 alrz

there is a special case that does not clearly translate to IEnumerable,

(EDIT: used two null args to trigger array creation)

void M(params string[] args) {}
M(null, null); // OK args = new string[] { null, null }

void M(params IEnumerable args) {}
M(null, null); // ERROR? args = new[] { null, null } won't help

alrz avatar Jan 04 '18 12:01 alrz

void M(params string[] args) {}
M(null); // OK args = new string[] { null }

That is not how it works. A method will be called in unexpanded (normal) form preferentially to being called in expanded form. M(null) will pass a null array.

gafter avatar Jan 04 '18 16:01 gafter

Given the variety of solutions now proposed, and especially considering the latest #1366 issue, would it make sense for the compiler to support any type that supports collection initialization?

This would mean we could put params in front of any type. public void SomeMethodCall(params CustomType p) { ... }

The callsite is where the magic happens. Anywhere it would be possible to type: SomeMethodCall(new CustomType {p1, p2, p3})

It would also be possible to type: SomeMethodCall(p1, p2, p3)

Or is that crazy?

EDIT: Actually, it is crazy, and doesn't really help with all the interfaces. Ignore me!

rossdempster avatar Mar 12 '18 08:03 rossdempster

It would require teh compiler having some baked in knowledge, but i would also love "params ImmutableArray array" support.

I like that too, though dotnet/runtime#22928 would give most of the benefits with params ReadOnlySpan<T>.

Speaking of which, I'm in favor of params ReadOnlySpan<T>, and even having just that one instead of params Span<T> if it has to be just one or the other. It doesn't feel like the ability to write back to the original array is an important part of the params story, and the fact that you can do that today is probably just an unfortunate consequence of the tools that were available during the original .NET Framework design.

I'd be perfectly satisfied if all I had were:

  • params T[] so existing things don't break
  • params IEnumerable<T> for most things
  • params ReadOnlySpan<T> for new things where I worry about performance at all

Given those, even if I found myself in a situation where I could really, really benefit from knowing the count in advance if available without forcing ReadOnlySpan<T> (and this feels like a stretch), this looks OK enough:

void Do(params IEnumerable<int> args)
{
    int count;
    switch (args)
    {
        case null:
            throw new ArgumentNullException(nameof(args));

        case ICollection<int> coll1:
            count = coll1.Count;
            break;

        case IReadOnlyCollection<int> coll2:
            count = coll2.Count;
            break;

        default:
            List<int> argsList = args.ToList();
            args = argsList;
            count = argsList.Count;
            break;
    }

    // count is now available to us
    // same idea works for IList<T> / IReadOnlyList<T> if the indexer matters too
}

airbreather avatar Mar 17 '18 12:03 airbreather

I can't tell you how much I would love this feature.

TonyValenti avatar Mar 20 '18 10:03 TonyValenti

I'm working on a prototype for this (https://github.com/dotnet/roslyn/pull/24080). here's my open questions for LDM to confirm:

  • do we want to support all array generic interfaces? (supporting all these types is not nontrivial)
  • do we want to support params Span<T> and params ReadOnlySpan<T>? (https://github.com/dotnet/csharplang/issues/535) (we can reuse stackalloc initializer lowering in codegen where possible, otherwise we fallback to arrays)
    • this probably need dotnet/roslyn#22046 to be implemented first.
  • do we want to forbid conflicting params overloads with identical element types? (otherwise all overloads would be always applicable in the expanded form and therefore ambiguous; note: the compiler wouldn't give ambiguous result for that case "for free", if we permit that, the overload resolution should be adjusted accordingly)

alrz avatar Mar 20 '18 14:03 alrz

(supporting all these types is not nontrivial).

Do you mean that it's trivial? Or that it's nontrivial? The 'not' in there is throwing me off.

CyrusNajmabadi avatar Mar 20 '18 18:03 CyrusNajmabadi

do we want to support params Span<T> and params ReadOnlySpan<T>

I think this would be fantastic. One of hte primary concerns around params in teh first place (and thus extending it to more situations) is that it incurs a array allocation. If we could have nice synchronous variadic APIs that didn't require allocations, i think that would be fantastic for the ecosystem.

CyrusNajmabadi avatar Mar 20 '18 18:03 CyrusNajmabadi

Do you mean that it's trivial? Or that it's nontrivial?

I meant the former; just not wanted to stress on "trivial". I've implemented that part by relaxing some checks in the compiler (and because an implicit conversion exists, there was no need to adjust lowering), but since it's not thoroughly tested I wasn't confident that's all it would take to support it.

alrz avatar Mar 20 '18 19:03 alrz

I meant the former; just not wanted to stress on "trivial".

In that case, my preference would be to support them all. It's reasonable for me to say params IReadOnlyList<int> or params IList<int> and whatnot. And, in most cases, where one of my APIs is already taking one of those types, i would like to be able to just add "params" to it and enable these scenarios.

The one thing that's a pity is that i would really love params ImmutableArray<T>. But that would def take more interesting work to figure out how to supply that sort of thing.

CyrusNajmabadi avatar Mar 20 '18 23:03 CyrusNajmabadi

params ImmutableArray<T>

we could lower to collection initializers for params on arbitrary types, but currently, there's no mechanism that enables it for a type like ImmutableArray<T>. if/when we extend collection initializers to work with ImmutableArray<T> (probably through some convention), we can add params support on top of it.

alrz avatar Mar 20 '18 23:03 alrz

@alrz The worst part is that collection initializer on ImmutableArray<T> compiles, but doesn't actually work (it throws NullReferenceException, since new ImmutableArray<T>() a.k.a. default(ImmutableArray<T>) is not really a valid value).

svick avatar Mar 21 '18 00:03 svick

@alrz The worst part is that collection initializer on ImmutableArray<T> compiles, but doesn't actually work (it throws NullReferenceException, since new ImmutableArray<T>() a.k.a. default(ImmutableArray<T>) is not really a valid value).

Note: it wouldn't work even if it started with an empty instance. Collection initializers work by calling .Add for each element added. But adding to an immutable array does nothing to the original instance.

No matter what, we'd need some sort of new system that recognizes, by some system, the appropriate way to actually initialize things like ImmutableArray.

CyrusNajmabadi avatar Mar 21 '18 01:03 CyrusNajmabadi

Collection initializers work by calling .Add for each element added. But adding to an immutable array does nothing to the original instance.

If collection initializers could recognize the return type and call Add methods on that instance we could make this work somehow, but I think that's a breaking change now. (and still it wouldn't work with the current implementation because new ImmutableArray<T>().Add(e) throws -- it's not supposed to be used that way.)

we'd need some sort of new system that recognizes, by some system, the appropriate way to actually initialize things like ImmutableArray.

Actually, ImmutableArray<T> is carefully designed to not expose the underlying array and therefore has some specific properties that makes it really hard to define a general convention for its initialization. IMO it's something that we should have in the language in the first place, (e.g. https://github.com/dotnet/csharplang/issues/955 or more generally #421)

That being said, I think it wouldn't worth it to support arbitrary types for params parameters or even worse, to make ImmutableArray a well-known type in the compiler.

alrz avatar Mar 21 '18 10:03 alrz

@alrz The discussion you're having now sounds like it would be eminently relevant to my recent proposal for target-typed collection literals https://github.com/dotnet/csharplang/issues/1399. Namely around how the compiler might figure out how to construct a collection it doesn't recognise explicitly.

Perhaps the features even have enough overlap to be merged?

Richiban avatar Mar 21 '18 10:03 Richiban

I formulated an alternative proposal for params improvement over here, based on observations related to revamping expression tree APIs, which suffer from defensive copies (for sake of guaranteeing immutability after construction) of params Expression[] arrays passed to factory methods.

bartdesmet avatar Jun 22 '18 02:06 bartdesmet

@bartdesmet

A good example is the System.Linq.Expressions API where factory methods accept a params Expression[] but need to create a defensive copy of the array to ensure immutability (using an internal TrueReadOnlyCollection<T> type). This causes excessive allocations, even if the params array was constructed by the compiler, which ensures only the callee can observe the reference to the array, so external mutation by the caller is not possible.

To avoid defensive copy to ensure immutability, you would need to have sealed-class collection type in the method signature which I don't believe is a good idea for public API as it discloses internal workings of said method.

jveselka avatar Aug 29 '18 11:08 jveselka

I, for one, see no urgency in opening flood gates (collection initializers and whatelse) just to have only IEnumerable<> supported. It's the overwhelmingly most useful and broadly applicable situation, imo.

bjorn-ali-goransson avatar Jul 23 '19 13:07 bjorn-ali-goransson

I find that params has constraints that are not useful in practice, namely that it has to be at the end of the parameter list, which eliminates the possibility of default parameters. The main inconvenience I find often is a single item vs a collection that in either case should be handled the same.

public void Foo(int number, bool flag=true) {
   Bar(new int[] {number}, flag);
}

public void Bar(IEnumerable<int> numberSet, bool flag=true) {
    foreach (int num in numberSet)
       // ...
}

So what I'd like is a prefix that allows auto wrapping a single item into an IEnumerable such that I do not need that first wrapper method Foo. The name for that prefix could be params since it is similar in behavior, but better should be multi since it is a short word that indicates the idea that the following parameter has multiple API signatures. The above code would become:

public void Bar(multi IEnumerable<int> numberSet, bool flag=true) {
    foreach (int num in numberSet)
       // ...
}

Since there is no restriction to have the multi parameter at the end, I could even use more than one multi parameter:

public void Bar(multi IEnumerable<int> numberSet, multi IEnumerable<string> labelSet, bool flag=true) {
    foreach (int num in numberSet)
       // ...
}

And of course it would be great if the multi keyword could be prefixed to any type of container/set to auto convert a single item into a container/set with having that one item. So besides IEnumerable also for arrays, lists, etc.

IngmarBitter avatar Jan 30 '20 19:01 IngmarBitter

@IngmarBitter What would this do?

public void Method()
{
    Method1(true, false); // Does boolSet have 2 elements or is flag false?
    Method1(false); // Does boolSet have an element or is flag false?
}

public void Method1(multi IEnumerable<bool> boolSet, bool flag=true) {

}

DanFTRX avatar Jan 30 '20 19:01 DanFTRX

@DanFTRX your example would be:

public void Foo() {
    Bar(true, false); // same as {true}, false
    Bar(false); // same as {false}, true
}
public void Bar(multi IEnumerable<bool> boolSet, bool flag=true) {
}

I suppose I was not clear enough before that I do not find useful the params feature of collecting multiple comma separated parameters into a set. multi should not do that. A multi parameter is always a single parameter. The only flexibility it adds is that a single element is converted into a set with that element. The case that someone wants to explicitly declare the set of multiple elements at the point of method call is rare. I that case one can do:

Bar(new bool[] {true, true, false}, false); 

PS: thanks for the syntax coloring info

IngmarBitter avatar Jan 30 '20 20:01 IngmarBitter