Skip to content
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

draft: dummy change to see what's affected by this code path #58833

Closed
wants to merge 2 commits into from

Conversation

Youssef1313
Copy link
Member

(For continuing #58832 later)

Running Build.cmd -testCoreClr didn't report any relevant test failures. So letting CI tell me.

@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 13, 2022
@@ -576,7 +576,7 @@ protected void VisitLvalue(BoundExpression node)
var backingField = (access.PropertySymbol as SourcePropertySymbolBase)?.BackingField;
if (backingField != null)
{
VisitFieldAccessInternal(access.ReceiverOpt, backingField);
//VisitFieldAccessInternal(access.ReceiverOpt, backingField);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really very weird that CI is still passing with this change.

@AlekseyTs Is this, somehow, a dead code? Or is test coverage not enough?

I'm not able to see what we need to do around the Binder.AccessingAutoPropertyFromConstructor for this code path if I don't know what it currently affects. Can you help please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to see what we need to do around the Binder.AccessingAutoPropertyFromConstructor for this code path if I don't know what it currently affects. Can you help please?

I think the better way to see what Binder.AccessingAutoPropertyFromConstructor affects is to pretend that it never returns true on this code path and see what happens then. It is quite possible that not calling VisitFieldAccessInternal is pretty much unobservable, but the change doesn't test what we would do instead if Binder.AccessingAutoPropertyFromConstructor return false.

My theory is that here we are trying to analyze a property assignment as a backing field assignment when we are certain that that is what the assignment would ultimately do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants