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

Inconsistent mapping when mapping simple object to a constructor or property. #449

Closed
TimothyMakkison opened this issue May 20, 2023 · 4 comments

Comments

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented May 20, 2023

The bug
Possible bug: I ran into trying to add mutliple mapping parameters.
Mapperly will map the source to a target constructor of the same type even if they don't have matching names. Mapperly will not map source to a property even if they have matching names.

To reproduce

[Mapper]
public static partial class Mapper
{
    public static partial A MapToA(int Value); // matching name constructor mapping
    public static partial A MapToA((int V, int T) tuple); // won't construct, complex object doesn't have matching name
    public static partial A MapToADiff(int v); // different name constructor mapping
    public static partial B MapToB(int Value); // matches property but won't map
}

public record A(int Value);

public record B
{
    public int Value { get; set; }
}
public static partial global::Riok.Mapperly.Sample.A MapToA(int Value)
{
    return new global::Riok.Mapperly.Sample.A(Value);
}

public static partial global::Riok.Mapperly.Sample.A MapToA((int V, int T) tuple)
{
    var target = new global::Riok.Mapperly.Sample.A(); // error
    return target;
}

public static partial global::Riok.Mapperly.Sample.A MapToADiff(int v)
{
    return new global::Riok.Mapperly.Sample.A(v);
}

public static partial global::Riok.Mapperly.Sample.B MapToB(int Value)
{
    // Could not generate mapping
    throw new System.NotImplementedException();
}

Expected behaviour
Mapperly should either: use the source in target constructors if they have matching names and types or instead should not directly map the source as a parameter for contructors.

In the future to support #103 if the source/additional parameter matches a property it should assign to it.
This may require Mapperly to change how function reuse is done as it is currently name agnostic. This might??? lead to a situation where the order in which mappings are generated alters the generated code due to matching type signatures but different parameter names.

@latonz
Copy link
Contributor

latonz commented May 22, 2023

A MapToA((int V, int T) tuple);'s generated implementation is wrong and should also lead to // Could not generate mapping / throw new System.NotImplementedException(); as Mapperly cannot know how to construct A based on the information provided (it does not know what value to fill in the ctor param Value). There should also be additional warnings that the fields V and T (and probably Item1 and Item2) are unmapped (related to #448).
All other implementations are generated as expected.

@TimothyMakkison
Copy link
Collaborator Author

A MapToA((int V, int T) tuple);'s generated implementation is wrong and should also lead to // Could not generate mapping / throw new System.NotImplementedException();

Can't 100% remember what that was meant to prove. It could be showing that Mapperly checks property names for complex objects when mapping to a constructor.

All other implementations are generated as expected.

Interesting. Why does Mapperly make exceptions for constructors but not properties?

@latonz
Copy link
Contributor

latonz commented May 22, 2023

Can't 100% remember what that was meant to prove. It could be showing that Mapperly checks property names for complex objects when mapping to a constructor.

Mapperly tries to use constructors by mapping property names to the constructor parameter names. This does not work for Tuples yet (see #448), however they don't match in this sample anyway.

Interesting. Why does Mapperly make exceptions for constructors but not properties?

What do you mean by making an exception? There is a MappingBuilder which tries to convert one type to another by using a constructor on the target type which has one parameter which has the same type as the source type of the mapping. This is only used if the entire source object can be passed to that single parameter constructor. This should not be mixed up with when Mapperly tries to map an object to another class and tries to map properties to constructor parameters of available constructors if no parameterless constructor is available (see ctor mapping docs).

@TimothyMakkison
Copy link
Collaborator Author

That makes sense, thanks for clearing this up for me.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants