Skip to content

Conversation

@StephenMolloy
Copy link
Member

@StephenMolloy StephenMolloy commented May 8, 2021

Keep looking up derived chain for Property.Get.

When dealing with a derived type, we have code that looks up
the derived chain looking for property getters if it doesn't
exist directly on the class being serialized. This code was tripping
up when a class in the chain declared a setter for the property
without a getter though. In this case, we should keep going
up the chain looking for a getter instead of failing.

Fixes #36181 and #38025

@StephenMolloy StephenMolloy requested review from HongGit and mconnew May 8, 2021 21:51
@ghost ghost added the area-Serialization label May 8, 2021
Copy link
Member

@mconnew mconnew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a regression from NetFx or a long time bug that's now been resolved for .NET 6?

@StephenMolloy StephenMolloy force-pushed the Ser-Overridden-Getless-Prop branch from 21cabff to d15494e Compare May 20, 2021 06:45
@StephenMolloy
Copy link
Member Author

@mconnew and @HongGit, can you guys look this over again. The PR now includes enabling 1/2 property overrides in the reflection-based serializer as well. Since that wasn't a scenario that worked before, it is a little more code than the quick fix for the IL-generated serializer.

@StephenMolloy StephenMolloy requested review from HongGit and mconnew July 9, 2021 22:14
// getter at this level of inheritance. If that's the case, we need to look
// up the chain to find the right PropertyInfo for the getter.
if (memberInfo is PropertyInfo propInfo && propInfo.GetMethod == null)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a base type has a property called Foo and the derived type has a method called Foo? GetMember would return the MethodInfo for the dervived Foo method and this code won't search the base type for the property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crikey! But as we've talked about before, 'hidden' members are a mess in the serializers right now. This would fall under the broader work we need to do in #53051.

// it might be hiding. Either way, check to see if the derived
// property has a getter that is useable for serialization.
if (info.GetMethod != null && !info.GetMethod!.IsPublic
&& memberInfoToBeReplaced is PropertyInfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would info.GetMethod?.IsPublic be more concise?

@mconnew mconnew self-requested a review July 9, 2021 22:51
Copy link
Member

@mconnew mconnew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the question of a method hiding the property is one to be too concerned with as I believe it's an existing problem which we haven't had any complaints about.

@StephenMolloy StephenMolloy merged commit c8b3051 into dotnet:main Jul 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Null reference exception calling XmlSerialiser (works in net framework)

3 participants