Support ref field declarations from source and metadata#60416

Merged

cston merged 31 commits intodotnet:features/ref-fieldsfrom

cston:ref-field-decl

May 2, 2022

Contributor

cston commented Mar 28, 2022 •

edited

Loading

Proposal: low-level-struct-improvements.md
Test plan: #59194

Included:

API: added IFieldSymbol.RefKind and IFieldSymbol.RefCustomModifiers
Parsing: allow ref and ref readonly modifiers for fields
Binding: allow ref fields as l-values and r-values
Code gen: allow ref fields as l-values and r-values
Metadata decoding/encoding: read/write ByReference and RefCustomModifiers
-langversion: report error for ref declarations and use-sites with -langversion:10 or below
SymbolDisplay: include ref modifiers and RefCustomModifiers from C#

Not included:

Use-site error using ref field from VB
Updates to val and ref escape calculations
ref auto-properties
Unit tests do not execute binaries from CI builds

ghost added the Area-Compilers label

cston force-pushed the ref-field-decl branch 6 times, most recently from 16dca7b to a0543aa Compare

April 4, 2022 18:53

runfoapp bot mentioned this pull request

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

cston force-pushed the ref-field-decl branch 3 times, most recently from fcfce5f to d463a86 Compare

April 5, 2022 21:36


          Support ref field declarations from source and metadata

369bc83

cston force-pushed the ref-field-decl branch from d463a86 to 369bc83 Compare

April 5, 2022 21:47

cston changed the base branch from main to features/ref-fields

April 5, 2022 21:52

cston marked this pull request as ready for review

April 5, 2022 21:53

cston requested review from a team as code owners

April 5, 2022 21:53

Contributor Author

cston commented Apr 5, 2022

cc @jaredpar, @AaronRobinsonMSFT


          Update tests

cf131c6

RikkiGibson self-assigned this

AaronRobinsonMSFT mentioned this pull request

ref field support in .NET runtimes dotnet/runtime#63768

Closed

13 tasks

cston added 4 commits

April 6, 2022 11:14


          Update tests

92b6a1f


          Report -langver error at use-site

3e1744b


          Fix tests

7dd1026


          Update tests

a8f002f

RikkiGibson reviewed

View reviewed changes

Member

RikkiGibson left a comment

LGTM with some minor nits. Will review tests shortly.

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Outdated Show resolved Hide resolved

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Show resolved Hide resolved

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Show resolved Hide resolved

src/Compilers/CSharp/Portable/Symbols/Source/FieldSymbolWithAttributesAndModifiers.cs Show resolved Hide resolved

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs Outdated Show resolved Hide resolved

src/Compilers/Core/Portable/PEWriter/Members.cs Outdated Show resolved Hide resolved


          PR feedback

d976ff1

RikkiGibson reviewed

View reviewed changes

Member

RikkiGibson left a comment

Had various comments on the tests, discussed offline but leaving the comments here to ensure we can go through and resolve.

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Outdated

+                          Assert.Equal(RefKind.RefReadOnly, field.RefKind);
+                          // Currently, source symbols cannot declare RefCustomModifiers. If that
+                          // changes, update this test to verify retargeting of RefCutomModifiers.
+                          Assert.Equal(new string[0], field.RefCustomModifiers.SelectAsArray(m => m.Modifier.ToTestDisplayString()));

Member

RikkiGibson Apr 8, 2022 •

edited by cston

Loading

nit: prefer Assert.Empty #Resolved

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

+                      [Fact]
+                      public void DefiniteAssignment_02()
+                      {
+                          // Should we report a warning when assigning a value rather than a ref in the

Member

RikkiGibson Apr 8, 2022 •

edited by cston

Loading

It seems like this should have a PROTOTYPE marker or tracking issue. #Resolved

Contributor Author

cston Apr 8, 2022 •

edited

Loading

There's an open question for language design in the test plan so I'll leave this as a regular comment.

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Outdated

+                              // (20,21): error CS1510: A ref or out value must be an assignable variable
+                              //             F = ref GetValue();
+                              Diagnostic(ErrorCode.ERR_RefLvalueExpected, "GetValue()").WithLocation(20, 21),
+                              // (22,21): error CS8331: Cannot assign to method 'S<T>.GetRefReadonly()' because it is a readonly variable

Member

RikkiGibson Apr 8, 2022 •

edited by cston

Loading

It looks like this message is expected but the language seems confusing. It feels like we should say something like "cannot take a writable ref to a readonly variable". Not suggesting you fix it in this PR or even this feature, but perhaps we can improve the message separately. #Resolved

Contributor Author

cston Apr 25, 2022

Added a PROTOTYPE comment.

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Show resolved Hide resolved

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

+              @"ref struct S<T>
+              {
+                  public ref T F;
+                  public ref T F1() => ref F;

Member

RikkiGibson Apr 8, 2022 •

edited by cston

Loading

Consider using unique method names across the test to make it a little easier to relate the diagnostics to the source. #Resolved

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

+                IL_0000:  ldarga.s   V_0
+                IL_0002:  ldfld      ""ref T S<T>.F""
+                IL_0007:  ldloca.s   V_0
+                IL_0009:  initobj    ""T""

Member

RikkiGibson Apr 8, 2022 •

edited by cston

Loading

I didn't understand why there's an initobj here. Consider including a PROTOTYPE comment. #Resolved

Contributor Author

cston Apr 9, 2022

The initobj is part of a if ((object)default(T) != null) { ... } test that is emitted for unconstrained T to test whether T is a struct (see CodeGenerator.EmitLoweredConditionalAccessExpression).

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Outdated

+                      Console.WriteLine((i, s));
+                  }
+              }";
+                          CompileAndVerify(source, verify: Verification.Skipped, expectedOutput: IncludeExpectedOutput(@"(1, Hello world)"));

Member

RikkiGibson Apr 8, 2022 •

edited by cston

Loading

Should we verify the IL of the deconstruction assignment itself? #Resolved

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Outdated

+                              // (5,27): error CS8145: Auto-implemented properties cannot return by reference
+                              //     public ref readonly T Q { get; }
+                              Diagnostic(ErrorCode.ERR_AutoPropertyCannotBeRefReturning, "Q").WithArguments("S<T>.Q").WithLocation(5, 27),
+                              // (8,9): error CS8373: The left-hand side of a ref assignment must be a ref local or parameter.

Member

RikkiGibson Apr 8, 2022 •

edited by cston

Loading

nit: we'll want to adjust this diagnostic language when we get the chance. Perhaps to simply say ref variable. #Resolved

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Show resolved Hide resolved

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

+                  }
+              }";
+                          var comp = CreateCompilation(source);
+                          // PROTOTYPE: Should not report ERR_RefReturnStructThis.

Member

RikkiGibson Apr 8, 2022 •

edited by cston

Loading

Spec says (deep in the detailed design section):

When scoped is applied to an instance method the this parameter will have the type scoped ref T. By default this is redundant as scoped ref T is the default type of this. #Resolved

Member

RikkiGibson Apr 8, 2022

Therefore I expected an error here, but no error once we are able to declare the unscoped version of the method.

Contributor Author

cston Apr 10, 2022

Good catch, thanks. I've split this into two tests, one for a value field, and one for a ref field, and moved the PROTOTYPE comment to the ref case. scoped and unscoped will be handled in a later PR.

jaredpar reviewed

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Outdated

Comment on lines +638 to +640

+                      s.F =  ref tValue;
+                      s.F =  ref tRef;
+                      s.F =  ref tOut;

Member

jaredpar Apr 11, 2022 •

edited by cston

Loading

These three should all be errors as well. The field is readonly ref here which means the ref cannot be re-pointed but the value is mutable. Hence any ref assignment outside the constructor should be an error. #Resolved

Contributor Author

cston commented Apr 27, 2022

        if (DeriveUseSiteInfoFromType(ref result, this.TypeWithAnnotations, AllowedRequiredModifierType.System_Runtime_CompilerServices_Volatile))

Added a PROTOTYPE comment.

In reply to: 1111564993

Refers to: src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs:351 in 6bfd88c. [](commit_id = 6bfd88c, deletion_comment = False)


          PR feedback

8e51523

AlekseyTs reviewed

View reviewed changes

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

-                              if (fieldSymbol.IsReadOnly)
+                              if ((RequiresAssignableVariable(valueKind) && fieldSymbol.RefKind == RefKind.None) ||
+                                  RequiresRefAssignableVariable(valueKind))

Contributor

AlekseyTs Apr 28, 2022 •

edited

Loading

RequiresRefAssignableVariable(valueKind)

Should this be combined with ref fields only? #Closed


          Misc.

7c8b1e3

AlekseyTs reviewed

View reviewed changes

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

                       {
                           Debug.Assert((object)field != null);
-                          Debug.Assert(RequiresAssignableVariable(kind));
+                          Debug.Assert(RequiresAssignableVariable(kind) && field.RefKind == RefKind.None || RequiresRefAssignableVariable(kind));

Contributor

AlekseyTs Apr 28, 2022 •

edited

Loading

RequiresRefAssignableVariable(kind)

Should this be combined with ref fields only? #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/Core/Portable/PEWriter/MetadataVisitor.cs Outdated

                       public virtual void Visit(IFieldReference fieldReference)
                       {
+                          this.Visit(fieldReference.RefCustomModifiers);

Contributor

AlekseyTs Apr 28, 2022 •

edited

Loading

this.Visit(fieldReference.RefCustomModifiers);

This doesn't look right. We are not visiting type here. Why visit custom modifiers? #Closed

Contributor

AlekseyTs commented Apr 28, 2022 •

edited

Loading

        AssertEx.Equal(new[] { "ref System.Int32 R<T>.F", "ref modopt(System.Int32) T R<T>.F", "ref modopt(System.Object) T R<T>.F" }, fields.ToTestDisplayStrings());

I am curious why private fields are imported, it doesn't look like we are requesting this explicitly

In reply to: 1111602390

In reply to: 1111602390

In reply to: 1111602390

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:484 in 7c8b1e3. [](commit_id = 7c8b1e3, deletion_comment = False)

AlekseyTs reviewed

View reviewed changes

src/Compilers/VisualBasic/Portable/Symbols/Metadata/PE/MemberRefMetadataDecoder.vb

-                                      Dim type As TypeSymbol = Me.DecodeFieldSignature(signaturePointer, customModifiers)
-                                      Return FindFieldBySignature(_containingType, memberName, customModifiers, type)
+                                      Dim fieldInfo As FieldInfo(Of TypeSymbol) = Me.DecodeFieldSignature(signaturePointer)
+                                      Return FindFieldBySignature(_containingType, memberName, fieldInfo.CustomModifiers, fieldInfo.Type)

Contributor

AlekseyTs Apr 28, 2022 •

edited

Loading

FindFieldBySignature(_containingType, memberName, fieldInfo.CustomModifiers, fieldInfo.Type)

It doesn't look right to ignore 'ref' here. Perhaps it is fine to not find a match for a ref field because VB doesn't support ref fields, but ignoring the presence of the 'ref' feels wrong. #Closed

Contributor Author

cston Apr 29, 2022

Added a test and PROTOTYPE comments for now.

Contributor Author

cston commented Apr 28, 2022

        AssertEx.Equal(new[] { "ref System.Int32 R<T>.F", "ref modopt(System.Int32) T R<T>.F", "ref modopt(System.Object) T R<T>.F" }, fields.ToTestDisplayStrings());

PENamedTypeSymbol.CreateFields() imports private fields of ordinary structs.

In reply to: 1111602390

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:484 in 7c8b1e3. [](commit_id = 7c8b1e3, deletion_comment = False)

AlekseyTs reviewed

View reviewed changes

src/Compilers/VisualBasic/Test/Semantic/Semantics/RefFieldTests.vb Outdated

+                      Inherits BasicTestBase
+                      <Fact>
+                      Public Sub RefField()

Contributor

AlekseyTs Apr 28, 2022 •

edited

Loading

RefField

I am not sure what this test is actually testing. Is the field actually imported since it is private? I would at least expect this test to cover PEFieldSymbol behavior, where custom modifiers go, etc. #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs

                           }
                       }
+                      public override RefKind RefKind

Contributor

AlekseyTs Apr 28, 2022 •

edited

Loading

RefKind

Do we have direct tests for the added APIs? #Closed

Contributor Author

cston Apr 28, 2022

Yes, RefFieldTests tests RefKind and RefCustomModifiers for various FieldSymbol implementations.

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs

                                   type = TypeWithAnnotations.Create(new PointerTypeSymbol(TypeWithAnnotations.Create(fixedElementType)));
                               }
+                              ImmutableInterlocked.InterlockedInitialize(ref _lazyRefCustomModifiers, CSharpCustomModifier.Convert(fieldInfo.RefCustomModifiers));

Contributor

AlekseyTs Apr 28, 2022 •

edited

Loading

_lazyRefCustomModifiers

Do we have direct tests asserting where regular and ref modifiers go? #Closed

Contributor Author

cston Apr 28, 2022

Yes.

Contributor

AlekseyTs commented Apr 28, 2022 •

edited

Loading

            if (customModifiersArray.IsEmpty && IsFixedBuffer(out fixedSize, out fixedElementType))

Can ref be combined with fixed buffer?

In reply to: 1111621176

In reply to: 1111621176

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs:309 in 7c8b1e3. [](commit_id = 7c8b1e3, deletion_comment = False)

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/RefReadonlyTests.cs

                               //     void M(readonly ref int p)
-                              Diagnostic(ErrorCode.ERR_SyntaxError, ")").WithArguments("(", ")").WithLocation(4, 30));
+                              Diagnostic(ErrorCode.ERR_SyntaxError, ")").WithArguments(",", ")").WithLocation(4, 30),
+                              // (5,6): error CS1002: ; expected

Contributor

AlekseyTs Apr 28, 2022 •

edited

Loading

// (5,6): error CS1002: ; expected

Consider adding a syntax tree test for this scenario. I am assuming we used to parse parameter as a method and now we parse it as a field. #Closed

Contributor

AlekseyTs commented Apr 28, 2022

Done with review pass (commit 26)

Contributor Author

cston commented Apr 28, 2022

            if (customModifiersArray.IsEmpty && IsFixedBuffer(out fixedSize, out fixedElementType))

Added tests and PROTOTYPE comments to disallow fixed ref declarations and use.

In reply to: 1111621176

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs:309 in 7c8b1e3. [](commit_id = 7c8b1e3, deletion_comment = False)

cston added 4 commits

April 27, 2022 22:34


          PR feedback

2f134c4


          PR feedback

a49068d


          Fix build

e4527ab


          Add comment

e3fda91

Contributor Author

cston commented Apr 29, 2022

@AlekseyTs, @RikkiGibson, please review the latest changes. I believe I have responded to all feedback, thanks.


          Merge remote-tracking branch 'upstream/features/ref-fields' into ref-…

fdee2df

…field-decl

RikkiGibson approved these changes

View reviewed changes

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

-                              if ((RequiresAssignableVariable(valueKind) && fieldSymbol.RefKind == RefKind.None) ||
-                                  RequiresRefAssignableVariable(valueKind))
+                              if (fieldSymbol.RefKind == RefKind.None ? RequiresAssignableVariable(valueKind) : RequiresRefAssignableVariable(valueKind))

Member

RikkiGibson Apr 29, 2022 •

edited by cston

Loading

Is this just a simplification of the same logic that was there previously?

                if ((RequiresAssignableVariable(valueKind) && fieldSymbol.RefKind == RefKind.None) ||
                    RequiresRefAssignableVariable(valueKind))

#Resolved

Contributor Author

cston Apr 29, 2022

It's a response to #60416 (comment). But it is also simpler.

Member

RikkiGibson Apr 29, 2022

Yeah, my mistake. The original form got us in here when RequiredRefAssignableVariable is true and RefKind is None.

AlekseyTs approved these changes

View reviewed changes

Contributor

AlekseyTs left a comment

LGTM (commit 31)

Contributor Author

cston commented May 2, 2022

Thanks for the detailed reviews!

cston merged commit 6b47535 into dotnet:features/ref-fields

cston deleted the ref-field-decl branch

May 2, 2022 14:53

hez2010 commented Jun 8, 2022

I think this PR doesn't cover below scenario:

ref struct Node
{
    ref Node _next; // should be ok, but actually error
}

Contributor Author

cston commented Jun 10, 2022

Thanks @hez2010. I've added an item for "ref field of the containing type" to the test plan.

ryzngard mentioned this pull request

Enable new inline rename by default #62992

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels