Ensure proper receiver value is used for a constrained call invocation #65642
Conversation
| private enum ReceiverCaptureMode | ||
| { | ||
| /// <summary> | ||
| /// No special cupture of the receiver, unless arguments need to refere to it. |
There was a problem hiding this comment.
cupture -> capture
refere -> refer #Resolved
| Default = 0, | ||
|
|
||
| /// <summary> | ||
| /// Used for a regular indexer coumpond assignment rewrite. |
There was a problem hiding this comment.
coumpond -> compound #Resolved
|
|
||
| BoundLocal cache = _factory.Local(_factory.SynthesizedLocal(receiverType)); | ||
|
|
||
| tempsOpt.Add(cache.LocalSymbol); |
There was a problem hiding this comment.
Consider not naming this Opt, as it's required. #Resolved
| && !receiverRefLocal.Type.IsReferenceType | ||
| && !receiverRefLocal.Type.IsValueType |
There was a problem hiding this comment.
Could these be incorporated into the pattern? #Resolved
There was a problem hiding this comment.
Could these be incorporated into the pattern?
In theory we could, but I prefer to keep this check out of the pattern that is already big enough to comprehend and keep track of what is actually checked there.
|
|
||
| var cache = _F.Local(_F.SynthesizedLocal(receiverType)); | ||
| receiverBuilder.AddLocal(cache.LocalSymbol); | ||
| receiverBuilder.AddStatement(_F.If(_F.Not(isValueTypeCheck), _F.Assignment(cache, receiver))); |
There was a problem hiding this comment.
Should we be asserting that this assignment is in line with IsComplexConditionalInitializationOfReceiverRef, like we do in LocalRewriter_Call? #Resolved
There was a problem hiding this comment.
Should we be asserting that this assignment is in line with
IsComplexConditionalInitializationOfReceiverRef, like we do inLocalRewriter_Call?
It is not in line with that and the assert would fail. An assignment that we create here doesn't have a special shape and we don't need to treat it specially anywhere.
| @@ -308,6 +308,11 @@ internal override bool IsPinned | |||
| } | |||
| } | |||
|
|
|||
| internal override bool IsKnownToReferToTempIfReferenceType | |||
There was a problem hiding this comment.
Should we seal this? #Resolved
| @@ -79,6 +79,11 @@ protected LocalSymbol() | |||
| get; | |||
| } | |||
|
|
|||
| internal abstract bool IsKnownToReferToTempIfReferenceType | |||
There was a problem hiding this comment.
Consider adding a comment this this to explain what it's used for. #Resolved
There was a problem hiding this comment.
Why not name it just RefersToTempIfReferenceType?
There was a problem hiding this comment.
Why not name it just
RefersToTempIfReferenceType?
I think the fact that RefersToTempIfReferenceType is false might be misinterpreted as the local doesn't refer to a temp if reference type. However, this isn't what I am trying to convey.
typo in this and a few following method names In reply to: 1330335753 Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenCallTests.cs:17544 in 36930c5. [](commit_id = 36930c5, deletion_comment = False) |
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
Show resolved
Hide resolved
It is probably not necessary to do a thorough review of every IL change in that file. If you need some help with specific IL changes, we can walk though the diff together offline. |
|
@dotnet/roslyn-compiler For the second review |
3 similar comments
|
@dotnet/roslyn-compiler For the second review |
|
@dotnet/roslyn-compiler For the second review |
|
@dotnet/roslyn-compiler For the second review |
|
@dotnet/roslyn-compiler For the second review, this PR is already one week old and I have another change in the pipeline, that builds on top of changes in this PR. |
ghost
added this to the 
Related to #63221.