[semi-auto-props]: Support assigning properties from constructor #60601

Youssef1313 · 2022-04-06T07:04:27Z

Test plan: #57012

Note: Code I modified is used for both allowing assigning from constructors and also for some data flow. Changes made here were more focused on the first part. Meaning that it could be not correct for data flow which (if possible) I want to leave for now (there are PROTOTYPE comments there).

This implements the following part from LDM notes:

Properties can be assigned in the constructor of a type if that property has a setter, or is a get-only property whose backing field will be emitted.

And also, from the original proposal:

If the auto-property does not have a set accessor, the backing field can still be assigned to in the body of a constructor of the enclosing class. Such an assignment assigns directly to the backing field of the property.

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

AlekseyTs · 2022-04-06T13:31:22Z

Has "auto-default struct" feature made it to the feature branch? It feels like it might impact some initialization scenarios.

In reply to: 1090276403

Youssef1313 · 2022-04-06T14:00:36Z

@AlekseyTs I think it got into 17.3 branch but not in main. So it didn't get in features/semi-auto-props yet

AlekseyTs · 2022-04-06T14:05:33Z

I think it got into 17.3 branch but not in main. So it didn't get in features/semi-auto-props yet

We should consider integrating main-vs-deps directly into the feature branch then. I think some of the design decisions around this feature were made assuming the "auto-default struct" feature is in place.

In reply to: 1090313856

Youssef1313 · 2022-04-06T14:12:21Z

@AlekseyTs I opened #60603.

… semi-assign-ctor

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

Youssef1313 · 2022-04-08T16:24:42Z

@AlekseyTs This is ready for review. Can you take a look please? Thanks!

AlekseyTs · 2022-04-08T16:32:10Z

This is ready for review.

Is the expected behavior clearly described in the feature specification (a link to the relevant section would be good). Otherwise, it would be good to clearly describe in details what behavior this PR is implementing so that reviewers could "compare" the intent and the implementation.

In reply to: 1093069013

Youssef1313 · 2022-04-08T16:57:51Z

@AlekseyTs I updated the PR description. Thanks!

AlekseyTs · 2022-04-12T17:05:30Z

CC @333fred

In reply to: 1096978766

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

Youssef1313 · 2022-04-30T12:32:34Z

@AlekseyTs @333fred This should be ready for another look.

Youssef1313 · 2022-05-03T18:34:52Z

@AlekseyTs CI is still failing after it was restarted. The failure doesn't seem related to the changes. Should a complete new build be started?

AlekseyTs · 2022-05-04T13:27:32Z

/azp run

azure-pipelines · 2022-05-04T13:27:54Z

Azure Pipelines successfully started running 4 pipeline(s).

AlekseyTs · 2022-05-04T13:28:07Z

CC @333fred

In reply to: 1117314610

333fred

Overall this is looking good to me. Just a minor refactor suggestion. (commit 36)

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

AlekseyTs · 2022-05-05T17:16:02Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

@@ -202,6 +202,9 @@ private void SetGlobalErrorIfTrue(bool arg)
                    new LocalizableResourceString(messageResourceName, CodeAnalysisResources.ResourceManager, typeof(CodeAnalysisResources)));
            }

+            addCircularStructDiagnostics(compilation.GlobalNamespace);


compilation.GlobalNamespace

It looks like we are traversing things from added modules and referenced assemblies. See methodCompiler.CompileNamespace call above. #Closed

Yeah compilation.GlobalNamespace is alot of unnecessary work. I'm switching to compilation.SourceModule.GlobalNamespace. Thanks!

Yeah compilation.GlobalNamespace is alot of unnecessary work.

It not only a lot of unnecessary work, but also the work that is likely to have negative impact on referenced compilations (work done out of "order").

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

AlekseyTs · 2022-05-05T19:00:40Z

Done with review pass (commit 37)

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

AlekseyTs · 2022-05-05T20:48:46Z

Done with review pass (commit 38). Besides code changes, I am still tracking this thread for tests #60601 (comment).

Youssef1313 · 2022-05-05T21:30:53Z

@AlekseyTs

diagnostics.AddRange(compilation.CircularStructDiagnostics);

It looks like diagnostics isn't guaranteed to have dependencies bag while CircularStructDiagnostics does, causing this assert to fail:

roslyn/src/Compilers/Core/Portable/Binding/BindingDiagnosticBag.cs

Line 119 in 525e465

    
           Debug.Assert(allowMismatchInDependencyAccumulation || !other.AccumulatesDependencies || this.AccumulatesDependencies);

I passed allowMismatchInDependencyAccumulation: true since we can have a dependencies bag for diagnostics. But let me know if you think this is wrong.

Youssef1313 · 2022-05-05T21:31:48Z

Done with review pass (commit 38). Besides code changes, I am still tracking this thread for tests #60601 (comment).

Responded with a question on it.

Thanks a lot for the review and the help you're providing. I very much appreciate it.

AlekseyTs · 2022-05-05T22:17:39Z

I passed allowMismatchInDependencyAccumulation: true since we can have a dependencies bag for diagnostics.

That is the right thing to do in our situation.

In reply to: 1119065377

AlekseyTs · 2022-05-05T22:35:23Z

Done with review pass (commit 40)

AlekseyTs · 2022-05-09T23:04:24Z

@Youssef1313 It looks like there is merge conflict to resolve.

Youssef1313 · 2022-05-12T12:32:24Z

@333fred @AlekseyTs Is this ready to merge? For the next PR*, I'm planning to address small prototype comments (e.g, renaming ContainsFieldKeyword). Then I'm not sure if I should start working on mixed scenarios?

*I can get to the next PR in few days. So feel free if someone wants to take some work over before I do.

AlekseyTs

LGTM (commit 42)

[semi-auto-props]: Support assigning properties from constructor

884827e

Youssef1313 requested a review from a team as a code owner April 6, 2022 07:04

ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 6, 2022

dotnet-issue-labeler bot added the Area-Compilers label Apr 6, 2022

Youssef1313 commented Apr 6, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs Outdated Show resolved Hide resolved

Youssef1313 added 5 commits April 6, 2022 09:51

Update tests

92abdde

Update tests

ea5a0fc

Update tests

ef9ed78

Fix cycle

24acdbb

Unskip a test

0e1283c

Merge branch 'features/semi-auto-props' into semi-assign-ctor

90e306b

Youssef1313 added 5 commits April 6, 2022 17:15

Fix formatting

60e3cc6

Merge remote-tracking branch 'upstream/features/semi-auto-props' into…

e8f985a

… semi-assign-ctor

Update failing tests

310585f

Support auto-default struct in conjunction of semi-auto-prop

2b3703f

Fix test

2b81ca9

Youssef1313 closed this Apr 8, 2022

Youssef1313 reopened this Apr 8, 2022

Remove assert

67964c7

Youssef1313 commented Apr 8, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Apr 11, 2022

VSTypeScriptHandlerTests flaky #60694

Closed

333fred reviewed Apr 12, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs Outdated Show resolved Hide resolved

Youssef1313 mentioned this pull request May 3, 2022

[semi-auto-props]: Require overriding all accessors #61114

Merged

333fred approved these changes May 4, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs Outdated Show resolved Hide resolved

Update BaseTypeAnalysis.cs

6553698

AlekseyTs reviewed May 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs Show resolved Hide resolved

AlekseyTs reviewed May 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs Show resolved Hide resolved

AlekseyTs reviewed May 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Outdated Show resolved Hide resolved

Address review comments

0b3dcee

AlekseyTs reviewed May 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits May 5, 2022 23:17

Address review comment

3de5181

Fix assertion failure

fcb5e3f

Youssef1313 added 2 commits May 10, 2022 11:11

Merge branch 'features/semi-auto-props' into semi-assign-ctor

7ea8b7f

Add test

c42ed4a

AlekseyTs approved these changes May 12, 2022

View reviewed changes

333fred merged commit 5dc2199 into dotnet:features/semi-auto-props May 12, 2022

Youssef1313 deleted the semi-assign-ctor branch May 12, 2022 17:14

cston mentioned this pull request Sep 18, 2024

Field-backed properties: definite assignment #75116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[semi-auto-props]: Support assigning properties from constructor #60601

[semi-auto-props]: Support assigning properties from constructor #60601

Youssef1313 commented Apr 6, 2022 •

edited

Loading

AlekseyTs commented Apr 6, 2022 •

edited

Loading

Youssef1313 commented Apr 6, 2022

AlekseyTs commented Apr 6, 2022 •

edited

Loading

Youssef1313 commented Apr 6, 2022

Youssef1313 commented Apr 8, 2022

AlekseyTs commented Apr 8, 2022 •

edited

Loading

Youssef1313 commented Apr 8, 2022

AlekseyTs commented Apr 12, 2022 •

edited

Loading

Youssef1313 commented Apr 30, 2022

Youssef1313 commented May 3, 2022 •

edited

Loading

AlekseyTs commented May 4, 2022

azure-pipelines bot commented May 4, 2022

AlekseyTs commented May 4, 2022 •

edited

Loading

333fred left a comment

AlekseyTs May 5, 2022 •

edited

Loading

Youssef1313 May 5, 2022

AlekseyTs May 5, 2022

AlekseyTs commented May 5, 2022

AlekseyTs commented May 5, 2022 •

edited

Loading

Youssef1313 commented May 5, 2022

Youssef1313 commented May 5, 2022

AlekseyTs commented May 5, 2022

AlekseyTs commented May 5, 2022

AlekseyTs commented May 9, 2022

Youssef1313 commented May 12, 2022 •

edited

Loading

AlekseyTs left a comment

[semi-auto-props]: Support assigning properties from constructor #60601

[semi-auto-props]: Support assigning properties from constructor #60601

Conversation

Youssef1313 commented Apr 6, 2022 • edited Loading

AlekseyTs commented Apr 6, 2022 • edited Loading

Youssef1313 commented Apr 6, 2022

AlekseyTs commented Apr 6, 2022 • edited Loading

Youssef1313 commented Apr 6, 2022

Youssef1313 commented Apr 8, 2022

AlekseyTs commented Apr 8, 2022 • edited Loading

Youssef1313 commented Apr 8, 2022

AlekseyTs commented Apr 12, 2022 • edited Loading

Youssef1313 commented Apr 30, 2022

Youssef1313 commented May 3, 2022 • edited Loading

AlekseyTs commented May 4, 2022

azure-pipelines bot commented May 4, 2022

AlekseyTs commented May 4, 2022 • edited Loading

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Youssef1313 May 5, 2022

Choose a reason for hiding this comment

AlekseyTs May 5, 2022

Choose a reason for hiding this comment

AlekseyTs commented May 5, 2022

AlekseyTs commented May 5, 2022 • edited Loading

Youssef1313 commented May 5, 2022

Youssef1313 commented May 5, 2022

AlekseyTs commented May 5, 2022

AlekseyTs commented May 5, 2022

AlekseyTs commented May 9, 2022

Youssef1313 commented May 12, 2022 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

Youssef1313 commented Apr 6, 2022 •

edited

Loading

AlekseyTs commented Apr 6, 2022 •

edited

Loading

AlekseyTs commented Apr 6, 2022 •

edited

Loading

AlekseyTs commented Apr 8, 2022 •

edited

Loading

AlekseyTs commented Apr 12, 2022 •

edited

Loading

Youssef1313 commented May 3, 2022 •

edited

Loading

AlekseyTs commented May 4, 2022 •

edited

Loading

AlekseyTs May 5, 2022 •

edited

Loading

AlekseyTs commented May 5, 2022 •

edited

Loading

Youssef1313 commented May 12, 2022 •

edited

Loading