Support consumption scenarios for checked user-defined operators#60032
Conversation
333fred
left a comment
There was a problem hiding this comment.
Done review pass (commit 2). A couple of small questions before I sign off.
| if (operators.Count != 0) | ||
| { | ||
| continue; | ||
| for (int i = operators2.Count - 1; i >= 0; i--) |
There was a problem hiding this comment.
Do you think there's a reasonable way to share this code between unary and binary resolution? Maybe with a Func<T, MethodSymbol> getSignature parameter for extracting the method from a signature? This is basically the same code between the two locations. #Resolved
There was a problem hiding this comment.
Do you think there's a reasonable way to share this code between unary and binary resolution?
Perhaps, but I prefer not going through this trouble for just only two call sites.
| } | ||
| } | ||
|
|
||
| if (SyntaxFacts.IsCheckedOperator(method.Name) && |
There was a problem hiding this comment.
Interesting. Does this mean that, if I have a dependency that updates to include such an operator and make no other changes, my code stops compiling? Is it worth confirming with LDM that's the desired behavior? #Resolved
There was a problem hiding this comment.
Interesting. Does this mean that, if I have a dependency that updates to include such an operator and make no other changes, my code stops compiling? Is it worth confirming with LDM that's the desired behavior?
We have chosen to use the checked operator, not simply detected a presence of such declaration
There was a problem hiding this comment.
I do not see anything special about this case by comparison to other features. Adding an operator might break a consumer by changing what it calls or by causing an ambiguity.
| } | ||
|
|
||
| return new BoundNullCoalescingOperator(syntax, rewrittenLeft, rewrittenRight, leftPlaceholder, leftConversion, resultKind, rewrittenResultType); | ||
| return new BoundNullCoalescingOperator(syntax, rewrittenLeft, rewrittenRight, leftPlaceholder, leftConversion, resultKind, @checked: false, rewrittenResultType); |
There was a problem hiding this comment.
Why is false correct to pass here? Shouldn't it be based on whether the original context was checked? #Resolved
There was a problem hiding this comment.
Why is false correct to pass here? Shouldn't it be based on whether the original context was checked?
We are in lowering. All conversions have been figured out, the flag is not interesting.
There was a problem hiding this comment.
I'll add a comment to BoundNodes.xml
|
|
||
| // Note: The trailing expression was already converted to the submission result type in Binder.BindGlobalStatement. | ||
| boundStatements.Add(new BoundReturnStatement(lastStatement.Syntax, RefKind.None, trailingExpression)); | ||
| boundStatements.Add(new BoundReturnStatement(lastStatement.Syntax, RefKind.None, trailingExpression, @checked: false)); |
There was a problem hiding this comment.
What if the last expression was a checked expression? Or would that be fine, because the return itself isn't checked, but the expression it contains is? #Resolved
There was a problem hiding this comment.
What if the last expression was a checked expression? Or would that be fine, because the return itself isn't checked, but the expression it contains is?
This flag is not interesting post lowering. It is only interesting for inference for lambdas and is supposed to reflect the context to use for a possible conversion if return statement is supposed to perform one. The actual expression in this case makes no impact on the context for conversion.
There was a problem hiding this comment.
I'll add a comment to BoundNodes.xml
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 2). The comment to BoundNodes.xml sounds good, thanks.
|
@chsienki, @dotnet/roslyn-compiler For the second review. |
|
@chsienki, @dotnet/roslyn-compiler For the second review. |
|
@chsienki, https://github.com/orgs/dotnet/teams/roslyn-compiler For the second review. |
|
@chsienki, @dotnet/roslyn-compiler For the second review. |
|
Looking |
https://github.com/dotnet/csharplang/blob/main/proposals/checked-user-defined-operators.md