-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Determine element type for write-only CollectionBuilder collection types #7895
Changes from 5 commits
09893dc
31611b9
4e17042
a7e6bd2
dc84f02
dec6133
fc15e53
ee553b1
e08804d
7bd7e2f
6b0c13b
44ff697
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -103,22 +103,25 @@ Collection literals are [target-typed](https://github.com/dotnet/csharplang/blob | |||||
A *collection expression conversion* allows a collection expression to be converted to a type. | ||||||
|
||||||
An implicit *collection expression conversion* exists from a collection expression to the following types: | ||||||
* A single dimensional *array type* `T[]` | ||||||
* A single dimensional *array type* `T[]`, in which case the *element type* is `T` | ||||||
* A *span type*: | ||||||
* `System.Span<T>` | ||||||
* `System.ReadOnlySpan<T>` | ||||||
* A *type* with a *[create method](#create-methods)* with an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) determined from a `GetEnumerator` instance method or enumerable interface, not from an extension method | ||||||
* `System.ReadOnlySpan<T>` | ||||||
in which cases the *element type* is `T` | ||||||
* A *type* with an appropriate *[create method](#create-methods)* and a corresponding *element type* resulting from that determination | ||||||
* A *struct* or *class type* that implements `System.Collections.IEnumerable` where: | ||||||
* The *type* has an *[applicable](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/expressions.md#11642-applicable-function-member)* constructor that can be invoked with no arguments, and the constructor is accessible at the location of the collection expression. | ||||||
jcouv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* The *type* has an *[applicable](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/expressions.md#11642-applicable-function-member)* instance or extension method `Add` that can be invoked with a single argument of the [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement), and the method is accessible at the location of the collection expression. | ||||||
* The *type* has an *[applicable](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/expressions.md#11642-applicable-function-member)* instance or extension method `Add` that can be invoked with a single argument of the [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement), and the method is accessible at the location of the collection expression. | ||||||
in which case the *element type* is the *iteration type* of the *type*. | ||||||
jcouv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* An *interface type*: | ||||||
* `System.Collections.Generic.IEnumerable<T>` | ||||||
* `System.Collections.Generic.IReadOnlyCollection<T>` | ||||||
* `System.Collections.Generic.IReadOnlyList<T>` | ||||||
* `System.Collections.Generic.ICollection<T>` | ||||||
* `System.Collections.Generic.IList<T>` | ||||||
* `System.Collections.Generic.IList<T>` | ||||||
in which cases the *element type* is `T` | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
The implicit conversion exists if the type has an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) `U` where for each *element* `Eᵢ` in the collection expression: | ||||||
The implicit conversion exists if the type has an [*element type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) `U` where for each *element* `Eᵢ` in the collection expression: | ||||||
jcouv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* If `Eᵢ` is an *expression element*, there is an implicit conversion from `Eᵢ` to `U`. | ||||||
* If `Eᵢ` is an *spread element* `Sᵢ`, there is an implicit conversion from the *iteration type* of `Sᵢ` to `U`. | ||||||
|
||||||
|
@@ -158,7 +161,7 @@ namespace System.Runtime.CompilerServices | |||||
The attribute can be applied to a `class`, `struct`, `ref struct`, or `interface`. | ||||||
The attribute is not inherited although the attribute can be applied to a base `class` or an `abstract class`. | ||||||
|
||||||
The collection type must have an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement). | ||||||
The determination of an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) must be from a `GetEnumerator` instance method or enumerable interface, not from an extension method. | ||||||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ..., 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):
Now we need to describe how the inference of the "element type" from the method is done. Basically 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
I suggest to say the following instead: The builder type must be a non-generic First, the set of applicable create methods
Methods declared on base types or interfaces are ignored and not part of the If the Second, an attempt is made to determine iteration type of the collection type from a
Third, an attempt is made to infer the element type. Otherwise, type An error is reported if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why not allow any non-generic type which can declare methods to declare the static method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The restriction is taken from the current state of the document. It used to be one of the bullet points. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AlekseyTs Thanks for the alternate draft for "create method" section. 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. |
||||||
|
||||||
For the *create method*: | ||||||
|
||||||
|
@@ -167,9 +170,14 @@ For the *create method*: | |||||
* 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, and there is an [*identity conversion*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/conversions.md#1022-identity-conversion) from `E` to the [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) of the *collection type*. | ||||||
* The method must have a single parameter of type `System.ReadOnlySpan<E>`, passed by value, and if the collection type has an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) (with above restriction), there is an [*identity conversion*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/conversions.md#1022-identity-conversion) from `E` to the *iteration type*. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The arity of the method must match the arity of the collection type and any containing types.
The builder type can be nested but not generic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like implementation is doing the right thing, so the spec needs an adjustment.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not quite, the following doesn't work:
I guess
should be adjusted/clarified as well, i.e. non-generic all the way up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, non-generic here is intended to mean the type and all containing types are non-generic. |
||||||
* There is an [*identity conversion*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/conversions.md#1022-identity-conversion), [*implicit reference conversion*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/conversions.md#1028-implicit-reference-conversions), or [*boxing conversion*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/conversions.md#1029-boxing-conversions) from the method return type to the *collection type*. | ||||||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
Determination of the *element type*: | ||||||
|
||||||
* If the collection type has an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) (with above restriction) then the *element type* is the *iteration type*. | ||||||
* Otherwise, if there is a single non-generic *create method* then the *element type* is `E` given by the method's only parameter (of type `System.ReadOnlySpan<E>`). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't the create method be a generic method (in a non-generic containing type) if the collection type is generic? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you elaborate? The create method is in a builder type, so I didn't follow how the collection type being generic is relevant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) { ... }
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I'll remove the "non-generic" portion. By the point we consider create methods, they will have been fully substituted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wfm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
||||||
An error is reported if the `[CollectionBuilder]` attribute does not refer to an invocable method with the expected signature. | ||||||
|
||||||
Method overloads on the *builder type* with distinct signatures are ignored. Methods declared on base types or interfaces are ignored. | ||||||
|
@@ -377,7 +385,7 @@ The existing rules for the [*first phase*](https://github.com/dotnet/csharpstand | |||||
> | ||||||
> An *input type inference* is made *from* an expression `E` *to* a type `T` in the following way: | ||||||
> | ||||||
> * If `E` is a *collection expression* with elements `Eᵢ`, and `T` is a type with an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) `Tₑ` or `T` is a *nullable value type* `T0?` and `T0` has an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) `Tₑ`, then for each `Eᵢ`: | ||||||
> * If `E` is a *collection expression* with elements `Eᵢ`, and `T` is a type with an *element type* `Tₑ` or `T` is a *nullable value type* `T0?` and `T0` has an *element type* `Tₑ`, then for each `Eᵢ`: | ||||||
> * If `Eᵢ` is an *expression element*, then an *input type inference* is made *from* `Eᵢ` *to* `Tₑ`. | ||||||
> * If `Eᵢ` is an *spread element* with an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) `Sᵢ`, then a [*lower-bound inference*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/expressions.md#116310-lower-bound-inferences) is made *from* `Sᵢ` *to* `Tₑ`. | ||||||
> * *[existing rules from first phase]* ... | ||||||
|
@@ -386,7 +394,7 @@ The existing rules for the [*first phase*](https://github.com/dotnet/csharpstand | |||||
> | ||||||
> An *output type inference* is made *from* an expression `E` *to* a type `T` in the following way: | ||||||
> | ||||||
> * If `E` is a *collection expression* with elements `Eᵢ`, and `T` is a type with an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) `Tₑ` or `T` is a *nullable value type* `T0?` and `T0` has an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/statements.md#1295-the-foreach-statement) `Tₑ`, then for each `Eᵢ`: | ||||||
> * If `E` is a *collection expression* with elements `Eᵢ`, and `T` is a type with an *element type* `Tₑ` or `T` is a *nullable value type* `T0?` and `T0` has an *element type* `Tₑ`, then for each `Eᵢ`: | ||||||
> * If `Eᵢ` is an *expression element*, then an *output type inference* is made *from* `Eᵢ` *to* `Tₑ`. | ||||||
> * If `Eᵢ` is an *spread element*, no inference is made from `Eᵢ`. | ||||||
> * *[existing rules from output type inferences]* ... | ||||||
|
@@ -437,7 +445,7 @@ In the updated rules: | |||||
> | ||||||
> * **`E` is a *collection expression* and one of the following holds:** | ||||||
> * **`T₁` is `System.ReadOnlySpan<E₁>`, and `T₂` is `System.Span<E₂>`, and an implicit conversion exists from `E₁` to `E₂`** | ||||||
> * **`T₁` is `System.ReadOnlySpan<E₁>` or `System.Span<E₁>`, and `T₂` is an *array_or_array_interface* with *iteration type* `E₂`, and an implicit conversion exists from `E₁` to `E₂`** | ||||||
> * **`T₁` is `System.ReadOnlySpan<E₁>` or `System.Span<E₁>`, and `T₂` is an *array_or_array_interface* with *element type* `E₂`, and an implicit conversion exists from `E₁` to `E₂`** | ||||||
> * **`T₁` is not a *span_type*, and `T₂` is not a *span_type*, and an implicit conversion exists from `T₁` to `T₂`** | ||||||
> * **`E` is not a *collection expression* and one of the following holds:** | ||||||
> * `E` exactly matches `T₁` and `E` does not exactly match `T₂` | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -47,28 +47,30 @@ A *parameter_collection* consists of an optional set of *attributes*, a `params` | |||||||
a *type*, and an *identifier*. A parameter collection declares a single parameter of the given type with the given name. | ||||||||
The *type* of a parameter collection shall be one of the following valid target types for a collection expression | ||||||||
(see https://github.com/dotnet/csharplang/blob/main/proposals/csharp-12.0/collection-expressions.md#conversions): | ||||||||
- A single dimensional *array type* `T[]` | ||||||||
- A single dimensional *array type* `T[]`, in which case the *element type* is `T` | ||||||||
- A *span type* | ||||||||
- `System.Span<T>` | ||||||||
- `System.ReadOnlySpan<T>` | ||||||||
- A *type* with a *[create method](https://github.com/dotnet/csharplang/blob/main/proposals/csharp-12.0/collection-expressions.md#create-methods)*, | ||||||||
which is at least as accessible as the declaring member, and with an [*iteration type*](https://github.com/dotnet/csharpstandard/blob/draft-v9/standard/statements.md#1395-the-foreach-statement) | ||||||||
determined from a `GetEnumerator` instance method or enumerable interface, not from an extension method. | ||||||||
- `System.ReadOnlySpan<T>` | ||||||||
in which cases the *element type* is `T` | ||||||||
Comment on lines
+53
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
- A *type* with an appropriate *[create method](https://github.com/dotnet/csharplang/blob/main/proposals/csharp-12.0/collection-expressions.md#create-methods)*, | ||||||||
which is at least as accessible as the declaring member and a corresponding *element type* resulting from that determination | ||||||||
jcouv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
- A *struct* or *class type* that implements `System.Collections.IEnumerable` where: | ||||||||
- The *type* has a constructor that can be invoked with no arguments, and the constructor is at least as accessible as the declaring member. | ||||||||
jcouv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
- The *type* has an instance (not an extension) method `Add` that can be invoked with a single argument of | ||||||||
the [*iteration type*](https://github.com/dotnet/csharpstandard/blob/draft-v9/standard/statements.md#1395-the-foreach-statement), | ||||||||
and the method is at least as accessible as the declaring member. | ||||||||
and the method is at least as accessible as the declaring member. | ||||||||
in which case the *element type* is the *iteration type* | ||||||||
jcouv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
- An *interface type* | ||||||||
- `System.Collections.Generic.IEnumerable<T>`, | ||||||||
- `System.Collections.Generic.IReadOnlyCollection<T>`, | ||||||||
- `System.Collections.Generic.IReadOnlyList<T>`, | ||||||||
- `System.Collections.Generic.ICollection<T>`, | ||||||||
- `System.Collections.Generic.IList<T>` | ||||||||
- `System.Collections.Generic.IList<T>` | ||||||||
in which cases the *element type* is `T` | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
||||||||
In a method invocation, a parameter collection permits either a single argument of the given parameter type to be specified, or | ||||||||
it permits zero or more arguments of the collection [iteration type](https://github.com/dotnet/csharpstandard/blob/draft-v9/standard/statements.md#1395-the-foreach-statement) | ||||||||
to be specified. Parameter collections are described further in *[Parameter collections](#parameter-collections)*. | ||||||||
it permits zero or more arguments of the collection's *element type* to be specified. | ||||||||
jcouv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
Parameter collections are described further in *[Parameter collections](#parameter-collections)*. | ||||||||
|
||||||||
A *parameter_collection* may occur after an optional parameter, but cannot have a default value – the omission of arguments for a *parameter_collection* | ||||||||
would instead result in the creation of an empty collection. | ||||||||
|
@@ -87,7 +89,7 @@ A parameter collection permits arguments to be specified in one of two ways in a | |||||||
- The argument given for a parameter collection can be a single expression that is implicitly convertible to the parameter collection type. | ||||||||
In this case, the parameter collection acts precisely like a value parameter. | ||||||||
- Alternatively, the invocation can specify zero or more arguments for the parameter collection, where each argument is an expression | ||||||||
that is implicitly convertible to the parameter collection [iteration type](https://github.com/dotnet/csharpstandard/blob/draft-v9/standard/statements.md#1395-the-foreach-statement). | ||||||||
that is implicitly convertible to the parameter collection's *element type*. | ||||||||
In this case, the invocation creates an instance of the parameter collection type according to the rules specified in | ||||||||
[Collection expressions](https://github.com/dotnet/csharplang/blob/main/proposals/csharp-12.0/collection-expressions.md) | ||||||||
as though the arguments were used as expression elements in a collection expression in the same order, | ||||||||
|
@@ -117,7 +119,7 @@ The [Applicable function member](https://github.com/dotnet/csharpstandard/blob/d | |||||||
If a function member that includes a parameter collection is not applicable in its normal form, the function member might instead be applicable in its ***expanded form***: | ||||||||
|
||||||||
- The expanded form is constructed by replacing the parameter collection in the function member declaration with | ||||||||
zero or more value parameters of the parameter collection [iteration type](https://github.com/dotnet/csharpstandard/blob/draft-v9/standard/statements.md#1395-the-foreach-statement) | ||||||||
zero or more value parameters of the parameter collection's *element type* | ||||||||
such that the number of arguments in the argument list `A` matches the total number of parameters. | ||||||||
If `A` has fewer arguments than the number of fixed parameters in the function member declaration, | ||||||||
the expanded form of the function member cannot be constructed and is thus not applicable. | ||||||||
|
@@ -158,7 +160,7 @@ In case the parameter type sequences `{P₁, P₂, ..., Pᵥ}` and `{Q₁, Q₂, | |||||||
- **params collection of `Mᵢ` is `System.ReadOnlySpan<Eᵢ>`, and params collection of `Mₑ` is `System.Span<Eₑ>`, and an implicit conversion exists from `Eᵢ` to `Eₑ`** | ||||||||
- **params collection of `Mᵢ` is `System.ReadOnlySpan<Eᵢ>` or `System.Span<Eᵢ>`, and params collection of `Mₑ` is | ||||||||
an *[array_or_array_interface__type](https://github.com/dotnet/csharplang/blob/main/proposals/csharp-12.0/collection-expressions.md#overload-resolution)* | ||||||||
with *[iteration type](https://github.com/dotnet/csharpstandard/blob/draft-v9/standard/statements.md#1395-the-foreach-statement)* `Eₑ`, and an implicit conversion exists from `Eᵢ` to `Eₑ`** | ||||||||
with *element type* `Eₑ`, and an implicit conversion exists from `Eᵢ` to `Eₑ`** | ||||||||
- **both params collections are not *span_type*s, and an implicit conversion exists from params collection of `Mᵢ` to params collection of `Mₑ`** | ||||||||
- Otherwise, no function member is better. | ||||||||
|
||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.