Nullable compilation error

[KirkMunroSagent]

In the upgrade docs, you state the following:

IMapper has nullable annotations

Besides the build-time impact, there is also a behaviour change. Non-generic Map overloads require now either a destination type or a non-null destination object.

Yet in the Target .NET 6 PR, the IMapperBase and IMapper definitions have no overloads (generic or non-generic) that accept a non-null destination object and return a non-null object (which is contrary to what you are stating in the documentation).

i.e. A simple version 12 to version 13 upgrade results in a breaking change for all existing calls to generic Map overloads in projects that use #nullable enable because the inputs and return type for those has changed to be nullable objects.

We have code with calls to generic Map overloads where we're passing in non-null source object and receiving a non-null destination object, and those calls now result in compilation errors with the v13 upgrade.

That's an upgrade blocker, and a poor implementation of nullable enable that should be fixed asap. You should also consider pulling the release/marking it as obsolete so that people don't upgrade until this is fixed.

The change that needs to be revisited is in src/AutoMapper/Mapper.cs. It appears that the developer who implemented that change simply made everything nullable (or close to it), including the return types, on all generic and non-generic overloads, which is completely wrong.

[KirkMunroSagent]

To demonstrate the issue described above, here's some rough (uncompiled, untested, just typed into this editor) sample code that should compile with v12 but will not compile if you upgrade to v13:

#nullable enable

namespace BreakingChangeExample;

public class ThingConverter
{
    private readonly IMapper _mapper;

    public ThingConverter(IMapper mapper)
    {
        _mapper = mapper;
    }

    public ConvertedThing ConvertTheThing(UnconvertedThing unconvertedThing) =>
        _mapper.Map<ConvertedThing>(unconvertedThing);
}

You would also run into unexpected compilation issues on upgrade if you were using #nullable enable and you did something like this:

ConvertedThing convertedThing = _mapper.Map<ConvertedThing>(unconvertedThing);

[jbogard]

Another option here is to use the null forgiving aka dammit operator:

#nullable enable

namespace BreakingChangeExample;

public class ThingConverter
{
    private readonly IMapper _mapper;

    public ThingConverter(IMapper mapper)
    {
        _mapper = mapper;
    }

    public ConvertedThing ConvertTheThing(UnconvertedThing unconvertedThing) =>
        _mapper.Map<ConvertedThing>(unconvertedThing)!;
}

[lbargaoanu]

This is by design and it was discussed before.

[KirkMunroSagent]

Ok, I found one of the discussions.

Regarding this comment: #4297 (comment)

Using the possibility of a custom mapper that returns null as justification to always have every Map method interface return nullable types forcing callers to add nullable checking code all over the place makes no sense. You could have overloads that return non-null, keeping existing code working, and internally within those overloads check for null, throwing an exception about a mapping returning null when non-null was expected. Then it's on the caller to call the appropriate Map method. IMHO that's a much better solution for all of the consumers of AutoMapper that use nullable enable, allowing their existing code to continue to work as is, but if they are one of those customers using a custom mapper that can return null, then they'll need to update their code to use the overloads that return nullable types for those scenarios.

[jbogard]

You could have overloads that return non-null, keeping existing code working

I don't think that's true. You can't declare nullable and non-nullable overloads, since it's all compiler magic and not actually separate types (unlike nullable value types).

[lbargaoanu]

I'd rather not go down that rabbit hole. It seems to me that your code assumes things that are not true.

[KirkMunroSagent]

These overloads:

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

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

    TDestination? Map<TDestination>(object? source, Action<IMappingOperationOptions<object, TDestination>> opts);

    TDestination? Map<TSource, TDestination>(TSource? source, Action<IMappingOperationOptions<TSource, TDestination>> opts);

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

If I'm using a generic overload, where I specify the TSource and TDestination types, then it's up to me to decide if either of those types are nullable, but I can't, because the method definitions themselves incorrectly and blindly force TSource and TDestination to be nullable by using TSource? and TDestination? in the method arguments and/or the return type. That's what's wrong with the v13 release.

[jbogard]

The reality is that underneath the covers we use expression tree compilation, where none of these nullablility rules are enforced anyway. You're making assumptions that just aren't enforced in reality by AutoMapper. This isn't too far off other expression-tree-based tools, where we just can't make those guarantees. So we don't. We let you enforce that guarantee and assumption.

[lbargaoanu]

In the end, we could easily revert the whole thing :) But it sure looks a lot like lying.

[KirkMunroSagent]

So is the issue I'm struggling to see here that Map can always return null, no matter what? Or that it can return null if I am using some custom mapping logic to have it return null? How do AllowNullDestinationValues and AllowNullCollections come into play here?

If this really has to be this way, I'll adjust. It just seems odd to push a requirement for null checking and exception handling out this way though, when I imagine teams will end up creating wrappers that have better null-handling to absorb what seems absorbable within AutoMapper.

[EklipZgit]

Certainly feels like a new method MapNonNull or MapNullable something should have been added. We've had quite literally 0 problems with .Map returning nulls unexpectedly in our codebase, and yet consuming new automapper is going to generate something like 2000 nullability errors.
The 99.99% use case is going to be the only input that results in a null output will be a null input. There's already hinting for that,
[return: NotNullIfNotNull("input")]

We'll definitely end up with our own automapper wrapper that makes .Map (or more likely, MapNullable) include [return: NotNullIfNotNull("input")]. Just would be nice if that were a new Method that did that for you (and threw if your map produces a null for a non-null input).

I applaud the effort to make your API correctly declare the truth, however some thought should be given to the thousands of projects that use nullable and also declare non-null-input->non-null-output maps like you'd expect, and how their quality of life is impacted by this change. If 99% of your null forgiveness operators in an enterprise codebase are all at the end of mapper.Map()!, proooooobably theres a better approach.

[lbargaoanu]

You don't need an AM wrapper, you can write an extension method on IMapper. Unless you call that a wrapper :)

[KirkMunroSagent]

I pulled down the repo yesterday to take a closer look and had toyed around with pairing each Map interface method with MapNullable, while also adding where TSource: notnull and where TDestination: notnull generic constraints to the Map methods. That would also have been a breaking change, but only for consumers who are using null in their inputs or outputs, which is likely a small minority. IMHO that would have been a better approach than simply adding nullable to the interfaces without providing something that makes upgrading to version 13 easier for all consumers of AutoMapper. The way it is now, you're hearing from a small group because this was just released, but I can't see how this won't make many more of your consumers unhappy if they're either already using #nullable enable or as they start using #nullable enable. It's simply creating work that should be unnecessary at the leaf level (the consumer) rather than addressing it more appropriately in the root (AutoMapper).

[jbogard]

Reviewed the MS guidelines a bit more closely, here's a stab at documenting the intended/observed behavior:

#4387

[EklipZgit]

This looks good to me, I like this approach. Technically a new MapNullable interface that enforces the non-null-for-not-null would be the most truthful way to approach this and solve @lbargaoanu's concerns, but if you'd prefer to keep only the existing Map methods, this PR is definitely the way to go.

[PaulVrugt]

Also running into this, I suddenly need to fix a million nullable warnings, which in facts aren't warnings at all. I'm not upgrading right now, waiting for a fix. There was nothing wrong with the original code, and I think we can safely assume that when you pass in an object into the Map method that is not null, you will get something back that is not null. I'm not planning of adding thousands of unnecessary nullability checks

[lbargaoanu]

Until someone adds a map that returns null for a non-null source. I don't think these annotations are statistical :)

[PaulVrugt]

I'm sure there are edge cases in which a non null value maps to null, but let's be practical here. Everyone using nullable reference types is going to run into this when updating. And 99,999% of the time the warning is unnecessary.

You might even consider not allowing mapping non null to null in the future to fix this. Or create a different overload that does allow null output for non null values

[jbogard]

I'm just going to revert nullables for now, clearly this was a bit too disruptive and will need a bit more careful thought to introduce.

[jbogard]

Reverted: #4388