Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support generic user implemented mappings #451

Open
latonz opened this issue May 22, 2023 Discussed in #444 · 6 comments
Open

Support generic user implemented mappings #451

latonz opened this issue May 22, 2023 Discussed in #444 · 6 comments
Labels
enhancement New feature or request

Comments

@latonz
Copy link
Contributor

latonz commented May 22, 2023

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 #357.
Discussed in #444.

@latonz latonz added the enhancement New feature or request label May 22, 2023
@Fube
Copy link

Fube commented Jun 21, 2023

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
Copy link
Contributor Author

latonz commented Jun 21, 2023

@Fube that's the idea!

@Fube
Copy link

Fube commented Jun 21, 2023

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
Copy link
Contributor Author

latonz commented Jun 21, 2023

@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
Copy link

Fube commented Jun 21, 2023

@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
Copy link
Contributor Author

latonz commented Jun 22, 2023

#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants