Fix a bug and finish ref-reassignment test plan#25911
Fix a bug and finish ref-reassignment test plan#25911agocke merged 2 commits intodotnet:dev15.7.xfrom
Conversation
|
ping @dotnet/roslyn-compiler for review |
|
@jaredpar for ask mode approval |
There was a problem hiding this comment.
I think this will always compute the same value as collectionValEscape above that is already computed (since there are no arguments going to either GetEnumerator, nor to the Current).
Should just use that as a ref escape for the local.
There was a problem hiding this comment.
And we probably should just say that in the spec - "ref-escape of a local in a ref-foreach is a val-escape of the collection".
We are saying something similar in the val case. It is simpler - No need to involve how it is lowered.
There was a problem hiding this comment.
Right. We do not respect PEVerify-compat when that would result in functional break of the feature.
Another example is ref-returning a readonly field. PEVerify compat flag generally makes us to do local copies before taking a ref to a readonly field, but returning a ref to a local clone makes no sense. Having a choice between not compatible and completely broken we pick not compatible.
VSadov
left a comment
There was a problem hiding this comment.
LGTM,
but I think it is worth making simplification to the ref-escape of the ref-foreach local.
Lets do this in 15.8. |
|
@agocke you just hit a merge conflict here. Can you clean up the conflict and get the commit re-pushed here? |
|
By "simplification" I meant - "not computing the value twice". It is an easy thing to fix. |
94fcc57 to
16c1a6f
Compare
Customer scenario
Quality for new feature. Bug fix is in the first commit, additional tests are in the second commit.
Bugs this fixes
While implementing the tests for the ref-reassignment test plan (#22466)
I found a bug in the handling of the ref-escape scope of a ref local in foreach. That bug is fixed
and the rest of the items in the test plan should have a test.
Workarounds, if any
None.
Risk
Small bug fix, localized change only to ref locals in foreach statements.
Performance impact
Small, no additional allocations, minimal additional calculation.
Is this a regression from a previous update?
No.
Root cause analysis
New feature.
How was the bug found?
Uncovered in test plan.