-
-
Notifications
You must be signed in to change notification settings - Fork 734
Fix detecting duplicate keys #1971
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this. I think what you're striving for is good, and you've got a great battery of added tests.
However your logic change and the docs that explain them may not be quite accurate. And I wonder if the right fix is to re-think how we do this because I don't understand why virtual or interfaces should even come into it at all (though I didn't write the original code).
{ | ||
return false; | ||
// Implicit interface implementations of get/set methods are marked 'virtual final' | ||
// Therefore an overridable property should be 'IsVirtual == true && IsFinal == false' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily. Ordinary properties (without interfaces) can also be virtual and final.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this:
// According to MethodBase.IsVirtual docs an overridable property should be 'IsVirtual == true && IsFinal == false'.
// Property methods can be marked 'virtual final' in case of 'sealed override' or when implementing interface implicitly.
Doc comment might not be ideal. Though it will be hard to spot public class A {
[Key(0)]
public virtual string Prop { get; set; }
}
public class B : A {
[Key(1)]
public sealed override string Prop { get; set; }
}
public class C : B {
[Key(1)]
public new string Prop { get; set; }
}
For virtual properties you want to have only one definition, because virtual method table will always give you latest implementation from a derivied type. If we re-think of design, one solution would be to force var b = new B() { Prop = "a" };
Console.WriteLine("=>" + (b as A).Prop); // Prints "=>a" Speaking of interfaces, they are virtual because it is a CLR implementation detail. When you read method attributes using reflection, you get CLR attributes. Also there are problems detecting public class A {
public string Prop1 { get; set; }
public virtual string Prop2 { get; set; }
}
public class B : A {
public new string Prop1 { get; set; }
public new virtual string Prop2 { get; set; }
} |
I believe Reflection can distinguish between the two cases, if you know what to look for. It probably isn't in But these are extremely narrow cases and it sounds like your PR improves a more common case. |
Ah, can you also please apply your proposed comment change? That's good. |
Updated comment. Didn't want to change it twice. As for the |
Runtime code generation doesn't detect duplicate keys for properties with virtual attribute. Seems like issue was introduced in #1083. This issue can be reproduced on both
v2.5
andv3
.For types attributed with
[MessagePackObject]
this issue is covered by analyzer which prevents duplicate keys. However if there is no reference to analyzer or[DataContract]
attribute is used, then duplicate keys will be not detected.It can be very confusing when you use interfaces. As CLR requires interface implementations to be marked as
virtual
. See MethodBase.IsVirtual remarks.