[release/5.0] Support polymorphic value-type converters and add validation on values returned by custom converters (#40914)

[layomia]

Ports #40914 to release/5.0.

Fixes #40878, #36329, and #41146 in .NET 5.

• Support polymorphic converters that return value types (TypeToConvert == typeof(object) or an interface).
  • Works for both Nullable and non-Nullable value types.
• Validate the return value from converters so they don't return an incompatible object on deserialization.

Customer impact

The serializer now has correct semantics to handle polymorphic converters, and will (de)serialize as expected in valid scenarios, and will throw the right exceptions for invalid cases. Exceptions avoided are InvalidProgramException and AccessViolationException which were leaked due to the serializer generating invalid IL for property getters/setters for these scenarios. Also prevents behavior regressions for previously supported polymorphic converter usages.

Testing and risk

The changes in this PR are specific to handling when polymorphic custom converters for value types are used i.e the type to convert is a struct, while the provided converter returns typeof(object) or an interface. This is an edge case scenario and will not affect the vast majority of custom converter usages/implementations.

This change is well tested. Tests have been added for various permutations of previously problematic inputs to the serializer.

[steveharter]

LGTM; direct port

[ericstj]

I agree this meets the bar for RC.

Tests have been added for various permutations of previously problematic inputs to the serializer.

Thanks for the info on testing: I assume we've done a pretty thorough run-through of the missing scenarios and increased coverage. Did you make sure to examine the 3.x behavior when establishing test assertions for these new cases?

Is there any perf concern that would warrant examining perf numbers before/after this change?

[layomia]

I assume we've done a pretty thorough run-through of the missing scenarios and increased coverage.

Yes, @steveharter and I reviewed the missing polymorphic converter scenarios closely offline and believe this PR and the added tests give us good coverage.

Did you make sure to examine the 3.x behavior when establishing test assertions for these new cases?

Yes, I manually ran each new test on the 3.1 build locally and verified that we have no behavorial regressions. In multiple cases, we were previously (in 3.1) leaking bad exceptions (InvalidCastException, NullReferenceException), but now we have appropriate support and throw intentional exceptions where applicable.

On the issue of tests, I added 8830d04 to this PR to prevent test issues due to non-deterministic reflection order on serialization.

Is there any perf concern that would warrant examining perf numbers before/after this change?

Wrt. the additional boxing/unboxing in the generated IL to call property getters/setters; this additional IL is only emitted in the very specific and edge-case scenario where a polymorphic converter is used for a value-type property. As such, I didn't expect any regressions to our main benchmarks. I verified this by running the serializer benchmarks; barring benchmark noise, there are no regressions.

The added validation for the value returned by external converters (user-provided custom converters) did cause a small throughput perf regression (~6%). This is mitigated with a new commit (8e6478e) by restricting the new checks to only happen when the converter is external and polymorphic.

---

The new changes will be committed in the 6.0 branch via #41427.

polymorphic_pr_converter_benchmarks.zip
polymorphic_pr_main_benchmarks.zip

[ericstj]

Meets 5.0 bar since it is a regression from 3.1.

Seems like we've done a sufficient amount of risk mitigation. Thank you for considering my requests.

I'd like one more sign-off on the final change since it has a couple small diffs with what @steveharter previously reviewed. @jozkee could you give it a look?

[jozkee]

Already discussed #41427 with @layomia offline and the changes in the last two commits are identical.

Lgtm.

[layomia]

Test failures unrelated:

System.Net.Http.Functional.Tests
System.Threading.Tasks.Parallel.Tests