Configuring Default Behaviour For ImmutableArray

[jacob7395]

Hey, not sure if this is supported behaviour I can't spot anything in the docs. There is something I would consider an issue with ImmutableArray (may be the same with other immutable types) where ImmutableArray<int> x = default is not the same as setting it to empty, and has the tendency to throw runtime exceptions if the devs don't deal with it correctly.

This extra trick when using this type in an API request where the API model might be null but the internal can't be null. When mapping the code is generating something like this;

Packages = request.Packages != null ? global::System.Collections.Immutable.ImmutableArray.ToImmutableArray(
    global::System.Linq.Enumerable.Select(request.Packages, x => MapToPackageRequest(x))
) : default,

Is there a way to easily configure the default/null behaviour for a given types so it generates something like;

Packages = request.Packages != null ? global::System.Collections.Immutable.ImmutableArray.ToImmutableArray(
    global::System.Linq.Enumerable.Select(request.Packages, x => MapToPackageRequest(x))
) : ImmutableArray<Packages>.Empty,

This would save some potential explosions if our tests don't catch the issue.

I know this could possibly be caught with some validation on the incoming request but in this case it's not something we can do.

#250 is where this feature was implemented but this was not raised.

This could be to do with how we are dealing with nulls possible something similar to how Newtonsoft handles this with [JsonProperty(NullValueHandling = NullValueHandling.Ignore)]. Would you be open to expanding how the null behaviour is used so haveing something like;

[Mapper]
public partial class CarMapper
{
    [MapperIgnoreSourceWhenNull(nameof(Car car))]
    public partial CarDto ToDto(Car car);
}

Where the mapper would not set the value on the target if the source is null? This would allow our domain model to deal with it, in this case we set it to empty instead of default. This works fine with json but has an issue with the mapper as shown in the generated code using default.

[latonz]

In fact we didn't think about default being uninitialized for immutable collections.
Doesn't AllowNullPropertyAssignment work for your use-case (which AFAIK is similar to NullValueHandling.Ignore)? (See also docs).

[jacob7395]

Looking through the mapper this is similar to how it treats classes when doing the same null to non null.

An extra complication with it being { get; init; } you can't just not set the value. So I guess it would have to read if the target has a defaulted value and use that.

I think this is why an attribute would be the best way around it, and maybe rather then ignore if null as that wouldn't work with init properties, have an attribute that allows you to overwrite the default value for the mapper. Not sure how difficult that will me I have had a look at the code base and it goes way over my head, need to read up on source code generators. (If you have an recommendation for reading material it would be appreciated 😄)

For now I have made a way round it by using a UserMapper

[MapProperty(
    nameof(CreateShipmentsRequest.Shipments),
    nameof(CreateShipmentBatchRequest.Shipments),
    Use = nameof(MapShipments))]
public static partial CreateShipmentBatchRequest ToRequest(this CreateShipmentsRequest request);

[UserMapping(Default = false)]
private static ImmutableArray<ShipmentCreateInfoRequest> MapShipments(List<CreateShipmentRequest>? shipments)
    => shipments.IsNullOrEmpty()
        ? ImmutableArray<ShipmentCreateInfoRequest>.Empty
        : shipments!.Select(s => s.ToRequest()).ToImmutableArray();

The above is basically doing what

[latonz]

Does the AllowNullPropertyAssignment approach work if the target property has not init but set?

With #1285 the user mapping approach would get a lot easier:

[UserMapping(Default = true)]
private static ImmutableArray<T> MapImmutableArray<T>(List<T>? items)
    => items != null
        ? ImmutableArray<T>.Empty
        : Map(items);

Do you have a proposal on how this feature could work exactly? How could the API look like? What cases are exactly affected?

A few starting points to dive into Roslyn source gens:

• source generators cookbook,
• incremental generators cookbook
• mapperly contributors docs

[jacob7395]

Ye looks like it works as expect when it's not init;

public record Test1
{
    public List<int>? Prop { get; set; }
}

public record Test2
{
    public ImmutableArray<int> Prop { get; set; }
}

public static partial class TestMappers
{
    internal static partial Test2 ToDomain(this Test1 source);
}

Generates

var target = new global::Arcanaloth.Server.Costs.Infrastructure.Accountants.Dtos.Test2();
if (source.Prop != null)
{
  target.Prop = global::System.Collections.Immutable.ImmutableArray.ToImmutableArray(source.Prop);
}
return target;

It generates the same with or without the null assignment.

[jacob7395]

Before saying saying how I think this could look it's worth clarify the behavior using, the example above, the defaul assignment happens when ThrowOnMappingNullMismatch is set to false. AllowNullPropertyAssignment doesn't appear to effect this. So when generating for these types;

public record Test1
{
    public List<int>? Prop { get; set; }
}

public record Test2
{
    public ImmutableArray<int> Prop { get;  init; }
}

[Mapper(ThrowOnMappingNullMismatch = false)]
public static partial class TestMappers
{
    internal static partial Test2 ToDomain(this Test1 source);
}

It currently produces;

            var target = new global::Arcanaloth.Server.Costs.Infrastructure.Accountants.Dtos.Test2()
            {
                Prop = source.Prop != null ? global::System.Collections.Immutable.ImmutableArray.ToImmutableArray(source.Prop) : default,
            };
            return target;

If ThrowOnMappingNullMismatch is not set of set to true, then instead of default it will thrown and exception.

The problem with this situation is the use of init as shown above when it's a get there are not issues but I like immutable types so would like so see them supported.

Proposed Options

In my opinion I would lean towards something explicit so either an addition to the MapProperty attribute like;

    [MapProperty(nameof(Test1.Prop), nameof(Test2.Prop), WithDefaultValue = ImmutableArray<int>.Empty)]
    internal static partial Test2 ToDomain(this Test1 source);

But this isn't a compile time constant so can't go in an attribute.

Maybe then using a property or method similar to how user maps work;

    [MapProperty(nameof(Test1.Prop), nameof(Test2.Prop), WithDefaultValue = nameof(DefaultValue))]
    internal static partial Test2 ToDomain(this Test1 source);
    
    internal static ImmutableArray<int> DefaultValue => ImmutableArray<int>.Empty;

Another option would be;

[Mapper(ThrowOnMappingNullMismatch = false)]
public static partial class TestMappers
{
    internal static partial Test2 ToDomain(this Test1 source);
    
    [UserDefaultValue]
    internal static ImmutableArray<int> DefaultValue => ImmutableArray<int>.Empty;
}

I am not sure if either of these are a good solution for the issue 🤷 I had a look through #1285 and that might work I need a bit more time to think it though. I feel like the above options are trying to solve a problem with init properties when there could be a null issue by introducing a feature that allows for users to set the default options.

Maybe the best approach would be for Mapperly to incorporate default assignment values. So given this;

public record Test1
{
    public List<int>? Prop { get; set; }
}

public record Test2
{
    public ImmutableArray<int> Prop { get;  init; } = ImmutableArray<int>.Empty;
}

[Mapper(UseDefaultValues = true)]
public static partial class TestMappers
{
    internal static partial Test2 ToDomain(this Test1 source);
}

Mapperly would pick up that Test2.Prop has got a default value of ImmutableArray<int>.Empty, when building the assignments it would use this default value instead of assigning it in this case to default.

This is only a special case for init as get will just not be assigned and use the default value, however support init/constructer properties like this seems to most intuitive.

Thoughts?

[latonz]

I like the

[UserDefaultValue]
internal static ImmutableArray<int> DefaultValue => ImmutableArray<int>.Empty;

approach.

I'm not sure if we should name the attribute UserDefaultValue or MappingNullSubstitute. NullSubstitute is probably more common among other well-established mapping libraries in .NET but is not 100% true for value types.