Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Open issues for collection expression construction #7542

Closed
cston opened this issue Sep 18, 2023 · 6 comments
Closed

Open issues for collection expression construction #7542

cston opened this issue Sep 18, 2023 · 6 comments

Comments

@cston
Copy link
Member

cston commented Sep 18, 2023

Open issues: collection expression construction

See #7541.

Use .ctor(int capacity)?

For collection initializer types, should the compiler use a .ctor(int capacity) if the length of the collection is known at compile time and the constructor overload exists?

MyCollection<int> x = [a, b, c]; // x = new MyCollection<int>(3); x.Add(a); ...

class MyCollection<T> : IEnumerable<T>
{
    public MyCollection<T>() { /*...*/ }
    public MyCollection<T>(int capacity) { /*...*/ }
    public void Add(T t) { /*...*/ }
}

Use AddRange() or other patterns?

For collection initializer types, construction involves calling the applicable Add() methods which may be instance or extension methods.

For spread elements, should the compiler invoke applicable AddRange() methods if available? If so, should extension methods be supported or just instance methods? Should the compiler always use AddRange() if available, or can the compiler choose?

MyCollection<int> x = [a, b, c];
MyCollection<int> y = [..x, d]; // y = new(); y.AddRange(x); y.Add(d);

class MyCollection<T> : IEnumerable<T>
{
    // ...
    public void Add(T t) { /*...*/ }
    public void AddRange(IEnumerable<T> e) { /*...*/ }
}

Should the compiler use a handful of other common patterns if available on the collection type or spread type?

From construction:

  • For each element in order:

    • ...
    • If the element is a spread element then one of the following is used:
      • An applicable GetEnumerator instance or extension method is invoked on the spread element expression ... .
      • An applicable AddRange instance or extension method is invoked on the collection instance with the spread element expression as the argument.
      • An applicable CopyTo instance or extension method is invoked on the spread element expression with the collection instance and int index as arguments.
  • During the construction steps above, an applicable EnsureCapacity instance or extension method may be invoked one or more times on the collection instance with an int capacity argument.

Avoid intermediate buffer if length is known?

For target types other than collection initializer types, construction involves element assignment to an instance of a well-known type (an array, span, or list). Ideally for these cases, the compiler should avoid creating an intermediate buffer during construction when the collection has a known length at runtime – that is, when all spread elements have an appropriate Length or Count property. However, that means evaluating all elements (up to the last spread element) before allocating the collection. For these cases:

Should the compiler guarantee that a temporary buffer is avoided when all spread elements have a known length? Or should we use heuristics to avoid creating a temporary buffer under a limited set of conditions (for instance, no more than 8 elements)?

int[] a = [v1, v2, v3];
int[] b = [v4, /* ... */, v100, ..a];

// _tmp1 = v1;
// ...
// _tmp100 = v100;
// _tmp101 = a;
//
// int[] b = new int[100 + _tmp101.Length];
// b[0] = _tmp1;
// ...

Evaluate all elements before calling Add() or AddRange()?

[Related to the previous questions.]

Should the compiler use .ctor(int capacity) for collection initializer types when the collection expression contains spread elements, or only when there are no spread elements? The question is really: Should Add() and AddRange() calls be interleaved with element evaluation (which matches classic collection initializer behavior), or should all elements be evaluated before any are added?

MyCollection<int> z = [..F1(), F2()];

// z.AddRange(F1());
// z.Add(F2());

// -or-

// _tmp1 = F1();
// _tmp2 = F2();
// z.AddRange(_tmp1);
// z.Add(_tmp2);

Type inference: lower bound inference from spread element iteration type

From type inference, a lower bound inference is made from the spread element iteration type.

Confirm this is the expected behavior.

byte[] x = [1, 2];

Print([..x]);     // Print<byte>(byte[])
Print([..x, 3]);  // Print<int>(int[])

static void Print<T>(T[] args) { }

Overload resolution: no fallback to better conversion target

Overload resolution was previously discussed LDM-2023-08-14, with WG follow-up covered in email.

From overload resolution, there is no fallback to a better conversion target if the ref struct and non- ref struct types are ambiguous.

Confirm this is the expected behavior.

static void ArrayDerived(Span<object> value) { }
static void ArrayDerived(string[] value)     { }

ArrayDerived([string.Empty]); // error: ambiguous
static void TwoArgs(object x, string[] y)     { }
static void TwoArgs(string x, Span<object> y) { }

TwoArgs("3", ["4"]); // ok: TwoArgs(string x, Span<object> y)

Collections and well-defined behavior

Collection expressions are assumed to be well-behaved:

  • Elements are evaluated left to right.
  • Length and Count properties return the number of items in the spread element expression.
  • Iteration of a spread element expression is not affected by evaluation of subsequent elements or iteration of other spread elements.

The compiler is free to change how it emits construction of collection instances assuming collection expressions are well-behaved. If the collection expression is not well-behaved, the result is undefined.

From construction:

A spread element may be iterated before or after the subsequent elements in the collection expression are evaluated.

Collection expression conversion existence

Currently, the spec says this for when a type is a valid target type for a collection expression:

  • To a struct or class type T that implements System.Collections.IEnumerable where:
    • The type contains an applicable instance constructor that can be invoked with no arguments.
    • For each expression element Ei there is an applicable instance or extension method Add for T invocable with a single argument Ei.
    • For each spread element Si there is an applicable instance or extension method Add for T invocable with a single argument of the iteration type of Si.

This is very broad, and introduces a problem that we redesigned interpolated string handlers to avoid: whether or not a conversion exists depends on a successful bind of construction of the type. This makes the presence of the conversion brittle to user errors, both from type authors and from errors user's own code; it also significantly complicates the implementation of the conversion logic. We'd like to propose the following rules instead:

  • To a struct or class type that implements System.Collections.IEnumerable<T> where:
    • For each element Ei there is an implicit conversion to T.
  • To a struct or class type that implements System.Collections.IEnumerable and does not implement System.Collections.IEnumerable<T>.

This has a few benefits:

  • It removes construction binding from the calculation. User types that are missauthored (missing an appropriate Add method, for example) now fail for that reason, instead of being silently discarded by overload resolution.
  • It aligns the calculation for types that implement IEnumerable<T> with the calculation for IEnumerable<T> itself.

However, the second rule may be overly broad, and cause ambiguities when old, non-IEnumerable<T> APIs are mixed with new APIs. Therefore, another option that we think may be viable is simply removing the second rule; this would mean that collection expressions cannot be used for collection types that implement IEnumerable but not IEnumerable<T> and do not have CollectionBuilderAttribute applied to them.

Nature of the constructor check

If we decide to keep the constructor check, we should decide whether to use a specific lookup or just regular binding.
For instance, if the constructor takes an optional parameter, should we recognize it as a proper constructor?
If we stick with regular binding rules, then any constructor that can be invoked without argument would be recognized. That is equivalent to the user typing new type().

If we decide to recognize such optional parameters, there is the question of whether use-site errors on those parameter types should count as a failure to bind the constructor or not. There may also be some scenarios with [RequiresCompilerFeature].

@CyrusNajmabadi
Copy link
Member

For spread elements, should the compiler invoke applicable AddRange() methods if available?

We should also be flexible enough to support x.CopyTo(destArray, index) when we're writing into an array. We should also support .ToArray() directly when just spreading a different collection directly to an array.

@CyrusNajmabadi
Copy link
Member

which matches classic collection initializer behavior

This is def a non-goal. Class collection initializers don't do a good job perf-wise, which is what we're trying to avoid here. :)

@CyrusNajmabadi
Copy link
Member

Should the compiler guarantee that a temporary buffer is avoided when all spread elements have a known length? Or should we use heuristics to avoid creating a temporary buffer under a limited set of conditions (for instance, no more than 8 elements)?

We should avoid a temporary buffer if:

  1. statically we know we can determine the capacity. So this means either no spreads. Or all spreads have a statically available .Length/.Count we can call.
  2. dynamically determining the capacity succeeds. This means that TryGetNonEnumeratedCount succeeds on all spreads that do not statically expose a .Length/.Count but which are IEnumerable<T>.

In both these cases, we can figure out the correct capacity and initialize the destination accordingly so we do not do anything wasteful.

The only times we should make temporary storage are when we have spreads without staticly or dynamically determinable counts. In that case, we can't really do anything better other than at least setting the initial capacity to the number of fixed elements. i.e. if we have 100 fixed elements, no point using default capacity, sicne it will have to double 6 times to reach that size.

@karakasa

This comment was marked as resolved.

@CyrusNajmabadi
Copy link
Member

@karakasa deadlines meant it was more important to get a correct impl in first. The team is continuing to work on perf optimizations. And, of course, prs welcome :-)

@karakasa
Copy link

deadlines

thank you. didn't realize that. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants