Skip to content

Implement ref safety analysis for instance compound assignment operators #78199

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 22, 2025
Merged

Implement ref safety analysis for instance compound assignment operators #78199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c2a32a
Implement ref safety analysis for instance compound assignment operators
AlekseyTs Apr 18, 2025
a34084c
Add requested tests
AlekseyTs Apr 21, 2025
a0bc0b6
Add another test
AlekseyTs Apr 21, 2025
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
30 changes: 4 additions & 26 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4489,6 +4489,8 @@ internal SafeContext GetValEscape(BoundExpression expr, SafeContext localScopeDe
case BoundKind.CompoundAssignmentOperator:
var compound = (BoundCompoundAssignmentOperator)expr;

// https://github.com/dotnet/roslyn/issues/78198 It looks like we don't have a single test demonstrating significance of the code below.

if (compound.Operator.Method is { } compoundMethod)
{
if (compoundMethod.IsStatic)
Expand All @@ -4506,17 +4508,7 @@ internal SafeContext GetValEscape(BoundExpression expr, SafeContext localScopeDe
}
else
{
// PROTOTYPE: Follow-up and test this code path
return GetInvocationEscapeScope(
MethodInfo.Create(compoundMethod),
receiver: compound.Left,
receiverIsSubjectToCloning: ThreeState.False,
compoundMethod.Parameters,
argsOpt: [compound.Right],
argRefKindsOpt: default,
argsToParamsOpt: default,
localScopeDepth: localScopeDepth,
isRefEscape: false);
return GetValEscape(compound.Left, localScopeDepth);
}
}

Expand Down Expand Up @@ -5314,21 +5306,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, SafeContext
}
else
{
// PROTOTYPE: Follow-up and test this code path
return CheckInvocationEscape(
compound.Syntax,
MethodInfo.Create(compoundMethod),
receiver: compound.Left,
receiverIsSubjectToCloning: ThreeState.Unknown,
compoundMethod.Parameters,
argsOpt: [compound.Right],
argRefKindsOpt: default,
argsToParamsOpt: default,
checkingReceiver: checkingReceiver,
escapeFrom: escapeFrom,
escapeTo: escapeTo,
diagnostics,
isRefEscape: false);
return CheckValEscape(compound.Left.Syntax, compound.Left, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics);
}
}

Expand Down
18 changes: 14 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ private BoundExpression BindCompoundAssignment(AssignmentExpressionSyntax node,
leftPlaceholder: null, leftConversion: null, finalPlaceholder: null, finalConversion: null,
resultKind: LookupResultKind.Viable,
originalUserDefinedOperatorsOpt: ImmutableArray<MethodSymbol>.Empty,
leftType);
getResultType(node, leftType, diagnostics));
Copy link
Member

@RikkiGibson RikkiGibson Apr 21, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Could you please explain why we are doing this? I thought that ordinarily, the type of an expression doesn't depend on whether the expression is used. #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Apr 21, 2025

Choose a reason for hiding this comment

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

I thought that ordinarily, the type of an expression doesn't depend on whether the expression is used.

Yes, usually this is the case. However, some forms of ++/-- are not supported when result is used. In addition, lowering of ++/--/ and compound assignments is different (produces much simpler result, and results in much simpler IL) when we know that result is not used. In such cases lowering produce a node with void type (the operator method invocation) and we generally don't like changing types of expressions during lowering. We even have an assert about that in LocalRewriter.VisitExpressionImpl. Basically this change cleans up and aligns implementation for compound operators with implementation for ++/--. The change is also somewhat related to ref safety analysis because it is often performed based on the type of expression.

BTW, there is another example of changing type of expression to void based on whether it is used - conditional access.


methods.Free();
}
Expand All @@ -370,7 +370,7 @@ private BoundExpression BindCompoundAssignment(AssignmentExpressionSyntax node,
leftPlaceholder: null, leftConversion: null, finalPlaceholder: null, finalConversion: null,
resultKind: LookupResultKind.OverloadResolutionFailure,
originalUserDefinedOperatorsOpt: methodsArray,
leftType);
getResultType(node, leftType, diagnostics));
}
else
{
Expand All @@ -383,6 +383,11 @@ private BoundExpression BindCompoundAssignment(AssignmentExpressionSyntax node,

return inPlaceResult;
}

TypeSymbol getResultType(ExpressionSyntax node, TypeSymbol leftType, BindingDiagnosticBag diagnostics)
{
return ResultIsUsed(node) ? leftType : GetSpecialType(SpecialType.System_Void, diagnostics, node);
}
}

#nullable disable
Expand Down Expand Up @@ -2609,7 +2614,7 @@ private BoundExpression BindIncrementOperator(ExpressionSyntax node, ExpressionS
resultConversion: null,
LookupResultKind.Viable,
ImmutableArray<MethodSymbol>.Empty,
resultIsUsed ? operandType : GetSpecialType(SpecialType.System_Void, diagnostics, node));
getResultType(node, operandType, resultIsUsed, diagnostics));

methods.Free();
}
Expand All @@ -2634,7 +2639,7 @@ private BoundExpression BindIncrementOperator(ExpressionSyntax node, ExpressionS
resultConversion: null,
LookupResultKind.OverloadResolutionFailure,
methodsArray,
resultIsUsed ? operandType : GetSpecialType(SpecialType.System_Void, diagnostics, node));
getResultType(node, operandType, resultIsUsed, diagnostics));
}
else
{
Expand All @@ -2647,6 +2652,11 @@ private BoundExpression BindIncrementOperator(ExpressionSyntax node, ExpressionS

return inPlaceResult;
}

TypeSymbol getResultType(ExpressionSyntax node, TypeSymbol operandType, bool resultIsUsed, BindingDiagnosticBag diagnostics)
{
return resultIsUsed ? operandType : GetSpecialType(SpecialType.System_Void, diagnostics, node);
}
}

private ArrayBuilder<MethodSymbol>? LookupUserDefinedInstanceOperators(TypeSymbol lookupInType, string? checkedName, string ordinaryName, int parameterCount, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
Expand Down
35 changes: 35 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,41 @@ private void RemoveLocalScopes(LocalSymbol local)
public override BoundNode? VisitCompoundAssignmentOperator(BoundCompoundAssignmentOperator node)
{
base.VisitCompoundAssignmentOperator(node);

if (!node.HasErrors && node.Operator.Method is { } compoundMethod)
{
if (compoundMethod.IsStatic)
{
CheckInvocationArgMixing(
node.Syntax,
MethodInfo.Create(compoundMethod),
receiverOpt: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
compoundMethod.Parameters,
argsOpt: [node.Left, node.Right],
argRefKindsOpt: default,
argsToParamsOpt: default,
_localScopeDepth,
_diagnostics);
}
else
{
CheckInvocationArgMixing(
node.Syntax,
MethodInfo.Create(compoundMethod),
receiverOpt: node.Left,
receiverIsSubjectToCloning: ThreeState.False,
compoundMethod.Parameters,
argsOpt: [node.Right],
argRefKindsOpt: default,
argsToParamsOpt: default,
_localScopeDepth,
_diagnostics);

return null;
}
}

ValidateAssignment(node.Syntax, node.Left, node, isRef: false, _diagnostics);
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/MethodSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ internal static bool IsValidUnscopedRefAttributeTarget(this MethodSymbol method)
return !method.IsStatic &&
method.ContainingType is NamedTypeSymbol containingType &&
(containingType.IsStructType() == true || (containingType.IsInterface && method.IsImplementable())) &&
method.MethodKind is (MethodKind.Ordinary or MethodKind.ExplicitInterfaceImplementation or MethodKind.PropertyGet or MethodKind.PropertySet) &&
method.MethodKind is (MethodKind.Ordinary or MethodKind.ExplicitInterfaceImplementation or MethodKind.PropertyGet or MethodKind.PropertySet or MethodKind.UserDefinedOperator) &&
!method.IsInitOnly;
}

Expand Down
Loading