Collection literals natural type #67364
Conversation
| var useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics); | ||
| // PROTOTYPE: Confirm with LDM that we use the element expressions rather than | ||
| // the element types to infer best common type. | ||
| var commonType = BestTypeInferrer.InferBestType(node.Initializers, Conversions, ref useSiteInfo, out _); |
There was a problem hiding this comment.
yes, using hte expressions is my expectation here. i would also expect this to match what we do for new[] { x, y, z }
| collectionType = GetWellKnownType(WellKnownType.System_Collections_Generic_List_T, diagnostics, location).Construct(commonType); | ||
| } | ||
|
|
||
| // PROTOTYPE: hasErrors is dropped. |
There was a problem hiding this comment.
maybe prototype comment about binding to the int capacity constructor. #Resolved
There was a problem hiding this comment.
we'll probably want LDM sign off on that behavior as well.
There was a problem hiding this comment.
maybe prototype comment about binding to the int capacity constructor.
Added.
cston
force-pushed
the
collections-2
branch
3 times, most recently
from
eb7fcc3 to
bb8bc82
Compare
cston
force-pushed
the
collections-2
branch
2 times, most recently
from
90e23a9 to
63504c2
Compare
cston
force-pushed
the
collections-2
branch
2 times, most recently
from
52a561d to
eafbd88
Compare
cston
force-pushed
the
collections-2
branch
4 times, most recently
from
ba5354f to
a19e527
Compare
|
|
||
| // This implementation differs from Binder.CollectionInitializerTypeImplementsIEnumerable(). | ||
| // That method checks for an implicit conversion from IEnumerable to the collection type, | ||
| // but that would allow: Nullable<StructCollection> s = []; |
There was a problem hiding this comment.
Should we adjust the behavior of that method and use it here? It seems like new StructCollection? { 1, 2, 3 } is always broken. Currently it seems like this errors because no Add method is available. But it definitely wouldn't make sense for this collection initializer to work, using the IEnumerable-ness of the underlying type, and the Add method of the nullable type. #Resolved
There was a problem hiding this comment.
Agreed. I don't like these being different.
| : targetType.AllInterfacesNoUseSiteDiagnostics; | ||
| return allInterfaces.Any(static (a, b) => a.Equals(b, TypeCompareKind.AllIgnoreOptions), ienumerableType); | ||
| } | ||
|
|
||
| BoundExpression convertArrayElement(BoundExpression element, TypeSymbol elementType, BindingDiagnosticBag diagnostics) |
There was a problem hiding this comment.
Are there any functions here which have just been moved but not changed?
| // Check that each element (even those without a type) can be converted to the best type. | ||
| foreach (var initializer in initializers) | ||
| { | ||
| if (!Conversions.ClassifyImplicitConversionFromExpression(initializer, bestType, ref useSiteInfo).Exists) |
There was a problem hiding this comment.
So it's possible to choose a best type for a set of expressions, even if one of the expressions is not convertible to that type? That seems so strange.
There was a problem hiding this comment.
Why would we clear this? It seems like the better experience would be to let it be found as the best type and then have conversion errors?
There was a problem hiding this comment.
So it's possible to choose a best type for a set of expressions, even if one of the expressions is not convertible to that type?
BestTypeInferrer.InferBestType() ignores expressions that do not contribute a type, so we'll choose a type of int for [1, null] for instance.
Why would we clear this? It seems like the better experience would be to let it be found as the best type and then have conversion errors?
Perhaps, although for reference, this matches the behavior for switch expressions, see SwitchExpressionBinder.InferResultType().
| @@ -5571,7 +5638,7 @@ private BoundExpression BindUnexpectedComplexElementInitializer(InitializerExpre | |||
| } | |||
|
|
|||
| private BoundExpression BindCollectionInitializerElementAddMethod( | |||
| ExpressionSyntax elementInitializer, | |||
| SyntaxNode elementInitializer, | |||
There was a problem hiding this comment.
When is this not an expression syntax? Or was this just changed for convenience (removing need for casting?) #Resolved
There was a problem hiding this comment.
Currently, we can end up here for spread elements and dictionary elements when the element is a BoundBadExpression, but it seems questionable to call this method in those cases. I've added a PROTOTYPE comment to revisit in an upcoming PR.
|
|
||
| case BoundKind.UnconvertedCollectionLiteralExpression: | ||
| { | ||
| if (operand.Type is null) |
There was a problem hiding this comment.
What is generally signaled by operand.Type being null? Lack of a best type among the collection elements? #Resolved
| @@ -1309,7 +1309,6 @@ private IOperation CreateBoundCollectionInitializerCollectionLiteralExpression(B | |||
| } | |||
| else | |||
| { | |||
| // PROTOTYPE: Temporary until natural type is supported. | |||
There was a problem hiding this comment.
I'm assuming we now enter the previous 'if' in the natural type scenario? #Resolved
| @@ -657,7 +658,8 @@ private Conversion ClassifyStandardImplicitConversion(BoundExpression sourceExpr | |||
|
|
|||
| Conversion conversion = ClassifyImplicitBuiltInConversionFromExpression(sourceExpression, source, destination, ref useSiteInfo); | |||
| if (conversion.Exists && | |||
| !conversion.IsInterpolatedStringHandler) | |||
| !conversion.IsInterpolatedStringHandler && | |||
| !conversion.IsCollectionLiteral) | |||
There was a problem hiding this comment.
Comment above needs to be updated as well, last paragraph calling out exceptions. #Resolved
|
|
||
| if (destination is ArrayTypeSymbol arrayType) | ||
| { | ||
| if (arrayType.IsSZArray) |
There was a problem hiding this comment.
Are there intentions to support MDArrays, with a syntax like [[1, 2], [3, 4]]? #Pending
There was a problem hiding this comment.
Added to open questions in the test plan #66418.
| static bool isSpanType(CSharpCompilation compilation, TypeSymbol targetType, WellKnownType spanType, [NotNullWhen(true)] out TypeSymbol? elementType) | ||
| { | ||
| if (targetType is NamedTypeSymbol { Arity: 1 } namedType | ||
| && TypeSymbol.Equals(namedType.OriginalDefinition, compilation.GetWellKnownType(spanType), TypeCompareKind.AllIgnoreOptions)) |
There was a problem hiding this comment.
Don't we need to call the binder version of GetWellKnownType that tracks dependencies? #Resolved
There was a problem hiding this comment.
This is checking if the given type from the compilation is a span type. If it's not, that shouldn't affect dependencies, and if it is, the compilation should already be tracking the dependency.
| return false; | ||
| } | ||
|
|
||
| static bool supportsCollectionInitializer(CSharpCompilation compilation, TypeSymbol targetType) |
There was a problem hiding this comment.
This is missing the Add checks on whether a type supports collection initializers. Perhaps rename this to just typeImplementsIEnumerable? #Resolved
|
|
||
| // This implementation differs from Binder.CollectionInitializerTypeImplementsIEnumerable(). | ||
| // That method checks for an implicit conversion from IEnumerable to the collection type, | ||
| // but that would allow: Nullable<StructCollection> s = []; |
There was a problem hiding this comment.
Agreed. I don't like these being different.
| // Check that each element (even those without a type) can be converted to the best type. | ||
| foreach (var initializer in initializers) | ||
| { | ||
| if (!Conversions.ClassifyImplicitConversionFromExpression(initializer, bestType, ref useSiteInfo).Exists) |
There was a problem hiding this comment.
Why would we clear this? It seems like the better experience would be to let it be found as the best type and then have conversion errors?
|
|
||
| if (bestType is { }) | ||
| { | ||
| return Compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T).Construct(bestType); |
There was a problem hiding this comment.
Please test when List<T> isn't found. #Resolved
There was a problem hiding this comment.
CollectionLiteralTests.MissingList.
| Debug.Assert(false); // PROTOTYPE: Add test. | ||
| // There was an explicit cast on top of this | ||
| type = convertedCollection.NaturalTypeOpt; | ||
| Debug.Assert(false); // Add test if this assert fails. |
There was a problem hiding this comment.
I think I'd rather return the values below, to avoid an unhandled exception. (The returned values follow other cases here.)
| var model = comp.GetSemanticModel(tree); | ||
| var collections = tree.GetRoot().DescendantNodes().OfType<CollectionCreationExpressionSyntax>().ToArray(); | ||
| Assert.Equal(3, collections.Length); | ||
| VerifyTypes(model, collections[0], "?", "System.Object", ConversionKind.NoConversion); |
There was a problem hiding this comment.
Please use named args with the constants. Sorry. #Resolved
| // (6,18): error CS0103: The name 'Unknown' does not exist in the current context | ||
| // var b = [Unknown, 2]; | ||
| Diagnostic(ErrorCode.ERR_NameNotInContext, "Unknown").WithArguments("Unknown").WithLocation(6, 18), | ||
| // (7,17): error CS9503: No best type found for implicitly-typed collection literal. |
There was a problem hiding this comment.
It feels likely that we will have a better error experience if we ignore error types when finding the BCT of a collection literal. Less cascading errors down through the rest of the file, for example.
| string source = """ | ||
| class Program | ||
| { | ||
| static object F1((int, int) a, (int X, int Y) b) |
There was a problem hiding this comment.
Is there a test with a having different names, not just no names? #Resolved
| var c1 = [a, b]; | ||
| return c1; | ||
| } | ||
| static object F2(dynamic[] a, object[] b) |
There was a problem hiding this comment.
Worth testing spreads for this and below? #Pending
There was a problem hiding this comment.
Testing spreads containing with element types object and dynamic will be included in #67942.
| } | ||
| } | ||
| """; | ||
| var comp = CreateCompilation(new[] { source, s_collectionExtensions }, options: TestOptions.ReleaseExe, targetFramework: TargetFramework.Net70); |
There was a problem hiding this comment.
Consider verifying the .NET 6 behavior as well. #Resolved
| } | ||
| static void F(bool b) | ||
| { | ||
| var x = b ? [1] : []; |
There was a problem hiding this comment.
Consider a version of this with [null] in the other hand. Also consider a version where they're target-typed to int?[] #Resolved
There was a problem hiding this comment.
Ah, I see you have _32 for the null suggestion. Consider adding one for the target-typed suggestion though.
There was a problem hiding this comment.
Added TargetType_01 and _02.
| } | ||
| } | ||
| """; | ||
| // PROTOTYPE: Should these calls be ambiguous when we treat IEnumerable<T> as a valid target type (since it is an interface of List<T>)? |
There was a problem hiding this comment.
int[] implements IEnumerable<int>, so I expect that no, these would not be ambiguous. #Resolved
There was a problem hiding this comment.
If int[] and IEnumerable<T> are both considered constructible collection types, should better function member prefer one over the other?
I've updated the PROTOTYPE comment and added an open question to the test plan #66418.
There was a problem hiding this comment.
i def think we would want a preference. having an array and an IEnumerable overload would be pretty normal. Having that be ambiguous woudl be unfortunate.
| } | ||
| } | ||
| """; | ||
| // PROTOTYPE: Should these calls be ambiguous when we treat IEnumerable<T> as a valid target type (since it is an interface of List<T>)? |
| using System.Collections.Generic; | ||
| class Program | ||
| { | ||
| static List<int> F1(List<int> arg) => arg; |
There was a problem hiding this comment.
Consider a version of this test with int? and long type args. #Resolved
| { | ||
| string source = """ | ||
| string sourceA = """ | ||
| namespace System |
There was a problem hiding this comment.
Can we not just use MakeTypeMissing/MakeMemberMissing (for the test below)? #Resolved
There was a problem hiding this comment.
We could, and I have used MakeTypeMissing() and MakeMemberMissing() for other tests. But testing without the type at all seems useful.
|
Marking as draft while natural type is being discussed. |
Proposal: collection-literals.md
Test plan: #66418