Support 'foreach' for inline arrays. #68297

AlekseyTs · 2023-05-22T21:39:00Z

https://github.com/dotnet/csharplang/blob/main/proposals/inline-arrays.md#the-foreach-statement

AlekseyTs · 2023-05-24T15:17:08Z

@cston, @dotnet/roslyn-compiler Please review.

…neArrays_19

AlekseyTs · 2023-05-26T14:11:52Z

@cston, @dotnet/roslyn-compiler Please review.

cston · 2023-05-26T14:56:34Z

src/Compilers/Core/Portable/CodeGen/PrivateImplementationDetails.cs

@@ -41,6 +41,9 @@ internal sealed class PrivateImplementationDetails : DefaultTypeDef, Cci.INamesp
        internal const string SynthesizedInlineArrayAsSpanName = "InlineArrayAsSpan";
        internal const string SynthesizedInlineArrayAsReadOnlySpanName = "InlineArrayAsReadOnlySpan";

+        internal const string SynthesizedInlineArrayElementRefName = "InlineArrayElementRef";
+        internal const string SynthesizedInlineArrayElementReadOnlyRefName = "InlineArrayElementReadOnlyRef";


ReadOnlyRef

Should this be "RefReadOnly" instead? #Resolved

Should this be "RefReadOnly" instead?

This method is not meant for public consumption, We can call it whatever we want. Are you suggesting to change the name?

Are you suggesting to change the name?

Yes. "RefReadOnly" would match the signature.

Yes. "RefReadOnly" would match the signature.

Renamed

cston · 2023-05-26T15:13:42Z

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

+                                    checkingReceiver: false,
+                                    diagnostics);
+                            }
+                            else


else

Are we testing this code path? #Resolved

Are we testing this code path?

For example, Foreach_Variable_ReadOnly_02 and Foreach_Value_01.

cston · 2023-05-26T16:55:40Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

@@ -1967,6 +1967,46 @@ internal MethodSymbol EnsureInlineArrayAsReadOnlySpanExists(SyntaxNode syntaxNod
            return (MethodSymbol)privateImplClass.GetMethod(PrivateImplementationDetails.SynthesizedInlineArrayAsReadOnlySpanName)!.GetInternalSymbol()!;
        }

+        internal MethodSymbol EnsureInlineArrayElementRefExists(SyntaxNode syntaxNode, NamedTypeSymbol intType, DiagnosticBag diagnostics)


EnsureInlineArrayElementRefExists

It looks like these new methods and the methods above have the same boilerplate for the GetMethod(), TryAddSynthesizedMethod(), GetMethod() calls. Consider extracting a helper method that takes a method name and a delegate to create the method. #Pending

It looks like these new methods and the methods above have the same boilerplate for the GetMethod(), TryAddSynthesizedMethod(), GetMethod() calls. Consider extracting a helper method that takes a method name and a delegate to create the method.

Since this refactoring going to include methods that are not related to the feature. I prefer to implement this refactoring in a separate PR, if there are no objections.

cston · 2023-05-26T17:32:16Z

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

+                                                                            if (valueStore is object)
+                                                                            {
+                                                                                collectionVarInitializationPreamble = rewriter._factory.ExpressionStatement(valueStore);
+                                                                                preambleLocal = boundLocal.LocalSymbol; ;


;

Extra ; #Resolved

cston · 2023-05-26T17:44:05Z

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

+                                                                            BoundLocal boundLocal = rewriter._factory.StoreToTemp(rewrittenExpression, out BoundAssignmentOperator? valueStore);
+                                                                            rewrittenExpression = boundLocal;
+
+                                                                            if (valueStore is object)


if (valueStore is object)

It looks like StoreToTemp() always generates a non-null value. #Resolved

cston · 2023-05-26T17:51:11Z

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

+                                                                    }
+
+                                                                    TypeSymbol inlineArrayType = boundArrayVar.Type;
+                                                                    elementRef = elementRef.Construct(inlineArrayType, inlineArrayType.TryGetInlineArrayElementField()!.Type);


elementRef

Should the code for the 0-th ref TElement be emitted outside the loop in getPreamble, with getItem simply calling Unsafe.Add() to offset for the particular index?

And if so, would the PrivateImplementationDetails methods still be necessary? #Resolved

Should the code for the 0-th ref TElement be emitted outside the loop in getPreamble, with getItem simply calling Unsafe.Add() to offset for the particular index?

I prefer to stick to the existing 'for' reawrite strategy where we always calculate element ref inside the loop and outside we have a local with a special SynthesizedLocalKind.ForEachArray, which represents the array itself. This is likely important for debugging and EnC scenarios. I have a PROTOTYPE comment regarding this in MethodToStateMachineRewriter. Happy to discuss this offline in more details.

And if so, would the PrivateImplementationDetails methods still be necessary?

Yes, they also reinterpret the type of the reference from array type to the element type.

cston · 2023-05-31T04:19:25Z

}

What effect does GetEnumerator() have?

In reply to: 1569473616

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/InlineArrayTests.cs:14623 in c3ffbf1. [](commit_id = c3ffbf1, deletion_comment = False)

AlekseyTs · 2023-06-02T04:00:58Z

}

What effect does GetEnumerator() have?

It doesn't have any effect, and the verification confirms that

In reply to: 1569473616

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/InlineArrayTests.cs:14623 in c3ffbf1. [](commit_id = c3ffbf1, deletion_comment = False)

AlekseyTs · 2023-06-06T12:58:23Z

@jjonescz, @dotnet/roslyn-compiler For the second review

jjonescz · 2023-06-07T15:14:48Z

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

@@ -380,7 +380,7 @@ private BoundIndexerAccess BindIndexerDefaultArguments(BoundIndexerAccess indexe
        /// method returns a BoundBadExpression node. The method returns the original
        /// expression without generating any error if the expression has errors.
        /// </summary>
-        private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind, BindingDiagnosticBag diagnostics)
+        internal BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind, BindingDiagnosticBag diagnostics)


Would protected be enough? #Resolved

Would protected be enough?

It turns out this change isn't necessary after all

jjonescz · 2023-06-08T07:37:44Z

src/Compilers/CSharp/Test/Emit2/Semantics/InlineArrayTests.cs

+            comp.VerifyDiagnostics(
+                // (6,32): error CS8415: Asynchronous foreach statement cannot operate on variables of type 'Buffer4<int>' because 'Buffer4<int>' does not contain a public instance or extension definition for 'GetAsyncEnumerator'. Did you mean 'foreach' rather than 'await foreach'?
+                //         await foreach(var s in GetBuffer())
+                Diagnostic(ErrorCode.ERR_AwaitForEachMissingMemberWrongAsync, "GetBuffer()").WithArguments("Buffer4<int>", "GetAsyncEnumerator").WithLocation(6, 32)


Consider testing when there's a method GetAsyncEnumerator (as instance or extension). #Resolved

Consider testing when there's a method GetAsyncEnumerator (as instance or extension).

I will add a test like that in one of the next PRs

I will add a test like that in one of the next PRs

Since I needed to make some other change, I ended up adding a couple of tests below.

jjonescz · 2023-06-08T08:09:13Z

src/Compilers/CSharp/Test/Emit2/Semantics/InlineArrayTests.cs

+
+namespace System
+{
+    public readonly ref struct Span<T>


Consider adding a test with a Span<T> where T has some constraints. #Resolved

Consider adding a test with a Span where T has some constraints.

The Foreach_UnsupportedElementType unit-test already covers constraint verification. Every generic type has implicit constraints. I do not think it is worth testing against a malformed Span type.

jjonescz · 2023-06-08T08:26:54Z

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

+                result = getEnumeratorInfo(ref span, isAsync: false, enumeratorInfoDiagnostics, out builder);
+
+#if DEBUG
+                Debug.Assert(span == originalSpan);


What if Span<T> is defined as a nullable class, could this assert fail then? #Resolved

What if Span<T> is defined as a nullable class, could this assert fail then?

No, IsNullableType returns true only for System.Nullable<T>. The span can never have type like that.

Support 'foreach' for inline arrays.

eb9bad3

https://github.com/dotnet/csharplang/blob/main/proposals/inline-arrays.md#the-foreach-statement

AlekseyTs added Area-Compilers New Feature - Inline Arrays labels May 22, 2023

AlekseyTs requested a review from cston May 22, 2023 21:39

AlekseyTs requested a review from a team as a code owner May 22, 2023 21:39

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label May 22, 2023

Language version check and async/iterator methods

d42ed47

AlekseyTs added 2 commits May 25, 2023 07:36

Merge remote-tracking branch 'dotnet/features/InlineArrays' into Inli…

6463590

…neArrays_19

Move DeconstructionTests back

c3ffbf1

cston reviewed May 26, 2023

View reviewed changes

cston approved these changes May 31, 2023

View reviewed changes

AlekseyTs added 2 commits June 2, 2023 06:48

PR feedback

ab8bc6a

File rename

64b602d

jjonescz approved these changes Jun 8, 2023

View reviewed changes

PR feedback

9cbbb19

AlekseyTs enabled auto-merge (squash) June 8, 2023 14:59

AlekseyTs merged commit cf124b4 into dotnet:features/InlineArrays Jun 8, 2023

allisonchou mentioned this pull request Jun 30, 2023

[Automated] PRs inserted in VS build main-33829.535 #68838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'foreach' for inline arrays. #68297

Support 'foreach' for inline arrays. #68297

AlekseyTs commented May 22, 2023

AlekseyTs commented May 24, 2023

AlekseyTs commented May 26, 2023

cston May 26, 2023 •

edited by AlekseyTs

Loading

AlekseyTs May 26, 2023 •

edited

Loading

cston May 26, 2023

AlekseyTs Jun 2, 2023

cston May 26, 2023 •

edited by AlekseyTs

Loading

AlekseyTs May 26, 2023

cston May 26, 2023 •

edited by AlekseyTs

Loading

AlekseyTs May 26, 2023

cston May 26, 2023 •

edited by AlekseyTs

Loading

cston May 26, 2023 •

edited by AlekseyTs

Loading

cston May 26, 2023 •

edited by AlekseyTs

Loading

AlekseyTs May 26, 2023 •

edited

Loading

cston commented May 31, 2023 •

edited by AlekseyTs

Loading

AlekseyTs commented Jun 2, 2023

AlekseyTs commented Jun 6, 2023

jjonescz Jun 7, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jun 8, 2023

jjonescz Jun 8, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jun 8, 2023

AlekseyTs Jun 8, 2023

jjonescz Jun 8, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jun 8, 2023

jjonescz Jun 8, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jun 8, 2023 •

edited

Loading

Support 'foreach' for inline arrays. #68297

Support 'foreach' for inline arrays. #68297

Conversation

AlekseyTs commented May 22, 2023

AlekseyTs commented May 24, 2023

AlekseyTs commented May 26, 2023

cston May 26, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 26, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 26, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 26, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston May 26, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston May 26, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs May 26, 2023 • edited Loading

Choose a reason for hiding this comment

cston commented May 31, 2023 • edited by AlekseyTs Loading

AlekseyTs commented Jun 2, 2023

AlekseyTs commented Jun 6, 2023

jjonescz Jun 7, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Jun 8, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Jun 8, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Jun 8, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

cston May 26, 2023 •

edited by AlekseyTs

Loading

AlekseyTs May 26, 2023 •

edited

Loading

cston May 26, 2023 •

edited by AlekseyTs

Loading

cston May 26, 2023 •

edited by AlekseyTs

Loading

cston May 26, 2023 •

edited by AlekseyTs

Loading

cston May 26, 2023 •

edited by AlekseyTs

Loading

cston May 26, 2023 •

edited by AlekseyTs

Loading

AlekseyTs May 26, 2023 •

edited

Loading

cston commented May 31, 2023 •

edited by AlekseyTs

Loading

jjonescz Jun 7, 2023 •

edited by AlekseyTs

Loading

jjonescz Jun 8, 2023 •

edited by AlekseyTs

Loading

jjonescz Jun 8, 2023 •

edited by AlekseyTs

Loading

jjonescz Jun 8, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jun 8, 2023 •

edited

Loading