Skip to content
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

Keep looking up derived chain for Property.Get if it doesn't exist. #52509

Merged
merged 4 commits into from
Jul 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ private static ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate Get
(Type, string) typeMemberNameTuple = (o.GetType(), memberName);
if (!s_setMemberValueDelegateCache.TryGetValue(typeMemberNameTuple, out ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate? result))
{
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetMember(o.GetType(), memberName);
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveSetInfo(o.GetType(), memberName);
Debug.Assert(memberInfo != null, "memberInfo could not be retrieved");
Type memberType;
if (memberInfo is PropertyInfo propInfo)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ private void WriteStructMethod(StructMapping mapping, string n, string? ns, obje
[RequiresUnreferencedCode("Calls GetType on object")]
private object? GetMemberValue(object o, string memberName)
{
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetMember(o.GetType(), memberName);
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveGetInfo(o.GetType(), memberName);
object? memberValue = GetMemberValue(o, memberInfo);
return memberValue;
}
Expand Down Expand Up @@ -1356,7 +1356,7 @@ private enum WritePrimitiveMethodRequirement
internal static class ReflectionXmlSerializationHelper
{
[RequiresUnreferencedCode("Reflects over base members")]
public static MemberInfo GetMember(Type declaringType, string memberName)
public static MemberInfo? GetMember(Type declaringType, string memberName, bool throwOnNotFound)
{
MemberInfo[] memberInfos = declaringType.GetMember(memberName);
if (memberInfos == null || memberInfos.Length == 0)
Expand All @@ -1377,7 +1377,11 @@ public static MemberInfo GetMember(Type declaringType, string memberName)

if (!foundMatchedMember)
{
throw new InvalidOperationException(SR.Format(SR.XmlInternalErrorDetails, $"Could not find member named {memberName} of type {declaringType}"));
if (throwOnNotFound)
{
throw new InvalidOperationException(SR.Format(SR.XmlInternalErrorDetails, $"Could not find member named {memberName} of type {declaringType}"));
}
return null;
}

declaringType = currentType!;
Expand All @@ -1398,5 +1402,61 @@ public static MemberInfo GetMember(Type declaringType, string memberName)

return memberInfo;
}

[RequiresUnreferencedCode(XmlSerializer.TrimSerializationWarning)]
public static MemberInfo GetEffectiveGetInfo(Type declaringType, string memberName)
{
MemberInfo memberInfo = GetMember(declaringType, memberName, true)!;

// For properties, we might have a PropertyInfo that does not have a valid
// 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.

var parent = declaringType.BaseType;

while (parent != null)
{
var mi = GetMember(parent, memberName, false);

if (mi is PropertyInfo pi && pi.GetMethod != null && pi.PropertyType == propInfo.PropertyType)
{
return pi;
}

parent = parent.BaseType;
}
}

return memberInfo;
}

[RequiresUnreferencedCode(XmlSerializer.TrimSerializationWarning)]
public static MemberInfo GetEffectiveSetInfo(Type declaringType, string memberName)
{
MemberInfo memberInfo = GetMember(declaringType, memberName, true)!;

// For properties, we might have a PropertyInfo that does not have a valid
// setter at this level of inheritance. If that's the case, we need to look
// up the chain to find the right PropertyInfo for the setter.
if (memberInfo is PropertyInfo propInfo && propInfo.SetMethod == null)
{
var parent = declaringType.BaseType;

while (parent != null)
{
var mi = GetMember(parent, memberName, false);

if (mi is PropertyInfo pi && pi.SetMethod != null && pi.PropertyType == propInfo.PropertyType)
{
return pi;
}

parent = parent.BaseType;
}
}

return memberInfo;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1219,7 +1219,10 @@ private static bool ShouldBeReplaced(
replacedInfo = info;
if (replacedInfo != memberInfoToBeReplaced)
{
if (!info.GetMethod!.IsPublic
// The property name is a match. It might be an override, or
// 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?

&& ((PropertyInfo)memberInfoToBeReplaced).GetMethod!.IsPublic
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2810,13 +2810,14 @@ public static void DerivedTypeWithDifferentOverrides()
[Fact]
public static void DerivedTypeWithDifferentOverrides2()
{
DerivedTypeWithDifferentOverrides2 value = new DerivedTypeWithDifferentOverrides2() { Name1 = "Name1", Name2 = "Name2", Name3 = "Name3", Name4 = "Name4", Name5 = "Name5", Name6 = "Name6" };
DerivedTypeWithDifferentOverrides2 value = new DerivedTypeWithDifferentOverrides2() { Name1 = "Name1", Name2 = "Name2", Name3 = "Name3", Name4 = "Name4", Name5 = "Name5", Name6 = "Name6", Name7 = "Name7" };
((DerivedTypeWithDifferentOverrides)value).Name5 = "MidLevelName5";
((DerivedTypeWithDifferentOverrides)value).Name4 = "MidLevelName4";
((SerializationTypes.BaseType)value).Name4 = "BaseLevelName4";
((DerivedTypeWithDifferentOverrides)value).Name6 = "MidLevelName6";
((SerializationTypes.BaseType)value).Name6 = "BaseLevelName6";
DerivedTypeWithDifferentOverrides2 actual = SerializeAndDeserialize<DerivedTypeWithDifferentOverrides2>(value, @"<?xml version=""1.0""?><DerivedTypeWithDifferentOverrides2 xmlns:xsi=""http://www.w3.org/2001/XMLSchema-instance"" xmlns:xsd=""http://www.w3.org/2001/XMLSchema""><Name1>Name1</Name1><Name2>Name2</Name2><Name3>Name3</Name3><Name4>BaseLevelName4</Name4><Name5>MidLevelName5</Name5><Name6>BaseLevelName6</Name6></DerivedTypeWithDifferentOverrides2>");
((DerivedTypeWithDifferentOverrides)value).Name7 = "MidLevelName7";
DerivedTypeWithDifferentOverrides2 actual = SerializeAndDeserialize<DerivedTypeWithDifferentOverrides2>(value, @"<?xml version=""1.0""?><DerivedTypeWithDifferentOverrides2 xmlns:xsi=""http://www.w3.org/2001/XMLSchema-instance"" xmlns:xsd=""http://www.w3.org/2001/XMLSchema""><Name1>Name1</Name1><Name2>Name2</Name2><Name3>Name3</Name3><Name4>BaseLevelName4</Name4><Name5>MidLevelName5</Name5><Name6>BaseLevelName6</Name6><Name7>MidLevelName7</Name7></DerivedTypeWithDifferentOverrides2>");
Assert.Equal(value.Name1, actual.Name1);
Assert.Equal(value.Name2, actual.Name2);
Assert.Equal(value.Name3, actual.Name3);
Expand All @@ -2829,6 +2830,8 @@ public static void DerivedTypeWithDifferentOverrides2()
Assert.Null(actual.Name6);
Assert.Equal(((DerivedTypeWithDifferentOverrides)actual).Name6, ((SerializationTypes.BaseType)actual).Name6);
Assert.Equal(((SerializationTypes.BaseType)actual).Name6, ((SerializationTypes.BaseType)actual).Name6);
Assert.Equal(((DerivedTypeWithDifferentOverrides)actual).Name7, ((SerializationTypes.BaseType)actual).Name7);
Assert.Equal(actual.Name7, ((SerializationTypes.BaseType)actual).Name7);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1085,7 +1085,7 @@ public static void DCS_WithListOfXElement()
public static void DCS_DerivedTypeWithDifferentOverrides()
{
var x = new DerivedTypeWithDifferentOverrides() { Name1 = "Name1", Name2 = "Name2", Name3 = "Name3", Name4 = "Name4", Name5 = "Name5" };
var y = DataContractSerializerHelper.SerializeAndDeserialize<DerivedTypeWithDifferentOverrides>(x, @"<DerivedTypeWithDifferentOverrides xmlns=""http://schemas.datacontract.org/2004/07/SerializationTypes"" xmlns:i=""http://www.w3.org/2001/XMLSchema-instance""><Name1>Name1</Name1><Name2 i:nil=""true""/><Name3 i:nil=""true""/><Name4 i:nil=""true""/><Name5 i:nil=""true""/><Name6 i:nil=""true""/><Name2>Name2</Name2><Name3>Name3</Name3><Name5>Name5</Name5></DerivedTypeWithDifferentOverrides>");
var y = DataContractSerializerHelper.SerializeAndDeserialize<DerivedTypeWithDifferentOverrides>(x, @"<DerivedTypeWithDifferentOverrides xmlns=""http://schemas.datacontract.org/2004/07/SerializationTypes"" xmlns:i=""http://www.w3.org/2001/XMLSchema-instance""><Name1>Name1</Name1><Name2 i:nil=""true""/><Name3 i:nil=""true""/><Name4 i:nil=""true""/><Name5 i:nil=""true""/><Name6 i:nil=""true""/><Name7 i:nil=""true""/><Name2>Name2</Name2><Name3>Name3</Name3><Name5>Name5</Name5></DerivedTypeWithDifferentOverrides>");

Assert.Equal(x.Name1, y.Name1);
Assert.Equal(x.Name2, y.Name2);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,8 @@ public class BaseType
public string @Name5 { get; set; }

public virtual string Name6 { get; set; }

public virtual string Name7 { get; set; }
}

public class DerivedTypeWithDifferentOverrides : BaseType
Expand All @@ -852,6 +854,8 @@ public class DerivedTypeWithDifferentOverrides : BaseType
public new string Name5 { get; set; }

public override string Name6 { get; set; }

public override string Name7 { set { base.Name7 = value; } }
}

public class DerivedTypeWithDifferentOverrides2 : DerivedTypeWithDifferentOverrides
Expand Down