Collection literals: type inference #68703

cston · 2023-06-20T22:52:28Z

See proposals/collection-literals.md#type-inference.

333fred

Done review pass (commit 1)

333fred · 2023-06-22T00:19:23Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs

+            {
+                var builder = new ForEachEnumeratorInfo.Builder();
+                BoundExpression collectionExpr = new BoundValuePlaceholder(syntax, collectionType);
+                return binder.GetEnumeratorInfoAndInferCollectionElementType(


This makes me thing that we should do this up front, when we do the initial unconverted collection literal info calculation, and not here. If we're in some generic overload scenario where we need to perform a number of type inferences, this is going to become extremely expensive extremely quickly. Needing pass a binder into the MethodTypeInferrer is, to me, a signal that the bound node is missing a critical piece of information.

The collection type in this case is the parameter type rather than the collection literal argument type, so we don't have a bound node in the tree that is directly associated with the parameter where we could reasonably cache the iteration type.

It feels like caching GetEnumeratorInfo in the bound tree for the call site doesn't work because we're doing a type inference for each overload just once and not really revisiting them (not getting cache hits).

We might be able to keep a GetEnumeratorInfo cache on the parameter symbol or the parameter type symbol. But it would only be for non-extension cases. We'd still have to do some call-site-specific work in the extension case because it's dependent on the calling context. Maybe it would still be a good "fast path"?

I feel like it would be good to include an end-to-end test which stresses this code path a bit, to make sure it doesn't fall over at a moderate scale. If we can confirm that, I'd personally be OK with perf work on this being pushed to after feature merge or perhaps even to the "debt payoff" milestone.

How do we solve this for tuple literals? Presumably they have a similar problem when you have a signature that takes a (T, T), but they were able to solve it without a binder.

For the collection case, where we're inferring from a collection literal passed as an argument to a parameter that is a collection type, we need to determine the iteration type, and that requires a binder in particular for the foreach-able pattern cases. By doing that here, in MethodTypeInferrer, we essentially calculating the iteration type lazily, when we know we need it.

333fred · 2023-06-22T00:19:58Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -7228,9 +7228,11 @@ private static NullableAnnotation GetNullableAnnotation(BoundExpression expr)
                    case BoundKind.UnboundLambda:
                    case BoundKind.UnconvertedObjectCreationExpression:
                    case BoundKind.ConvertedTupleLiteral:
+                    case BoundKind.UnconvertedCollectionLiteralExpression:


Note that this will crash IOperation

It wasn't clear to me why this is OK for UnconvertedObjectCreationExpression, but not for UnconvertedCollectionLiteralExpression.

It's not. That codepath is either never hit, or only hit during initial nullable analysis of something like an unbound lambda (and if so, we really should have left a comment). If CSharpOperationFactory sees an UnconvertedObjectCreationExpression, it will crash.

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

cston · 2023-06-22T06:15:43Z

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

+                }
+                """;
+            var comp = CreateCompilation(source);
+            // PROTOTYPE: Should inference succeed?


It looks like MethodTypeInferrer.MakeOutputTypeInferences() should walk into collection literals, similar to the behavior for tuples.

See dotnet/csharplang#7293.

This is precisely what I was hoping to discover when asking for these tests, so I agree 😄.

RikkiGibson · 2023-06-22T17:17:05Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs

+            {
+                var builder = new ForEachEnumeratorInfo.Builder();
+                BoundExpression collectionExpr = new BoundValuePlaceholder(syntax, collectionType);
+                return binder.GetEnumeratorInfoAndInferCollectionElementType(


It feels like caching GetEnumeratorInfo in the bound tree for the call site doesn't work because we're doing a type inference for each overload just once and not really revisiting them (not getting cache hits).

We might be able to keep a GetEnumeratorInfo cache on the parameter symbol or the parameter type symbol. But it would only be for non-extension cases. We'd still have to do some call-site-specific work in the extension case because it's dependent on the calling context. Maybe it would still be a good "fast path"?

I feel like it would be good to include an end-to-end test which stresses this code path a bit, to make sure it doesn't fall over at a moderate scale. If we can confirm that, I'd personally be OK with perf work on this being pushed to after feature merge or perhaps even to the "debt payoff" milestone.

RikkiGibson · 2023-06-22T17:18:46Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -7228,9 +7228,11 @@ private static NullableAnnotation GetNullableAnnotation(BoundExpression expr)
                    case BoundKind.UnboundLambda:
                    case BoundKind.UnconvertedObjectCreationExpression:
                    case BoundKind.ConvertedTupleLiteral:
+                    case BoundKind.UnconvertedCollectionLiteralExpression:


It wasn't clear to me why this is OK for UnconvertedObjectCreationExpression, but not for UnconvertedCollectionLiteralExpression.

RikkiGibson · 2023-06-22T17:20:31Z

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

                    }
                }
                """;
-            CompileAndVerify(new[] { source, s_collectionExtensions }, expectedOutput: "System.Int32[][][[], [1, 2, 3]], ");
+            CompileAndVerify(new[] { source, s_collectionExtensions }, expectedOutput: "System.Int32[][][[], [1, 2, 3]], System.Int32[][][][[[]], [[1, 2, 3]]], ");


I'm sure it would be a pain to adjust all the baselines but man it would be nice in these cases if there were a space, or parens, or something between the type and the expression. Maybe parens on the type to make it look like a cast.

Added parentheses around the type name and a space following.

RikkiGibson · 2023-06-22T17:22:33Z

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

                    {
-                        return t;
+                        var x = new[] { [ulong.MaxValue], [1, 2, 3] };


It might be good to demonstrate the new[] {...} and [...] behaviors side-by-side to show that collection literals actually can do this.

I don't think the test is useful after all, since neither element in the implicitly-type array has a type. Removed.

RikkiGibson · 2023-06-22T17:24:08Z

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

+                    {
+                        var x = args.Length > 0 ? new int[0] : [1, 2, 3];
+                        x.Report(includeType: true);
+                        var y = args.Length == 0 ? [[4, 5]] : new[] { new byte[0] };


Did this scenario work before this PR?

Yes, this did work previously. The behavior of the BestCommonType_* tests didn't change in this PR. Originally, BestTypeInferrer was modified, and the tests seemed useful, so I left them in after reverting BestTypeInferrer.

RikkiGibson · 2023-06-22T17:30:09Z

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

-                // 0.cs(9,17): error CS0411: The type arguments for method 'Program.AsArray<T>(params T[])' cannot be inferred from the usage. Try specifying the type arguments explicitly.
-                //         var a = AsArray([1, 2, 3]);
-                Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "AsArray").WithArguments("Program.AsArray<T>(params T[])").WithLocation(9, 17));
+            CompileAndVerify(new[] { source, s_collectionExtensions }, expectedOutput: "[1, 2, 3], ");


Is params expected to test something different about the scenario here? It feels like it wouldn't have an effect outside of expanded form, and in expanded form we're using existing rules, not the new collection literal inference rules?

You are correct, params is not testing anything different here, and this test is not particularly interesting. I've merged this case with _03 above.

RikkiGibson · 2023-06-22T17:55:58Z

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

+            string source = """
+                class Program
+                {
+                    static T[] F1<T>(T[] x, T[] y) => y;


I think it would be good to include a method T F0(T[] x, T y) => y; and call F0(new byte[0], 1), to emphasize the analogy with existing scenarios.

RikkiGibson · 2023-06-22T17:57:05Z

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

+                    static List<T[]> AsListOfArray<T>(List<T[]> arg) => arg;
+                    static void Main()
+                    {
+                        var x = AsListOfArray([[4, 5], []]);


It's really nice that we can just figure this one out.

RikkiGibson · 2023-06-22T18:02:40Z

src/Compilers/CSharp/Test/Semantic/Semantics/CollectionLiteralTests.cs

+                //         F([Main()]);
+                Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "F").WithArguments("Program.F<T>(T[])").WithLocation(8, 9));


Feels like an additional error for Main() outside an expression statement would be nice, since it's always going to be broken in this context, but I wouldn't spend much time on it.

…nto collections-inference

333fred · 2023-06-23T19:00:01Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs

@@ -609,8 +611,12 @@ private void MakeExplicitParameterTypeInferences(BoundExpression argument, TypeW
                ExplicitParameterTypeInference(argument, target, ref useSiteInfo);
                ExplicitReturnTypeInference(argument, target, ref useSiteInfo);
            }
+            else if (argument.Kind == BoundKind.UnconvertedCollectionLiteralExpression)


Once we have natural typing, this (and the corresponding elements in output type inference) are going to need to change, as we are not making any type inferences from the entire collection type to T at the moment. Future work though.

333fred · 2023-06-23T19:15:17Z

A followup will also need to include lambda tests like () => [1, 2] when target-typed to Func<T[]>

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 20, 2023

cston force-pushed the collections-inference branch 2 times, most recently from a375464 to bf36b25 Compare June 21, 2023 19:16

Collection literals: type inference

d3f0043

cston force-pushed the collections-inference branch from bf36b25 to d3f0043 Compare June 21, 2023 19:46

cston marked this pull request as ready for review June 21, 2023 20:22

cston requested a review from a team as a code owner June 21, 2023 20:22

333fred reviewed Jun 22, 2023

View reviewed changes

cston requested a review from RikkiGibson June 22, 2023 02:45

cston added 2 commits June 21, 2023 20:14

Address feedback

183999a

More tests

c4e3e1a

cston commented Jun 22, 2023

View reviewed changes

RikkiGibson self-assigned this Jun 22, 2023

RikkiGibson approved these changes Jun 22, 2023

View reviewed changes

cston added 8 commits June 22, 2023 12:05

Merge remote-tracking branch 'upstream/features/CollectionLiterals' i…

cad8374

…nto collections-inference

Merge remote-tracking branch 'upstream/features/CollectionLiterals' i…

dd075f8

…nto collections-inference

Update tests

dc92b4f

Recurse through elements for parameter and output type inferences

2b012fa

Address feedback

f8673ba

Add parens around type name

5f1e143

Merge tests

72c7946

Add existing scenario to test

69f6b5a

333fred approved these changes Jun 23, 2023

View reviewed changes

cston merged commit 4feb306 into dotnet:features/CollectionLiterals Jun 23, 2023

cston deleted the collections-inference branch June 23, 2023 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection literals: type inference #68703

Collection literals: type inference #68703

cston commented Jun 20, 2023 •

edited

Loading

333fred left a comment

333fred Jun 22, 2023

cston Jun 22, 2023 •

edited

Loading

RikkiGibson Jun 22, 2023

333fred Jun 23, 2023

cston Jun 23, 2023

333fred Jun 22, 2023

RikkiGibson Jun 22, 2023

333fred Jun 23, 2023

cston Jun 22, 2023 •

edited

Loading

333fred Jun 23, 2023

RikkiGibson Jun 22, 2023

RikkiGibson Jun 22, 2023

RikkiGibson Jun 22, 2023

cston Jun 23, 2023

RikkiGibson Jun 22, 2023

cston Jun 23, 2023

RikkiGibson Jun 22, 2023

cston Jun 23, 2023 •

edited

Loading

RikkiGibson Jun 22, 2023

cston Jun 23, 2023

RikkiGibson Jun 22, 2023

RikkiGibson Jun 22, 2023

RikkiGibson Jun 22, 2023

333fred Jun 23, 2023

333fred commented Jun 23, 2023

		// F([Main()]);
		Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "F").WithArguments("Program.F<T>(T[])").WithLocation(8, 9));

Collection literals: type inference #68703

Collection literals: type inference #68703

Conversation

cston commented Jun 20, 2023 • edited Loading

333fred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jun 23, 2023

cston commented Jun 20, 2023 •

edited

Loading

cston Jun 22, 2023 •

edited

Loading

cston Jun 22, 2023 •

edited

Loading

cston Jun 23, 2023 •

edited

Loading