Throw when non-nullable struct converters indicate that they handle nullable structs #40824
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
they explicitly handle nullable structs
layomia
changed the title
steveharter
reviewed
Aug 14, 2020
| </data> | ||
| </root> | ||
| <data name="ConverterCanConvertNullableRedundant" xml:space="preserve"> | ||
| <value>The converter '{0}' which handles type '{1}' should not indicate that it also handles type '{2}'. The converter will be used automatically if no converter which handles type '{2}' is provided.</value> |
Contributor
There was a problem hiding this comment.
Suggest some rewording like
The converter '{0}' handles type '{1}' but is being asked to convert type '{2}'. Either create a separate converter for type '{2}' or change the converter's CanConvert to only return 'true' for a single type.
Contributor
Author
There was a problem hiding this comment.
We should also point to the custom converter docs here and elsewhere when we have relevant samples up. cc @tdykstra
steveharter
approved these changes
Aug 14, 2020
Contributor
steveharter
left a comment
steveharter
left a comment
There was a problem hiding this comment.
Some misc rewording comments, but otherwise LGTM.
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #36621.
The issue repro'd when a converter had this sort of
CanConvertimplementation:This caused the converter itself to be set as the converter for a nullable-struct property, rather than a
NullableConverterwrapping the custom converter. This means we were trying to set an arg ofTrather thanT?in the generated IL method which called the setter, which subsequently caused theInvalidProgramException.The serializer automatically uses an available
JsonConverter<T>when noJsonConverter<T?>is present i.e. the sampleCanConvertimplementation above is redundant. Without this override, the issue did not repro as aNullableConverterinstance wrapping the custom converter is used.Rather than leaking an
InvalidProgramException, we change the exception to anInvalidOperationExceptionwith a meaningful message. The resolution is forJsonConverter<T>converters not to have the above kind ofCanConvertimplementation, where thetypeToConvert == typeof(TestStruct?)is what would cause an exception.