Nullable annotations of the IMapper(base)

[Erythnul]

Hi, hope you are well.

I've been playing around with the preview build (because of issues with the Include/IncludeBase being fixed there and not in the master) and found that in commit 7bd1b81 nullable annotations were enabled for the IMapper interfaces.
This is great, however, I feel the implementation could be smarter using some of the static analysis annotations.

Currently, using the preview package would generate loads of nullability warnings in my code. The code mainly uses the method

TDestination? Map<TDestination>(object? source);

This doesn't seem entirely correct to me, as I make sure that the type going in is not null in these cases and expect a non-null output.

Assumptions (based on my experience):

• If the source is not null
  AND
• The typeMap is well-configured
  THEN
  The output TDestination cannot be null.

if the typeMap is not well-configured I expect an exception to be thrown, correct?

So we could solve this here by using the following:

    [return: NotNullIfNotNull(nameof(source))]
    TDestination? Map<TDestination>(object? source);

For the generic variants I'm not sure what you would prefer. The commit made it into the following:

    TDestination? Map<TSource, TDestination>(TSource? source);
    TDestination? Map<TSource, TDestination>(TSource? source, TDestination? destination);

But then, if I use it like this:

        var list = new List<TDestination>();
        var value = JsonConvert.DeserializeObject<TSource>(jsonString);

        if(value != null)
        {
            var mapped = context.Mapper.Map<TSource, TDestination>(value);
            viewModels.Add(mapped);
        }

I will still get a nullability warning on the viewModels.Add(mapped).
Here, I personally would expect nullability to be governed by the nullability of the generic parameters in the signature of the method, like so:
TDestination Map<TSource, TDestination>(TSource source);

This would allow for usage like above without nullability warnings, and properly warn you if you are potentially sending a null value into the mapper where you do not intend it. You could also explicitly enable nullability for the mapper like so:

            var mapped = context.Mapper.Map<TSource?, TDestination?>(value);
            viewModels.Add(mapped);

Alternatively, NotNullIfNotNull could also help here, but feels less flexible to me.

I could go on, but you get the point. Of course, I am willing to open up a PR so we can discuss this further.

What do you think?

Kind regards.

[lbargaoanu]

The problem is that you can have a custom map and that one can return null whenever. And there are also the built-in settings that affect the result, AllowNullDestinationValues and AllowNullCollections. So I'm not sure we can do better here.

[Erythnul]

Hmm, of course you are right.
It's a shame though, because this will clutter up our code with loads of unnecessary null checking or we suffer warnings that can not be fixed.
I still feel there's value-add in fixing up the generic methods so the nullability of the input/output is governed by the values the user puts in, but I will leave the decisions to you!
Then again, I don't really see a solution either, so I'll close this.
Thanks for your time!

[lbargaoanu]

You do have a point, but I'm not sure what to do about it. Check this. Somewhat similar and it has the annotation on the result. Maybe not the best example, but I'm open to see a better one.

[Erythnul]

To be completely honest, that one also bugs me.
We still user Newtonsoft, but they do the same, and it forces me to write code that to me, is counter-intuitive.
If I'm requesting a non-nullable type from a generic method, I expect it not to return null as that's not a valid value for the return type. I, personally, would expect a JSON deserialization to throw if deserialization fails and not to return null to a non-nullable type, same for a mapping. If I wanted null to be a valid value, I'd be requesting a nullable type from the generic.
For deserialization, we generally now use a null-coalescing throw expression, but it still makes me somewhat sad ;)