Determine element type for write-only CollectionBuilder collection types

[jcouv]

This PR is a proposed change allowing write-only CollectionBuilder collection types (proposal) and rephrasing the existing spec mostly in terms of element type instead of iteration type.

The plan is to review this proposal with the collection expression and params working groups, then review in LDM before merging this PR if approved.
 
Open issue: is this treated as a C# 12 bugfix or a C# 13 feature? (this affects whether we should update the C# 12 collection expression speclet)

Tagging @CyrusNajmabadi @cston @RikkiGibson @AlekseyTs for review.

[CyrusNajmabadi]

nit: i don't mind it. but my mind is being blown a bit with the fact that these create methods are often generic, and thus inference is involved. I'm not sure if anything needs to be stated about that, or if this clearly just falls out. #Closed

[jcouv]

Updated to clarify (the single create method must be non-generic)

[CyrusNajmabadi]

seems unfortunate that this is restated in params-collections, instead of being able to reference collection-exprs (but that's outside the scope of this pr).

[cston]

Won't the create method be a generic method (in a non-generic containing type) if the collection type is generic? #Closed

[jcouv]

Could you elaborate? The create method is in a builder type, so I didn't follow how the collection type being generic is relevant.

[cston]

I would expect a create method for a collection type to have the same arity as the collection type, so if the collection type is generic, the create method would be generic. For instance:

[CollectionBuilder(typeof(MyCollectionBuilder), "Create")]
class MyCollection<T> { }

class MyCollectionBuilder
{
    public static MyCollection<T> Create<T>(ReadOnlySpan<T> items) { ... }
}

[jcouv]

Thanks. I'll remove the "non-generic" portion. By the point we consider create methods, they will have been fully substituted.
FYI @CyrusNajmabadi

[CyrusNajmabadi]

wfm

[AlekseyTs]

if there is a single non-generic create method

This contradicts

The arity of the method must match the arity of the collection type.

from the list above, and everything "falls apart" I think. #Closed

[AlekseyTs]

• The builder type must be a non-generic class or struct.

This bullet can be pulled out of the fork, that I am suggesting to add, and shared #Closed

---

Refers to: proposals/csharp-12.0/collection-expressions.md:168 in dc84f02. [](commit_id = dc84f02, deletion_comment = False)

[AlekseyTs]

At this point I suggest to create a "fork".

This a very "rough" version of what it might look like:

If the collection has an iteration type determined like ...,
then the element type is the iteration type and the create method is determined as the list below states (the list in its original form with "iteration type" replaced with "element type").

Otherwise, a create method satisfying the following requirements is looked up (some of the requirements are the same as from the previous branch, but that is Ok, I think. What we are primarily after is the clarity):

• The method must be defined on the builder type directly.
• The method must be static.
• The method must be accessible where the collection expression is used.
• The arity of the method must match the arity of the collection type.
• The method must have a single parameter of type System.ReadOnlySpan<E>, passed by value
• There is an identity conversion, implicit reference conversion, or boxing conversion from the method return type to the collection type.

Now we need to describe how the inference of the "element type" from the method is done. Basically E is the element type of the collection type constructed with method's type parameters. To "go back" to the definition of the collection type, we simply need to substitute type parameters of the method with type parameters of the collection type in E. What we get after this substitution is the "element type" inferred from the method

Now we can say, if there is a single create method that satisfies the requirements, this is the crate method and the collection element type is the element type that we inferred from it.
#Closed

[cston]

I would rather we share the common bullets between the two cases if it can be done simply enough, so it's clear where the differences are. If the only difference between the bullets is the second half of "The method must have a single parameter...", consider separating out the second half that bullet and sharing the rest.

[AlekseyTs]

I think it is better to separate the "iteration case" and the inference case at the top. trying to fit that into the middle is going to be confusing. Saying something twice is not a big deal, in my opinion.

In any case, I recommend starting with the separate "paths", get those into the right shape and clarity. Then we can see what can be unified and how to do that.

[AlekseyTs]

Ok, I think I was able to get some unification between the two cases. So, starting from this line (inclusively) and through the following line (inclusively)

Method overloads on the builder type with distinct signatures are ignored. Methods declared on base types or interfaces are ignored.

I suggest to say the following instead:

The builder type must be a non-generic class or struct.

First, the set of applicable create methods CM is determined.
It consists of methods that meet the following requirements:

• The method must have name specified in the [CollectionBuilder(...)] attribute.
• The method must be defined on the builder type directly.
• The method must be static.
• The method must be accessible where the collection expression is used.
• The arity of the method must match the arity of the collection type.
• The method must have a single parameter of type System.ReadOnlySpan<E>, passed by value.
• There is an identity conversion, implicit reference conversion, or boxing conversion from the method return type to the collection type.

Methods declared on base types or interfaces are ignored and not part of the CM set.

If the CM set is empty, then the collection type doesn't have element type and doesn't have create method. None of the following steps apply.

Second, an attempt is made to determine iteration type of the collection type from a GetEnumerator instance method or enumerable interface, not from an extension method.

• If the process results in an unambiguous iteration type, then the element type of the collection type is the iteration type. If only one method among those in the CM set has an identity conversion from E to the element type of the collection type, that is the create method for the collection type. Otherwise, the collection type doesn't have create method. None of the following steps apply.
• If the process results in an ambiguous iteration type, then the collection type doesn't have element type and doesn't have create method. None of the following steps apply.

Third, an attempt is made to infer the element type.
If the CM set contains more than one method, the inference fails and the collection type doesn't have element type and doesn't have create method.

Otherwise, type E1 is determined by substituting type parameters of the only method from the CM set (M) with corresponding collection type type parameters in its E. If any generic constraints are violated for E1, the collection type doesn't have element type and doesn't have create method. Otherwise, E1 is the element type and M is the create method for the collection type.

An error is reported if the [CollectionBuilder] attribute does not refer to an invokable method with the expected signature.

[jnm2]

The builder type must be a non-generic class or struct.

Why not allow any non-generic type which can declare methods to declare the static method?

[AlekseyTs]

The builder type must be a non-generic class or struct.

Why not allow any non-generic type which can declare methods to declare the static method?

The restriction is taken from the current state of the document. It used to be one of the bullet points.
Also, I am not sure what scenario do you have in mind? Could you please elaborate? Are you talking about interfaces?

[jcouv]

@AlekseyTs Thanks for the alternate draft for "create method" section.
The proposed structure looks good to me overall and it fixes the generic scenario which I didn't handle well.

However, I don't plan to incorporate the concept of "unambiguous iteration type" at this point. I'll stick with existing binary for now (having an iteration type or not) and will include an open question with an example where the difference is observable, so that we can discuss it.

[AlekseyTs]

Done with review pass (commit 5)

[AlekseyTs]

@cston

• The arity of the method must match the arity of the collection type.

Is this accurate for the case when collection type is nested into another generic type? Shouldn't enclosing type type parameters be included as well? That is assuming that the builder type is not declared "next to" the collection type. #Closed

[cston]

The arity of the method must match the arity of the collection type and any containing types.

That is assuming that the builder type is not declared "next to" the collection type.

The builder type can be nested but not generic.

[AlekseyTs]

It looks like implementation is doing the right thing, so the spec needs an adjustment.

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

class C<T>
{
    [CollectionBuilder(typeof(MyCollectionBuilder1), nameof(MyCollectionBuilder1.Create))]
    public class MyCollection1
    {
        public IEnumerator<T> GetEnumerator() => throw null;
    }
}

class MyCollectionBuilder1
{
    public static C<T>.MyCollection1 Create<T>(ReadOnlySpan<T> items) => null;
}

class Program
{
    static void Main()
    {
        C<int>.MyCollection1 x = [1,2,3];
    }
}

[AlekseyTs]

The builder type can be nested but not generic.

Not quite, the following doesn't work:

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

class C<T>
{
    [CollectionBuilder(typeof(MyCollectionBuilder1), nameof(MyCollectionBuilder1.Create))]
    public class MyCollection1
    {
        public IEnumerator<T> GetEnumerator() => throw null;
    }

    class MyCollectionBuilder1
    {
        public static MyCollection1 Create(ReadOnlySpan<T> items) => null;
    }
}


class Program
{
    static void Main()
    {
        C<int>.MyCollection1 x = [1,2,3];
    }
}

I guess

• The builder type must be a non-generic class or struct.

should be adjusted/clarified as well, i.e. non-generic all the way up.

[cston]

• The builder type must be a non-generic class or struct.

Yes, non-generic here is intended to mean the type and all containing types are non-generic.

[cston]

This final bullet seems to indicate if the iteration type cannot be determined, we fail, rather than falling through to the "Third, ..." step below.

[cston]

If the final bullet is removed, it seems the previous item could be written without a bullet.

[AlekseyTs]

by substituting the type parameters of the collection type for the type parameters of M in E

This looks backwards and doesn't match the wording that was suggested. E is a type in M's signature. it cannot refer to "type parameters of the collection type", there is nothing to substitute there in the suggested way. #Closed

[AlekseyTs]

Done with review pass (commit 9)

[CyrusNajmabadi]

Suggested change

	* `System.ReadOnlySpan<T>`
	in which cases the *element type* is `T`
	* `System.ReadOnlySpan<T>`, in which cases the *element type* is `T`

[CyrusNajmabadi]

Suggested change

	in which cases the *element type* is `T`
	in which, for all cases, the *element type* is `T`

[CyrusNajmabadi]

total nit: but for ultra pedantry, we probably want to say that the method is not an explicit-impl. but maybe that falls out since an explicit impl method would not be accessible.

[jcouv]

That lapse seems to predate this PR. I also noticed that the current speclet doesn't mention an substitution for the create method. Those should probably be addressed separately.

[AlekseyTs]

I think, we do not care if the method explicitly implements anything as long as it is accessible. And yes, such methods exist in metadata. What we, perhaps, should care about that is whether the name in the attribute is a valid identifier.

[CyrusNajmabadi]

Suggested change

	- `System.ReadOnlySpan<T>`
	in which cases the *element type* is `T`
	- `System.ReadOnlySpan<T>`, in which cases the *element type* is `T`

[CyrusNajmabadi]

Suggested change

	in which cases the *element type* is `T`
	in which, for all cases, the *element type* is `T`

[CyrusNajmabadi]

perhaps say "unambiguous enumerable interface"? but nbd here.

[jcouv]

This is intentional. See note in OP.

[AlekseyTs]

LGTM (commit 10)

[RikkiGibson]

Is this the same as saying "An error is reported if the type decorated with [CollectionBuilder] does not have a create method"?

[jcouv]

I may have misunderstood the question. The type decorated with [CollectionBuilder] is the collection type. The [CollectionBuilder] attribute points to a builder type and a create method.
So we're not expecting a create method in the type decorated with [CollectionBuilder].

[jcouv]

Btw, this line attempts to describe existing rules (which were not fully spec'ed). This line doesn't propose a change in behavior.

[RikkiGibson]

So is an error reported for this attribute by the design in this PR?