Reference conversion on this in init-only phase by jcouv · Pull Request #50424 · dotnet/roslyn

jcouv · 2021-01-13T07:35:00Z

From discussion with Jared and Mads, the set of conversions that we should consider for this scenario is identity and reference conversions.

AlekseyTs · 2021-01-14T14:50:58Z

Is there a matching spec update? #Closed

jaredpar · 2021-01-14T16:55:59Z

Is there a matching spec update?

Think this is a change where we should ensure there is a spec update. The concept here I believe is easy to understand from a compiler developer perspective: conversions on this which are identity preserving. Can likely just go through ConversionKind and identify the ones that match. At the same time I'm not sure that type of concept exists in the language spec (perhaps @MadsTorgersen was able to point one out). Want to make sure this is explainable in the language spec in conjunction with this change.

jcouv · 2021-01-14T21:46:31Z

I'll update the spec.
I talked to Mads already and we settled on identity+reference conversions. We don't seem to have a concept of identity/representation-preserving conversions.

AlekseyTs · 2021-01-15T17:33:50Z

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

+                }
+
                // bad: other.InitOnlyProperty = ...
                if (!(receiver is BoundThisReference || receiver is BoundBaseReference))


receiver is BoundBaseReference [](start = 56, length = 30)

Should we disallow conversions on top of base? It is illegal to convert base in general. #Closed

I think it's likely fine to handle it here. We'll report an error on the conversion itself.

In reply to: 558459166 [](ancestors = 558459166)

AlekseyTs · 2021-01-15T17:36:13Z

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

+";
+
+            var comp = CreateCompilation(new[] { source, IsExternalInitTypeDefinition }, options: TestOptions.DebugExe);
+            comp.VerifyDiagnostics();


.VerifyDiagnostics() [](start = 16, length = 20)

Consider calling this on CompileAndVerify instead. #Closed

Is there even any need to call VerifyDiagnostics?

In reply to: 558460437 [](ancestors = 558460437)

AlekseyTs · 2021-01-15T17:37:31Z

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

+    public int SomethingElse
+    {
+        get => ((ISomething)this).Property;
+        init => ((ISomething)this).Property = value;


this [](start = 29, length = 4)

Consider adding a test with base. #Closed

Also a test where this hides an init-only property from base.

In reply to: 558461131 [](ancestors = 558461131)

AlekseyTs

LGTM (commit 1)

333fred · 2021-01-15T22:34:34Z

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

+
+            var comp = CreateCompilation(new[] { source, IsExternalInitTypeDefinition }, options: TestOptions.DebugExe);
+            comp.VerifyDiagnostics(
+                // (13,17): error CS8852: Init-only property or indexer 'ISomething.Property' can only be assigned in an object initializer, or on 'this' or 'base' in an instance constructor or an 'init' accessor.


Would it be reasonable to add a more specific error for this, or do you think that would be more effort than it's worth? The error as it exists is misleading.

Looking at the code I don't think it would be a ton of work to add, and I really don't like the error as it exists because it's just wrong.

In reply to: 558627354 [](ancestors = 558627354)

333fred · 2021-01-15T22:55:24Z

Done review pass (commit 1)

AlekseyTs · 2021-01-20T18:09:39Z

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

                    return true;
                }

+                while (receiver is BoundConversion boundConversion)


receiver is BoundConversion boundConversion [](start = 23, length = 43)

Should we also handle BoundAsOperator? #Closed

AlekseyTs

LGTM (commit 2)

AlekseyTs

LGTM (commit 2)

AlekseyTs · 2021-01-28T15:39:17Z

Should this PR target 16.9 branch at this point?

jcouv · 2021-01-28T17:38:02Z

Retargeted PR to 16.9 release branch

jcouv · 2021-01-28T23:20:42Z

Merged into release branch with single review since this is a test-only change.

jcouv added Area-Compilers Feature - Records Records labels Jan 13, 2021

jcouv self-assigned this Jan 13, 2021

jcouv added this to the 16.9 milestone Jan 13, 2021

Reference conversion on this in init-only phase

0fd202e

jcouv force-pushed the init-conversion branch from f34b5c4 to 0fd202e Compare January 13, 2021 20:26

jcouv marked this pull request as ready for review January 14, 2021 10:09

jcouv requested a review from a team as a code owner January 14, 2021 10:09

AlekseyTs mentioned this pull request Jan 14, 2021

Enable consumption of init-only properties in VB. #50414

Merged

AlekseyTs reviewed Jan 15, 2021

View reviewed changes

AlekseyTs approved these changes Jan 15, 2021

View reviewed changes

333fred reviewed Jan 15, 2021

View reviewed changes

AlekseyTs reviewed Jan 20, 2021

View reviewed changes

jcouv marked this pull request as draft January 20, 2021 19:23

AlekseyTs mentioned this pull request Jan 26, 2021

VB init-only: Conversions in init-only phase #50770

Merged

Revert code changes

8d828c8

jcouv marked this pull request as ready for review January 28, 2021 02:02

AlekseyTs reviewed Jan 28, 2021

View reviewed changes

AlekseyTs approved these changes Jan 28, 2021

View reviewed changes

jcouv changed the base branch from master to release/dev16.9 January 28, 2021 17:37

jcouv added the Test Test failures in roslyn-CI label Jan 28, 2021

jcouv merged commit c6b76a6 into dotnet:release/dev16.9 Jan 28, 2021

jcouv deleted the init-conversion branch January 28, 2021 23:20

Conversation

jcouv commented Jan 13, 2021

Uh oh!

AlekseyTs commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Jan 14, 2021

Uh oh!

jcouv commented Jan 14, 2021

Uh oh!

AlekseyTs Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

333fred Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

333fred commented Jan 15, 2021

Uh oh!

AlekseyTs Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jan 28, 2021

Uh oh!

jcouv commented Jan 28, 2021

Uh oh!

jcouv commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlekseyTs commented Jan 14, 2021 •

edited

Loading

AlekseyTs Jan 15, 2021 •

edited

Loading

AlekseyTs Jan 15, 2021 •

edited

Loading

AlekseyTs Jan 15, 2021 •

edited

Loading

AlekseyTs Jan 20, 2021 •

edited

Loading