-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix crash in SpillSequenceSpiller #47428
Conversation
I will probably enable nullable in this file in the near future, but we want this to be ready to backport to 16.7, so won't do it in this PR. |
88bffa5
to
bb536d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix looks good. Some suggestion for extra tests to add here.
} | ||
|
||
[Fact, WorkItem(47191, "https://github.com/dotnet/roslyn/issues/47191")] | ||
public void AssignInstanceStructField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add similar tests for VB. If we regressed this in C# then seems a fair bet we could accidentally regress it in VB as well.
@@ -6319,5 +6319,66 @@ .maxstack 3 | |||
IL_009e: ret | |||
}"); | |||
} | |||
|
|||
[Fact, WorkItem(47191, "https://github.com/dotnet/roslyn/issues/47191")] | |||
public void AssignStaticStructField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test where we grab a static
field w/o a qualifier by means of using static
. Basically not just a direct static
field access on the current type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1193818 was approved for Ask Mode. |
Closes #47191
The bug was that we assume that a field access has a receiver even if the field is static. I can't see any need to spill the receiver of a static field, so I made the compiler skip that step in that scenario.
I found that referencing the static field via a type name also prevents the crash in master, e.g. using
C.s1.field
in the below test instead ofs1.field
. I was surprised that the ReceiverOpt is present using the former but it is null with the latter."Hide whitespace changes" is recommended when reviewing.