Skip to content

Commit

Permalink
Handle captured primary constructor parameters in readonly context. (#…
Browse files Browse the repository at this point in the history 
…66685)

dotnet/csharplang#6938
  • Loading branch information
@AlekseyTs
AlekseyTs authored Feb 8, 2023
1 parent 7ceec17 commit a2af422
Show file tree
Hide file tree
Showing 4 changed files with 1,115 additions and 54 deletions.
4 changes: 1 addition & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder.CapturedParametersFinder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,7 @@ public static IReadOnlyDictionary<ParameterSymbol, FieldSymbol> GetCapturedParam
// We should define GeneratedNameKind entry (e..g P looks free to use) and add a helper to GeneratedNames that produces the name.
// I'd also just keep the name as short as possible to avoid unnecessary metadata bloat. Could be just <name>P.
string name = "<" + parameter.Name + ">PC__BackingField";

// PROTOTYPE(PrimaryConstructors): Ever read-only?
result.Add(parameter, new SynthesizedPrimaryConstructorParameterBackingFieldSymbol(parameter, name));
result.Add(parameter, new SynthesizedPrimaryConstructorParameterBackingFieldSymbol(parameter, name, isReadOnly: containingType.IsReadOnly));
}

captured.Free();
Expand Down
187 changes: 138 additions & 49 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -689,15 +689,7 @@ internal bool CheckValueKind(SyntaxNode node, BoundExpression expr, BindValueKin
var isValueType = ((BoundThisReference)expr).Type.IsValueType;
if (!isValueType || (RequiresAssignableVariable(valueKind) && (this.ContainingMemberOrLambda as MethodSymbol)?.IsEffectivelyReadOnly == true))
{
var errorCode = GetThisLvalueError(valueKind, isValueType);
if (errorCode is ErrorCode.ERR_InvalidAddrOp or ErrorCode.ERR_IncrementLvalueExpected or ErrorCode.ERR_RefReturnThis or ErrorCode.ERR_RefLocalOrParamExpected or ErrorCode.ERR_RefLvalueExpected)
{
Error(diagnostics, errorCode, node);
}
else
{
Error(diagnostics, errorCode, node, node);
}
ReportThisLvalueError(node, valueKind, isValueType, diagnostics);
return false;
}

Expand Down Expand Up @@ -795,6 +787,19 @@ bool checkArrayAccessValueKind(SyntaxNode node, BindValueKind valueKind, Immutab
}
}

private static void ReportThisLvalueError(SyntaxNode node, BindValueKind valueKind, bool isValueType, BindingDiagnosticBag diagnostics)
{
var errorCode = GetThisLvalueError(valueKind, isValueType);
if (errorCode is ErrorCode.ERR_InvalidAddrOp or ErrorCode.ERR_IncrementLvalueExpected or ErrorCode.ERR_RefReturnThis or ErrorCode.ERR_RefLocalOrParamExpected or ErrorCode.ERR_RefLvalueExpected)
{
Error(diagnostics, errorCode, node);
}
else
{
Error(diagnostics, errorCode, node, node);
}
}

private static bool CheckNotNamespaceOrType(BoundExpression expr, BindingDiagnosticBag diagnostics)
{
switch (expr.Kind)
Expand Down Expand Up @@ -900,6 +905,8 @@ internal partial class Binder
{
private bool CheckParameterValueKind(SyntaxNode node, BoundParameter parameter, BindValueKind valueKind, bool checkingReceiver, BindingDiagnosticBag diagnostics)
{
Debug.Assert(!RequiresAssignableVariable(BindValueKind.AddressOf));

ParameterSymbol parameterSymbol = parameter.ParameterSymbol;

// all parameters can be passed by ref/out or assigned to
Expand All @@ -915,6 +922,36 @@ private bool CheckParameterValueKind(SyntaxNode node, BoundParameter parameter,
return false;
}

Debug.Assert(parameterSymbol.RefKind != RefKind.None || !RequiresRefAssignableVariable(valueKind));

// It is an error to capture 'in', 'ref' or 'out' parameters.
// Skipping them to simplify the logic.
if (parameterSymbol.RefKind == RefKind.None &&
parameterSymbol.ContainingSymbol is SynthesizedPrimaryConstructor primaryConstructor &&
primaryConstructor.GetCapturedParameters().TryGetValue(parameterSymbol, out FieldSymbol backingField))
{
Debug.Assert(backingField.RefKind == RefKind.None);
Debug.Assert(!RequiresRefAssignableVariable(valueKind));

if (backingField.IsReadOnly)
{
Debug.Assert(backingField.RefKind == RefKind.None);

if (RequiresAssignableVariable(valueKind) &&
!CanModifyReadonlyField(receiverIsThis: true, backingField))
{
reportReadOnlyParameterError(parameterSymbol, node, valueKind, checkingReceiver, diagnostics);
return false;
}
}

if (RequiresAssignableVariable(valueKind) && !backingField.ContainingType.IsReferenceType && (this.ContainingMemberOrLambda as MethodSymbol)?.IsEffectivelyReadOnly == true)
{
ReportThisLvalueError(node, valueKind, isValueType: true, diagnostics);
return false;
}
}

if (this.LockedOrDisposedVariables.Contains(parameterSymbol))
{
// Consider: It would be more conventional to pass "symbol" rather than "symbol.Name".
Expand All @@ -926,6 +963,52 @@ private bool CheckParameterValueKind(SyntaxNode node, BoundParameter parameter,

return true;
}

static void reportReadOnlyParameterError(ParameterSymbol parameterSymbol, SyntaxNode node, BindValueKind valueKind, bool checkingReceiver, BindingDiagnosticBag diagnostics)
{
// It's clearer to say that the address can't be taken than to say that the field can't be modified
// (even though the latter message gives more explanation of why).
Debug.Assert(valueKind != BindValueKind.AddressOf); // If this assert fails, we probably should report ErrorCode.ERR_InvalidAddrOp

if (checkingReceiver)
{
ErrorCode errorCode;

if (valueKind == BindValueKind.RefReturn)
{
errorCode = ErrorCode.ERR_RefReturnReadonly2;
}
else if (RequiresRefOrOut(valueKind))
{
errorCode = ErrorCode.ERR_RefReadonly2;
}
else
{
errorCode = ErrorCode.ERR_AssgReadonly2;
}

Error(diagnostics, errorCode, node, parameterSymbol);
}
else
{
ErrorCode errorCode;

if (valueKind == BindValueKind.RefReturn)
{
errorCode = ErrorCode.ERR_RefReturnReadonly;
}
else if (RequiresRefOrOut(valueKind))
{
errorCode = ErrorCode.ERR_RefReadonly;
}
else
{
errorCode = ErrorCode.ERR_AssgReadonly;
}

Error(diagnostics, errorCode, node);
}
}
}

internal partial class RefSafetyAnalysis
Expand Down Expand Up @@ -1030,7 +1113,6 @@ internal partial class Binder
private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess, BindValueKind valueKind, bool checkingReceiver, BindingDiagnosticBag diagnostics)
{
var fieldSymbol = fieldAccess.FieldSymbol;
var fieldIsStatic = fieldSymbol.IsStatic;

if (fieldSymbol.IsReadOnly)
{
Expand All @@ -1041,37 +1123,11 @@ 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.RefKind == RefKind.None ? RequiresAssignableVariable(valueKind) : RequiresRefAssignableVariable(valueKind))
if ((fieldSymbol.RefKind == RefKind.None ? RequiresAssignableVariable(valueKind) : RequiresRefAssignableVariable(valueKind)) &&
!CanModifyReadonlyField(fieldAccess.ReceiverOpt is BoundThisReference, fieldSymbol))
{
var canModifyReadonly = false;

Symbol containing = this.ContainingMemberOrLambda;
if ((object)containing != null &&
fieldIsStatic == containing.IsStatic &&
(fieldIsStatic || fieldAccess.ReceiverOpt.Kind == BoundKind.ThisReference) &&
(Compilation.FeatureStrictEnabled
? TypeSymbol.Equals(fieldSymbol.ContainingType, containing.ContainingType, TypeCompareKind.AllIgnoreOptions)
// We duplicate a bug in the native compiler for compatibility in non-strict mode
: TypeSymbol.Equals(fieldSymbol.ContainingType.OriginalDefinition, containing.ContainingType.OriginalDefinition, TypeCompareKind.AllIgnoreOptions)))
{
if (containing.Kind == SymbolKind.Method)
{
MethodSymbol containingMethod = (MethodSymbol)containing;
MethodKind desiredMethodKind = fieldIsStatic ? MethodKind.StaticConstructor : MethodKind.Constructor;
canModifyReadonly = (containingMethod.MethodKind == desiredMethodKind) ||
isAssignedFromInitOnlySetterOnThis(fieldAccess.ReceiverOpt);
}
else if (containing.Kind == SymbolKind.Field)
{
canModifyReadonly = true;
}
}

if (!canModifyReadonly)
{
ReportReadOnlyFieldError(fieldSymbol, node, valueKind, checkingReceiver, diagnostics);
return false;
}
ReportReadOnlyFieldError(fieldSymbol, node, valueKind, checkingReceiver, diagnostics);
return false;
}
}

Expand Down Expand Up @@ -1099,7 +1155,7 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess,

if (RequiresRefAssignableVariable(valueKind))
{
Debug.Assert(!fieldIsStatic);
Debug.Assert(!fieldSymbol.IsStatic);
Debug.Assert(valueKind == BindValueKind.RefAssignable);

switch (fieldSymbol.RefKind)
Expand All @@ -1116,19 +1172,56 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess,
}

// r/w fields that are static or belong to reference types are writeable and returnable
if (fieldIsStatic || fieldSymbol.ContainingType.IsReferenceType)
if (fieldSymbol.IsStatic || fieldSymbol.ContainingType.IsReferenceType)
{
return true;
}

// for other fields defer to the receiver.
return CheckIsValidReceiverForVariable(node, fieldAccess.ReceiverOpt, valueKind, diagnostics);
}

private bool CanModifyReadonlyField(bool receiverIsThis, FieldSymbol fieldSymbol)
{
// A field is writeable unless
// (1) it is readonly and we are not in a constructor or field initializer
// (2) the receiver of the field is of value type and is not a variable or object creation expression.
// For example, if you have a class C with readonly field f of type S, and
// S has a mutable field x, then c.f.x is not a variable because c.f is not
// writable.

var fieldIsStatic = fieldSymbol.IsStatic;
var canModifyReadonly = false;

Symbol containing = this.ContainingMemberOrLambda;
if ((object)containing != null &&
fieldIsStatic == containing.IsStatic &&
(fieldIsStatic || receiverIsThis) &&
(Compilation.FeatureStrictEnabled
? TypeSymbol.Equals(fieldSymbol.ContainingType, containing.ContainingType, TypeCompareKind.AllIgnoreOptions)
// We duplicate a bug in the native compiler for compatibility in non-strict mode
: TypeSymbol.Equals(fieldSymbol.ContainingType.OriginalDefinition, containing.ContainingType.OriginalDefinition, TypeCompareKind.AllIgnoreOptions)))
{
if (containing.Kind == SymbolKind.Method)
{
MethodSymbol containingMethod = (MethodSymbol)containing;
MethodKind desiredMethodKind = fieldIsStatic ? MethodKind.StaticConstructor : MethodKind.Constructor;
canModifyReadonly = (containingMethod.MethodKind == desiredMethodKind) ||
isAssignedFromInitOnlySetterOnThis(receiverIsThis);
}
else if (containing.Kind == SymbolKind.Field)
{
canModifyReadonly = true;
}
}

return canModifyReadonly;

bool isAssignedFromInitOnlySetterOnThis(BoundExpression receiver)
bool isAssignedFromInitOnlySetterOnThis(bool receiverIsThis)
{
// bad: other.readonlyField = ...
// bad: base.readonlyField = ...
if (!(receiver is BoundThisReference))
if (!receiverIsThis)
{
return false;
}
Expand Down Expand Up @@ -2721,11 +2814,7 @@ private static void ReportReadOnlyFieldError(FieldSymbol field, SyntaxNode node,

// It's clearer to say that the address can't be taken than to say that the field can't be modified
// (even though the latter message gives more explanation of why).
if (kind == BindValueKind.AddressOf)
{
Error(diagnostics, ErrorCode.ERR_InvalidAddrOp, node);
return;
}
Debug.Assert(kind != BindValueKind.AddressOf); // If this assert fails, we probably should report ErrorCode.ERR_InvalidAddrOp

ErrorCode[] ReadOnlyErrors =
{
Expand Down
5 changes: 3 additions & 2 deletions ...e/Symbols/Synthesized/Records/SynthesizedPrimaryConstructorParameterBackingFieldSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ internal sealed class SynthesizedPrimaryConstructorParameterBackingFieldSymbol :

public SynthesizedPrimaryConstructorParameterBackingFieldSymbol(
ParameterSymbol parameterSymbol,
string name)
: base(name, isReadOnly: false, isStatic: false)
string name,
bool isReadOnly)
: base(name, isReadOnly, isStatic: false)
{
ParameterSymbol = parameterSymbol;
}
Expand Down
Loading

0 comments on commit a2af422

Please sign in to comment.