Skip to content

Fix NullReferenceException for inherited ShouldSerialize methods#125859

Closed
haltandcatchwater wants to merge 1 commit intodotnet:mainfrom
haltandcatchwater:fix-shouldserialize-inherited-method
Closed

Fix NullReferenceException for inherited ShouldSerialize methods#125859
haltandcatchwater wants to merge 1 commit intodotnet:mainfrom
haltandcatchwater:fix-shouldserialize-inherited-method

Conversation

@haltandcatchwater
Copy link
Copy Markdown
Contributor

Fixes #120561

Summary

ReflectionXmlSerializationWriter used BindingFlags.DeclaredOnly when looking up ShouldSerialize* methods at two call sites (attribute members loop and element/text members loop). This prevented finding methods declared on base classes, causing a NullReferenceException when serializing derived types.

The MemberMapping already stores the correctly resolved MethodInfo in CheckShouldPersistMethodInfo, resolved without DeclaredOnly during import. The IL-gen path (XmlSerializationWriterILGen) already uses this pre-resolved MethodInfo directly. This change aligns the reflection path to match.

Changes

  • ReflectionXmlSerializationWriter.cs: Replace redundant GetMethod lookups with m.CheckShouldPersistMethodInfo at both call sites
  • SerializationTypes.cs: Add BaseTypeWithShouldSerializeMethod and DerivedTypeWithInheritedShouldSerialize test types with both [XmlAttribute] and element properties
  • XmlSerializerTests.cs: Add tests for inherited ShouldSerialize with default and non-default values, exercising both attribute and element serialization paths

…herited ShouldSerialize methods

ReflectionXmlSerializationWriter used BindingFlags.DeclaredOnly when
looking up ShouldSerialize methods at two call sites, which prevented
finding methods declared on base classes. This caused a
NullReferenceException when serializing derived types whose base class
defines the ShouldSerialize method.

The MemberMapping already stores the correctly resolved MethodInfo
(CheckShouldPersistMethodInfo), which is resolved without DeclaredOnly
during import and used by the IL-gen path. Use it directly in the
reflection path to match the IL-gen behavior.

Fix dotnet#120561
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Mar 20, 2026
@haltandcatchwater
Copy link
Copy Markdown
Contributor Author

haltandcatchwater commented Mar 20, 2026 via email

StephenMolloy pushed a commit that referenced this pull request Apr 25, 2026
…rializationReader (#126035)

## Summary

`ReflectionXmlSerializationReader` 
This pull request improves the XML serialization and deserialization
logic by optimizing how "ShouldSerialize" and "Specified" members are
handled, and adds new tests to ensure correct behavior for inherited
serialization methods and field-backed "Specified" members.

**Serialization and Deserialization Logic Improvements:**

* Refactored the invocation of "ShouldSerialize" methods by using cached
`MethodInfo` instances (`CheckShouldPersistMethodInfo`) instead of
repeated reflection lookups, improving performance and maintainability.
[[1]](diffhunk://#diff-a7d501d15214a1968cd73f5cab3996ae3b6143e01076c91deda858f5b4123f97L664-R664)
[[2]](diffhunk://#diff-a7d501d15214a1968cd73f5cab3996ae3b6143e01076c91deda858f5b4123f97L696-R694)
* Updated the mechanism for setting "Specified" members during
deserialization to use a delegate (`GetSetMemberValueDelegate`) rather
than reflection on every call, making the process more efficient and
robust.

**Test Coverage Enhancements:**

* Added new test cases to verify correct serialization and
deserialization behavior for types with inherited "ShouldSerialize"
methods, ensuring that inherited logic is respected.
[[1]](diffhunk://#diff-cb25d8cca02e9c7959514c29f15823cfbdcae1b51cd148ddc69e2a8a6fb780e4R1717-R1752)
[[2]](diffhunk://#diff-c73de78f6d9328eba5ba3745a05436eb60f682fb66efb6ee0b17ec443cb5c1a9R982-R1011)
* Added a test for types with field-backed "Specified" members to
confirm that these are correctly set during deserialization.
[[1]](diffhunk://#diff-cb25d8cca02e9c7959514c29f15823cfbdcae1b51cd148ddc69e2a8a6fb780e4R1717-R1752)
[[2]](diffhunk://#diff-c73de78f6d9328eba5ba3745a05436eb60f682fb66efb6ee0b17ec443cb5c1a9R982-R1011)

**Test Types Additions:**

* Introduced new test types: `BaseTypeWithShouldSerializeMethod`,
`DerivedTypeWithInheritedShouldSerialize`, and
`TypeWithFieldBackedSpecifiedMember` to support the above tests.used
`GetMethod("set_X")` to set `XxxSpecified` members during
deserialization. This only finds property setters — for field-backed
`Specified` members, `GetMethod` returns null and the setter is silently
skipped, leaving the `Specified` flag as `false` even when the element
is present in the XML.

The rest of the reflection reader already uses
`GetSetMemberValueDelegate` (which handles both fields and properties
and caches the delegate). The ILGen reader handles both via `ILGenSet`.
This was the only call site using the raw `GetMethod("set_X")` pattern.

Related to #125859 (same class of reflection-path drift from the ILGen
path).

## Changes

- **ReflectionXmlSerializationReader.cs**: Replace `GetMethod("set_X")`
+ `?.Invoke` with `GetSetMemberValueDelegate`, resolving once and
caching
- **SerializationTypes.cs**: Add `TypeWithFieldBackedSpecifiedMember`
with a `public bool FooSpecified` field
- **XmlSerializerTests.cs**: Add test verifying `FooSpecified` is set to
`true` after round-trip deserialization
@StephenMolloy
Copy link
Copy Markdown
Member

This fix was already included as part of #126035. Closing, since that PR was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Serialization community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ReflectionXmlSerializationWriter throws NullReferenceException when base class contains "ShouldSerialize" methods

2 participants