Reference conversion on this in init-only phase #50424
Conversation
jcouv
force-pushed
the
init-conversion
branch
from
f34b5c4
to
0fd202e
Compare
|
Is there a matching spec update? #Closed |
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 |
|
I'll update the spec. |
|
|
||
| receiver = boundConversion.Operand; | ||
| } | ||
|
|
||
| // bad: other.InitOnlyProperty = ... | ||
| if (!(receiver is BoundThisReference || receiver is BoundBaseReference)) |
There was a problem hiding this comment.
receiver is BoundBaseReference [](start = 56, length = 30)
Should we disallow conversions on top of base? It is illegal to convert base in general. #Closed
There was a problem hiding this comment.
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)
| "; | ||
|
|
||
| var comp = CreateCompilation(new[] { source, IsExternalInitTypeDefinition }, options: TestOptions.DebugExe); | ||
| comp.VerifyDiagnostics(); |
There was a problem hiding this comment.
.VerifyDiagnostics() [](start = 16, length = 20)
Consider calling this on CompileAndVerify instead. #Closed
There was a problem hiding this comment.
| public int SomethingElse | ||
| { | ||
| get => ((ISomething)this).Property; | ||
| init => ((ISomething)this).Property = value; |
There was a problem hiding this comment.
this [](start = 29, length = 4)
Consider adding a test with base. #Closed
There was a problem hiding this comment.
Also a test where this hides an init-only property from base.
In reply to: 558461131 [](ancestors = 558461131)
|
|
||
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
Done review pass (commit 1) |
| @@ -1196,6 +1196,17 @@ bool isAllowedInitOnlySet(BoundExpression receiver) | |||
| return true; | |||
| } | |||
|
|
|||
| while (receiver is BoundConversion boundConversion) | |||
There was a problem hiding this comment.
receiver is BoundConversion boundConversion [](start = 23, length = 43)
Should we also handle BoundAsOperator? #Closed
|
Should this PR target 16.9 branch at this point? |
|
Retargeted PR to 16.9 release branch |
|
Merged into release branch with single review since this is a test-only change. |
Fixes #50053
From discussion with Jared and Mads, the set of conversions that we should consider for this scenario is identity and reference conversions.