fix ref returns in expression bodied local functions#42333
Conversation
| RefKind refKind = RefKind.None; | ||
| ExpressionSyntax expressionSyntax = expressionBody.Expression.CheckAndUnwrapRefExpression(diagnostics, out refKind); | ||
| BindValueKind requiredValueKind = GetRequiredReturnValueKind(refKind); | ||
| ExpressionSyntax expressionSyntax = expressionBody.Expression.CheckAndUnwrapRefExpression(diagnostics, out var refKind); |
There was a problem hiding this comment.
var [](start = 119, length = 3)
Please revert the change. At the very least the type of the out declaration should be spelled out #Closed
| ExpressionSyntax expressionSyntax = expressionBody.Expression.CheckAndUnwrapRefExpression(diagnostics, out var refKind); | ||
| BindValueKind requiredValueKind = bodyBinder.GetRequiredReturnValueKind(refKind); | ||
| BoundExpression expression = bodyBinder.BindValue(expressionSyntax, diagnostics, requiredValueKind); | ||
| expression = ValidateEscape(expression, Binder.ExternalScope, refKind != RefKind.None, diagnostics); |
There was a problem hiding this comment.
ValidateEscape [](start = 25, length = 14)
It looks like this is an instance method. Let's use bodyBinder to call it as well. Since it is very easy to make a mistake and call something on this, consider extracting a helper method, either a static local function, or a new instance method which will be called with only the right binder available to them. #Closed
| CreateCompilation(source).VerifyDiagnostics( | ||
| // (4,17): warning CS8321: The local function 'M1' is declared but never used | ||
| // ref int M1(in int i) => ref i; | ||
| Diagnostic(ErrorCode.WRN_UnreferencedLocalFunction, "M1").WithArguments("M1").WithLocation(4, 17), |
There was a problem hiding this comment.
WRN_UnreferencedLocalFunction [](start = 37, length = 29)
Consider adding a #pragma to disable this warning for the test scenario. #Closed
| class C { | ||
| ref int M(){ | ||
| throw new System.Exception(); | ||
| ref readonly int M1(in int i) => ref i; |
There was a problem hiding this comment.
ref readonly int M1(in int i) => ref i; [](start = 8, length = 39)
It looks like these scenarios are tested above? Is there something special about this specific scenario? #Closed
There was a problem hiding this comment.
This is a different bug caused by the same underlying issue, namely #42259 (comment)
| CreateCompilation(source).VerifyDiagnostics( | ||
| // (5,26): warning CS8321: The local function 'M1' is declared but never used | ||
| // ref readonly int M1(in int i) => ref i; | ||
| Diagnostic(ErrorCode.WRN_UnreferencedLocalFunction, "M1").WithArguments("M1").WithLocation(5, 26), |
There was a problem hiding this comment.
WRN_UnreferencedLocalFunction [](start = 41, length = 29)
Same comment, consider adding usage, or disabling the warning with pragma. #Closed
|
Done with review pass (iteration 2) #Closed |
| Diagnostic(ErrorCode.ERR_RefReturnReadonlyNotField, "i").WithArguments("variable", "in int").WithLocation(5, 37), | ||
| // (6,43): error CS8333: Cannot return variable 'in int' by writable reference because it is a readonly variable | ||
| // ref int M2(in int i) { return ref i; } | ||
| Diagnostic(ErrorCode.ERR_RefReturnReadonlyNotField, "i").WithArguments("variable", "in int").WithLocation(6, 43)); |
There was a problem hiding this comment.
); [](start = 132, length = 2)
Consider moving ); on a new line. Makes it easier to add/remove expected diagnostics in the future. Also, it looks like the lines we shifted to the right on an extra tab in the 3rd commit. We usually use single tab (4 spaces) offset. #Closed
|
|
||
| return bodyBinder.CreateBlockFromExpression(expressionBody, bodyBinder.GetDeclaredLocalsForScope(expressionBody), refKind, expression, expressionSyntax, diagnostics); | ||
| // Use static local function to prevent accidentally calling instance methods on `this` instead of `bodyBinder` | ||
| static BoundBlock BindExpressionBodyAsBlockInternal(ArrowExpressionClauseSyntax expressionBody, Binder bodyBinder, DiagnosticBag diagnostics) |
There was a problem hiding this comment.
BindExpressionBodyAsBlockInternal [](start = 30, length = 33)
We have a convention to start names of local functions with a small letter. #Closed
jaredpar
left a comment
There was a problem hiding this comment.
Looks good module minor style issue. Can you get those cleaned up so that we can merge?
Co-Authored-By: Jared Parsons <jaredpparsons@gmail.com>
Co-Authored-By: Jared Parsons <jaredpparsons@gmail.com>
|
@jaredpar Done |
…tps://github.com/yairhalberstadt/roslyn into fix-ref-return-expression-bodied-local-functions
|
@YairHalberstadt Thanks for the contribution. |
Fix #42259 caused by use of incorrect binder when checking ref return type.