Disallow setter-only EqualityContract property#48798
Conversation
| { | ||
| if (_equalityContract.GetMethod is null) | ||
| { | ||
| // There the equality contract isn't usable, an error was reported elsewhere |
There was a problem hiding this comment.
There the equality contract [](start = 23, length = 27)
"The equality contract ..."? #Resolved
| { | ||
| if (_equalityContract.GetMethod is null) | ||
| { | ||
| // There the equality contract isn't usable, an error was reported elsewhere |
There was a problem hiding this comment.
There the equality contract [](start = 23, length = 27)
"The equality contract ..."? #Resolved
| #endregion diagnostics introduced for C# 9.0 | ||
|
|
||
| ERR_UnexpectedVarianceStaticMember = 9100, | ||
| ERR_EqualityContractRequiresGetter = 9101, |
There was a problem hiding this comment.
ERR_EqualityContractRequiresGetter = 9101, [](start = 8, length = 42)
It looks like this branch is significantly behind. I think in master the last used error number is 8905 #Closed
|
|
||
| try | ||
| { | ||
| if (_equalityContract.GetMethod is null) |
There was a problem hiding this comment.
if (_equalityContract.GetMethod is null) [](start = 16, length = 40)
Should we limit this check to the code path that actually uses the property? #Closed
|
|
||
| try | ||
| { | ||
| if (_equalityContract.GetMethod is null) |
There was a problem hiding this comment.
if (_equalityContract.GetMethod is null) [](start = 16, length = 40)
Should we limit this check to the code path that actually uses the property? #Closed
| var src = @" | ||
| record R | ||
| { | ||
| protected virtual System.Type EqualityContract { set { } } |
There was a problem hiding this comment.
set { } [](start = 53, length = 7)
Consider adding a test with a property that has both accessors, unless we already have one. I assume that should be a success scenario? Or should we disallow property like that? #Closed
There was a problem hiding this comment.
Also, consider adding test(s) when a record class has only setter or both accessors in metadata and we try to derive another record from it with explicit/implicit definition of EqualityContract.
In reply to: 508933817 [](ancestors = 508933817)
There was a problem hiding this comment.
Or should we disallow property like that [with both accessors]?
I think it's okay to allow, as it meets our requirements and doesn't get in the way of what we need to do.
In reply to: 508941602 [](ancestors = 508941602,508933817)
|
|
||
| var comp = CreateCompilation(src); | ||
| comp.VerifyEmitDiagnostics( | ||
| // (4,35): error CS8906: Record equality contract property 'R.EqualityContract' must have a get accessor. |
There was a problem hiding this comment.
nit: "equality contract" seems unnecessarily repetitive in the message.
|
|
||
| [Fact] | ||
| [WorkItem(48723, "https://github.com/dotnet/roslyn/issues/48723")] | ||
| public void EqualityContract_27_GetterAndSetterOnlyProperty_InMetadata() |
There was a problem hiding this comment.
Only [](start = 55, length = 4)
Is the "Only" part in the name kept to convey something important? #Resolved
| } | ||
|
|
||
| // Property has a getter and setter | ||
| .property instance class [mscorlib]System.Type EqualityContract() |
There was a problem hiding this comment.
.property instance class [mscorlib]System.Type EqualityContract() [](start = 4, length = 65)
It looks like we decided to allow a declaration like that in source. Is there a particular reason to use IL to define this property for the test? #Resolved
| "; | ||
|
|
||
| var comp = CreateCompilationWithIL(src, il); | ||
| comp.VerifyEmitDiagnostics(); |
There was a problem hiding this comment.
comp.VerifyEmitDiagnostics(); [](start = 11, length = 30)
I think that for scenarios tested in this test we would want to test runtime behavior in order to verify that overriding is indeed happening and has expected effect on equality behavior. #Resolved
AlekseyTs
left a comment
There was a problem hiding this comment.
LGTM (iteration 3) with some comments for tests.
Fixes #48723