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.

Sign up for GitHub

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

Jump to bottom

[semi-auto-props]: Refactor and rename 'AccessingAutoPropertyFromConstructor' #58832

Merged
merged 12 commits into from
Jan 26, 2022
Merged

[semi-auto-props]: Refactor and rename 'AccessingAutoPropertyFromConstructor' #58832

merged 12 commits into from
Jan 26, 2022

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jan 13, 2022 •
edited
Loading

Got some time today to get a small PR :)

Test plan: #57012

@Youssef1313 Youssef1313 requested a review from a team as a code owner January 13, 2022 11:58
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 13, 2022
@Youssef1313 Youssef1313 marked this pull request as draft January 13, 2022 11:58
@Youssef1313 Youssef1313 mentioned this pull request Jan 13, 2022
Closed
@runfoapp runfoapp bot mentioned this pull request Jan 13, 2022
Closed
Youssef1313
Youssef1313 commented Jan 13, 2022
View reviewed changes
Copy link
Member Author

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

@AlekseyTs Just to clarify, I renamed the method name per my understanding to the caller in Binder.ValueChecks.cs

For AbstractFlowPass, I'm not sure how the method should behave. I tried to see if #58833 can help me understand more, but looks like there is no good test coverage? This is something I hope you can provide some explanation/hints for.

For DefiniteAssignment, I haven't taken a look at all. Waiting for LDM decision around what behavior we want.

So if the name and implementation make sense for the caller in Binder.ValueChecks.cs, we probably want other APIs for other callers?

@AlekseyTs
Copy link
Contributor

So if the name and implementation make sense for the caller in Binder.ValueChecks.cs, we probably want other APIs for other callers?

Yes, it is very likely we will need to replace one API with several more specific abstractions.

@AlekseyTs
Copy link
Contributor 

    private bool RegularPropertyAccess(BoundExpression expr) // PROTOTYPE(semi-auto-props): Usages of this should be reviewed.

It looks like this helper is specifically used to examine write access. And it looks like it is deciding if the write access is equivalent to white access on a backing field. I think it would be good to reflect that in its name and the comment.

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs:1911 in 6748146. [](commit_id = 6748146, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 13, 2022 •
edited
Loading

Done with review pass (commit 1)

In reply to: 1012312365

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jan 16, 2022 •
edited
Loading

@AlekseyTs Feedback around DefiniteAssignment isn't yet addressed. Other comments are "hopefully" addressed.

@Youssef1313 Youssef1313 requested a review from AlekseyTs January 16, 2022 19:25
Youssef1313
Youssef1313 commented Jan 17, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs Outdated
sourceProperty.IsAutoPropertyWithGetAccessor &&
return propertySymbol is SourcePropertySymbolBase sourceProperty &&
// To be assigned through backing field, either SetMethod is null, or it's equivalent to backing field write
// PROTOTYPE(semi-auto-props): TODO: Do we need to use `GetOwnOrInheritedSetMethod` instead of `SetMethod`? Add tests.
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to find a scenario that can be affected by this, but couldn't. @AlekseyTs Can you think of any? if not we can delete this prototype comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to find a scenario that can be affected by this, but couldn't. Can you think of any? if not we can delete this prototype comment.

Legacy auto-properties are required to override all accessors. Therefore, we don't need to worry about an overriding case for them. If that invariant is "broken" for semi-auto properties, then I think we will need to strengthen the check, detect the presence of an inherited setter and return false for that case. Capturing this information in a PROTOTYPE comment would be useful.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 8)

@Youssef1313 Youssef1313 requested a review from AlekseyTs January 24, 2022 16:28
AlekseyTs
AlekseyTs approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 10)

@Youssef1313 Youssef1313 marked this pull request as ready for review January 25, 2022 15:08
@AlekseyTs
Copy link
Contributor

@333fred, @dotnet/roslyn-compiler For the second review.

333fred
333fred reviewed Jan 25, 2022
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass. Overall looks fine to me, just one question that could use your input @AlekseyTs.

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs Show resolved Hide resolved
src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs Show resolved Hide resolved
333fred
333fred approved these changes Jan 26, 2022
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 12)

@333fred 333fred merged commit c0a7756 into dotnet:features/semi-auto-props Jan 26, 2022
@Youssef1313 Youssef1313 deleted the semi-more-refactor branch January 26, 2022 00:30
@Youssef1313
Copy link
Member Author

@AlekseyTs I looked through the remaining IsAutoProperty and IsAutoPropertyWithGetAccessor usages. There are:

  • An assertion that should fail later on when the work for assigning semi auto props without setters is done.
  • Language version checks in an if (IsAutoProperty) that I don't think they need to be refactored.
  • CheckInitializer call where some work will need to be done for it, but I don't think it should be the next PR.
  • AllowedAttributeLocations which has a PROTOTYPE comment currently. Should be the next PR? (I think it shouldn't)
  • A check for ERR_FieldAutoPropCantBeByRefLike, this has a test that's behaving incorrectly with a PROTOTYPE comment. Should this be the next PR?

Should the next PR be one of those usages? or should it be around semantic model API tests?

@333fred
Copy link
Member

333fred commented Jan 26, 2022

I would suggest one of those things. Public api is often best left to last, when the behavior of the feature is finalized.

@AlekseyTs
Copy link
Contributor

@Youssef1313

Should the next PR be one of those usages? or should it be around semantic model API tests?

As a quick answer, I do not have a strong preference in terms of what PR should come first. Feel free to decide according to you preference. These issues look orthogonal (independent) to me and could be worked on in parallel in different PRs. One is test only, the other a refactoring in implementation. My preference is that we complete these work items before starting focusing on supporting new scenarios.

@cston cston mentioned this pull request Sep 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@333fred 333fred 333fred approved these changes

Assignees
No one assigned
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Youssef1313 @AlekseyTs @333fred