POC Extra parameters

[TimothyMakkison]

POC for #103. This is a simpler version of #550, parameters do not attempt to find a matching memberpath nor are they passed down to sub methods.

• IMapping has a ImmutableEquatableArray<MethodParameter> Parameters property
• All user defined mappings should be able to support extra parameters
• Unlike POC Additional parameter mapping #550, passing down parameters is severly limited. Therefore, the initialization of MethodMapping is not significantly changed.
• Extra parameters can resolve constructor arguements, required/init and object member mapping, with support for [MapProperty].

public static partial UserDto Map(User src, int id):

public static partial UserDto Map(User src, int id)
{
    var target = new UserDto();
    target.Username = src.Username;
    target.Id = id;
}

Parameter usage

This PR has a simple way of using parameters, if a target member has the name username, mapperly will attempt to resolve this from the source parameter and then look for a parameter named username.
It will not attempt to a parameter with a matching memberpath i.e. given a member username match with parameter ,User user) where user.Name -> username

This does diverge from how the source parameters are treated, but given that extra parameters use their name to resolve paths, it perhaps makes more sense. This could easily be changed.

Method resolving

I believe that user defined mappers should be able to call other user defined/implemented mappers if they have compatible signatures and paramters. I have not decided on the best approach for method resolving. Part of the challenge is choosing a resolving approach that will not prevent future changes.

• Should parameters match using type and name or just name and map to type?
• Similarly to Parameter usage should matching member paths be used?

Approaches

• Only use another mapper if it has the same signature and parameters.
• Try to match parameters by name while preserving the order of the parameters:
  • (long id, string name, int age) can use (string name, int age)
  • (long id, string name, int age) can't use (int age, long id)
• Match parameters by name - could cause issues if mapperly created methods are allowed to use extra parameters:
  • (long id, string name, int age) can use (string name, int age)
  • (long id, string name, int age) can use (int age, long id)

[latonz]

Thank you for this PR 🚀 I did not start the code review yet, I'll review it in the upcoming days. Overall this looks like a promising feature 😊 I prefer the simple approach of not passing the additional parameters, because otherwise it can become really hard to understand when which methods are invoked/generated and why.

Regarding your description of the PR:

It will not attempt to a parameter with a matching memberpath i.e. given a member username match with parameter ,User user) where user.Name -> username

I don't get this. Do you mean MemberPathCandidateBuilder.BuildMemberPathCandidates is not used for additional source parameters? If so, whats the reasoning behind this?

Method resolving

In a first version of this feature, I would not pass additional parameters anywhere, too keep it simple. Otherwise this could get quite complex when and why another method is called/generated. I'm not even sure if there is demand for this advanced feature. For now if this is needed, it is always possible to make keep a context inside the mapper class, instantiate a mapper with the context and use after map to set these fields.

[TimothyMakkison]

Thank you for this PR 🚀 I did not start the code review yet, I'll review it in the upcoming days. Overall this looks like a promising feature 😊 I prefer the simple approach of not passing the additional parameters, because otherwise it can become really hard to understand when which methods are invoked/generated and why.

Thanks, please don't spend too much time reviewing this, it will likely change if I modify it. I would appreciate some pointers with my abstractions, i.e. should IMapping know about the parameters etc.

Regarding your description of the PR:

It will not attempt to a parameter with a matching memberpath i.e. given a member username match with parameter ,User user) where user.Name -> username

I don't get this. Do you mean MemberPathCandidateBuilder.BuildMemberPathCandidates is not used for additional source parameters? If so, whats the reasoning behind this?

The previous PR would create the following.

public record UserInfo (int Id);;

public record UserDto(string UserId, ...);

public static partial UserDto MapTo(SourceInfo src, UserInfo user);

// generates the following
public static partial UserDto MapTo(SourceInfo src, UserInfo user)
{
    var target = new UserDto(user.Id, ...);
    return target;
}

Here the extra parameter named user matches to the target UserId because the parameters name combined with the member Id match UserId.
I removed this because I thought this behaviour could be confusing as the source parameters name is never combined with its members.

public static partial DogDto Map(Dog dog)
// does not generate
target.DogOwnder = dog.Owner;

On the other hand, extra parameters already diverge from the source parameter by using their name. So not having this behaviour could also be confusing. I can readd this.

Method resolving

In a first version of this feature, I would not pass additional parameters anywhere, too keep it simple. Otherwise this could get quite complex when and why another method is called/generated. I'm not even sure if there is demand for this advanced feature. For now if this is needed, it is always possible to make keep a context inside the mapper class, instantiate a mapper with the context and use after map to set these fields.

I agree 😊. As I found before, passing extra parameters to auto-generated methods means you have to account for scopes/configuration contexts. I had to monkey-patch a way of changing the order of mapping generation from a simple queue into a recursive depth-first generator while also accounting for loops.

I was thinking of how to support extra parameters for user implemented and defined mappers. This wouldn't have the above issues. We can see if this is needed.

For example in the below code, mapperly could use the user defined method MapProducer because it has a matching parameter.

[Mapper(EnumMappingStrategy = EnumMappingStrategy.ByName)]
public static partial class CarMapper
{
    [MapProperty(nameof(Car.Manufacturer), nameof(CarDto.Producer))] // Map property with a different name in the target type
    public static partial CarDto MapCarToDto(Car car, string name);
    
    public static ProducerDto MapProducer(Producer src, string name)
    {
        // ...
     }
}

[latonz]

I converted this PR to draft for now.

For example in the below code, mapperly could use the user defined method MapProducer because it has a matching parameter.

I think this would still result in weird behaviours. For example if the Car has a property of type Tire which then has a property of type Producer. The mapping of the tire is not user implemented but generated (without the name parameter). Then the user implemented MapProducer is never called, because the name parameter got lost in the generated map method for the tire mapping. But if the Producer was directly on the Car, the name parameter would have been passed. IMO this behaviour would be hard difficult to follow.
I think for nested / advanced scenarios a new MyMapper(myAdditionalContext).Map(...) with before/after map is still the best solution.

[TimothyMakkison]

Great point, this and the method resolution issues could make this pretty janky.

Thankfully [MapProperty] can be used with extra parameters.

I'll add member resolution for extra parameters.

[latonz]

I did a quick review but I think a lot of code will change when we only consider additional parameters in the user defined mapping methods and don't pass them anywhere. Therefore I'll do a complete review once this is adjusted 😊
I added the few things I noticed as review feedback.

[latonz]

Instead of skipping the expected parameter count, keep a set of already processed parameter ordinals and check the additional parameter ordinals are not included in this set.

[latonz]

With the approach that we never pass parameters to other methods, only user defined mapping methods should not support parameters. For eg. user implemented mappings a diagnostic should be emitted if additional parameters are provided (RMG001).

[latonz]

With the current architecture, if the user defined mapping is not an object mapping (eg. string to int), RMG048 is not emitted.

[latonz]

As there seems to be no updates for several weeks I'm closing this for now. Feel free to reopen / open a new PR with updates 😊