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

Support initializers in semi auto property #58512

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Dec 28, 2021

@AlekseyTs I interpreted #57076 (comment) as "let's start reviewing API usages"

This PR is currently focused on usages of IsAutoProperty, specifically adjusting the CheckInitializer call.

This caused the number of accessor binding to change in some cases. I can't think of a way that we can reduce the number back.

I didn't continue reviewing other API usages to keep the PR more focused and minimal as possible.

Test plan: #57012
Proposal: dotnet/csharplang#140

@Youssef1313 Youssef1313 requested a review from a team as a code owner December 28, 2021 08:54
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 28, 2021
/// </remarks>
internal void MarkBackingFieldAsCalculated()
{
Interlocked.CompareExchange(ref _lazyBackingFieldSymbol, null, _lazyBackingFieldSymbolSentinel);
((SynthesizedBackingFieldSymbol)_lazyBackingFieldSymbol)?.MarkIsBoundToFieldKeywordAsCalculated();
Copy link
Member Author

Choose a reason for hiding this comment

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

No tests currently cover this. I'll either remove it if it's redundant or add a test.

Copy link
Contributor

Choose a reason for hiding this comment

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

No tests currently cover this. I'll either remove it if it's redundant or add a test.

Let's try getting into a working mode when a change without a proper testing is not presented for a review. If you'd like to make a change that changes a behavior, try starting with a test first.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 28, 2021

Here my initial thoughts. I haven't looked at any of the changes in this PR, so it is quite possible that my opinion will change after looking at the changes in more details.

Support initializers in semi auto property

It is my understanding that we haven't figured out the design for the initializers. I.e. we are not sure when they should be allowed and what they are supposed to do. Therefore, doing any work to enable them is premature, in my opinion.

I interpreted #57076 (comment) as "let's start reviewing API usages"

That wasn't involving changing any behavior or enabling any new scenarios. A pure refactoring, that is meant to clean up some usages like the if statement around MethodBodySynthesizer.ConstructAutoPropertyAccessorBody call in MethodCompiler and determining the real intent behind usages of the APIs.

This PR is currently focused on usages of IsAutoProperty, specifically adjusting the CheckInitializer call.

Does it also focus on usages IsAutoPropertyWithGetAccessor?

This caused the number of accessor binding to change in some cases. I can't think of a way that we can reduce the number back.

I already commented on the desire to enable initializers. I would prefer putting this on hold for now. BTW, supporting initializer was way down in our initial plan at #57076 (comment).

Other things that I think we should figure out is whether we like SemanticModel behavior we currently have. That is going to inform us what we should worry about while continuing working on the feature. My recommendation would be to start a separate PR that adds SemanticModel tests we identified earlier.

@Youssef1313 Youssef1313 marked this pull request as draft December 28, 2021 16:18
@Youssef1313
Copy link
Member Author

Youssef1313 commented Dec 28, 2021

@AlekseyTs Most usages of IsAutoPropertyWithGetAccessor will need to get updated to support mixed scenario (eg get; set => field = value;). set-only semi auto property

Should I just update them and leave the tests until mixed scenario is worked on? or should I start supporting the mixed scenario in this PR? or something else?

@Youssef1313 Youssef1313 reopened this Dec 28, 2021
@AlekseyTs
Copy link
Contributor

@Youssef1313

Should I just update them and leave the tests until mixed scenario is worked on? or should I start supporting the mixed scenario in this PR? or something else?

I recommend putting this PR on hold for now and start working on pure refactoring PR that has nothing to do with semi-auto properties as the plan suggested. It would be fine to identify places that might need future adjustments and mark them with PROTOTYPE comments. If you are not sure what to start with, start with clean up of the if statement around MethodBodySynthesizer.ConstructAutoPropertyAccessorBody call in MethodCompiler. Then we will take a look at other API usages.

@Youssef1313 Youssef1313 deleted the semi-initializers branch December 28, 2021 16:45
@Youssef1313 Youssef1313 restored the semi-initializers branch December 28, 2021 16:45
@Youssef1313 Youssef1313 deleted the semi-initializers branch December 28, 2021 16:45
@Youssef1313 Youssef1313 restored the semi-initializers branch December 28, 2021 16:46
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.

None yet

2 participants