Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

pkindruk
Copy link
Contributor

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 and v3.
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.

[MessagePackObject]
public class Request2
{
    [Key(1)]
    public virtual string Id { get; set; }

    [Key(1)]
    public virtual string ClientIp { get; set; }
}

It can be very confusing when you use interfaces. As CLR requires interface implementations to be marked as virtual. See MethodBase.IsVirtual remarks.

public interface IRequest
{
    public string Id { get; set; }

    public string ClientIp { get; set; }
}

[MessagePackObject]
public class Request1 : IRequest
{
    [Key(1)]
    public string Id { get; set; }

    [Key(1)]
    public string ClientIp { get; set; }
}

Copy link
Collaborator

@AArnott AArnott left a 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'
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@pkindruk
Copy link
Contributor Author

pkindruk commented Oct 1, 2024

Doc comment might not be ideal. Though it will be hard to spot sealed override property with same name. In the context of method conflictingMember will be the first member with such key encountered (base class members come first). Which can be done with something like code below (though I'm not sure if it is a valid usecase).

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; }
}

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

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 [IgnoreMember] on overrides within analyzer.

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.
EDIT: In IL interfaces looks much like pure abstract classes and for performance reasons property methods are made final unless property is declared virtual itself (sharplab).

Also there are problems detecting new keyword using reflection. I don't see any difference in MethodAttributes for the following case:

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; }
}

@AArnott
Copy link
Collaborator

AArnott commented Oct 1, 2024

Also there are problems detecting new keyword using reflection. I don't see any difference in MethodAttributes for the following case:

I believe Reflection can distinguish between the two cases, if you know what to look for. It probably isn't in MethodAttributes as you say.

But these are extremely narrow cases and it sounds like your PR improves a more common case.
I'll push a fix for the build break and merge.

@AArnott
Copy link
Collaborator

AArnott commented Oct 1, 2024

Ah, can you also please apply your proposed comment change? That's good.

@pkindruk
Copy link
Contributor Author

pkindruk commented Oct 2, 2024

Updated comment. Didn't want to change it twice.

As for the new keyword it is a very narrow case and not goal of this PR.

@AArnott AArnott added this to the v3.0 milestone Oct 2, 2024
@AArnott AArnott changed the title Fix detecting duplicate keys. Fix detecting duplicate keys Oct 2, 2024
@AArnott AArnott merged commit 4b4f92f into MessagePack-CSharp:develop Oct 2, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.