Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ref field declarations from source and metadata #60416

Merged
merged 31 commits into from
May 2, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,12 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess,
}
}

if (fieldSymbol.RefKind == RefKind.RefReadOnly)
{
ReportReadOnlyError(fieldSymbol, node, valueKind, checkingReceiver, diagnostics);
return false;
}

if (fieldSymbol.IsFixedSizeBuffer)
{
Error(diagnostics, GetStandardLvalueError(valueKind), node);
Expand All @@ -838,8 +844,18 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess,

if (RequiresRefAssignableVariable(valueKind))
{
Error(diagnostics, ErrorCode.ERR_RefLocalOrParamExpected, node);
return false;
switch (fieldSymbol.RefKind)
{
case RefKind.None:
Debug.Assert(fieldSymbol.RefKind == RefKind.None);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
Error(diagnostics, ErrorCode.ERR_RefLocalOrParamExpected, node);
return false;
case RefKind.Ref:
case RefKind.RefReadOnly:
return true;
default:
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
throw ExceptionUtilities.UnexpectedValue(fieldSymbol.RefKind);
}
}

// r/w fields that are static or belong to reference types are writeable and returnable
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7076,6 +7076,16 @@ protected BoundExpression BindFieldAccess(
CheckReceiverAndRuntimeSupportForSymbolAccess(node, receiver, fieldSymbol, diagnostics);
}

// If this is a ref field from another compilation, check for support for ref fields.
// No need to check for a reference to a field declared in this compilation since
// we check at the declaration site. (Check RefKind after IsFromCompilation() to
// avoid cycles for source symbols.
if (!fieldSymbol.OriginalDefinition.IsFromCompilation(Compilation) &&
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fieldSymbol.OriginalDefinition.IsFromCompilation(Compilation)

Consider comparing containing module instead. i.e. Compilation.SourceModule != fieldSymbol.ContainingModule #Closed

fieldSymbol.RefKind != RefKind.None)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
CheckFeatureAvailability(node, MessageID.IDS_FeatureRefFields, diagnostics);
}

TypeSymbol fieldType = fieldSymbol.GetFieldType(this.FieldsBeingBound).Type;
BoundExpression expr = new BoundFieldAccess(node, receiver, fieldSymbol, constantValueOpt, resultKind, fieldType, hasErrors: (hasErrors || hasError));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ public static RefKind GetRefKind(this BoundExpression node)
case BoundKind.Parameter:
return ((BoundParameter)node).ParameterSymbol.RefKind;

case BoundKind.FieldAccess:
return ((BoundFieldAccess)node).FieldSymbol.RefKind;

case BoundKind.Call:
return ((BoundCall)node).Method.RefKind;

Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6642,6 +6642,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureStructFieldInitializers" xml:space="preserve">
<value>struct field initializers</value>
</data>
<data name="IDS_FeatureRefFields" xml:space="preserve">
<value>ref fields</value>
</data>
<data name="IDS_FeatureVarianceSafetyForStaticInterfaceMembers" xml:space="preserve">
<value>variance safety for static interface members</value>
</data>
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ private LocalDefinition EmitSequenceAddress(BoundSequence sequence, AddressKind
return result;
}

private LocalSymbol DigForValueLocal(BoundSequence topSequence, BoundExpression value)
private static LocalSymbol DigForValueLocal(BoundSequence topSequence, BoundExpression value)
{
switch (value.Kind)
{
Expand Down Expand Up @@ -546,7 +546,7 @@ private LocalDefinition EmitInstanceFieldAddress(BoundFieldAccess fieldAccess, A
// taking field addresses, so we have to turn Constrained into writeable.
var tempOpt = EmitReceiverRef(fieldAccess.ReceiverOpt, addressKind == AddressKind.Constrained ? AddressKind.Writeable : addressKind);

_builder.EmitOpCode(ILOpCode.Ldflda);
_builder.EmitOpCode(field.RefKind == RefKind.None ? ILOpCode.Ldflda : ILOpCode.Ldfld);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
EmitSymbolToken(field, fieldAccess.Syntax);

// when loading an address of a fixed field, we actually
Expand Down
50 changes: 38 additions & 12 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,20 @@ private void EmitFieldLoad(BoundFieldAccess fieldAccess, bool used)
Debug.Assert(!field.IsConst || field.ContainingType.SpecialType == SpecialType.System_Decimal,
"rewriter should lower constant fields into constant expressions");

EmitFieldLoadNoIndirection(fieldAccess, used);

if (used && field.RefKind != RefKind.None)
{
EmitLoadIndirect(field.Type, fieldAccess.Syntax);
}

EmitPopIfUnused(used);
}

private void EmitFieldLoadNoIndirection(BoundFieldAccess fieldAccess, bool used)
{
var field = fieldAccess.FieldSymbol;

// static field access is sideeffecting since it guarantees that ..ctor has run.
// we emit static accesses even if unused.
if (field.IsStatic)
Expand Down Expand Up @@ -1062,7 +1076,6 @@ private void EmitFieldLoad(BoundFieldAccess fieldAccess, bool used)
EmitSymbolToken(field, fieldAccess.Syntax);
}
}
EmitPopIfUnused(used);
}

private LocalDefinition EmitFieldLoadReceiver(BoundExpression receiver)
Expand Down Expand Up @@ -1128,10 +1141,10 @@ private bool EmitFieldLoadReceiverAddress(BoundExpression receiver)
return false;
}

// ldfld can work with structs directly or with their addresses
// ldfld can work with structs directly or with their addresses.
// In some cases it results in same native code emitted, but in some cases JIT pushes values for real
// resulting in much worse code (on x64 in particular).
// So, we will always prefer references here except when receiver is a struct non-ref local or parameter.
// So, we will always prefer references here except when receiver is a struct, non-ref local, or parameter.
private bool FieldLoadPrefersRef(BoundExpression receiver)
{
// only fields of structs can be accessed via value
Expand Down Expand Up @@ -2089,7 +2102,7 @@ private void EmitAssignmentExpression(BoundAssignmentOperator assignmentOperator
EmitAssignmentPostfix(assignmentOperator, temp, useKind);
}

// sometimes it is possible and advantageous to get an address of the lHS and
// sometimes it is possible and advantageous to get an address of the LHS and
// perform assignment as an in-place initialization via initobj or constructor invocation.
//
// 1) initobj
Expand Down Expand Up @@ -2309,10 +2322,15 @@ private bool EmitAssignmentPreamble(BoundAssignmentOperator assignmentOperator)
case BoundKind.FieldAccess:
{
var left = (BoundFieldAccess)assignmentTarget;
if (!left.FieldSymbol.IsStatic)
if (left.FieldSymbol.RefKind != RefKind.None &&
!assignmentOperator.IsRef)
{
EmitFieldLoadNoIndirection(left, used: true);
}
else if (!left.FieldSymbol.IsStatic)
{
var temp = EmitReceiverRef(left.ReceiverOpt, AddressKind.Writeable);
Debug.Assert(temp == null, "temp is unexpected when assigning to a field");
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug.Assert(temp == null, "temp is unexpected when assigning to a field");

Is there a specific reason to drop the assert? #Closed

FreeOptTemp(temp);
lhsUsesStack = true;
}
}
Expand Down Expand Up @@ -2586,7 +2604,7 @@ private void EmitStore(BoundAssignmentOperator assignment)
switch (expression.Kind)
{
case BoundKind.FieldAccess:
EmitFieldStore((BoundFieldAccess)expression);
EmitFieldStore((BoundFieldAccess)expression, assignment.IsRef);
break;

case BoundKind.Local:
Expand Down Expand Up @@ -2794,7 +2812,7 @@ private void EmitVectorElementStore(ArrayTypeSymbol arrayType, SyntaxNode syntax
}
}

private void EmitFieldStore(BoundFieldAccess fieldAccess)
private void EmitFieldStore(BoundFieldAccess fieldAccess, bool refAssign)
{
var field = fieldAccess.FieldSymbol;

Expand All @@ -2803,14 +2821,21 @@ private void EmitFieldStore(BoundFieldAccess fieldAccess)
_builder.EmitOpCode(ILOpCode.Volatile);
}

_builder.EmitOpCode(field.IsStatic ? ILOpCode.Stsfld : ILOpCode.Stfld);
EmitSymbolToken(field, fieldAccess.Syntax);
if (field.RefKind != RefKind.None && !refAssign)
{
//NOTE: we should have the actual field already loaded,
//now need to do a store to where it points to
EmitIndirectStore(field.Type, fieldAccess.Syntax);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EmitIndirectStore(field.Type, fieldAccess.Syntax);

It looks like we are applying volatile semantics to the field as well as to the underlying value. Is this intentional? Perhaps specification should mention that explicitly. Based on the placement of the required custom modifier, it applies to the underlying value rather than to the ref. #Closed

Copy link
Member Author

@cston cston Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec has been updated to include the following:

A ref field cannot be declared static, volatile or const

And the test plan includes an item to disallow volatile ref fields.

}
else
{
_builder.EmitOpCode(field.IsStatic ? ILOpCode.Stsfld : ILOpCode.Stfld);
EmitSymbolToken(field, fieldAccess.Syntax);
}
}

private void EmitParameterStore(BoundParameter parameter, bool refAssign)
{
int slot = ParameterSlot(parameter);

if (parameter.ParameterSymbol.RefKind != RefKind.None && !refAssign)
{
//NOTE: we should have the actual parameter already loaded,
Expand All @@ -2819,6 +2844,7 @@ private void EmitParameterStore(BoundParameter parameter, bool refAssign)
}
else
{
int slot = ParameterSlot(parameter);
_builder.EmitStoreArgumentOpcode(slot);
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1034,6 +1034,7 @@ private void EmitCatchBlock(BoundCatchBlock catchBlock)
var left = (BoundFieldAccess)exceptionSource;
Debug.Assert(!left.FieldSymbol.IsStatic, "Not supported");
Debug.Assert(!left.ReceiverOpt.Type.IsTypeParameter());
Debug.Assert(left.FieldSymbol.RefKind == RefKind.None);

var stateMachineField = left.FieldSymbol as StateMachineFieldSymbol;
if (((object)stateMachineField != null) && (stateMachineField.SlotIndex >= 0))
Expand All @@ -1052,7 +1053,7 @@ private void EmitCatchBlock(BoundCatchBlock catchBlock)
_builder.EmitLocalLoad(temp);
FreeTemp(temp);

EmitFieldStore(left);
EmitFieldStore(left, refAssign: false);
break;

default:
Expand Down
5 changes: 2 additions & 3 deletions src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1022,8 +1022,7 @@ private static bool IsIndirectAssignment(BoundAssignmentOperator node)
var lhs = node.Left;

Debug.Assert(!node.IsRef ||
(lhs is BoundLocal local && local.LocalSymbol.RefKind != RefKind.None) ||
(lhs is BoundParameter param && param.ParameterSymbol.RefKind != RefKind.None),
(lhs.Kind is BoundKind.Local or BoundKind.Parameter or BoundKind.FieldAccess && lhs.GetRefKind() != RefKind.None),
"only ref symbols can be a target of a ref assignment");

switch (lhs.Kind)
Expand Down Expand Up @@ -2055,7 +2054,7 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
}

// indirect local store is not special. (operands still could be rewritten)
// NOTE: if Lhs is a stack local, it will be handled as a read and possibly duped.
// NOTE: if lhs is a stack local, it will be handled as a read and possibly duped.
var isIndirectLocalStore = left.LocalSymbol.RefKind != RefKind.None && !node.IsRef;
if (isIndirectLocalStore)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ Cci.ITypeReference Cci.IFieldReference.GetType(EmitContext context)
}
}

ImmutableArray<Cci.ICustomModifier> Cci.IFieldReference.RefCustomModifiers =>
ImmutableArray<Cci.ICustomModifier>.CastUp(AdaptedFieldSymbol.RefCustomModifiers);

bool Cci.IFieldReference.IsByReference => AdaptedFieldSymbol.RefKind != RefKind.None;

Cci.IFieldDefinition Cci.IFieldReference.GetResolvedField(EmitContext context)
{
return ResolvedFieldImpl((PEModuleBuilder)context.Module);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ Cci.ITypeReference Cci.IFieldReference.GetType(EmitContext context)
}
}

ImmutableArray<Cci.ICustomModifier> Cci.IFieldReference.RefCustomModifiers =>
ImmutableArray<Cci.ICustomModifier>.CastUp(_underlyingField.RefCustomModifiers);

bool Cci.IFieldReference.IsByReference => _underlyingField.RefKind != RefKind.None;

Cci.IFieldDefinition Cci.IFieldReference.GetResolvedField(EmitContext context)
{
return null;
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ internal enum MessageID
IDS_FeatureCacheStaticMethodGroupConversion = MessageBase + 12816,
IDS_FeatureRawStringLiterals = MessageBase + 12817,
IDS_FeatureDisposalPattern = MessageBase + 12818,
IDS_FeatureRefFields = MessageBase + 12819,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -361,6 +362,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureListPattern: // semantic check
case MessageID.IDS_FeatureCacheStaticMethodGroupConversion: // lowering check
case MessageID.IDS_ParameterNullChecking: // syntax check
case MessageID.IDS_FeatureRefFields: // semantic check
return LanguageVersion.Preview;

// C# 10.0 features.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,17 +221,8 @@ private BoundExpression MakeStaticAssignmentOperator(
}

case BoundKind.Local:
{
Debug.Assert(!isRef || ((BoundLocal)rewrittenLeft).LocalSymbol.RefKind != RefKind.None);
return new BoundAssignmentOperator(
syntax,
rewrittenLeft,
rewrittenRight,
type,
isRef: isRef);
}

case BoundKind.Parameter:
case BoundKind.FieldAccess:
{
Debug.Assert(!isRef || rewrittenLeft.GetRefKind() != RefKind.None);
return new BoundAssignmentOperator(
Expand Down
10 changes: 5 additions & 5 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2874,7 +2874,7 @@ private MemberDeclarationSyntax ParseMemberDeclarationCore(SyntaxKind parentKind
parse_member_name:;
ExplicitInterfaceSpecifierSyntax explicitInterfaceOpt;

// If we've seen the ref keyword, we know we must have an indexer, method, or property.
// If we've seen the ref keyword, we know we must have an indexer, method, field, or property.
if (type.Kind != SyntaxKind.RefType)
{
// Check here for operators
Expand All @@ -2883,11 +2883,11 @@ private MemberDeclarationSyntax ParseMemberDeclarationCore(SyntaxKind parentKind
{
return this.ParseOperatorDeclaration(attributes, modifiers, type, explicitInterfaceOpt);
}
}

if (IsFieldDeclaration(isEvent: false))
{
return this.ParseNormalFieldDeclaration(attributes, modifiers, type, parentKind);
}
if (IsFieldDeclaration(isEvent: false))
{
return this.ParseNormalFieldDeclaration(attributes, modifiers, type, parentKind);
}

// At this point we can either have indexers, methods, or
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ public override void VisitField(IFieldSymbol symbol)
this.isFirstSymbolVisited &&
!IsEnumMember(symbol))
{
switch (symbol.RefKind)
{
case RefKind.Ref:
AddRefIfRequired();
break;
case RefKind.RefReadOnly:
AddRefReadonlyIfRequired();
break;
}

AddCustomModifiersIfRequired(symbol.RefCustomModifiers);

VisitFieldType(symbol);
AddSpace();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsB
return _property.TypeWithAnnotations;
}

public override RefKind RefKind => RefKind.None;

public override ImmutableArray<CustomModifier> RefCustomModifiers => ImmutableArray<CustomModifier>.Empty;

public override string Name
{
get { return GeneratedNames.MakeAnonymousTypeBackingFieldName(_property.Name); }
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ public TypeWithAnnotations TypeWithAnnotations
}
}

public abstract RefKind RefKind { get; }

public abstract ImmutableArray<CustomModifier> RefCustomModifiers { get; }

public abstract FlowAnalysisAnnotations FlowAnalysisAnnotations { get; }

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@ internal Symbol FindMember(MemberReferenceHandle memberRef, bool methodsOnly)
return null;
}

ImmutableArray<ModifierInfo<TypeSymbol>> customModifiers;
TypeSymbol type = this.DecodeFieldSignature(ref signaturePointer, out customModifiers);
return FindFieldBySignature(_containingType, memberName, customModifiers, type);
FieldInfo<TypeSymbol> fieldInfo;
this.DecodeFieldSignature(ref signaturePointer, out fieldInfo);
return FindFieldBySignature(_containingType, memberName, fieldInfo.CustomModifiers, fieldInfo.Type);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FindFieldBySignature

It feels like ref-ness and ref-custom-modifiers should be taken into consideration #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added checks for RefKind and RefCustomModifiers, however it's not clear that we can hit a case where the decoded field does not match, even without the current Type or CustomModifiers comparisons.


default:
// error: unexpected calling convention
Expand Down
Loading