Diagnostic for mapping null mismatch #1592
Conversation
There was a problem hiding this comment.
Thank you for this contribution!
I think in general all call sites of FindOrBuildLooseNullableMapping need to report this diagnostic (that is what you are already doing as there is only one). I think additionally we should report another diagnostic in Riok.Mapperly.Descriptors.MappingBuilders.NullableMappingBuilder.
| @@ -84,6 +85,17 @@ public static bool TryBuild( | |||
| return false; | |||
| } | |||
|
|
|||
| if (memberMappingInfo.IsSourceNullable && !targetMember.MemberType.IsNullable()) | |||
There was a problem hiding this comment.
Couldn't you use delegateMapping.TargetType.IsNullable() instead of !targetMember.MemberType.IsNullable() which should solve the ShouldPassNullValueToNullableUserMappingMethod test.
There was a problem hiding this comment.
I hope you can forgive me if I am misunderstanding you, I changed it to !delegateMapping.TargetType.IsNullable() but that test is still failing, and I added the diagnostic for the NullableMappingBuilder
There was a problem hiding this comment.
Oh my bad I mixed it up a bit. I cannot try it myself right now, but I think you would need to check source and target... memberMappingInfo contains the source/and target member being mapped. delegateMapping is the mapping used to map from the source to the target (convert the types). This means nullability could mismatch from source to the source of the delegateMapping and from the target of the delegateMapping to the target member.
So I think the correct condition would be (!memberMappingInfo.TargetMember.MemberType.IsNullable() && delegateMapping.TargetType.IsNullable()) || (!delegateMapping.SourceType.IsNullable() && memberMappingInfo.TargetMember.MemberType.IsNullable()).
There was a problem hiding this comment.
That clarifies it a lot, thanks. I've changed the check and in these two cases it does seem to work the way I want now. There are lots of tests still failing mainly because of Verify but also some valid cases I think, e.g. the DictionaryToDictionaryNullableToNonNullable which is not having the result I would expect but I am going to look into it again tomorrow
There was a problem hiding this comment.
Everything seems to work as expected with the exception of this test:
DisabledNullableClassPropertyToNonNullableProperty now gives a diagnostic, don't know if that's desired
And I would like your opinion on the setting to disable the warning. This was requested in the original ticket, but it can be achieved with pragma disable as well ofcourse, it's not a setting like the other settings which actually change how the mappings work. So:
a. Is this something that should be added?
b. If so, make it one setting for both these diagnostics together or separate settings? I would lean to one setting personally
There was a problem hiding this comment.
Regarding DisabledNullableClassPropertyToNonNullableProperty: Unfortunately, at the beginning of Mapperly, we decided to treat nullable-disabled contexts as if they were nullable-enabled contexts with all possible nullable annotations set. In retrospect, this was a bad decision, and we want to change it in the future, but it will take a lot of work. There are scenarios where nullable-disabled needs to be treated as non-nullable (for example here, in queryable projections, ...)...
Regarding the setting: The severity of these diagnostics can be set quite flexibly via editorconfig files (on a per-file or per-directory basis). However, I think enabling or disabling it for a given member is not possible with editorconfig/pragma/SuppressMessageAttribute, so a config is nice. I'd implement this in a separate PR, as this PR already adds a useful feature. My API suggestion for this would be to add a new property bool SuppressNullMismatchDiagnostic to the MapPropertyAttribute.
src/Riok.Mapperly/Descriptors/MappingBodyBuilders/MemberMappingBuilder.cs
Outdated
Show resolved
Hide resolved
| @@ -84,6 +85,17 @@ public static bool TryBuild( | |||
| return false; | |||
| } | |||
|
|
|||
| if (memberMappingInfo.IsSourceNullable && !targetMember.MemberType.IsNullable()) | |||
There was a problem hiding this comment.
Regarding DisabledNullableClassPropertyToNonNullableProperty: Unfortunately, at the beginning of Mapperly, we decided to treat nullable-disabled contexts as if they were nullable-enabled contexts with all possible nullable annotations set. In retrospect, this was a bad decision, and we want to change it in the future, but it will take a lot of work. There are scenarios where nullable-disabled needs to be treated as non-nullable (for example here, in queryable projections, ...)...
Regarding the setting: The severity of these diagnostics can be set quite flexibly via editorconfig files (on a per-file or per-directory basis). However, I think enabling or disabling it for a given member is not possible with editorconfig/pragma/SuppressMessageAttribute, so a config is nice. I'd implement this in a separate PR, as this PR already adds a useful feature. My API suggestion for this would be to add a new property bool SuppressNullMismatchDiagnostic to the MapPropertyAttribute.
jorisBarkema
changed the title
|
I made a mistake while rebasing this PR, which resulted in an inability to push to the original branch. As a result, the PR was automatically closed. To resolve this, I’ve opened a new PR (#1612) with the same changes. Please note that while the PR is new, the commit author remains the original one, so the history should still reflect the original work. Unfortunately the PR Review history is only visible in this PR. Apologies for the inconvenience, and thank you for your understanding! |
|
Alright no problem, thanks for the reviews and for maintaining this project! |
Diagnostic for mapping null mismatch
Description
Adds a diagnostic when a nullable source value is mapped to a non-nullable target value
Related issue: #602
Fixes #602
Checklist