Implement ref safety analysis for instance compound assignment operators #78199
Conversation
ghost
added
the
untriaged
jaredpar
removed
the
untriaged
| // return X(c += c1); | ||
| Diagnostic(ErrorCode.ERR_EscapeVariable, "c").WithArguments("scoped C c").WithLocation(7, 18) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Are we testing by-ref parameters? Consider testing the following, with/without [UnscopedRef]:
using System.Diagnostics.CodeAnalysis;
ref struct C
{
private ref readonly int _i;
public void operator +=([UnscopedRef] in int right) { _i = ref right; }
public C M1(C c)
{
return c += 1;
}
}
``` #ResolvedThere was a problem hiding this comment.
Added CompoundAssignment_00752_Consumption_RefSafety and CompoundAssignment_00753_Consumption_RefSafety
|
|
||
| public ref struct C | ||
| { | ||
| [UnscopedRef] |
There was a problem hiding this comment.
Does this attribute have any effect on the test?
I don't think the attribute affects the behavior, it still feels useful to observe.
| // return c += c1; | ||
| Diagnostic(ErrorCode.ERR_EscapeVariable, "c1").WithArguments("scoped C c1").WithLocation(6, 21) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Added as CompoundAssignment_00749_Consumption_RefSafety
| // return Y(c = X(c, c1)); | ||
| Diagnostic(ErrorCode.ERR_EscapeVariable, "c = X(c, c1)").WithArguments("scoped C c").WithLocation(12, 18) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Added UserDefinedBinaryOperator_RefStruct_Compound_ScopedTarget_03
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM except for one question.
| resultKind: LookupResultKind.Viable, | ||
| originalUserDefinedOperatorsOpt: ImmutableArray<MethodSymbol>.Empty, | ||
| leftType); | ||
| getResultType(node, leftType, diagnostics)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| var source = """ | ||
| public ref struct C | ||
| { | ||
| public static C operator +(C left, scoped C right) => left; |
There was a problem hiding this comment.
Added UserDefinedBinaryOperator_RefStruct_Compound_ScopedTarget_04
AlekseyTs
merged commit 96a2dcd
into
dotnet:features/UserDefinedCompoundAssignment
4 participants
No description provided.