riok / mapperly

A .NET source generator for generating object mappings. No runtime reflection.
https://mapperly.riok.app
Apache License 2.0
2.7k stars 144 forks source link

Support generic user implemented mappings #451

Open latonz opened 1 year ago

latonz commented 1 year ago

Add support for user implemented generic mapping methods:

class Optional<T> where T : notnull { public T? Value; }
class User { string Id; }
class UserDto { string Id; }
class Document { Optional<User> ModifiedBy; }
class DocumentDto { Optional<UserDto> ModifiedBy; }

[Mapper]
public partial class Mapper
{
    public partial DocumentDto MapDocument(Document source);

    private Optional<T> MapOptional<TTarget, TSource>(Optional<TSource> source)
    {
        return new Optional<TTarget> { Value = Map<TTarget>(source.Value) };
    }

    private partial T Map(object source);

    private partial UserDto MapUser(User source);
}

Mapperly should use the user implemented MapOptional method to map Document.ModifiedBy to DocumentDto.ModifiedBy. Depends on https://github.com/riok/mapperly/issues/357. Discussed in #444.

Fube commented 1 year ago

Would a situation like this fall within what you are describing in this issue:

    [Fact]
    public void WithGenericSourceTypeConstraintsAndUserImplemented()
    {
        var source = TestSourceBuilder.MapperWithBodyAndTypes(
            """
                partial To Map(From source);

                public OtherValue PickMeForAAndB<TSource>(TSource source)
                    where TSource : IValue => new OtherValue { Value = source.Value };

                public OtherValue PickMeForIValue(IValue source) => new OtherValue { Value = source.Value };
                """,
            "public interface IValue { public string Value { get; } }",
            "public class OtherValue { public string Value { get; set; } }",

            "public class A: IValue{ public string Value {get; set;} }",
            "public class B: IValue{ public string Value {get; set;} }",

            "public class From { public A AV { get; set;} public B BV { get; set;} public IValue IV { get; set;} }",
            "public class To { public OtherValue AV { get; set; } public OtherValue BV { get; set; } public OtherValue IV { get; set; } }"
        );
        TestHelper
            .GenerateMapper(source)
            .Should()
            .HaveMapMethodBody(
                """
                var target = new global::To();
                target.AV = PickMeForAAndB(source.AV);
                target.BV = PickMeForAAndB(source.BV);
                target.IV = PickMeForIValue(source.IV);
                return target;
                """
            );
    }
latonz commented 1 year ago

@Fube that's the idea!

Fube commented 1 year ago

Would it not be possible to implement this by modifying the MappingCollection.Find method such that it looks not only for exact matches but also for "type" matches?

I'm able to get that specific test case to pass with this (extremely hacky and specific) solution:

    public ITypeMapping? Find(ITypeSymbol sourceType, ITypeSymbol targetType)
    {
        _mappings.TryGetValue(new TypeMappingKey(sourceType, targetType), out var mapping);
        if (mapping != null)
        {
            return mapping;
        }

        var sourceIndirectlyRegistered = _mappings
            .Where(x => x.Key._target.Equals(targetType)) // _target and _source of TypeMappingKey were made public
            .Select(x => (method: x.Key._source.ContainingSymbol as IMethodSymbol, kvp: x))
            .Where(x => x.method is not null)
            .Where(x =>
            {
                if (x.method?.TypeParameters[0].ConstraintTypes[0] is INamedTypeSymbol ns)
                {
                    return sourceType.AllInterfaces.Any(y => SymbolEqualityComparer.Default.Equals(y, ns));
                }

                return false;
            })
            .SingleOrDefault();

        return sourceIndirectlyRegistered.kvp.Value;
    }

I know there's a lot more to take into consideration, I'm just wondering if those things can be accounted for within MappingCollection or if it is a bigger change 😀

latonz commented 1 year ago

@Fube I think this works for methods where the source is the only generic type argument (would definitely need some cleanup 😄). If the target is (also) generic, not only the lookup side, but also the syntax generation side needs to be adjusted as explicit specification of the generic type arguments is required.

Fube commented 1 year ago

@latonz

would definitely need some cleanup 😄

Yes, very much so 😅

explicit specification of the generic type arguments is required

Could you please go into more detail as to why that would be needed? My understanding of the codebase is too shallow to grasp that

How would addressing this enhancement affect the bug described in issue #421? The syntax seems close enough that you would not be able to pickup when something is meant as a user mapping and when it is a helper method that just so happens to exist within the mapper Would we want to introduce something like [MapperIgnore] to differentiate them?

latonz commented 1 year ago

https://github.com/riok/mapperly/issues/421 occurred when Mapperly didn't have any support for generic methods, but still tried to call them. If a generic method was implemented by a user, Mapperly handled the generic type as any other type. Therefore it did type checks with typeof(TGeneric) which won't work outside the method itself, also methods with a generic target type were called like any other method, without explicit type arguments. But if the return type is a type argument, the c# compiler cannot infer the type. The fix to #421 was to just ignore them for now and open this issue to implement support. Example of how Mapperly would currently invoke generic user implemented methods if it has a generic return type:

// user implemented mapping method
TTarget Map<TSource, TTarget>(TSource source)
    where TSource : IMyInterface
    where TTarget : new(), IMyOtherInterface
  => new TTarget { MyValue = source.MyValue };

// would currently be called by Mapperly
var target = Map(source);

// instead of
var target = Map<MySource, MyTarget>(soruce);

To support this, as you figured out, Mapperly needs to implement generic mapping support during the mapping resolution in MappingCollection and UserMethodMappingExtractor.BuildUserImplementedMapping. Additionally the syntax generation needs to be aware of generics and whether they need to be specified explicitly. Probably a new GenericUserImplementedMethodMapping is needed or the existing UserImplementedMethodMapping could be adjusted. In the Build method of the generic mapping, the Invocation needs to be a GenericInvocation if type arguments need to be specified explicitly.