feat: added tuple mapping support

[TimothyMakkison]

Description
Fixes #448 Added support for tuple mapping, supported mapping attributes and repeat member mappings are no longer created.

• Class -> Tuple and Tuple -> Class will map via field names, or if a field doesn't have a name its default positional name is used.
• Tuple to Tuple will try to map via field names, otherwise it will attempt to match fields by position.
• Support for IgnoreSource, IgnoreProperty and MapProperty.
• • IgnoreSource and MapProperty will only use field names when available, otherwise if the field doesn't have a name it will use the default positional name.

Question: currently a simple tuple expression is used, should the targets field names be added?

public (int A, int B) Map((int C, int D) source)
{
  // current mapping
  return (source.C, source.D);

  // slightly verbose alternative mapping
  return (A: source.C, B: source.D);
}

I'll update the integration test when we decide on this behaviour

[codecov]

Codecov Report

Merging #467 (d384b57) into main (242da02) will decrease coverage by 0.51%.
The diff coverage is 79.84%.

❗ Current head d384b57 differs from pull request most recent head bcf4c0b. Consider uploading reports for the commit bcf4c0b to get more accurate results

@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
- Coverage   91.09%   90.59%   -0.51%     
==========================================
  Files         167      173       +6     
  Lines        5660     5921     +261     
  Branches      717      757      +40     
==========================================
+ Hits         5156     5364     +208     
- Misses        351      397      +46     
- Partials      153      160       +7     

Impacted Files	Coverage Δ
...rMappings/ValueTupleConstructorParameterMapping.cs	64.70% <64.70%> (ø)
...ingBodyBuilders/NewValueTupleMappingBodyBuilder.cs	74.32% <74.32%> (ø)
...riptors/Mappings/NewValueTupleExpressionMapping.cs	88.88% <88.88%> (ø)
...rContext/NewValueTupleConstructorBuilderContext.cs	100.00% <100.00%> (ø)
...erContext/NewValueTupleExpressionBuilderContext.cs	100.00% <100.00%> (ø)
...criptors/MappingBodyBuilders/MappingBodyBuilder.cs	100.00% <100.00%> (ø)
...tors/MappingBuilders/ExplicitCastMappingBuilder.cs	100.00% <100.00%> (ø)
...tors/MappingBuilders/ImplicitCastMappingBuilder.cs	100.00% <100.00%> (ø)
...uilders/NewInstanceObjectPropertyMappingBuilder.cs	91.17% <100.00%> (+3.17%)	⬆️
...ings/MemberMappings/ConstructorParameterMapping.cs	58.82% <100.00%> (ø)
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[latonz]

Thank you for another great contribution!

I added my comments 😊 Overall it looks really really good 💪 Super happy with your valuable contributions 😊

I'd prefer the more verbose style (A: source.C, B: source.D) for named tuples as this could improve the readability of the generated code and is also aligned with the generated code for property initializers where Mapperly always generates explicit value assignments.

[TimothyMakkison]

Thanks for the review 🙏
I've added the named fields, opting to not add the name colon when the mapping is positional so: (int, int A) -> (int, int A) becomes: return (source.Item1, A: source.A)

I'll squash the commits and faff around with the integration tests when you're happy with these changes 👍

[latonz]

Looks good to me 😊
Only open discussion to be resolved: #467 (comment)

[TimothyMakkison]

tuple expressions (eg. (A: 10, B: 20)) are not valid in expression trees (eg. this does not work: Expression<Func<(int, int)>> x = () => (10, 20);.

Weird, I wonder why they never added this.

I've hackily added ValueTuple support although I haven't cleaned up or properly added the integration tests. Please don't review these changes.

I added ValueTuple by adding an isExpression bool in the constructor, this is used to determine whether a tuple expression or ValueTuple should be used. valueTupleSymbol is used to get the identifier for ValueTuple, I know passing ISymbol to a mapping is an anti pattern, should a constant string be used instead?

Should I instead create NewTupleExpressionMapping and NewValueTupleMapping and reuse the mapping logic?

[latonz]

@TimothyMakkison I think separating the two mappings makes the code easier to read, therefore I'd separate them as ValueTupleExpressionMapping ((A: 10, B: 20)) and ValueTupleConstructorMapping (new ValueTuple<int, int>(10, 10)).

[TimothyMakkison]

Should be ready to merge 👍

• Rebased, renamed files and updated tests.

I wonder if it's possible to configure Verify to show the line number and text diff of any issues. It might make updating the tests faster.

[latonz]

Thank you for the updates. Few smaller comments...

VerifyTests has awesome plugins to view the diff, see VerifyTests supported tools, for example Rider Verify Tests Support.

Whats still missing is a new MappingConversionType, please also update the docs (14-conversions.md).

[TimothyMakkison]

This PR is near completion.

I've added support for MapProperty making TupleExpression a method mapping. I'm not a massive fan of this because it results in code bloat and the MapProperty feature is unlikely to be used with tuples. Ideally there'd be a way to be a MethodMapping but conditionally not create a method body instead returning an normaly tuple expression for Build(TypeMappingBuildContext ctx)

I have experimented with making tuple methods either directly return the tuple if it doesn't map to any child members, or use var target = (src.A, src.B); target.B.C = src.C; return target; syntax. Ideally I'd skip creating a method when it doesn't use MapProperty but doing it to mapperly feel a little hacky.

[TimothyMakkison]

• Added MappingConversionType.Tuple
• Added full MapProperty support, including for nested properties. - The logic is a little hacky but could be reused for new instance initializers.
• Tuple expression will either be a normal expression (target.T = (src.A, ...)) or generate a method if MapProperty is used on nested properties (target.T = MapToTuple(src)). I did this by making MethodDelcarationSyntax nullable, I suspect a neater api change could be done instead.

Draft until I double check this

[latonz]

Thank you for the updates.
Added a few smaller feedbacks...

Regarding the inlined method mapping: For now I'd keep it simple and always generate a method for a tuple expression mapping. The compiler will probably inline it anyway. If we come to the conclusion it is worth to inline these one-liner methods, IMO we should do so in a separate PR, which could probably also affect other method mappings and maybe even be handled on a MethodMapping level.

[TimothyMakkison]

Thanks will make the changes later. 👍

Regarding the inlined method mapping: For now I'd keep it simple and always generate a method for a tuple expression mapping. The compiler will probably inline it anyway. If we come to the conclusion it is worth to inline these one-liner methods, IMO we should do so in a separate PR, which could probably also affect other method mappings and maybe even be handled on a MethodMapping level.

I'm already using a slightly hacky way of method inlining. Do you want me to remove it?

[latonz]

@TimothyMakkison I reviewed it, I'd remove it for now, but keep this commit somewhere. We can probably reintroduce a similar concept in another PR on the MethodMapping level in a later version of Mapperly.

[TimothyMakkison]

Thanks for the review, I've removed method inlining and performed your suggested changes.

I'm converting this pr to a draft until I proof read it.

[github-actions]

🎉 This PR is included in version 2.9.0-next.4 🎉

The release is available on:

• GitHub release
• v2.9.0-next.4

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

• GitHub release
• v3.0.0

Your semantic-release bot 📦🚀