"ref readonly" local functions should be treated as "ref readonly" #18419
Conversation
The code was ignoring "readonly" part for local functions, Fixes:dotnet#18357
|
CC: @OmarTawfik |
|
@dotnet/roslyn-compiler - please review |
| @@ -73,7 +73,7 @@ internal sealed class LocalFunctionSymbol : MethodSymbol | |||
| } | |||
|
|
|||
| _binder = binder; | |||
| _refKind = (syntax.ReturnType.Kind() == SyntaxKind.RefType) ? RefKind.Ref : RefKind.None; | |||
| _syntax.ReturnType.SkipRef(out _refKind); | |||
There was a problem hiding this comment.
Consider adding a helper to make this idiom clearer. "Skipping" the ref to get the RefKind is not very obvious. #Resolved
There was a problem hiding this comment.
Also consider removing this field entirely and just computing the RefKind based on syntax every time. I bet the time/memory trade off here is not useful. Same with SourceMemberMethodSymbol. #Resolved
There was a problem hiding this comment.
we do not keep the syntax. we keep the reference, so it is not as trivial. I think I can make ref kind lazy for local functions.
Not so sure about MemberMethod though - getting return type is complicated there because of overriding. I'll keep it eager for MemberMethod.
In reply to: 109756907 [](ancestors = 109756907)
There was a problem hiding this comment.
I agree with @OmarTawfik. I'd like to see a negative test case here as well.
|
@agocke - looks ok? |
There was a problem hiding this comment.
I meant that you could remove the refkind field and just have a property that computes the value every time. It seems trivial to calculate from the syntax and having an extra bool doesn't seem with it. But this is also fine and had the plus that all the calculation is in the same place. LGTM
|
LGTM |
The code was ignoring "readonly" part for local functions,
Fixes:#18357