Initial binding and lowering of collection literals with single-valued elements #67025

cston · 2023-02-23T20:11:52Z

Proposal: collection-literals.md
Test plan: #66418

…d elements

CyrusNajmabadi · 2023-02-27T07:21:57Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+                        _ = BindValue(spreadElementSyntax.Expression, diagnostics, BindValueKind.RValue);
+                        // PROTOTYPE: Temporary error until feature is implemented.
+                        Error(diagnostics, ErrorCode.ERR_InvalidExprTerm, syntax, syntax);
+                        return BadExpression(syntax);


nit: consider specific errors here to make it clear that these just aren't supported currently. saying that it's an invalid-expr seems like it could be confusing for anyone trying this out. #Resolved

CyrusNajmabadi · 2023-02-27T07:22:58Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

+    <Field Name="Placeholder" Type="BoundObjectOrCollectionValuePlaceholder" Null="disallow" SkipInVisitor="true"/>
+    <!-- Collection literal elements. -->
+    <Field Name="Initializers" Type="ImmutableArray&lt;BoundExpression&gt;"/>
+  </Node>


to me, it makes sense to perhaps have an abstract base type for the BoundCollectionLiteralExpression and a few specific concrete subclasses for the different cases (Array/span, vs collection-initializer). That way you don't have to know which properties are value for which types of collection literals. #Resolved

CyrusNajmabadi · 2023-02-27T07:24:17Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionLiteral.cs

+            }
+
+            if (node.SpanConstructor is { } spanConstructor)


nit : else if and else below to make it clear each branch is independent and does not impact the other.

CyrusNajmabadi · 2023-02-27T07:25:03Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs

@@ -57,6 +57,11 @@ public override BoundNode VisitConversion(BoundConversion node)
                                                 BoundDelegateCreationExpression { WasTargetTyped: true });

                    return objectCreation;
+
+                case ConversionKind.CollectionLiteral:
+                    // Skip through target-typed collection literals


nit: doc why skipping through is correct. (unclear to me).

CyrusNajmabadi · 2023-02-27T07:26:26Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+            bool isImplicit = boundCollectionLiteralExpression.WasCompilerGenerated;
+            ImmutableArray<IOperation> elementValues = CreateFromArray<BoundExpression, IOperation>(boundCollectionLiteralExpression.Initializers);
+            IOperation collectionCreation;
+            if (collectionType is IArrayTypeSymbol arrayType)


nit: consider extracting all these branches into a helper method. then that helper method just returns the IOp for collection creation, and each branch just does an unconditional 'return'. it makes it easier to understand versus having to see all these potentially mutate local state and then track that flow to the final line. #Resolved

Unfortunately, we return new NoneOperation() directly in the else so we'd need to check the result of helper method to avoid wrapping in a CollectionLiteralOperation. Perhaps we can extract the helper method once the else case (the natural type case) is supported.

333fred

Done review pass (commit 5). Did not go through tests yet.

333fred · 2023-03-01T21:39:37Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -1094,6 +1094,9 @@ private Conversion ClassifyImplicitBuiltInConversionFromExpression(BoundExpressi

                case BoundKind.UnconvertedObjectCreationExpression:
                    return Conversion.ObjectCreation;
+
+                case BoundKind.UnconvertedCollectionLiteralExpression:


I would have expected to see changes to ClassifyStandardImplicitConversion, or IsStandardImplicitConversion. We probably have a test hole around use of a collection literal as an input to a user-defined conversion. #Resolved

Will this become an issue once collection literals have natural types, or even within this change? I've added a test with a user-defined conversion to catch this case once natural types are supported. Let me know if I'm overlooking something.

Likely will require the natural type.

333fred · 2023-03-01T21:50:20Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionLiteral.cs

+            var arrayType = collectionType as ArrayTypeSymbol;
+            if (arrayType is null)


Suggested change

var arrayType = collectionType as ArrayTypeSymbol;

if (arrayType is null)

if (collectionType is not ArrayTypeSymbol arrayType)

333fred · 2023-03-01T21:56:10Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs

@@ -57,6 +57,11 @@ public override BoundNode VisitConversion(BoundConversion node)
                                                 BoundDelegateCreationExpression { WasTargetTyped: true });

                    return objectCreation;
+
+                case ConversionKind.CollectionLiteral:


This implies that we're going to need to do some work in the semantic model for returning type and converted type info. #Resolved

Thanks, added.

333fred · 2023-03-01T21:58:37Z

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml

+    </Comments>
+    <Property Name="CollectionCreation" Type="IOperation">
+      <Comments>
+        <summary>Collection creation.</summary>


What does this mean? #Resolved

333fred · 2023-03-01T21:59:45Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

@@ -6340,6 +6340,23 @@ IArrayInitializerOperation popAndAssembleArrayInitializerValues(IArrayInitialize
            }
        }

+        // PROTOTYPE: Add tests.


More than that, the lowering needs to be implemented. We do not leave collection initializers as literals for IOperation today, and I would not expect us to do that here either. #Resolved

333fred · 2023-03-01T22:01:07Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

@@ -300,6 +304,7 @@ public CSharpOperationFactory(SemanticModel semanticModel)
                case BoundKind.StackAllocArrayCreation:
                case BoundKind.TypeExpression:
                case BoundKind.TypeOrValueExpression:
+                case BoundKind.UnconvertedCollectionLiteralExpression: // PROTOTYPE: Is this correct?


No, I would expect this to be either a ICollectionLiteralOperation with a null type, or a IInvalidOperation. NoneOperations are just for things that we do not support, and we should not be adding anything to this list. #Resolved

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

333fred · 2023-03-07T21:49:50Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -1094,6 +1094,9 @@ private Conversion ClassifyImplicitBuiltInConversionFromExpression(BoundExpressi

                case BoundKind.UnconvertedObjectCreationExpression:
                    return Conversion.ObjectCreation;
+
+                case BoundKind.UnconvertedCollectionLiteralExpression:


Likely will require the natural type.

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

333fred

LGTM (commit 14)

RikkiGibson

Overall LGTM so far. Had a few comments and questions. Haven't reviewed the tests yet.

RikkiGibson · 2023-03-09T21:16:22Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs

@@ -631,6 +632,11 @@ public bool IsObjectCreation
            }
        }

+        /// <summary>
+        /// Returns true if the conversion is an implicit collection creation expression conversion.


nit: "collection literal expression conversion" #Resolved

RikkiGibson · 2023-03-09T21:20:14Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

@@ -3706,6 +3706,10 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres
                    var switchExpr = (BoundSwitchExpression)expr;
                    return GetValEscape(switchExpr.SwitchArms.SelectAsArray(a => a.Value), scopeOfTheContainingExpression);

+                case BoundKind.ArrayOrSpanCollectionLiteralExpression:
+                case BoundKind.CollectionInitializerCollectionLiteralExpression:
+                    return CallingMethodScope;


I would expect a prototype comment here. If the collection literal is converted to span I would expect us to use the local scope depth where the literal appears, or some other reasonable value like that. #Resolved

RikkiGibson · 2023-03-09T21:23:44Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

@@ -4184,6 +4188,10 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF

                    return true;

+                case BoundKind.ArrayOrSpanCollectionLiteralExpression:
+                case BoundKind.CollectionInitializerCollectionLiteralExpression:
+                    return true;


would also expect a prototype comment here. e.g. void M(out Span<int> span) => span = [1, 2, 3] should be a ref safety error. #Resolved

Added a PROTOTYPE comment to GetValEscape() and CheckValEscape(). As I understand the proposal though, span = [1, 2, 3] should not result an error because the array would be allocated on the heap in that case.

Discussed offline, I'm basically willing to buy in to this line of thinking. I want LDM to discuss the possibility of a stackalloc [1, 2, 3] expression. This would let users avoid inadvertently changing their code to introduce heap allocations.

RikkiGibson · 2023-03-09T21:26:15Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

+            BindingDiagnosticBag diagnostics)
+        {
+            var syntax = (CSharpSyntaxNode)node.Syntax;
+            var implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };


I wasn't sure what the effect of WasCompilerGenerated = true is here. #Resolved

This matches BindInitializerExpression, and seems reasonable to me since the placeholder is not in the source.

RikkiGibson · 2023-03-09T21:31:28Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

+                }
+                collectionLiteral = bindArrayOrSpan(syntax, spanConstructor: null, implicitReceiver, node.Initializers, arrayType.ElementType, diagnostics);
+            }
+            else if (isSpanType(targetType, WellKnownType.System_Span_T, out elementType))


Should there be a prototype comment to track stack-allocating collection literals converted to span? I'm not opposed to us punting on that in the initial implementation pass, though I'd probably want the ref safety rules to work as if the collection literal is stack-allocated.

My expectations around this area are based on reading https://github.com/dotnet/csharplang/blob/main/proposals/collection-literals.md#span-types. Let me know if that's not up to date. #Resolved

Added a comment to GetValEscape() and CheckValEscape() for now.

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

RikkiGibson · 2023-03-10T00:04:59Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionLiteral.cs

+            {
+                // PROTOTYPE: If we support _inExpressionLambda, see VisitNewT()
+                // which does not call MakeNewT() in that case.
+                rewrittenReceiver = MakeNewT(syntax, typeParameter);


I didn't understand why we need a separate path for type parameters here instead of being able to use a CollectionCreation node in all cases. #Resolved

Updated to use CollectionCreation in all cases, thanks.

RikkiGibson · 2023-03-10T00:05:40Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionLiteral.cs

+            }
+
+            // Create a temp for the collection.
+            BoundAssignmentOperator assignmentToTemp;


nit: consider using 'out var'.

RikkiGibson · 2023-03-10T00:07:40Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionLiteral.cs

+                    case BoundCollectionElementInitializer element:
+                        rewrittenInitializer = MakeCollectionInitializer(temp, element);
+                        break;
+                    default:


feels like this would read just a little more clearly as if (initializer is not BoundCollectionElementInitializer element) { throw; } rewrittenInitializer = MakeCollectionInitializer(temp, element);. Unless is this is planned to be expanded later? #Resolved

RikkiGibson · 2023-03-10T00:14:12Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+            }
+            else
+            {
+                var array = new ArrayCreationOperation(


It feels like this is "tipping our hand" that the span is backed by an array, when the implementation might not always work this way. Is that fine? #Resolved

Added a PROTOTYPE comment.

RikkiGibson · 2023-03-13T20:26:48Z

...pilers/CSharp/Test/IOperation/IOperation/IOperationTests_IArrayElementReferenceExpression.cs

+";
+            var expectedDiagnostics = new DiagnosticDescription[]
+            {
+                // (6,13): error CS0815: Cannot assign collection literals to an implicitly-typed variable


This error will go away eventually, right? #Resolved

Yes, this error should disappear when support for natural type is added.

RikkiGibson · 2023-03-13T20:43:08Z

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

+                    static void Main()
+                    {
+                        object[,] x = [];
+                        int[,] y = [null, 2];


It feels like people may attempt to do int[,] y = [[1, 2], [3, 4]] (if I'm recalling/understanding what that notations means correctly). Consider also testing that. #Resolved

RikkiGibson · 2023-03-13T20:46:50Z

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

+                """;
+            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net70);
+            comp.VerifyDiagnostics(
+                // (6,20): error CS8347: Cannot use a result of 'Program.F2<int>(in Span<int>)' in this context because it may expose variables referenced by parameter 's' outside of their declaration scope


Consider also testing 'scoped in {{spanType}} s`. #Resolved

RikkiGibson · 2023-03-13T20:56:26Z

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

+                Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "2").WithArguments("S", "Add").WithLocation(3, 9));
+        }
+
+        [Fact]


Have to pause here to have a meeting, will resume later in afternoon.

…nto collections-1

RikkiGibson · 2023-03-13T22:41:33Z

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

+                }
+                """;
+            var comp = CreateCompilation(source);
+            // PROTOTYPE: // 2 should be reported as an error (compare with array initializer: new object[] { null }).


Did you mean reported as a warning? #Resolved

Yes, corrected, thanks.

RikkiGibson · 2023-03-13T22:47:13Z

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

+                """;
+            var comp = CreateCompilation(source);
+            comp.VerifyEmitDiagnostics(
+                // (13,24): error CS9500: Cannot initialize type 'S<object>?' with a collection literal because the type is not constructible.


I didn't understand why this case was erroneous. #Resolved

The target type is Nullable<S<object>> which is not considered constructible because it does not implement IEnumerable.

RikkiGibson · 2023-03-13T22:48:41Z

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

+                """);
+        }
+
+        // Ensure collection literal conversions are not standard implicit conversions


Is there discussion I can review which explains why we want this? #Resolved

The only discussion was in #67025 (comment). I have added an open question to the test plan #66418.

cston force-pushed the collections-1 branch 8 times, most recently from 13dd522 to f1226a9 Compare February 27, 2023 06:08

Initial binding and lowering of collection literals with single-value…

93de7e7

…d elements

cston force-pushed the collections-1 branch from f1226a9 to 93de7e7 Compare February 27, 2023 06:15

Fix test

6953ffd

CyrusNajmabadi reviewed Feb 27, 2023

View reviewed changes

Respond to feedback

b9ff64d

cston marked this pull request as ready for review March 1, 2023 00:28

cston requested review from a team as code owners March 1, 2023 00:28

cston added 2 commits February 28, 2023 22:47

Respond to feedback

0badeaa

Fix build; update error ids

f77c095

333fred reviewed Mar 1, 2023

View reviewed changes

RikkiGibson self-assigned this Mar 1, 2023

cston added 6 commits March 4, 2023 15:22

Address feedback

b21b373

Update public API

939e7dd

Support BoundUnconvertedCollectionLiteralExpression

b574308

Update ControlFlowGraphVerifier

eb41971

Update test

85f1334

More tests

ced04c3

cston added 2 commits March 6, 2023 17:00

Add test

c6ac590

Update tests

489ff8d

333fred reviewed Mar 7, 2023

View reviewed changes

Add tests and comment

e29e59e

cston requested review from RikkiGibson and a team March 8, 2023 19:31

333fred approved these changes Mar 8, 2023

View reviewed changes

RikkiGibson reviewed Mar 10, 2023

View reviewed changes

cston added 2 commits March 12, 2023 12:56

Respond to feedback

1ac5058

Add comments

9b76688

cston force-pushed the collections-1 branch from ca927b8 to 9b76688 Compare March 13, 2023 17:05

333fred approved these changes Mar 13, 2023

View reviewed changes

RikkiGibson reviewed Mar 13, 2023

View reviewed changes

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

a2b465a

…nto collections-1

RikkiGibson reviewed Mar 13, 2023

View reviewed changes

RikkiGibson approved these changes Mar 13, 2023

View reviewed changes

cston added 2 commits March 13, 2023 16:06

Add primary constructor parameter tests

b4060e0

Additional tests

fa3a1bd

RikkiGibson approved these changes Mar 14, 2023

View reviewed changes

cston merged commit 02f55f6 into dotnet:features/CollectionLiterals Mar 14, 2023

cston deleted the collections-1 branch March 14, 2023 21:01

cston mentioned this pull request May 15, 2023

Initial binding support for collection literals #66481

Closed

		var arrayType = collectionType as ArrayTypeSymbol;
		if (arrayType is null)

	var arrayType = collectionType as ArrayTypeSymbol;
	if (arrayType is null)
	if (collectionType is not ArrayTypeSymbol arrayType)

Initial binding and lowering of collection literals with single-valued elements #67025

Initial binding and lowering of collection literals with single-valued elements #67025

Conversation

cston commented Feb 23, 2023 • edited Loading

CyrusNajmabadi Feb 27, 2023 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Feb 27, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Feb 27, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

333fred Mar 1, 2023 • edited by cston 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

333fred Mar 1, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Mar 1, 2023 • edited by cston Loading

Choose a reason for hiding this comment

333fred Mar 1, 2023 • edited by cston Loading

Choose a reason for hiding this comment

333fred Mar 1, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Mar 9, 2023 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Mar 9, 2023 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Mar 9, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 9, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 9, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 10, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 10, 2023 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Mar 10, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 13, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 13, 2023 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Mar 13, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 13, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 13, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 13, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Feb 23, 2023 •

edited

Loading

CyrusNajmabadi Feb 27, 2023 •

edited by cston

Loading

CyrusNajmabadi Feb 27, 2023 •

edited by cston

Loading

CyrusNajmabadi Feb 27, 2023 •

edited by cston

Loading

333fred Mar 1, 2023 •

edited by cston

Loading

333fred Mar 1, 2023 •

edited by cston

Loading

333fred Mar 1, 2023 •

edited by cston

Loading

333fred Mar 1, 2023 •

edited by cston

Loading

333fred Mar 1, 2023 •

edited by cston

Loading

RikkiGibson Mar 9, 2023 •

edited by cston

Loading

RikkiGibson Mar 9, 2023 •

edited by cston

Loading

RikkiGibson Mar 9, 2023 •

edited by cston

Loading

RikkiGibson Mar 9, 2023 •

edited by cston

Loading

RikkiGibson Mar 9, 2023 •

edited by cston

Loading

RikkiGibson Mar 10, 2023 •

edited by cston

Loading

RikkiGibson Mar 10, 2023 •

edited by cston

Loading

RikkiGibson Mar 10, 2023 •

edited by cston

Loading

RikkiGibson Mar 13, 2023 •

edited by cston

Loading

RikkiGibson Mar 13, 2023 •

edited by cston

Loading

RikkiGibson Mar 13, 2023 •

edited by cston

Loading

RikkiGibson Mar 13, 2023 •

edited by cston

Loading

RikkiGibson Mar 13, 2023 •

edited by cston

Loading

RikkiGibson Mar 13, 2023 •

edited by cston

Loading