Support ref field declarations from source and metadata #60416
Conversation
cston
force-pushed
the
ref-field-decl
branch
6 times, most recently
from
16dca7b
to
a0543aa
Compare
cston
force-pushed
the
ref-field-decl
branch
3 times, most recently
from
fcfce5f
to
d463a86
Compare
src/Compilers/CSharp/Portable/Symbols/Source/FieldSymbolWithAttributesAndModifiers.cs
Show resolved
Hide resolved
| 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())); |
There was a problem hiding this comment.
nit: prefer Assert.Empty #Resolved
| [Fact] | ||
| public void DefiniteAssignment_02() | ||
| { | ||
| // Should we report a warning when assigning a value rather than a ref in the |
There was a problem hiding this comment.
It seems like this should have a PROTOTYPE marker or tracking issue. #Resolved
There was a problem hiding this comment.
There's an open question for language design in the test plan so I'll leave this as a regular comment.
| // (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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Added a PROTOTYPE comment.
| @"ref struct S<T> | ||
| { | ||
| public ref T F; | ||
| public ref T F1() => ref F; |
There was a problem hiding this comment.
Consider using unique method names across the test to make it a little easier to relate the diagnostics to the source. #Resolved
| IL_0000: ldarga.s V_0 | ||
| IL_0002: ldfld ""ref T S<T>.F"" | ||
| IL_0007: ldloca.s V_0 | ||
| IL_0009: initobj ""T"" |
There was a problem hiding this comment.
I didn't understand why there's an initobj here. Consider including a PROTOTYPE comment. #Resolved
There was a problem hiding this comment.
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).
| Console.WriteLine((i, s)); | ||
| } | ||
| }"; | ||
| CompileAndVerify(source, verify: Verification.Skipped, expectedOutput: IncludeExpectedOutput(@"(1, Hello world)")); |
There was a problem hiding this comment.
Should we verify the IL of the deconstruction assignment itself? #Resolved
| // (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. |
There was a problem hiding this comment.
nit: we'll want to adjust this diagnostic language when we get the chance. Perhaps to simply say ref variable. #Resolved
| } | ||
| }"; | ||
| var comp = CreateCompilation(source); | ||
| // PROTOTYPE: Should not report ERR_RefReturnStructThis. |
There was a problem hiding this comment.
Spec says (deep in the detailed design section):
When
scopedis applied to an instance method thethisparameter will have the typescoped ref T. By default this is redundant asscoped ref Tis the default type ofthis. #Resolved
There was a problem hiding this comment.
Therefore I expected an error here, but no error once we are able to declare the unscoped version of the method.
There was a problem hiding this comment.
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.
| s.F = ref tValue; | ||
| s.F = ref tRef; | ||
| s.F = ref tOut; |
There was a problem hiding this comment.
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
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) |
| @@ -796,7 +796,8 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess, | |||
| // S has a mutable field x, then c.f.x is not a variable because c.f is not | |||
| // writable. | |||
|
|
|||
| if (fieldSymbol.IsReadOnly) | |||
| if ((RequiresAssignableVariable(valueKind) && fieldSymbol.RefKind == RefKind.None) || | |||
| RequiresRefAssignableVariable(valueKind)) | |||
| @@ -1890,7 +1919,7 @@ private static ErrorCode GetStandardRValueRefEscapeError(uint escapeTo) | |||
| private static void ReportReadOnlyFieldError(FieldSymbol field, SyntaxNode node, BindValueKind kind, bool checkingReceiver, BindingDiagnosticBag diagnostics) | |||
| { | |||
| Debug.Assert((object)field != null); | |||
| Debug.Assert(RequiresAssignableVariable(kind)); | |||
| Debug.Assert(RequiresAssignableVariable(kind) && field.RefKind == RefKind.None || RequiresRefAssignableVariable(kind)); | |||
| this.Visit(fieldDefinition.GetType(Context)); | ||
| } | ||
|
|
||
| public virtual void Visit(IFieldReference fieldReference) | ||
| { | ||
| this.Visit(fieldReference.RefCustomModifiers); |
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) |
| 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) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Added a test and PROTOTYPE comments for now.
In reply to: 1111602390 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:484 in 7c8b1e3. [](commit_id = 7c8b1e3, deletion_comment = False) |
| Inherits BasicTestBase | ||
|
|
||
| <Fact> | ||
| Public Sub RefField() |
| @@ -322,6 +350,24 @@ private PEModuleSymbol ContainingPEModule | |||
| } | |||
| } | |||
|
|
|||
| public override RefKind RefKind | |||
There was a problem hiding this comment.
Yes, RefFieldTests tests RefKind and RefCustomModifiers for various FieldSymbol implementations.
| @@ -287,6 +313,8 @@ private void EnsureSignatureIsLoaded() | |||
| type = TypeWithAnnotations.Create(new PointerTypeSymbol(TypeWithAnnotations.Create(fixedElementType))); | |||
| } | |||
|
|
|||
| ImmutableInterlocked.InterlockedInitialize(ref _lazyRefCustomModifiers, CSharpCustomModifier.Convert(fieldInfo.RefCustomModifiers)); | |||
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) |
| // 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 |
|
Done with review pass (commit 26) |
Added tests and PROTOTYPE comments to disallow 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, @RikkiGibson, please review the latest changes. I believe I have responded to all feedback, thanks. |
| @@ -796,8 +796,7 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess, | |||
| // S has a mutable field x, then c.f.x is not a variable because c.f is not | |||
| // writable. | |||
|
|
|||
| if ((RequiresAssignableVariable(valueKind) && fieldSymbol.RefKind == RefKind.None) || | |||
| RequiresRefAssignableVariable(valueKind)) | |||
| if (fieldSymbol.RefKind == RefKind.None ? RequiresAssignableVariable(valueKind) : RequiresRefAssignableVariable(valueKind)) | |||
There was a problem hiding this comment.
Is this just a simplification of the same logic that was there previously?
if ((RequiresAssignableVariable(valueKind) && fieldSymbol.RefKind == RefKind.None) ||
RequiresRefAssignableVariable(valueKind))#Resolved
There was a problem hiding this comment.
It's a response to #60416 (comment). But it is also simpler.
There was a problem hiding this comment.
Yeah, my mistake. The original form got us in here when RequiredRefAssignableVariable is true and RefKind is None.
|
Thanks for the detailed reviews! |
|
I think this PR doesn't cover below scenario: ref struct Node
{
ref Node _next; // should be ok, but actually error
} |
Proposal: low-level-struct-improvements.md
Test plan: #59194
Included:
IFieldSymbol.RefKindandIFieldSymbol.RefCustomModifiersrefandref readonlymodifiers for fieldsreffields as l-values and r-valuesreffields as l-values and r-valuesByReferenceandRefCustomModifiers-langversion: report error forrefdeclarations and use-sites with-langversion:10or belowSymbolDisplay: includerefmodifiers andRefCustomModifiersfrom C#Not included:
reffield from VBrefauto-properties